dilim/systemd
1
0
Fork 0
mirror of https://github.com/systemd/systemd.git synced 2026-07-13 02:51:14 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #11182 from poettering/fileio-more-paranoia

Browse Source 
More safety checks for fileio.c
This commit is contained in:
Lennart Poettering
2018-12-18 20:49:19 +01:00
committed by GitHub
parent a361cc99ae 7f9d1aedec
commit be2e1823ef
8 changed files with 177 additions and 136 deletions
Download Patch File Download Diff File Expand all files Collapse all files

27
TODO
View File

@@ -34,8 +34,6 @@ Features:
* when importing an fs tree with machined, complain if image is not an OS
* when we fork off generators and such, lower LIMIT_NOFILE soft limit to 1K
* Maybe introduce a helper safe_exec() or so, which is to execve() which
safe_fork() is to fork(). And then make revert the RLIMIT_NOFILE soft limit
to 1K implicitly, unless explicitly opted-out.
@@ -63,16 +61,11 @@ Features:
* when a socket unit is spawned with an AF_UNIX path in /var/run, complain and
patch it to use /run instead
* consider splitting out all temporary file creation APIs (we have so many in
fileio.h and elsewhere!) into a new util file of its own.
* set memory.oom.group in cgroupsv2 for all leaf cgroups (kernel v4.19+)
* add a new syscall group "@esoteric" for more esoteric stuff such as bpf() and
usefaultd() and make systemd-analyze check for it.
* drop umask() calls and suchlike from our generators, pid1 should set things up correctly anyway
* paranoia: whenever we process passwords, call mlock() on the memory
first. i.e. look for all places we use string_erase()/string_free_erase() and
augment them with mlock()
@@ -165,13 +158,6 @@ Features:
* nspawn: greater control over selinux label?
* cgroups: figure out if we can somehow communicate in a cleaner way whether a
systemd instance not running in the cgroup root shall or shall not manage the
attributes of its top-level cgroup. Currently it assumes it manages all, but
then might get EPERM due to permission porblems/userns, which is OK, but this
should be revisited to make clearer and also work if the payload systemd runs
with full privs and without userns.
* hibernate/s2h: make this robust and safe to enable in Fedora by default.
Specifically:
@@ -320,11 +306,6 @@ Features:
StateDirectory=, LogsDirectory=, CacheDirectory=, as well as RootDirectory= if it
is set, plus the whole disk space any image configured with RootImage=.
* Introduce "exit" as an EmergencyAction value, and allow to configure a
per-unit success/failure exit code to configure. This would be useful for
running commands inside of services inside of containers, which could then
propagate their failure state all the way up.
* In DynamicUser= mode: before selecting a UID, use disk quota APIs on relevant
disks to see if the UID is already in use.
@@ -341,7 +322,7 @@ Features:
* show whether a service has out-of-date configuration in "systemctl status" by
using mtime data of ConfigurationDirectory=.
* replace all uses of fgets() + LINE_MAX by read_line()
* replace all remaining uses of fgets() + LINE_MAX by read_line()
* Add AddUser= setting to unit files, similar to DynamicUser=1 which however
creates a static, persistent user rather than a dynamic, transient user. We
@@ -507,9 +488,6 @@ Features:
state.
http://lists.freedesktop.org/archives/systemd-devel/2015-April/030229.html
* Maybe add support for the equivalent of "ethtool advertise" to .link files?
http://lists.freedesktop.org/archives/systemd-devel/2015-April/030112.html
* The udev blkid built-in should expose a property that reflects
whether media was sensed in USB CF/SD card readers. This should then
be used to control SYSTEMD_READY=1/0 so that USB card readers aren't
@@ -635,7 +613,6 @@ Features:
* cgroups:
- implement per-slice CPUFairScheduling=1 switch
- handle jointly mounted controllers correctly
- introduce high-level settings for RT budget, swappiness
- how to reset dynamically changed unit cgroup attributes sanely?
- when reloading configuration, apply new cgroup configuration
@@ -904,8 +881,6 @@ Features:
PID 1...
- optionally automatically add FORWARD rules to iptables whenever nspawn is
running, remove them when shut down.
- maybe make copying of /etc/resolv.conf optional, and skip it if --read-only
is used
* dissect
- refuse mounting over a mount point

108
src/basic/fileio.c
View File

@@ -667,46 +667,6 @@ int fputs_with_space(FILE *f, const char *s, const char *separator, bool *space)
return fputs(s, f);
}
int read_nul_string(FILE *f, char **ret) {
_cleanup_free_ char *x = NULL;
size_t allocated = 0, n = 0;
assert(f);
assert(ret);
/* Reads a NUL-terminated string from the specified file. */
for (;;) {
int c;
if (!GREEDY_REALLOC(x, allocated, n+2))
return -ENOMEM;
c = fgetc(f);
if (c == 0) /* Terminate at NUL byte */
break;
if (c == EOF) {
if (ferror(f))
return -errno;
break; /* Terminate at EOF */
}
x[n++] = (char) c;
}
if (x)
x[n] = 0;
else {
x = new0(char, 1);
if (!x)
return -ENOMEM;
}
*ret = TAKE_PTR(x);
return 0;
}
/* A bitmask of the EOL markers we know */
typedef enum EndOfLineMarker {
EOL_NONE = 0,
@@ -715,11 +675,15 @@ typedef enum EndOfLineMarker {
EOL_THIRTEEN = 1 << 2, /* \r (aka CR) */
} EndOfLineMarker;
static EndOfLineMarker categorize_eol(char c) {
if (c == '\n')
return EOL_TEN;
if (c == '\r')
return EOL_THIRTEEN;
static EndOfLineMarker categorize_eol(char c, ReadLineFlags flags) {
if (!IN_SET(flags, READ_LINE_ONLY_NUL)) {
if (c == '\n')
return EOL_TEN;
if (c == '\r')
return EOL_THIRTEEN;
}
if (c == '\0')
return EOL_ZERO;
@@ -728,9 +692,10 @@ static EndOfLineMarker categorize_eol(char c) {
DEFINE_TRIVIAL_CLEANUP_FUNC(FILE*, funlockfile);
int read_line(FILE *f, size_t limit, char **ret) {
int read_line_full(FILE *f, size_t limit, ReadLineFlags flags, char **ret) {
size_t n = 0, allocated = 0, count = 0;
_cleanup_free_ char *buffer = NULL;
int r;
assert(f);
@@ -770,7 +735,7 @@ int read_line(FILE *f, size_t limit, char **ret) {
for (;;) {
EndOfLineMarker eol;
int c;
char c;
if (n >= limit)
return -ENOBUFS;
@@ -778,20 +743,13 @@ int read_line(FILE *f, size_t limit, char **ret) {
if (count >= INT_MAX) /* We couldn't return the counter anymore as "int", hence refuse this */
return -ENOBUFS;
errno = 0;
c = fgetc_unlocked(f);
if (c == EOF) {
/* if we read an error, and have no data to return, then propagate the error */
if (ferror_unlocked(f) && n == 0)
return errno > 0 ? -errno : -EIO;
/* EOF is line ending too. */
r = safe_fgetc(f, &c);
if (r < 0)
return r;
if (r == 0) /* EOF is definitely EOL */
break;
}
count++;
eol = categorize_eol(c);
eol = categorize_eol(c, flags);
if (FLAGS_SET(previous_eol, EOL_ZERO) ||
(eol == EOL_NONE && previous_eol != EOL_NONE) ||
@@ -800,10 +758,11 @@ int read_line(FILE *f, size_t limit, char **ret) {
* EOL marker has been seen right before? In either of these three cases we are
* done. But first, let's put this character back in the queue. */
assert_se(ungetc(c, f) != EOF);
count--;
break;
}
count++;
if (eol != EOL_NONE) {
previous_eol |= eol;
continue;
@@ -813,7 +772,7 @@ int read_line(FILE *f, size_t limit, char **ret) {
if (!GREEDY_REALLOC(buffer, allocated, n + 2))
return -ENOMEM;
buffer[n] = (char) c;
buffer[n] = c;
}
n++;
@@ -828,3 +787,30 @@ int read_line(FILE *f, size_t limit, char **ret) {
return (int) count;
}
int safe_fgetc(FILE *f, char *ret) {
int k;
assert(f);
/* A safer version of plain fgetc(): let's propagate the error that happened while reading as such, and
* separate the EOF condition from the byte read, to avoid those confusion signed/unsigned issues fgetc()
* has. */
errno = 0;
k = fgetc(f);
if (k == EOF) {
if (ferror(f))
return errno > 0 ? -errno : -EIO;
if (ret)
*ret = 0;
return 0;
}
if (ret)
*ret = k;
return 1;
}

16
src/basic/fileio.h
View File

@@ -61,6 +61,18 @@ int read_timestamp_file(const char *fn, usec_t *ret);
int fputs_with_space(FILE *f, const char *s, const char *separator, bool *space);
int read_nul_string(FILE *f, char **ret);
typedef enum ReadLineFlags {
READ_LINE_ONLY_NUL = 1 << 0,
} ReadLineFlags;
int read_line(FILE *f, size_t limit, char **ret);
int read_line_full(FILE *f, size_t limit, ReadLineFlags flags, char **ret);
static inline int read_line(FILE *f, size_t limit, char **ret) {
return read_line_full(f, limit, 0, ret);
}
static inline int read_nul_string(FILE *f, size_t limit, char **ret) {
return read_line_full(f, limit, READ_LINE_ONLY_NUL, ret);
}
int safe_fgetc(FILE *f, char *ret);

62
src/basic/process-util.c
View File

@@ -600,12 +600,14 @@ int get_process_root(pid_t pid, char **root) {
return get_process_link_contents(p, root);
}
#define ENVIRONMENT_BLOCK_MAX (5U*1024U*1024U)
int get_process_environ(pid_t pid, char **env) {
_cleanup_fclose_ FILE *f = NULL;
_cleanup_free_ char *outcome = NULL;
int c;
const char *p;
size_t allocated = 0, sz = 0;
const char *p;
int r;
assert(pid >= 0);
assert(env);
@@ -621,23 +623,28 @@ int get_process_environ(pid_t pid, char **env) {
(void) __fsetlocking(f, FSETLOCKING_BYCALLER);
while ((c = fgetc(f)) != EOF) {
for (;;) {
char c;
if (sz >= ENVIRONMENT_BLOCK_MAX)
return -ENOBUFS;
if (!GREEDY_REALLOC(outcome, allocated, sz + 5))
return -ENOMEM;
r = safe_fgetc(f, &c);
if (r < 0)
return r;
if (r == 0)
break;
if (c == '\0')
outcome[sz++] = '\n';
else
sz += cescape_char(c, outcome + sz);
}
if (!outcome) {
outcome = strdup("");
if (!outcome)
return -ENOMEM;
} else
outcome[sz] = '\0';
outcome[sz] = '\0';
*env = TAKE_PTR(outcome);
return 0;
@@ -870,9 +877,9 @@ int kill_and_sigcont(pid_t pid, int sig) {
int getenv_for_pid(pid_t pid, const char *field, char **ret) {
_cleanup_fclose_ FILE *f = NULL;
char *value = NULL;
bool done = false;
const char *path;
size_t l;
size_t l, sum = 0;
int r;
assert(pid >= 0);
assert(field);
@@ -895,6 +902,9 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) {
return 1;
}
if (!pid_is_valid(pid))
return -EINVAL;
path = procfs_file_alloca(pid, "environ");
f = fopen(path, "re");
@@ -908,24 +918,19 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) {
(void) __fsetlocking(f, FSETLOCKING_BYCALLER);
l = strlen(field);
for (;;) {
_cleanup_free_ char *line = NULL;
do {
char line[LINE_MAX];
size_t i;
if (sum > ENVIRONMENT_BLOCK_MAX) /* Give up searching eventually */
return -ENOBUFS;
for (i = 0; i < sizeof(line)-1; i++) {
int c;
r = read_nul_string(f, LONG_LINE_MAX, &line);
if (r < 0)
return r;
if (r == 0) /* EOF */
break;
c = getc(f);
if (_unlikely_(c == EOF)) {
done = true;
break;
} else if (c == 0)
break;
line[i] = c;
}
line[i] = 0;
sum += r;
if (strneq(line, field, l) && line[l] == '=') {
value = strdup(line + l + 1);
@@ -935,8 +940,7 @@ int getenv_for_pid(pid_t pid, const char *field, char **ret) {
*ret = value;
return 1;
}
} while (!done);
}
*ret = NULL;
return 0;

13
src/basic/terminal-util.c
View File

@@ -96,7 +96,7 @@ int read_one_char(FILE *f, char *ret, usec_t t, bool *need_nl) {
new_termios.c_cc[VTIME] = 0;
if (tcsetattr(fileno(f), TCSADRAIN, &new_termios) >= 0) {
int c;
char c;
if (t != USEC_INFINITY) {
if (fd_wait_for_event(fileno(f), POLLIN, t) <= 0) {
@@ -105,17 +105,12 @@ int read_one_char(FILE *f, char *ret, usec_t t, bool *need_nl) {
}
}
errno = 0;
c = fgetc(f);
if (c == EOF)
r = ferror(f) && errno > 0 ? -errno : -EIO;
else
r = 0;
r = safe_fgetc(f, &c);
(void) tcsetattr(fileno(f), TCSADRAIN, &old_termios);
if (r < 0)
return r;
if (r == 0)
return -EIO;
if (need_nl)
*need_nl = c != '\n';

12
src/machine/machine.c
View File

@@ -594,7 +594,7 @@ int machine_get_uid_shift(Machine *m, uid_t *ret) {
uid_t uid_base, uid_shift, uid_range;
gid_t gid_base, gid_shift, gid_range;
_cleanup_fclose_ FILE *f = NULL;
int k;
int k, r;
assert(m);
assert(ret);
@@ -643,7 +643,10 @@ int machine_get_uid_shift(Machine *m, uid_t *ret) {
return -ENXIO;
/* If there's more than one line, then we don't support this mapping. */
if (fgetc(f) != EOF)
r = safe_fgetc(f, NULL);
if (r < 0)
return r;
if (r != 0) /* Insist on EOF */
return -ENXIO;
fclose(f);
@@ -664,7 +667,10 @@ int machine_get_uid_shift(Machine *m, uid_t *ret) {
}
/* If there's more than one line, then we don't support this file. */
if (fgetc(f) != EOF)
r = safe_fgetc(f, NULL);
if (r < 0)
return r;
if (r != 0) /* Insist on EOF */
return -ENXIO;
/* If the UID and GID mapping doesn't match, we don't support this mapping. */

8
src/machine/machined-dbus.c
View File

@@ -637,8 +637,8 @@ static int clean_pool_done(Operation *operation, int ret, sd_bus_error *error) {
if (success) /* The resulting temporary file could not be updated, ignore it. */
return ret;
r = read_nul_string(f, &name);
if (r < 0 || isempty(name)) /* Same here... */
r = read_nul_string(f, LONG_LINE_MAX, &name);
if (r <= 0) /* Same here... */
return ret;
return sd_bus_error_set_errnof(error, ret, "Failed to remove image %s: %m", name);
@@ -660,10 +660,10 @@ static int clean_pool_done(Operation *operation, int ret, sd_bus_error *error) {
_cleanup_free_ char *name = NULL;
uint64_t size;
r = read_nul_string(f, &name);
r = read_nul_string(f, LONG_LINE_MAX, &name);
if (r < 0)
return r;
if (isempty(name)) /* reached the end */
if (r == 0) /* reached the end */
break;
errno = 0;

67
src/test/test-fileio.c
View File

@@ -610,9 +610,36 @@ static void test_tempfn(void) {
free(ret);
}
static const char chars[] =
"Aąę„”\n루";
static void test_fgetc(void) {
_cleanup_fclose_ FILE *f = NULL;
char c;
f = fmemopen((void*) chars, sizeof(chars), "re");
assert_se(f);
for (unsigned i = 0; i < sizeof(chars); i++) {
assert_se(safe_fgetc(f, &c) == 1);
assert_se(c == chars[i]);
assert_se(ungetc(c, f) != EOF);
assert_se(safe_fgetc(f, &c) == 1);
assert_se(c == chars[i]);
/* Check that ungetc doesn't care about unsigned char vs signed char */
assert_se(ungetc((unsigned char) c, f) != EOF);
assert_se(safe_fgetc(f, &c) == 1);
assert_se(c == chars[i]);
}
assert_se(safe_fgetc(f, &c) == 0);
}
static const char buffer[] =
"Some test data\n"
"Some weird line\r"
"루Non-ascii chars: ąę„”\n"
"terminators\r\n"
"and even more\n\r"
"now the same with a NUL\n\0"
@@ -631,7 +658,7 @@ static void test_read_line_one_file(FILE *f) {
assert_se(read_line(f, (size_t) -1, &line) == 15 && streq(line, "Some test data"));
line = mfree(line);
assert_se(read_line(f, (size_t) -1, &line) == 16 && streq(line, "Some weird line"));
assert_se(read_line(f, (size_t) -1, &line) > 0 && streq(line, "루Non-ascii chars: ąę„”"));
line = mfree(line);
assert_se(read_line(f, (size_t) -1, &line) == 13 && streq(line, "terminators"));
@@ -748,6 +775,40 @@ static void test_read_line4(void) {
}
}
static void test_read_nul_string(void) {
static const char test[] = "string nr. 1\0"
"string nr. 2\n\0"
"empty string follows\0"
"\0"
"final string\n is empty\0"
"\0";
_cleanup_fclose_ FILE *f = NULL;
_cleanup_free_ char *s = NULL;
assert_se(f = fmemopen((void*) test, sizeof(test)-1, "r"));
assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 13 && streq_ptr(s, "string nr. 1"));
s = mfree(s);
assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 14 && streq_ptr(s, "string nr. 2\n"));
s = mfree(s);
assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 21 && streq_ptr(s, "empty string follows"));
s = mfree(s);
assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 1 && streq_ptr(s, ""));
s = mfree(s);
assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 23 && streq_ptr(s, "final string\n is empty"));
s = mfree(s);
assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 1 && streq_ptr(s, ""));
s = mfree(s);
assert_se(read_nul_string(f, LONG_LINE_MAX, &s) == 0 && streq_ptr(s, ""));
}
int main(int argc, char *argv[]) {
test_setup_logging(LOG_DEBUG);
@@ -767,10 +828,12 @@ int main(int argc, char *argv[]) {
test_search_and_fopen_nulstr();
test_writing_tmpfile();
test_tempfn();
test_fgetc();
test_read_line();
test_read_line2();
test_read_line3();
test_read_line4();
test_read_nul_string();
return 0;
}