Re: [PATCH v5 1/7] proc: add proc_fs_info struct to store proc information

On Fri, May 11, 2018 at 03:49:13PM +0200, Jann Horn wrote:

On Fri, May 11, 2018 at 11:34 AM, Alexey Gladkov
[off-list ref] wrote:

quoted

From: Djalal Harouni <redacted>

This is a preparation patch that adds proc_fs_info to be able to store
different procfs options and informations. Right now some mount options
are stored inside the pid namespace which makes it hard to change or
modernize procfs without affecting pid namespaces. Plus we do want to
treat proc as more of a real mount point and filesystem. procfs is part
of Linux API where it offers some features using filesystem syscalls and
in order to support some features where we are able to have multiple
instances of procfs, each one with its mount options inside the same pid
namespace, we have to separate these procfs instances.

This is the same feature that was also added to other Linux interfaces
like devpts in order to support containers, sandboxes, and to have
multiple instances of devpts filesystem [1].

[1] http://lxr.free-electrons.com/source/Documentation/filesystems/devpts.txt?v=3.14

Cc: Kees Cook <redacted>
Suggested-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Djalal Harouni <redacted>
Signed-off-by: Alexey Gladkov <redacted>
---

[...]

quoted

 static struct dentry *proc_mount(struct file_system_type *fs_type,
        int flags, const char *dev_name, void *data)
 {
+       int error;
+       struct super_block *sb;
        struct pid_namespace *ns;
+       struct proc_fs_info *fs_info;
+
+       /*
+        * Don't allow mounting unless the caller has CAP_SYS_ADMIN over
+        * the namespace.
+        */
+       if (!(flags & MS_KERNMOUNT) && !ns_capable(current_user_ns(), CAP_SYS_ADMIN))
+               return ERR_PTR(-EPERM);

Is this correct?

The old code invoked a check with the same comment through mount_ns();
however, this patch changes the semantics of the check.
The old code checked that the caller has privileges over the user
namespace that contains the PID namespace; in other words, it checked
that the caller has privileges over the PID namespace. The current
code just checks that the caller is privileged over its own user
namespace.

As far as I can tell, this means that by doing something like this:

    unshare(CLONE_NEWNS|CLONE_NEWUSER);
    mount("none", "/", NULL, MS_REC|MS_PRIVATE, NULL);
    mount("proc", "/proc", "proc", 0, "newinstance,pids=all");

any process could create a new unrestricted procfs mount for its PID
namespace, even if it is only supposed to have access to a more
restricted procfs mount.

Hm... let me investigate this. It looks like mount with "newinstance"
option should fail if pid namespace is the same and the current and parent
user namespace do not match.

-- 
Rgrds, legion