From 4d885bd326d958827d926512ecc5e0d21f6ad76b Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Sun, 23 Oct 2016 23:24:14 +0200 Subject: core: first lookup and cache creds then apply them after namespace setup This fixes: https://github.com/systemd/systemd/issues/4357 Let's lookup and cache creds then apply them. We also switch from getgroups() to getgrouplist(). --- src/core/execute.c | 213 ++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 144 insertions(+), 69 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 1b7b4a928d..874f035b2e 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -730,74 +730,146 @@ static int ask_for_confirmation(char *response, char **argv) { return r; } -static int enforce_groups(const ExecContext *context, const char *username, gid_t gid) { - bool keep_groups = false; +static int get_fixed_user(const ExecContext *c, const char **user, + uid_t *uid, gid_t *gid, + const char **home, const char **shell) { int r; + const char *name; - assert(context); + assert(c); - /* Lookup and set GID and supplementary group list. Here too - * we avoid NSS lookups for gid=0. */ + if (!c->user) + return 0; - if (context->group || username) { - /* First step, initialize groups from /etc/groups */ - if (username && gid != 0) { - if (initgroups(username, gid) < 0) - return -errno; + /* Note that we don't set $HOME or $SHELL if they are not particularly enlightening anyway + * (i.e. are "/" or "/bin/nologin"). */ - keep_groups = true; - } + name = c->user; + r = get_user_creds_clean(&name, uid, gid, home, shell); + if (r < 0) + return r; - /* Second step, set our gids */ - if (setresgid(gid, gid, gid) < 0) + *user = name; + return 0; +} + +static int get_fixed_group(const ExecContext *c, const char **group, gid_t *gid) { + int r; + const char *name; + + assert(c); + + if (!c->group) + return 0; + + name = c->group; + r = get_group_creds(&name, gid); + if (r < 0) + return r; + + *group = name; + return 0; +} + +static int get_fixed_supplementary_groups(const ExecContext *c, + const char *user, + const char *group, + gid_t gid, + gid_t **supplementary_gids, int *ngids) { + char **i; + int r, k = 0; + int ngroups_max; + bool keep_groups = false; + gid_t *groups = NULL; + _cleanup_free_ gid_t *l_gids = NULL; + + assert(c); + + if (!c->supplementary_groups) + return 0; + + /* + * If user is given, then lookup GID and supplementary group list. + * We avoid NSS lookups for gid=0. + */ + if (user && gid_is_valid(gid) && gid != 0) { + /* First step, initialize groups from /etc/groups */ + if (initgroups(user, gid) < 0) return -errno; + + keep_groups = true; } - if (context->supplementary_groups) { - int ngroups_max, k; - gid_t *gids; - char **i; + assert_se((ngroups_max = (int) sysconf(_SC_NGROUPS_MAX)) > 0); - /* Final step, initialize any manually set supplementary groups */ - assert_se((ngroups_max = (int) sysconf(_SC_NGROUPS_MAX)) > 0); + l_gids = new(gid_t, ngroups_max); + if (!l_gids) + return -ENOMEM; - if (!(gids = new(gid_t, ngroups_max))) - return -ENOMEM; + if (keep_groups) { + /* + * Lookup the list of groups that the user belongs to, we + * avoid NSS lookups here too for gid=0. + */ + k = ngroups_max; + if (getgrouplist(user, gid, l_gids, &k) < 0) + return -EINVAL; + } else + k = 0; - if (keep_groups) { - k = getgroups(ngroups_max, gids); - if (k < 0) { - free(gids); - return -errno; - } - } else - k = 0; + STRV_FOREACH(i, c->supplementary_groups) { + const char *g; - STRV_FOREACH(i, context->supplementary_groups) { - const char *g; + if (k >= ngroups_max) + return -E2BIG; - if (k >= ngroups_max) { - free(gids); - return -E2BIG; - } + g = *i; + r = get_group_creds(&g, l_gids+k); + if (r < 0) + return r; - g = *i; - r = get_group_creds(&g, gids+k); - if (r < 0) { - free(gids); - return r; - } + k++; + } - k++; - } + /* + * Sets ngids to zero to drop all supplementary groups, happens + * when we are under root and SupplementaryGroups= is empty. + */ + if (k == 0) { + *ngids = 0; + return 0; + } - r = maybe_setgroups(k, gids); - if (r < 0) { - free(gids); + /* Otherwise get the final list of supplementary groups */ + groups = memdup(l_gids, sizeof(gid_t) * k); + if (!groups) + return -ENOMEM; + + *supplementary_gids = groups; + *ngids = k; + + groups = NULL; + + return 0; +} + +static int enforce_groups(const ExecContext *context, gid_t gid, + gid_t *supplementary_gids, int ngids) { + int r; + + assert(context); + + /* Handle SupplementaryGroups= even if it is empty */ + if (context->supplementary_groups) { + r = maybe_setgroups(ngids, supplementary_gids); + if (r < 0) return r; - } + } - free(gids); + if (gid_is_valid(gid)) { + /* Then set our gids */ + if (setresgid(gid, gid, gid) < 0) + return -errno; } return 0; @@ -806,6 +878,9 @@ static int enforce_groups(const ExecContext *context, const char *username, gid_ static int enforce_user(const ExecContext *context, uid_t uid) { assert(context); + if (!uid_is_valid(uid)) + return 0; + /* Sets (but doesn't look up) the uid and make sure we keep the * capabilities while doing so. */ @@ -2175,13 +2250,15 @@ static int exec_child( _cleanup_strv_free_ char **our_env = NULL, **pass_env = NULL, **accum_env = NULL, **final_argv = NULL; _cleanup_free_ char *mac_selinux_context_net = NULL; - const char *username = NULL, *home = NULL, *shell = NULL, *wd; + _cleanup_free_ gid_t *supplementary_gids = NULL; + const char *username = NULL, *groupname = NULL; + const char *home = NULL, *shell = NULL, *wd; dev_t journal_stream_dev = 0; ino_t journal_stream_ino = 0; bool needs_mount_namespace; uid_t uid = UID_INVALID; gid_t gid = GID_INVALID; - int i, r; + int i, r, ngids = 0; assert(unit); assert(command); @@ -2273,26 +2350,23 @@ static int exec_child( username = dcreds->user->name; } else { - if (context->user) { - username = context->user; - r = get_user_creds_clean(&username, &uid, &gid, &home, &shell); - if (r < 0) { - *exit_status = EXIT_USER; - return r; - } - - /* Note that we don't set $HOME or $SHELL if they are not particularly enlightening anyway - * (i.e. are "/" or "/bin/nologin"). */ + r = get_fixed_user(context, &username, &uid, &gid, &home, &shell); + if (r < 0) { + *exit_status = EXIT_USER; + return r; } - if (context->group) { - const char *g = context->group; + r = get_fixed_group(context, &groupname, &gid); + if (r < 0) { + *exit_status = EXIT_GROUP; + return r; + } - r = get_group_creds(&g, &gid); - if (r < 0) { - *exit_status = EXIT_GROUP; - return r; - } + r = get_fixed_supplementary_groups(context, username, groupname, + gid, &supplementary_gids, &ngids); + if (r < 0) { + *exit_status = EXIT_GROUP; + return r; } } @@ -2558,8 +2632,9 @@ static int exec_child( } } + /* Drop group as early as possbile */ if ((params->flags & EXEC_APPLY_PERMISSIONS) && !command->privileged) { - r = enforce_groups(context, username, gid); + r = enforce_groups(context, gid, supplementary_gids, ngids); if (r < 0) { *exit_status = EXIT_GROUP; return r; -- cgit v1.2.3-54-g00ecf From 86b838eaa36814f1a9c02f3289328cd0ec42d1ff Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Fri, 14 Oct 2016 10:32:27 +0200 Subject: test: Add simple test for supplementary groups --- Makefile.am | 1 + src/test/test-execute.c | 5 +++++ test/test-execute/exec-supplementarygroups.service | 7 +++++++ 3 files changed, 13 insertions(+) create mode 100644 test/test-execute/exec-supplementarygroups.service diff --git a/Makefile.am b/Makefile.am index 07acce347e..0debd38964 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1627,6 +1627,7 @@ EXTRA_DIST += \ test/test-execute/exec-passenvironment.service \ test/test-execute/exec-group.service \ test/test-execute/exec-group-nfsnobody.service \ + test/test-execute/exec-supplementarygroups.service \ test/test-execute/exec-ignoresigpipe-no.service \ test/test-execute/exec-ignoresigpipe-yes.service \ test/test-execute/exec-personality-x86-64.service \ diff --git a/src/test/test-execute.c b/src/test/test-execute.c index e8ff02adaf..cda035c6e7 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -218,6 +218,10 @@ static void test_exec_group(Manager *m) { log_error_errno(errno, "Skipping test_exec_group, could not find nobody/nfsnobody group: %m"); } +static void test_exec_supplementary_groups(Manager *m) { + test(m, "exec-supplementarygroups.service", 0, CLD_EXITED); +} + static void test_exec_environment(Manager *m) { test(m, "exec-environment.service", 0, CLD_EXITED); test(m, "exec-environment-multiple.service", 0, CLD_EXITED); @@ -390,6 +394,7 @@ int main(int argc, char *argv[]) { test_exec_systemcallerrornumber, test_exec_user, test_exec_group, + test_exec_supplementary_groups, test_exec_environment, test_exec_environmentfile, test_exec_passenvironment, diff --git a/test/test-execute/exec-supplementarygroups.service b/test/test-execute/exec-supplementarygroups.service new file mode 100644 index 0000000000..43a9a981f2 --- /dev/null +++ b/test/test-execute/exec-supplementarygroups.service @@ -0,0 +1,7 @@ +[Unit] +Description=Test for Supplementary Group + +[Service] +ExecStart=/bin/sh -x -c 'test "$$(id -G)" = "0 1"' +Type=oneshot +SupplementaryGroups=1 -- cgit v1.2.3-54-g00ecf From bf9ace96fc764fb6c795790a5febade4689f9aba Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Mon, 17 Oct 2016 10:06:18 +0200 Subject: test: add more tests for SupplementaryGroups= --- Makefile.am | 2 ++ src/test/test-execute.c | 2 ++ .../exec-supplementarygroups-single-group-user.service | 9 +++++++++ test/test-execute/exec-supplementarygroups-single-group.service | 8 ++++++++ 4 files changed, 21 insertions(+) create mode 100644 test/test-execute/exec-supplementarygroups-single-group-user.service create mode 100644 test/test-execute/exec-supplementarygroups-single-group.service diff --git a/Makefile.am b/Makefile.am index 0debd38964..c49edef7b2 100644 --- a/Makefile.am +++ b/Makefile.am @@ -1628,6 +1628,8 @@ EXTRA_DIST += \ test/test-execute/exec-group.service \ test/test-execute/exec-group-nfsnobody.service \ test/test-execute/exec-supplementarygroups.service \ + test/test-execute/exec-supplementarygroups-single-group.service \ + test/test-execute/exec-supplementarygroups-single-group-user.service \ test/test-execute/exec-ignoresigpipe-no.service \ test/test-execute/exec-ignoresigpipe-yes.service \ test/test-execute/exec-personality-x86-64.service \ diff --git a/src/test/test-execute.c b/src/test/test-execute.c index cda035c6e7..1254ef8a05 100644 --- a/src/test/test-execute.c +++ b/src/test/test-execute.c @@ -220,6 +220,8 @@ static void test_exec_group(Manager *m) { static void test_exec_supplementary_groups(Manager *m) { test(m, "exec-supplementarygroups.service", 0, CLD_EXITED); + test(m, "exec-supplementarygroups-single-group.service", 0, CLD_EXITED); + test(m, "exec-supplementarygroups-single-group-user.service", 0, CLD_EXITED); } static void test_exec_environment(Manager *m) { diff --git a/test/test-execute/exec-supplementarygroups-single-group-user.service b/test/test-execute/exec-supplementarygroups-single-group-user.service new file mode 100644 index 0000000000..ed6276d303 --- /dev/null +++ b/test/test-execute/exec-supplementarygroups-single-group-user.service @@ -0,0 +1,9 @@ +[Unit] +Description=Test for Supplementary Group with only one group and uid 1 + +[Service] +ExecStart=/bin/sh -x -c 'test "$$(id -G)" = "1" && test "$$(id -g)" = "1" && test "$$(id -u)" = "1"' +Type=oneshot +User=1 +Group=1 +SupplementaryGroups=1 diff --git a/test/test-execute/exec-supplementarygroups-single-group.service b/test/test-execute/exec-supplementarygroups-single-group.service new file mode 100644 index 0000000000..ee502b3d37 --- /dev/null +++ b/test/test-execute/exec-supplementarygroups-single-group.service @@ -0,0 +1,8 @@ +[Unit] +Description=Test for Supplementary Group with only one group + +[Service] +ExecStart=/bin/sh -x -c 'test "$$(id -G)" = "1" && test "$$(id -g)" = "1" && test "$$(id -u)" = "0"' +Type=oneshot +Group=1 +SupplementaryGroups=1 -- cgit v1.2.3-54-g00ecf From 8b6903ad4d0dc94cd0098f453a4ea8ab24a4a3f7 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Fri, 21 Oct 2016 22:22:56 +0200 Subject: core: lets move the setup of working directory before group enforce This is minor but lets try to split and move bit by bit cgroups and portable environment setup before applying the security context. --- src/core/execute.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/core/execute.c b/src/core/execute.c index 874f035b2e..a9b2b8f299 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -2632,6 +2632,13 @@ static int exec_child( } } + if (context->working_directory_home) + wd = home; + else if (context->working_directory) + wd = context->working_directory; + else + wd = "/"; + /* Drop group as early as possbile */ if ((params->flags & EXEC_APPLY_PERMISSIONS) && !command->privileged) { r = enforce_groups(context, gid, supplementary_gids, ngids); @@ -2641,13 +2648,6 @@ static int exec_child( } } - if (context->working_directory_home) - wd = home; - else if (context->working_directory) - wd = context->working_directory; - else - wd = "/"; - if (params->flags & EXEC_APPLY_CHROOT) { if (!needs_mount_namespace && context->root_directory) if (chroot(context->root_directory) < 0) { -- cgit v1.2.3-54-g00ecf