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

Make tmpfiles safe #8822

Merged
merged 26 commits into from Aug 3, 2018

Conversation

6 participants
@fbuihuu
Copy link
Contributor

fbuihuu commented Apr 26, 2018

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 keszybz changed the title Rfc tmpfiles safe upstream [RFC] tmpfiles safe upstream Apr 26, 2018


if (S_ISDIR(st->st_mode)) {
if (fstat(fd, &st) < 0) {
r = r ?: -errno;

This comment has been minimized.

@keszybz

keszybz Apr 26, 2018

Member

Downgrade-to-bool is frowned upon. Better to write this as

if (r >= 0)
     r = -errno;

This comment has been minimized.

@fbuihuu

fbuihuu Apr 26, 2018

Contributor

ok

if (r < 0)
return log_error_errno(r, "Failed to write file \"%s\": %m", path);

r = path_set_perms(i, path);

This comment has been minimized.

@keszybz

keszybz Apr 26, 2018

Member

return path_set_perms(...)?

This comment has been minimized.

@fbuihuu

fbuihuu Apr 26, 2018

Contributor

done

This comment has been minimized.

@poettering

poettering Apr 26, 2018

Member

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;

This comment has been minimized.

@keszybz

keszybz Apr 26, 2018

Member

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.

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

@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;

This comment has been minimized.

@keszybz

keszybz Apr 26, 2018

Member

Maybe get rid of the goto and just put the code that is jumped over in an else { } branch?

This comment has been minimized.

@fbuihuu

fbuihuu Apr 26, 2018

Contributor

ok

This comment has been minimized.

@poettering

poettering Apr 26, 2018

Member

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

This comment has been minimized.

@keszybz

keszybz Apr 26, 2018

Member

"and let the remaining operations fail with EROFS"

This comment has been minimized.

@fbuihuu

fbuihuu Apr 26, 2018

Contributor

ok

int fd;

dn = dirname_malloc(path);
if (dn == NULL)

This comment has been minimized.

@keszybz

This comment has been minimized.

@fbuihuu

fbuihuu Apr 26, 2018

Contributor

ok

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Apr 26, 2018

@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 ?

@fbuihuu fbuihuu force-pushed the fbuihuu:rfc-tmpfiles-safe-upstream branch from 7fd5f41 to ae60d0c Apr 26, 2018

@@ -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) {

This comment has been minimized.

@poettering

poettering Apr 26, 2018

Member

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);

This comment has been minimized.

@poettering

poettering Apr 26, 2018

Member

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;

This comment has been minimized.

@poettering

poettering Apr 26, 2018

Member

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.

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Apr 26, 2018

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.

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented May 23, 2018

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;
}

This comment has been minimized.

@poettering

poettering May 23, 2018

Member

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);

This comment has been minimized.

@poettering

poettering May 23, 2018

Member

why this change? we should always propagate error codes we get to the journal, and that's what log_debug_errno() does

@poettering

This comment has been minimized.

Copy link
Member

poettering commented May 23, 2018

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

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented May 24, 2018

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.

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented May 24, 2018

Also will rebase the serie

@fbuihuu fbuihuu force-pushed the fbuihuu:rfc-tmpfiles-safe-upstream branch from da900da to 03d458f May 24, 2018

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented May 24, 2018

@poettering I finally followed your suggestion about fstat() as it seems useful for the recursive path.

@poettering
Copy link
Member

poettering left a comment

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))

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

coding style nitpick: space between "if" and "("

This comment has been minimized.

@poettering

poettering Jul 2, 2018

Member

this comment has not been addressed?

st = &stbuf;
}


This comment has been minimized.

@poettering

poettering May 30, 2018

Member

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);

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

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;

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

@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;

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

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);

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

as mentioned earlier: don't pass EEXIST directly to log_error_errno()

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

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 */

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

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));

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

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);

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

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);

This comment has been minimized.

@poettering

poettering May 30, 2018

Member

this means we'll now log twice about EPERM, which we really shouldn't

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Jun 6, 2018

(btw, also need a rebase)

@fbuihuu fbuihuu force-pushed the fbuihuu:rfc-tmpfiles-safe-upstream branch from 03d458f to c6899e5 Jul 2, 2018

@fbuihuu fbuihuu changed the title [RFC] tmpfiles safe upstream Make tmpfiles safe Jul 2, 2018

@fbuihuu fbuihuu removed the dont-merge 💣 label Jul 2, 2018

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Jul 2, 2018

@poettering sorry for the delay, here is a new round with hopefully your last concerns addressed.

Please have a look.

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Jul 2, 2018

@fbuihuu still needs a rebase and the all CIs fail...

@fbuihuu fbuihuu force-pushed the fbuihuu:rfc-tmpfiles-safe-upstream branch from c6899e5 to f17d604 Jul 2, 2018

@evverx

This comment has been minimized.

Copy link
Member

evverx commented Jul 2, 2018

This pull request introduces 1 alert when merging f17d604 into 2479c4f - view on LGTM.com

new alerts:

  • 1 for FIXME comment

Comment posted by LGTM.com

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Jul 2, 2018

@poettering, rebased.

@poettering
Copy link
Member

poettering left a comment

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))

This comment has been minimized.

@poettering

poettering Jul 2, 2018

Member

this comment has not been addressed?

assert(path);

if (path_equal(path, "/") || !path_is_normalized(path))
return -EINVAL;

This comment has been minimized.

@poettering

poettering Jul 2, 2018

Member

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?

This comment has been minimized.

@fbuihuu

fbuihuu Jul 3, 2018

Contributor

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 */

This comment has been minimized.

@poettering

poettering Jul 2, 2018

Member

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);

This comment has been minimized.

@poettering

poettering Jul 2, 2018

Member

hmm, it's a bit strange that some flags are set in this variable and others merged in in the open(). pick one.

This comment has been minimized.

@fbuihuu

fbuihuu Jul 3, 2018

Contributor

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);

This comment has been minimized.

@poettering

poettering Jul 2, 2018

Member

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);

This comment has been minimized.

@poettering

poettering Jul 2, 2018

Member

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

@fbuihuu fbuihuu force-pushed the fbuihuu:rfc-tmpfiles-safe-upstream branch from f17d604 to 55aaff8 Jul 3, 2018

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Jul 3, 2018

@poettering thanks for your feedback, new version pushed.

@evverx

This comment has been minimized.

Copy link
Member

evverx commented Jul 3, 2018

This pull request introduces 1 alert when merging 55aaff8 into db2f8a2 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

Comment posted by LGTM.com

@fbuihuu fbuihuu force-pushed the fbuihuu:rfc-tmpfiles-safe-upstream branch from c50da5e to 7f6240f Jul 30, 2018

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Jul 30, 2018

@poettering thanks for your patience... new version forced pushed.

@evverx

This comment has been minimized.

Copy link
Member

evverx commented Jul 30, 2018

This pull request introduces 1 alert when merging 7f6240f into 1c57fa9 - view on LGTM.com

new alerts:

  • 1 for FIXME comment

Comment posted by LGTM.com

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Aug 3, 2018

@poettering we're close to the end, well I hope :D

if (isempty(p))
x = stpcpy(stpcpy(t, ".#"), extra);
else
x = stpcpy(stpcpy(stpcpy(t, p), "/.#"), extra);

This comment has been minimized.

@poettering

poettering Aug 3, 2018

Member

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

This comment has been minimized.

@fbuihuu

fbuihuu Aug 3, 2018

Contributor

yeah at least it makes the allocation simpler by allocating the largest size we will need.

return -errno;
}

r = fd_get_path(fd, &p);

This comment has been minimized.

@poettering

poettering Aug 3, 2018

Member

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

@poettering poettering merged commit 6854990 into systemd:master Aug 3, 2018

7 checks passed

Fedora Rawhide CI x86_64 rpm build [succeeded]
Details
LGTM analysis: C/C++ 1 new alert
Details
LGTM analysis: JavaScript No alert changes
Details
bionic-amd64 autopkgtest finished (success)
Details
bionic-i386 autopkgtest finished (success)
Details
bionic-s390x autopkgtest finished (success)
Details
semaphoreci The build passed on Semaphore.
Details
@poettering

This comment has been minimized.

Copy link
Member

poettering commented Aug 3, 2018

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

@poettering

This comment has been minimized.

Copy link
Member

poettering commented Aug 3, 2018

Anyway, merged! Thanks a ton for working on this!

@orlitzky

This comment has been minimized.

Copy link

orlitzky commented Aug 3, 2018

+1 this was a huge effort, thank you.

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Aug 3, 2018

Thanks @poettering for reviewing this stuff, this was a huge effort too.

I'll fix the SMACK stuff next week.

Cheers.

@fbuihuu fbuihuu deleted the fbuihuu:rfc-tmpfiles-safe-upstream branch Aug 3, 2018

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Aug 6, 2018

@poettering see PR #9802 for the SMACK stuff.

@anarcat

This comment has been minimized.

Copy link

anarcat commented Nov 14, 2018

are there efforts to backport this to previous versions? i am aware of Ubuntu making some work on 229 in https://launchpad.net/ubuntu/+source/systemd/229-4ubuntu21.8 - but what about older releases? I'm working on fixing security issues in Debian jessie's version (215).

@fbuihuu

This comment has been minimized.

Copy link
Contributor

fbuihuu commented Nov 20, 2018

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...

@anarcat

This comment has been minimized.

Copy link

anarcat commented Nov 20, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment