From 1753d3021564671fba3d3196a84da657d15fb632 Mon Sep 17 00:00:00 2001 From: Topi Miettinen Date: Sat, 12 Jun 2021 09:35:06 +0300 Subject: [PATCH] Revert "Mount all fs nosuid when NoNewPrivileges=yes" This reverts commit d8e3c31bd8e307c8defc759424298175aa0f7001. A poorly documented fact is that SELinux unfortunately uses nosuid mount flag to specify that also a fundamental feature of SELinux, domain transitions, must not be allowed either. While this could be mitigated case by case by changing the SELinux policy to use `nosuid_transition`, such mitigations would probably have to be added everywhere if systemd used automatic nosuid mount flags when `NoNewPrivileges=yes` would be implied. This isn't very desirable from SELinux policy point of view since also untrusted mounts in service's mount namespaces could start triggering domain transitions. Alternatively there could be directives to override this behavior globally or for each service (for example, new directives `SUIDPaths=`/`NoSUIDPaths=` or more generic mount flag applicators), but since there's little value of the commit by itself (setting NNP already disables most setuid functionality), it's simpler to revert the commit. Such new directives could be used to implement the original goal. --- man/systemd.exec.xml | 7 +++---- src/core/execute.c | 2 -- src/core/namespace.c | 32 -------------------------------- src/core/namespace.h | 1 - 4 files changed, 3 insertions(+), 39 deletions(-) diff --git a/man/systemd.exec.xml b/man/systemd.exec.xml index 96d18dd93bd..893b56d93ad 100644 --- a/man/systemd.exec.xml +++ b/man/systemd.exec.xml @@ -675,10 +675,9 @@ CapabilityBoundingSet=~CAP_B CAP_C SystemCallArchitectures=, SystemCallFilter=, or SystemCallLog= are specified. Note that even if this setting is overridden - by them, systemctl show shows the original value of this setting. In case the - service will be run in a new mount namespace anyway, all file systems are mounted with MS_NOSUID - flag. Also see - No New Privileges Flag. + by them, systemctl show shows the original value of this setting. Also see + No New + Privileges Flag. diff --git a/src/core/execute.c b/src/core/execute.c index 5c958b327b9..5bb41db8882 100644 --- a/src/core/execute.c +++ b/src/core/execute.c @@ -3189,8 +3189,6 @@ static int apply_mount_namespace( .protect_proc = context->protect_proc, .proc_subset = context->proc_subset, .private_ipc = context->private_ipc || context->ipc_namespace_path, - /* If NNP is on, we can turn on MS_NOSUID, since it won't have any effect anymore. */ - .mount_nosuid = context->no_new_privileges, }; } else if (!context->dynamic_user && root_dir) /* diff --git a/src/core/namespace.c b/src/core/namespace.c index 71fc73b9d30..6d77ce99674 100644 --- a/src/core/namespace.c +++ b/src/core/namespace.c @@ -1464,27 +1464,6 @@ static int make_noexec(const MountEntry *m, char **deny_list, FILE *proc_self_mo return 0; } -static int make_nosuid(const MountEntry *m, FILE *proc_self_mountinfo) { - bool submounts = false; - int r = 0; - - assert(m); - assert(proc_self_mountinfo); - - submounts = !IN_SET(m->mode, EMPTY_DIR, TMPFS); - - if (submounts) - r = bind_remount_recursive_with_mountinfo(mount_entry_path(m), MS_NOSUID, MS_NOSUID, NULL, proc_self_mountinfo); - else - r = bind_remount_one_with_mountinfo(mount_entry_path(m), MS_NOSUID, MS_NOSUID, proc_self_mountinfo); - if (r == -ENOENT && m->ignore) - return 0; - if (r < 0) - return log_debug_errno(r, "Failed to re-mount '%s'%s: %m", mount_entry_path(m), - submounts ? " and its submounts" : ""); - return 0; -} - static bool namespace_info_mount_apivfs(const NamespaceInfo *ns_info) { assert(ns_info); @@ -1681,17 +1660,6 @@ static int apply_mounts( } } - /* Fourth round, flip the nosuid bits without a deny list. */ - if (ns_info->mount_nosuid) - for (MountEntry *m = mounts; m < mounts + *n_mounts; ++m) { - r = make_nosuid(m, proc_self_mountinfo); - if (r < 0) { - if (error_path && mount_entry_path(m)) - *error_path = strdup(mount_entry_path(m)); - return r; - } - } - return 1; } diff --git a/src/core/namespace.h b/src/core/namespace.h index c9373a4adb1..737d6eae8b1 100644 --- a/src/core/namespace.h +++ b/src/core/namespace.h @@ -74,7 +74,6 @@ struct NamespaceInfo { bool mount_apivfs; bool protect_hostname; bool private_ipc; - bool mount_nosuid; ProtectHome protect_home; ProtectSystem protect_system; ProtectProc protect_proc;