Don't create our own temporary mount point for pivot_root (#304)

[smcv]

An attacker could pre-create /tmp/.bubblewrap-$UID and make it a
non-directory, non-symlink (in which case mounting our tmpfs would fail,
causing denial of service), or make it a symlink under their control
(potentially allowing bad things if the protected_symlinks sysctl is
not enabled).

Instead, temporarily mount the tmpfs on a directory that we are sure
exists and is not attacker-controlled. We already rely on /proc to have
those properties, so it seems as good a place as any. This doesn't appear
to have any impact on our ability to use /proc as either the source or
the destination of a bind-mount.

Fixes: #304
Bug-Debian: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=923557

[cgwalters]

I'd be curious to know how the reporter got in a situation without /run.

Hmm but if we do this it breaks /proc from the bwrap process' perspective until we do the final pivot right?

Seems to me like it's safer to just use mkdtemp.

[cgwalters]

And I still like using /run if we can since that's a known-safe quantity; so the mkdtemp fallback would be just if we couldn't use /run or so?

[smcv]

I'd be curious to know how the reporter got in a situation without /run.

I assume the reporter has at least one system that doesn't have systemd-logind, or is running bubblewrap as a uid that isn't in a logind session.

Hmm but if we do this it breaks /proc from the bwrap process' perspective until we do the final pivot right?

Probably, but we keep a fd to /proc open for the duration of the various pivots already, which we have to do because our root directory keeps changing.

Seems to me like it's safer to just use mkdtemp.

If we did that, I think we'd create a temp directory and never delete it?

[smcv]

And I still like using /run if we can since that's a known-safe quantity

Do you mean /run, or do you mean /run/user/$UID/.bubblewrap?

Strictly speaking, /run/user/$UID isn't API: it's an implementation detail of systemd-logind, which could equally well set XDG_RUNTIME_DIR for uid 1234 to /tmp/XDG_RUNTIME_DIR-for-1234-lw24hhd.

[cgwalters]

Strictly speaking, /run/user/$UID isn't API: it's an implementation detail of systemd-logind

Yeah but as a suid app we can't trust environment variables.

Probably, but we keep a fd to /proc open for the duration of the various pivots already, which we have to do because our root directory keeps changing.

Right but I'm thinking about things like glibc reading /proc/cpuinfo, anything else in the process that wants to read /proc/self/fd - both of these are unlikely probably but still...

If we just need a mountpoint...hum, maybe /usr/share/empty? Or we could ship a tmpfiles.d unit with bwrap that creates /run/bwrap or so.

In the suid case we could also create that before we drop privileges.

If we did that, I think we'd create a temp directory and never delete it?

Well, systemd's tmpfiles pruner would get it. But we could maybe fork off a process to rmdir it too (would be a bit ugly).

I'm OK personally "leaking" a tmpdir in the case where there isn't a properly set up /run too.

[smcv]

I think we do just need any arbitrary mount point. We could create /usr/share/bubblewrap or /usr/lib/bubblewrap in the packaging (and hope that people's package systems don't prune empty directories - dpkg doesn't but I don't know about others); or we could use a directory that library code doesn't have any business manipulating at that particular point in bubblewrap's lifetime, like /proc/sys, or /sys, or /usr, or /var, or /run, or /tmp?

Or we could ship a tmpfiles.d unit with bwrap that creates /run/bwrap or so.

bubblewrap doesn't currently require systemd, and making bubblewrap and flatpak require systemd would be politically problematic (in an "endless flamewars" way), at least in Debian. Sorry, but that particular war is still being fought :-(

[cgwalters]

I think we do just need any arbitrary mount point.

Agree.

We could create /usr/share/bubblewrap or /usr/lib/bubblewrap in the packaging

This seems simplest indeed; bikeshed more, /usr/share/bubblewrap/pivot-mount-point ?

like /proc/sys, or /sys, or /usr, or /var, or /run, or /tmp?

My worry about those is around what happens if people use e.g. --bin-ro /usr/blah or so would we be overwriting the /usr we'd need originally? But looking at the code, the original root ends up as /oldroot fairly quickly. This mount point is just very brief in our namespace.

I think that brings /tmp back as a possibility?

[smcv]

I think that brings /tmp back as a possibility?

I've updated this branch to use /tmp instead of /proc as the temporary mount point.

This seems simplest indeed; bikeshed more, /usr/share/bubblewrap/pivot-mount-point?

The reason I'm a little reluctant to do that is that it's all very well for a distro-included bwrap in a packaging system that preserves empty directories, but doesn't work if bwrap is bundled with an application (or installed with a deficient packaging system that can't preserve empty directories). Yes, we could fall back to /run/user/$UID or to mkdtemp or to /tmp if the empty mount point doesn't exist, but then we'd have multiple code paths, with all but the first being untested on the maintainers' systems. If we can, it seems better to have a single code path that works on all reasonable systems and is the one the maintainers test regularly.

At the moment, an application (even one that might be installed in a home directory from a tarball of binaries, like Steam) can bundle a copy of bwrap and have it work on at least some systems - it won't work on Debian, RHEL or other distros that restrict user namespace creation, because it won't be setuid, but it'll work on for example Ubuntu and Fedora. This is not entirely theoretical: I've been looking into whether Steam on Linux, which installs into the home directory and doesn't need root, could create bwrap-based containers to run individual games. On systems like Debian and RHEL the best it will be able to do is to require that either bwrap or Flatpak (with its bundled bwrap) is installed, but on systems like Ubuntu and Fedora there's no reason it should need to have that requirement.

[smcv]

My worry about those is around what happens if people use e.g. --bin-ro /usr/blah or so would we be overwriting the /usr we'd need originally? But looking at the code, the original root ends up as /oldroot fairly quickly. This mount point is just very brief in our namespace.

I added a test that asserts that /tmp gets mounted as we'd expect, and /tmp/oldroot, /tmp/newroot aren't exposed in the container.

[cgwalters]

@rh-atomic-bot r+ 1a50596

[rh-atomic-bot]

⌛ Testing commit 1a50596 with merge 5d59696...

[rh-atomic-bot]

💥 Test timed out

[smcv]

@rh-atomic-bot retry

[rh-atomic-bot]

⌛ Testing commit 1a50596 with merge 8f457ee...

[rh-atomic-bot]

☀️ Test successful - status-papr
Approved by: cgwalters
Pushing 8f457ee to master...