Overwrite removed env strings so they do not leak via /proc/PID/environ

[Abhinavmadake]

clearenv()/unsetenv() only drop pointers; the initial strings stay readable in /proc/PID/environ, and bwrap is pid 1 inside the sandbox. Overwrite them once unreferenced. Tests read /proc/1/environ under --unshare-pid.

Fixes #725

[smcv]

I don't think bubblewrap ever intended it to be a security guarantee that --unsetenv or --clearenv would make the environment variables' values not visible to processes inside the container - I'd always seen it as a way to clear environment variables that are functionally inappropriate for the container (for example when a Flatpak app swaps to a different /usr, it needs to clear the LD_LIBRARY_PATH).

As a general design principle, I would recommend reducing the scope of sensitive environment variables to be as narrow as possible, and preferring to reference a file containing secrets (for example this is how $XAUTHORITY and $DOCKER_CONFIG work) rather than directly putting the secret in the environment variable.

[smcv]

@alexlarsson, @cgwalters: do you think this makes sense as hardening, or do you think it's too much complexity? The implementation here looks reasonable to me at first glance.

[smcv]

cc @swick: same question as above ^

[swick]

While I do think that we should make the host environment inaccessible for anything inside the sandbox, I'm really not a fan of the implementation here. I'm also not really sure if there is a better way. But IIRC (load bearing if), there is an exec somewhere between the "initial" bwrap exec and it being PID 1 inside the sandbox, so that might be a more reasonable point to get rid of the environment?

[smcv]

But IIRC (load bearing if), there is an exec somewhere between the "initial" bwrap exec and it being PID 1 inside the sandbox

Unfortunately, no. After entering the sandbox, bubblewrap can't re-exec itself, because its executable will not usually be visible inside the sandbox - and even if our executable was made visible, the libraries it depends on will not.

In the Flatpak-like use-case with a new pid namespace, we have three processes, something like this:

  |   |   |   `-bwrap,1388669 --args 79 -- gnome-recipes
  |   |   |       `-bwrap,1388684 --args 79 -- gnome-recipes
  |   |   |           `-gnome-recipes,1388685

The grandparent (pid 1388669 here) is the original bwrap process, outside the container.

The parent (pid 1388684 here) is pid 1 in the container. It is this process's environment variables that @Abhinavmadake is trying to hide/remove.

The child (1388685) was forked from what would become pid 1, and then exec'd the final "payload" executable, in this case the gnome-recipes Flatpak app.

[smcv]

In fully-featured container-runners like Docker and Podman, the canonical way to have a pid 1 reaper/init inside the container is to have a statically-linked binary (Podman on Debian uses catatonit by default) which can be bind-mounted read-only into the container and exec'd. Because it's statically-linked, it doesn't have any dependency libraries, so it works.

I'm not sure we really want to be doing that in flatpak/bubblewrap, though?

[smcv]

I'm not sure we really want to be doing that in flatpak/bubblewrap, though?

Thinking about it, we can't do this generically in bubblewrap, because there is no location that we can guarantee to be able to write: the entire filesystem is under the caller's control, and bubblewrap doesn't reserve any locations for its own use.

podman uses /dev/init, but bubblewrap can't do that, because the user might be bind-mounting all of the host /dev (as in flatpak run --device=all) and therefore we can't create a mount point there.

[swick]

Unfortunately, no. After entering the sandbox, bubblewrap can't re-exec itself, because its executable will not usually be visible inside the sandbox - and even if our executable was made visible, the libraries it depends on will not.

I was thinking more about an exec while we still have the host mount namespace. Do we not have to do that anyway once for the user namespace to make us root and be able to set up the other namespaces?

[smcv]

I was thinking more about an exec while we still have the host mount namespace. Do we not have to do that anyway once for the user namespace to make us root and be able to set up the other namespaces?

No, we don't have to re-exec to get (limited) privileges: this isn't setuid or setcap. clone() will allow creating the other namespaces, as long as one of them is the userns (and I think there might be subtle behaviour differences between creating a new user namespace in one clone call and the rest of the namespaces in a subsequent clone call, vs. creating them all in one clone call, but I'm happy to defer to experts like Alex or Colin on this).

We aren't "root", but after the clone() call, we do have elevated capabilities in the new namespace, which is enough to call mount(2) and similar privileged syscalls.

In the example pstree that I pasted above, 1388669 → 1388684 is clone() (raw_clone in the bubblewrap source code), and 1388684 → 1388685 is fork() (just after debug ("forking for child") in the source). We don't exec until right at the end (look for execvp).

Also, if we re-exec'd bubblewrap, then all relevant state from "before" would have to be serialized into a pipe/socket/etc. and sent to the new bubblewrap process to de-serialize; so, another non-trivial parser written in C without the benefit of libraries. I don't think we want that!

And another problem with re-execing bubblewrap in an empty or minimal environment is that some environment variables might be functionally necessary to run it - for example it might be relying on a LD_LIBRARY_PATH to find its own dependency libraries like libcap.

If we do want to scrub the environment that's visible in /proc/1/environ, I think something reasonably similar to what's being done in this PR is probably going to be the least-bad way to achieve it.

One thing that might be nicer would be:

1. Copy all of the environment into a blob in memory, similar to g_get_environ(), perhaps using glibc's envz APIs
2. Overwrite the environment with zeroes, similar to what's done in this PR, but instead of clever filtering that keeps or discards environment variables individually, just clear it completely
3. Wherever we currently use getenv(), instead use envz_get()
4. Wherever we currently use setenv(), etc., instead edit the copy of the environment
5. When we execve() the payload command, give it the (edited) copy of the environment

(But I don't know whether the payload command would be able to bypass this by tracing the pid 1 that is visible to it, and reading its memory...)

[cgwalters]

Are there any valid use cases for bwrap itself using an environment variable but not passing it to a child? ($PATH perhaps?)

IOW what would break if we processed --setenv/--unsetenv/--clearenv very early on in main()? I guess there's likely to be things.

---

I am uncertain, I think perhaps the simplest immediate guidance to give here is "don't pass sensitive environment variables into bwrap at all, i.e. have the thing calling it filter out unnecessary environment variables first"?

[smcv]

As a completely different approach: could we make pid 1 non-dumpable, so that the payload command can't read its environment?

[smcv]

IOW what would break if we processed --setenv/--unsetenv/--clearenv very early on in main()? I guess there's likely to be things.

It doesn't really matter when we process them, the original environment block is still going to be what's visible at kernel level unless you go to heroic lengths to overwrite it (as is done in this PR). Normally setenv(), clearenv(), etc. only allocate new strings and update the environ array, they don't overwrite the block of memory where the kernel provided our inherited environment variables.

I think perhaps the simplest immediate guidance to give here is "don't pass sensitive environment variables into bwrap at all, i.e. have the thing calling it filter out unnecessary environment variables first"

That's what you need to do with all past and present versions of bubblewrap, regardless of whether we change the behaviour of future versions. #725 indicates that (some) users of bubblewrap were surprised by this.

[smcv]

Are there any valid use cases for bwrap itself using an environment variable but not passing it to a child? ($PATH perhaps?)

I think only the new use of DEBUG_INVOCATION for enabling/disabling debug log spam, if enabled at compile time. But I already made it read DEBUG_INVOCATION right at the beginning of main anyway, so that's fine.

In general bwrap doesn't read environment variables for its own purposes, because historically it was sometimes setuid, therefore it couldn't trust its environment to be non-malicious.