Nixpkgs: buildFHSUserEnv: Create a setuid wrapper

[spacekitteh]

Required in order to work on grsecurity.

related: #21327 and #19047

[Mic92]

Who ever wants to implement must be very clear about the consequence of writing setuid applications in general: http://docstore.mik.ua/orelly/networking/puis/ch23_04.htm
and about the consequence for mount namespaces in particular:
While this is safe in user namespaces:

CHROOTENV_EXTRA_BINDS=/tmp/shadow:/etc/shadow chroot-user.rb

It would become a privilege escalation when a setuid wrapper is used.
Having that said, I would stick to the current approach by default, if the running kernel support it.

[abbradar]

This is indeed very difficult to write, as there are a lot of potential things going on (what about symlinks? applications which trust /bin contents? bind-mounts, like @Mic92 mentioned)? I, for one, don't consider myself qualified to audit all potential security consequences. I'm not even sure whom to ask in our community -- I don't think we have something this critical in terms of required audit that was written for NixOS specifically. The worst we have now are setuid-wrappers that do orders of magnitude less.

[abbradar]

Actually, I think that if we make open-world assumption (that is, that anyone can run their environments) then this is impossible to accomplish because /bin, /etc and others are inherently trusted.

Because those environments require userns to have at least some security in the open world, making them setuid would essentially fully expose this API (I think you can do almost anything in our userenv that you want to do in your own userns). Therefore it defeats the security measure that grsec introduces (that is, no userns for regular users).

So, if one one wants buildFHSUserEnv to work I think it's reasonable to expect them to understand consequences, that is, to fully disable this measure in their kernel instead of relying on setuid (which can give one a false impression that it's more "secure").

What do you think?

[abbradar]

Let's close this -- if someone believes that setuid wrapper may still be useful in some form, feel free to reopen and describe your idea!

[joachifm]

Is it not possible to implement a limited stub that only sets up the userns or whatever, such that any operation that can be influenced by the caller happens after privs are dropped?

[abbradar]

We can do that, my point was that this gives regular users capability to circumvent the protection in grsecurity and do practically anything userns has to offer (yes, they don't do syscalls themselves but they still can run arbitrary environments inside so if there is a vulnerability in userns' security checks it most likely could be exploited that way). Therefore, to make implications clear for the user and do this in more "clean" way (disable things, not circumvent them) it seems better to me to re-enable this in kernel instead (potentially with a patch/kernel rebuild).

[joachifm]

I disagree that it would be "circumventing" grsec. Because installing a suid root binary is a privileged operation, I see it as being in keeping with the idea of userns being privileged, in the sense that it requires deliberate (and, one hopes, informed) intervention by the admin.

Having briefly read the chroot-user-env thing, it's unclear whether userns are for anything other than gaining the ability to setup bind mounts.

[joachifm]

Anyway, this comes down to whether unprivileged userns is considered a hard requirement for this feature; If it is, then grsec users will just have to do without it. In the end that's up to you, being the maintainer, so I'll leave it at that :)

[abbradar]

Yes, I agree that installation of setuid binary is a privileged operation, however that feels like "fixing the problem in the wrong place" to me as it exposes pretty much the entire surface of userns to the user. Why not just enable it in kernel then?

Userns are needed for chroot itself, too, and processes inside are running in this userns. That said, privileged userns would do too (so it's theoretically possible to write a not-less-secure setuid wrapper).

From a brief look at grsecurity kernel patches we can trivially re-allow non-privileged userns with this:

diff -ru3 linux-4.8.15-old/kernel/user_namespace.c linux-4.8.15/kernel/user_namespace.c
--- linux-4.8.15-old/kernel/user_namespace.c    2016-12-30 13:35:52.958240267 +0300
+++ linux-4.8.15/kernel/user_namespace.c    2016-12-30 13:36:29.609197139 +0300
@@ -84,21 +84,6 @@
        !kgid_has_mapping(parent_ns, group))
        return -EPERM;

-#ifdef CONFIG_GRKERNSEC
-   /*
-    * This doesn't really inspire confidence:
-    * http://marc.info/?l=linux-kernel&m=135543612731939&w=2
-    * http://marc.info/?l=linux-kernel&m=135545831607095&w=2
-    * Increases kernel attack surface in areas developers
-    * previously cared little about ("low importance due
-    * to requiring "root" capability")
-    * To be removed when this code receives *proper* review
-    */
-   if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SETUID) ||
-           !capable(CAP_SETGID))
-       return -EPERM;
-#endif
-
    ns = kmem_cache_zalloc(user_ns_cachep, GFP_KERNEL);
    if (!ns)
        return -ENOMEM;

This patch could be hidden behind a scary-looking switch but it seems a more proper solution to me (if my understanding that we would already expose nearly all attack surface with the setuid wrapper anyway is correct). What do you think?

[joachifm]

I'd still prefer a minimal stub to be used only for a specific purpose over making userns completely unprivileged. This is not a feature I rely on or feel confident in implementing, so I'm happy to let it remain wontfix. Maintaining patches ontop of grsec, however innocuous, I'll leave to somebody else :)

[abbradar]

Let's leave it as is then. For the record: I don't believe that stub is an inherently bad idea, but that it's much more hard to write securely than to disable this restriction altogether while exposing nearly the same things. In other words, if someone writes such a stub and claims that it's secure (and we agree) I'll merge.

Nikolay.

[spacekitteh]

:(

On Fri, 30 Dec 2016 at 21:28 Nikolay Amiantov [e-mail]
wrote:

Let's leave it as is then. For the record: I don't believe that stub is an
inherently bad idea, but that it's much more hard to write securely than to
disable this restriction altogether while exposing nearly the same things.
In other words, if someone writes such a stub and claims that it's secure

(and we agree) I'll merge.

Nikolay.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/NixOS/nixpkgs/issues/21387#issuecomment-269761269,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB2crPcYs9zuWzSZx1ti5ZSivn-6lO9Fks5rNOrWgaJpZM4LVKXx
.

[spacekitteh]

But disabling it entirely disables it for every process, whether or not it's for FHS envs. Surely a stub would only present a security risk for things running inside the FHS env?

[abbradar]

It's not much difference in terms of attack surface (again, in my opinion -- that is, if I rightly understand where the most security risk comes from with userns) -- one only needs now to perform the attack via blessed executable instead of their own.

[spacekitteh]

But surely, since it would only be a stub program which sets up the
namespace and exits, AND we control the executable, the risk is gone...?

On Sun, 1 Jan 2017 at 21:10 Nikolay Amiantov [e-mail]
wrote:

It's not much difference in terms of attack surface (again, in my opinion
-- that is, if I rightly understand where the most security risk comes from
with userns) -- one only needs now to perform the attack via blessed
executable instead of their own.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/NixOS/nixpkgs/issues/21387#issuecomment-269899100,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AB2crAoWUVcW7K217z3KNxhGPlvxZ27Nks5rN4mTgaJpZM4LVKXx
.

[joachifm]

@spacekitteh I think the idea is that, once the user namespace is in effect, you may happen upon code paths where the checking logic gets confused and lets you do things with elevated privilege that affects resources/whatever in the parent namespace. As long as you need to acquire sys_admin first, getting onto that path is a little harder (and you then don't even strictly need it, because sys_admin lets you do anything anyway).

[joachifm]

Now, the above presumes that the stub absolutely needs to run the target process within the user namespace. Even then, I think a stub is superior, because 1) it can be toggled by the admin without rebooting/switching kernels & so is much more convenient; 2) you can control who gets to use the stub via policy (at the process level), which ought to somewhat mitigate the issue.

[spacekitteh]

Are there similar stubs which set up a chroot and then drop privileges already in nixpkgs?

[abbradar]

@joachifm described this right, the main danger is that access rights checking may be confused. The stub really requires to run in a userns, because without it it's even worse -- it's essentially chroot() as a regular user with all its potential flaws. Again, stub is not _less_ secure than the kernel patch and is indeed more convenient -- _when written correctly_.

@spacekitteh Sadly no AFAIK -- if there were it would be much simpler to accomplish this.