Revert "Mount all fs nosuid when NoNewPrivileges=yes"

This reverts commit d8e3c31bd8.

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.
This commit is contained in:
Topi Miettinen 2021-06-12 09:35:06 +03:00 committed by Yu Watanabe
parent 2fbb5df8e9
commit 1753d30215
4 changed files with 3 additions and 39 deletions

View file

@ -675,10 +675,9 @@ CapabilityBoundingSet=~CAP_B CAP_C</programlisting>
<varname>SystemCallArchitectures=</varname>,
<varname>SystemCallFilter=</varname>, or
<varname>SystemCallLog=</varname> are specified. Note that even if this setting is overridden
by them, <command>systemctl show</command> 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 <ulink url="https://www.kernel.org/doc/html/latest/userspace-api/no_new_privs.html">
No New Privileges Flag</ulink>.</para></listitem>
by them, <command>systemctl show</command> shows the original value of this setting. Also see
<ulink url="https://www.kernel.org/doc/html/latest/userspace-api/no_new_privs.html">No New
Privileges Flag</ulink>.</para></listitem>
</varlistentry>
<varlistentry>

View file

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

View file

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

View file

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