From 5c3bde3fa8613e09e694198862ea9038566af422 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Fri, 27 Nov 2015 22:24:33 -0500 Subject: journal: move the gist of server_fix_perms to acl-util.[hc] Most of the function is moved to acl-util.c to make it possible to add tests in subsequent commit. Setting of the mode in server_fix_perms is removed: - we either just created the file ourselves, and the permission be better right, - or the file was already there, and we should not modify the permissions. server_fix_perms is renamed to server_fix_acls to better reflect new meaning, and made static because it is only used in one file. --- src/journal/journald-server.c | 57 +++++++------------------------------------ src/journal/journald-server.h | 1 - src/shared/acl-util.c | 31 +++++++++++++++++++++++ src/shared/acl-util.h | 1 + 4 files changed, 41 insertions(+), 49 deletions(-) (limited to 'src') diff --git a/src/journal/journald-server.c b/src/journal/journald-server.c index 7d11a568aa..97c5820c75 100644 --- a/src/journal/journald-server.c +++ b/src/journal/journald-server.c @@ -204,58 +204,19 @@ static int determine_space(Server *s, bool verbose, bool patch_min_use, uint64_t return determine_space_for(s, metrics, path, name, verbose, patch_min_use, available, limit); } -void server_fix_perms(Server *s, JournalFile *f, uid_t uid) { - int r; +static void server_add_acls(JournalFile *f, uid_t uid) { #ifdef HAVE_ACL - _cleanup_(acl_freep) acl_t acl = NULL; - acl_entry_t entry; - acl_permset_t permset; + int r; #endif - assert(f); - r = fchmod(f->fd, 0640); - if (r < 0) - log_warning_errno(errno, "Failed to fix access mode on %s, ignoring: %m", f->path); - #ifdef HAVE_ACL if (uid <= SYSTEM_UID_MAX) return; - acl = acl_get_fd(f->fd); - if (!acl) { - log_warning_errno(errno, "Failed to read ACL on %s, ignoring: %m", f->path); - return; - } - - r = acl_find_uid(acl, uid, &entry); - if (r <= 0) { - - if (acl_create_entry(&acl, &entry) < 0 || - acl_set_tag_type(entry, ACL_USER) < 0 || - acl_set_qualifier(entry, &uid) < 0) { - log_warning_errno(errno, "Failed to patch ACL on %s, ignoring: %m", f->path); - return; - } - } - - /* We do not recalculate the mask unconditionally here, - * so that the fchmod() mask above stays intact. */ - if (acl_get_permset(entry, &permset) < 0 || - acl_add_perm(permset, ACL_READ) < 0) { - log_warning_errno(errno, "Failed to patch ACL on %s, ignoring: %m", f->path); - return; - } - - r = calc_acl_mask_if_needed(&acl); - if (r < 0) { - log_warning_errno(r, "Failed to patch ACL on %s, ignoring: %m", f->path); - return; - } - - if (acl_set_fd(f->fd, acl) < 0) - log_warning_errno(errno, "Failed to set ACL on %s, ignoring: %m", f->path); - + r = add_acls_for_user(f->fd, uid); + if (r < 0) + log_warning_errno(r, "Failed to set ACL on %s, ignoring: %m", f->path); #endif } @@ -301,7 +262,7 @@ static JournalFile* find_journal(Server *s, uid_t uid) { if (r < 0) return s->system_journal; - server_fix_perms(s, f, uid); + server_add_acls(f, uid); r = ordered_hashmap_put(s->user_journals, UID_TO_PTR(uid), f); if (r < 0) { @@ -332,7 +293,7 @@ static int do_rotate( else log_error_errno(r, "Failed to create new %s journal: %m", name); else - server_fix_perms(s, *f, uid); + server_add_acls(*f, uid); return r; } @@ -971,7 +932,7 @@ static int system_journal_open(Server *s, bool flush_requested) { fn = strjoina(fn, "/system.journal"); r = journal_file_open_reliably(fn, O_RDWR|O_CREAT, 0640, s->compress, s->seal, &s->system_metrics, s->mmap, NULL, &s->system_journal); if (r >= 0) { - server_fix_perms(s, s->system_journal, 0); + server_add_acls(s->system_journal, 0); (void) determine_space_for(s, &s->system_metrics, "/var/log/journal/", "System journal", true, true, NULL, NULL); } else if (r < 0) { if (r != -ENOENT && r != -EROFS) @@ -1015,7 +976,7 @@ static int system_journal_open(Server *s, bool flush_requested) { } if (s->runtime_journal) { - server_fix_perms(s, s->runtime_journal, 0); + server_add_acls(s->runtime_journal, 0); (void) determine_space_for(s, &s->runtime_metrics, "/run/log/journal/", "Runtime journal", true, true, NULL, NULL); } } diff --git a/src/journal/journald-server.h b/src/journal/journald-server.h index dcc21bb7c3..1822765228 100644 --- a/src/journal/journald-server.h +++ b/src/journal/journald-server.h @@ -174,7 +174,6 @@ int config_parse_split_mode(const char *unit, const char *filename, unsigned lin const char *split_mode_to_string(SplitMode s) _const_; SplitMode split_mode_from_string(const char *s) _pure_; -void server_fix_perms(Server *s, JournalFile *f, uid_t uid); int server_init(Server *s); void server_done(Server *s); void server_sync(Server *s); diff --git a/src/shared/acl-util.c b/src/shared/acl-util.c index 35f2e1b67d..9f3b1ff51c 100644 --- a/src/shared/acl-util.c +++ b/src/shared/acl-util.c @@ -398,3 +398,34 @@ int acls_for_file(const char *path, acl_type_t type, acl_t new, acl_t *acl) { old = NULL; return 0; } + +int add_acls_for_user(int fd, uid_t uid) { + _cleanup_(acl_freep) acl_t acl = NULL; + acl_entry_t entry; + acl_permset_t permset; + int r; + + acl = acl_get_fd(fd); + if (!acl) + return -errno; + + r = acl_find_uid(acl, uid, &entry); + if (r <= 0) { + if (acl_create_entry(&acl, &entry) < 0 || + acl_set_tag_type(entry, ACL_USER) < 0 || + acl_set_qualifier(entry, &uid) < 0) + return -errno; + } + + /* We do not recalculate the mask unconditionally here, + * so that the fchmod() mask above stays intact. */ + if (acl_get_permset(entry, &permset) < 0 || + acl_add_perm(permset, ACL_READ) < 0) + return -errno; + + r = calc_acl_mask_if_needed(&acl); + if (r < 0) + return r; + + return acl_set_fd(fd, acl); +} diff --git a/src/shared/acl-util.h b/src/shared/acl-util.h index 256a6a5900..1d7f45e2a8 100644 --- a/src/shared/acl-util.h +++ b/src/shared/acl-util.h @@ -35,6 +35,7 @@ int add_base_acls_if_needed(acl_t *acl_p, const char *path); int acl_search_groups(const char* path, char ***ret_groups); int parse_acl(const char *text, acl_t *acl_access, acl_t *acl_default, bool want_mask); int acls_for_file(const char *path, acl_type_t type, acl_t new, acl_t *acl); +int add_acls_for_user(int fd, uid_t uid); /* acl_free takes multiple argument types. * Multiple cleanup functions are necessary. */ -- cgit v1.2.3-54-g00ecf From 5bb5b236fe8c663b7d4db5ccaf3e3e7942bf6abd Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sat, 28 Nov 2015 00:04:40 -0500 Subject: test-acl-util: add new test For now, only add_acls_for_user is tested. When run under root, it actually sets the acls. When run under non-root, it sets the acls for the user, which does nothing, but at least calls the functions. --- .gitignore | 1 + Makefile.am | 11 +++++++ src/test/test-acl-util.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 97 insertions(+) create mode 100644 src/test/test-acl-util.c (limited to 'src') diff --git a/.gitignore b/.gitignore index 98eb29d657..c400fea684 100644 --- a/.gitignore +++ b/.gitignore @@ -133,6 +133,7 @@ /systemd-vconsole-setup /tags /test-acd +/test-acl-util /test-af-list /test-architecture /test-arphrd-list diff --git a/Makefile.am b/Makefile.am index db206ae13e..cecd139e4f 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1499,6 +1499,11 @@ tests += \ test-dns-domain \ test-install-root +if HAVE_ACL +tests += \ + test-acl-util +endif + EXTRA_DIST += \ test/a.service \ test/basic.target \ @@ -1846,6 +1851,12 @@ test_install_root_SOURCES = \ test_install_root_LDADD = \ libshared.la +test_acl_util_SOURCES = \ + src/test/test-acl-util.c + +test_acl_util_LDADD = \ + libshared.la + test_namespace_LDADD = \ libcore.la diff --git a/src/test/test-acl-util.c b/src/test/test-acl-util.c new file mode 100644 index 0000000000..bdea04d7f0 --- /dev/null +++ b/src/test/test-acl-util.c @@ -0,0 +1,85 @@ +/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/ + +/*** + This file is part of systemd. + + Copyright 2015 Zbigniew Jędrzejewski-Szmek + + systemd is free software; you can redistribute it and/or modify it + under the terms of the GNU Lesser General Public License as published by + the Free Software Foundation; either version 2.1 of the License, or + (at your option) any later version. + + systemd is distributed in the hope that it will be useful, but + WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public License + along with systemd; If not, see . +***/ + +#include +#include +#include +#include + +#include "acl-util.h" +#include "fd-util.h" +#include "fileio.h" +#include "string-util.h" +#include "user-util.h" + +static void test_add_acls_for_user(void) { + char fn[] = "/tmp/test-empty.XXXXXX"; + _cleanup_close_ int fd = -1; + char *cmd; + uid_t uid; + int r; + + fd = mkostemp_safe(fn, O_RDWR|O_CLOEXEC); + assert_se(fd >= 0); + + /* Use the mode that user journal files use */ + assert(fchmod(fd, 0640) == 0); + + cmd = strjoina("ls -l ", fn); + assert_se(system(cmd) == 0); + + cmd = strjoina("getfacl -p ", fn); + assert_se(system(cmd) == 0); + + if (getuid() == 0) { + const char *nobody = "nobody"; + r = get_user_creds(&nobody, &uid, NULL, NULL, NULL); + if (r < 0) + uid = 0; + } else + uid = getuid(); + + r = add_acls_for_user(fd, uid); + assert_se(r >= 0); + + cmd = strjoina("ls -l ", fn); + assert_se(system(cmd) == 0); + + cmd = strjoina("getfacl -p ", fn); + assert_se(system(cmd) == 0); + + /* set the acls again */ + + r = add_acls_for_user(fd, uid); + assert_se(r >= 0); + + cmd = strjoina("ls -l ", fn); + assert_se(system(cmd) == 0); + + cmd = strjoina("getfacl -p ", fn); + assert_se(system(cmd) == 0); + + unlink(fn); +} + +int main(int argc, char **argv) { + test_add_acls_for_user(); +} -- cgit v1.2.3-54-g00ecf From 6debb3982612b1fce9b2dd878bad07fe5ae9c0a9 Mon Sep 17 00:00:00 2001 From: Zbigniew Jędrzejewski-Szmek Date: Sat, 28 Nov 2015 18:41:08 -0500 Subject: acl-util: only set the mask if not present MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When we have non-owner user or group entries, we need the mask for the acl to be valid. But acl_calc_mask() calculates the mask to include all permissions, even those that were masked before. Apparently this happens when we inherit *:r-x permissions from a parent directory — the kernel sets *:r-x, mask:r--, effectively masking the executable bit. acl_calc_mask() would set the mask:r-x, effectively enabling the bit. To avoid this, be more conservative when to add the mask entry: first iterate over all entries, and do nothing if a mask. This returns the code closer to J.A.Steffens' original version in v204-90-g23ad4dd884. Should fix https://github.com/systemd/systemd/issues/1977. --- src/shared/acl-util.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'src') diff --git a/src/shared/acl-util.c b/src/shared/acl-util.c index 9f3b1ff51c..b4028564c2 100644 --- a/src/shared/acl-util.c +++ b/src/shared/acl-util.c @@ -71,6 +71,7 @@ int acl_find_uid(acl_t acl, uid_t uid, acl_entry_t *entry) { int calc_acl_mask_if_needed(acl_t *acl_p) { acl_entry_t i; int r; + bool need = false; assert(acl_p); @@ -85,17 +86,16 @@ int calc_acl_mask_if_needed(acl_t *acl_p) { if (tag == ACL_MASK) return 0; - if (IN_SET(tag, ACL_USER, ACL_GROUP)) { - if (acl_calc_mask(acl_p) < 0) - return -errno; - - return 1; - } + if (IN_SET(tag, ACL_USER, ACL_GROUP)) + need = true; } if (r < 0) return -errno; - return 0; + if (need && acl_calc_mask(acl_p) < 0) + return -errno; + + return need; } int add_base_acls_if_needed(acl_t *acl_p, const char *path) { -- cgit v1.2.3-54-g00ecf