Make tmpfiles safe by fbuihuu · Pull Request #8822 · systemd/systemd
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
It's an RFC as a starting point to fix #7986 completely.
Currently this makes 'f' completely safe only. If this appears to be an acceptable fix, I'll fix the rest of the operations.
keszybz
changed the title
Rfc tmpfiles safe upstream
[RFC] tmpfiles safe upstream
|
|
||
| if (S_ISDIR(st->st_mode)) { | ||
| if (fstat(fd, &st) < 0) { | ||
| r = r ?: -errno; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Downgrade-to-bool is frowned upon. Better to write this as
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| if (r < 0) | ||
| return log_error_errno(r, "Failed to write file \"%s\": %m", path); | ||
|
|
||
| r = path_set_perms(i, path); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return path_set_perms(...)?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, so we follow symlinks for writing but not for application of perms. This deserves some comment here. (because otherwise the next one looking at this, like me, might think that fd_set_perms() would be the right call to make here, rather than path_set_perms()...
| flags = O_CREAT|O_NOFOLLOW|(i->type == CREATE_FILE ? O_EXCL : O_TRUNC); | ||
| /* 'f' operates on regular files exclusively. */ | ||
|
|
||
| flags = O_CREAT|O_EXCL|O_NOFOLLOW; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
flags has just a single use. I'd prefer to not have the variable and just put the flags definition in open below, and add the comment there.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbuihuu this comnment is still not addressed...
| if (!S_ISREG(st.st_mode)) | ||
| return log_error_errno(EEXIST, "%s exists and is not a regular file.", path); | ||
|
|
||
| goto check_perms; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe get rid of the goto and just put the code that is jumped over in an else { } branch?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, here too we'll stat the file twice in the end, I think it would be much nicer to pass "struct stat" if we have it, like in this case, and only query it in fd_set_perms() if we don't.
| return 0; | ||
| /* On a read-only filesystem, we don't want to fail if the | ||
| * target is already empty and the perms are set. So we still | ||
| * proceed with the sanity checks and let fails the remaining |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"and let the remaining operations fail with EROFS"
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
| int fd; | ||
|
|
||
| dn = dirname_malloc(path); | ||
| if (dn == NULL) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!dn
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
@keszybz before going into the details of fixing the code, I'd like to know if the general idea is acceptable.
As described in one commit this can break compatibility in some cases as "unsafe" transitions (the definition of unsafe is given by chase_symlink(... CHASE_SAFE ...)) are not followed anymore.
Maybe we could relax a bit the security checks and consider a transition unsafe only if it's done via a symlink ?
| @@ -769,8 +769,9 @@ static bool hardlink_vulnerable(const struct stat *st) { | |||
| return !S_ISDIR(st->st_mode) && st->st_nlink > 1 && dangerous_hardlinks(); | |||
| } | |||
|
|
|||
| static int fd_set_perms(Item *i, int fd, const struct stat *st) { | |||
| static int fd_set_perms(Item *i, int fd) { | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't like this i must say, because this means that path_set_perms() will then effectively stat all dirs twice...
A better approach might be to accept st being passed as NULL here, and if it is, gather the data fresh, so that callers may but don't have to pass in stat initialized.
struct stat stbuf; … if (!st) { if (fstat(fd, &stbuf) < 0) return -errno; st = &stbuf; }
| if (r < 0) | ||
| return log_error_errno(r, "Failed to write file \"%s\": %m", path); | ||
|
|
||
| r = path_set_perms(i, path); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, so we follow symlinks for writing but not for application of perms. This deserves some comment here. (because otherwise the next one looking at this, like me, might think that fd_set_perms() would be the right call to make here, rather than path_set_perms()...
| if (!S_ISREG(st.st_mode)) | ||
| return log_error_errno(EEXIST, "%s exists and is not a regular file.", path); | ||
|
|
||
| goto check_perms; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, here too we'll stat the file twice in the end, I think it would be much nicer to pass "struct stat" if we have it, like in this case, and only query it in fd_set_perms() if we don't.
hmm, so we follow symlinks for writing but not for application of perms. This deserves some comment here. (because otherwise the next one looking at this, like me, might think that fd_set_perms() would be the right call to make here, rather than path_set_perms()...
@poettering not really the goal here is to resolve and validate the path only once, therefore we want to call fd_set_perms() ultimately (as you expect) and this is done in a separate commit, which makes the function completely safe.
Hi guys, I force pushed a new version. It contains more commits I accumulated over the last month to make most of the code hopefully safe.
Please have a look.
| @@ -1528,8 +1528,7 @@ static int create_directory_or_subvolume(Item *i, const char *path) { | |||
| RUN_WITH_UMASK((~i->mode) & 0777) | |||
| r = btrfs_subvol_make(i->path); | |||
| } | |||
| } else | |||
| r = 0; | |||
| } | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dropping this looks like something that will trip up gcc sooner or later, as it tends to be too dumb to realize that r is always initialized in the r == -ENOTTY check in the if line immediately following...
Moreover, I'd prefer if we didn't write this code so fragile, as the order of the checks in the following if line suddenly matters, which is very much not obvious
| @@ -1562,9 +1561,9 @@ static int create_directory_or_subvolume(Item *i, const char *path) { | |||
| if (r == -ENOTTY) | |||
| log_debug_errno(r, "Couldn't adjust quota for subvolume \"%s\" (unsupported fs or dir not a subvolume): %m", i->path); | |||
| else if (r == -EROFS) | |||
| log_debug_errno(r, "Couldn't adjust quota for subvolume \"%s\" (fs is read-only).", i->path); | |||
| log_debug("Couldn't adjust quota for subvolume \"%s\" (fs is read-only).", i->path); | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this change? we should always propagate error codes we get to the journal, and that's what log_debug_errno() does
hmm, before i continue with the review, did you see my earlier comment? i think the fstat() bit of it is not addressed yet, is it?
also, needs a rebase
Erf sorry I forgot to reply to your comment.
So I saw your comment about fstat() but I'm not sure yet...
If you look at the end result of the patch series you'll see that only fd_set_perms() might be a concern (the other helpers doesn't need to stat the file at all).
Even in this case fstat() might be performed by 2 callers of fd_set_perms() but they're still corner cases:
- in create_file(), it's only done in the error code path
- in truncate_file(), the stat info might be outdated because we might truncate the file
The second case shows that if we rely (optionally) on the caller to pass the stat info, they might be outdated...
Also I think the minor simplification done in item_do() is valid but I'll split the patch so this part can be reviewed separately.
Also will rebase the serie
@poettering I finally followed your suggestion about fstat() as it seems useful for the recursive path.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another round. I figure the [RFC] should be dropped from the title of this PR no? and the "dont-merge", too, right? I mean, you intend to get this merged soon, right?
|
|
||
| if (i->type == EMPTY_DIRECTORY && !S_ISDIR(st.st_mode)) | ||
| return log_error_errno(EEXIST, "'%s' already exists and is not a directory. ", path); | ||
| if(!S_ISDIR(stbuf.st_mode)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coding style nitpick: space between "if" and "("
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment has not been addressed?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
coding style nitpick: double empty line
| assert(i->type == WRITE_FILE); | ||
|
|
||
| /* Follows symlinks */ | ||
| fd = open(path, O_NDELAY|O_CLOEXEC|O_WRONLY|O_NOCTTY, i->mode); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, we generally try to use O_NONBLOCK rather than O_NDELAY all across our codebase, since O_NDELAY is an older obsolete name... in fact we even have a coccinelle script in place that fixes that (see coccinelle/o-ndelay.cocci)
| flags = O_CREAT|O_NOFOLLOW|(i->type == CREATE_FILE ? O_EXCL : O_TRUNC); | ||
| /* 'f' operates on regular files exclusively. */ | ||
|
|
||
| flags = O_CREAT|O_EXCL|O_NOFOLLOW; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fbuihuu this comnment is still not addressed...
| * fifo nor a terminal device. Therefore we first open the file and make | ||
| * sure it's a regular one before truncating it. */ | ||
|
|
||
| flags = O_CREAT|O_NOFOLLOW; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above, no need to have flags as variable if we always set it to one value only...
| if (r < 0) | ||
| return log_error_errno(r, "is_dir() failed on file %s: %m", path); | ||
| if (r == 0) | ||
| return log_error_errno(EEXIST, "'%s' already exists and is not a directory. ", path); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as mentioned earlier: don't pass EEXIST directly to log_error_errno()
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also trailing whitespace in string
| @@ -78,6 +78,7 @@ enum { | |||
| CHASE_OPEN = 1U << 4, /* If set, return an O_PATH object to the final component */ | |||
| CHASE_TRAIL_SLASH = 1U << 5, /* If set, any trailing slash will be preserved */ | |||
| CHASE_STEP = 1U << 6, /* If set, just execute a single step of the normalization */ | |||
| CHASE_NOFOLLOW = 1U << 7, /* If set, don't follow the final component if it's a symlink */ | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, this should be clarified a bit. if we have a symlink change "a → b → [non-existing]", then resolving "a" would return "a" rather than "b", hence it's final only in the sense that it is the last part of the path, not the last part of the chain
| assert_se(pfd > 0); | ||
| assert_se(path_equal(result, q)); | ||
| assert_se(fstat(pfd, &st) >= 0); | ||
| assert_se(S_ISLNK(st.st_mode)); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd prefer a test here that shows what is returned if we have a symlink chain "a → b → [non-existant]" and resolve "a"
|
|
||
| fd = chase_symlinks(path, NULL, CHASE_OPEN|CHASE_SAFE|CHASE_NOFOLLOW, NULL); | ||
| if (fd == -EPERM) | ||
| log_error("Unsafe symlinks encountered in %s, aborting.", path); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
errors we recieve should be passed to log_error_errno(), so that they are included in the structure log message even if we don't show them in the text msg
| if (fd < 0) | ||
| return log_error_errno(errno, "Adjusting owner and mode for %s failed: %m", path); | ||
| return log_error_errno(fd, "Adjusting owner and mode for %s failed: %m", path); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this means we'll now log twice about EPERM, which we really shouldn't
(btw, also need a rebase)
fbuihuu
changed the title
[RFC] tmpfiles safe upstream
Make tmpfiles safe
@poettering sorry for the delay, here is a new round with hopefully your last concerns addressed.
Please have a look.
@fbuihuu still needs a rebase and the all CIs fail...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the biggie is that fd_get_path() should not be used if we can avoid it due to its ambiguity and unreliability. It's a bit hacky to use it at all, but i figure it's ok if it allows us to use openat() style apis more comprehensively, hence I am ok with it, but then at least we should not ignore the path if we have it anyway like your patch does it now
|
|
||
| if (i->type == EMPTY_DIRECTORY && !S_ISDIR(st.st_mode)) | ||
| return log_error_errno(EEXIST, "'%s' already exists and is not a directory. ", path); | ||
| if(!S_ISDIR(stbuf.st_mode)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment has not been addressed?
| assert(path); | ||
|
|
||
| if (path_equal(path, "/") || !path_is_normalized(path)) | ||
| return -EINVAL; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there any reason not to allow the root dir here?
this means people can't use tmpfiles to let's say add an xattr on the root dir. Why prohibit that though?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, I'll remove it.
| @@ -73,6 +73,7 @@ enum { | |||
| CHASE_OPEN = 1 << 4, /* If set, return an O_PATH object to the final component */ | |||
| CHASE_TRAIL_SLASH = 1 << 5, /* If set, any trailing slash will be preserved */ | |||
| CHASE_STEP = 1 << 6, /* If set, just execute a single step of the normalization */ | |||
| CHASE_NOFOLLOW = 1 << 7, /* If set, don't follow the final component of the passed path if it's a symlink */ | |||
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment should say: "only valid with CHASE_OPEN: when the path's right-most component refers to symlink return O_PATH fd of the symlink, rather than following it"
| /* FIXME: O_TRUNC is unspecified if file is neither a regular file nor a | ||
| * fifo nor a terminal device. Therefore we should fail if file is | ||
| * anything but a regular file with 'F'. */ | ||
| flags = O_CREAT|O_NOFOLLOW|(i->type == CREATE_FILE ? O_EXCL : O_TRUNC); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, it's a bit strange that some flags are set in this variable and others merged in in the open(). pick one.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "main" flags are set here, i.e. the one which are related to the creation of the file. But I wouldn't pay too much attention for this as it's gone in the end.
| if (fd < 0) { | ||
| if ((flags & LABEL_IGNORE_ENOENT) && errno == ENOENT) | ||
| return 0; | ||
| r = fd_get_path(fd, &path); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should use fd_get_path() only if we have to. i.e. when mac_smack_fix() is called, i.e. we know the real filename anyway, use that. fd_get_path() is problematic due the (deleted) ambiguity, and because of chroot and stuff...
|
|
||
| r = selabel_lookup_raw(label_hnd, &filecon, newpath, mode); | ||
| } | ||
| r = fd_get_path(fd, &path); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as for the smack stuff: please use any real path we might have if possible. fd_get_path() should be the fallback if we have nothing better, but only then
@poettering thanks for your feedback, new version pushed.
In this case it simply returns the random generated filename with anything prefixed.
This flag mimics what "O_NOFOLLOW|O_PATH" does for open(2) that is chase_symlinks() will not resolve the final pathname component if it's a symlink and instead will return a file descriptor referring to the symlink itself. Note: if CHASE_SAFE is also passed, no safety checking is performed on the transition done if the symlink would have been followed.
Since all path_set_*() helpers don't follow symlinks, it's possible to use chase_symlinks(CHASE_NOFOLLOW) flag to both open the files specified by the passed paths and check their validity (unlike their counterpart fd_set_*() helpers).
@poettering thanks for your patience... new version forced pushed.
| if (isempty(p)) | ||
| x = stpcpy(stpcpy(t, ".#"), extra); | ||
| else | ||
| x = stpcpy(stpcpy(stpcpy(t, p), "/.#"), extra); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
he new() allocation further up is now a bit sloppy, as it calculates 3 instead of just 2 btyes if t is the empty string... but i figure that doesn't matter
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah at least it makes the allocation simpler by allocating the largest size we will need.
| return -errno; | ||
| } | ||
|
|
||
| r = fd_get_path(fd, &p); |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as with the selinux code we should use 'path' as it is if it's already absolute, and use fd_get_path() only if we the path is not absolute yet
Only found the two issues above, which don't really matter I figure... I still would love to see the SMACK thing fixed, would be delighted to take a patch for that
Anyway, merged! Thanks a ton for working on this!
+1 this was a huge effort, thank you.
Thanks @poettering for reviewing this stuff, this was a huge effort too.
I'll fix the SMACK stuff next week.
Cheers.
fbuihuu
deleted the
rfc-tmpfiles-safe-upstream
branch
Given that it already requires many commits to be backported on top of v234, I wondering if it would be reasonable to backport the fix to older versions...
On 2018-11-20 15:40:02, Franck Bui wrote: Given that it already requires many commits to be backported on top of v234, I wondering if it would be reasonable to backport the fix to older versions...
My attempts at backporting the PR have more or less failed. My next approach is to simply backport all of the current tmpfiles.c (or any better version) to v215.