--bind can cause bwrap to fail during startup if it races with the mount table changing

[artli]

Try running this in one terminal to cause the mount table to change frequently:

$ mkdir from to
$ while true; do sudo mount --bind from to && sudo umount to; done

And run bwrap repeatedly in another terminal:

$ while true; do bwrap --bind / / -- /bin/true && echo -n .; done
...................................................................................................bwrap: Can't bind mount /oldroot/ on /newroot/: Unable to apply mount flags: remount "/newroot/<redacted>/to": Invalid argument
.....................................................................................................................................................................................................................................................................................................................................bwrap: Can't bind mount /oldroot/ on /newroot/: Unable to apply mount flags: remount "/newroot/<redacted>/to": Invalid argument
..............................................................................................................................................................................................................................................................................................................................................................................................................bwrap: Can't bind mount /oldroot/ on /newroot/: Unable to apply mount flags: remount "/newroot/<redacted>/to": Invalid argument
.....................................

This likely happens because the bind_mount function grabs the current mount table (

bubblewrap/bind-mount.c

Line 438 in 973fe36

mount_tab = parse_mountinfo (proc_fd, kernel_case_combination);

) and then remounts its entries one by one to ensure --bind works recursively but fails if any of the sub-mounts fail (

bubblewrap/bind-mount.c

Line 472 in 973fe36

mount ("none", mount_tab[i].mountpoint,

). The implicit assumption is that any mount that's already listed in the mount table is going to stay there and so we should be able to remount it, but mounts can definitely disappear in real-world scenarios (e.g. if the user unmounts something manually or if autofs is used with non-zero timeouts), so bwrap should be robust to that.

It already checks that errno != EACCES; perhaps this failure can be excluded in the same way.

[smcv]

Retitled because this is a graceful failure (inasmuch as anything bwrap does is ever graceful), not really a crash.

The implicit assumption is that any mount that's already listed in the mount table is going to stay there and so we should be able to remount it, but mounts can definitely disappear in real-world scenarios (e.g. if the user unmounts something manually or if autofs is used with non-zero timeouts), so bwrap should be robust to that.

In principle yes, but we need to be careful with making bwrap more robust against unexpected situations, because when it's used as a sandboxing tool (a security boundary), it's better for it to fail to start than to start successfully but without the desired security properties.

I'm not sure whether --bind has this sort of security implication (possibly for nodev, nosuid and similar?), but when we're doing a --ro-bind it's certainly security-sensitive that we successfully remount all of the filesystems below the requested mount point as read-only. If we fail to make one of them read-only, it's better to fail than to leave it read/write in the sandbox.

[rusty-snake]

The hole recursive apply flags topic (which all drawbacks like #384) can be avoided on kernels with mount_setattr (Linux 5.12 - 2021) AFAICTY. Leaving the inefficient resursive remount for older kernels as fallback.

struct mount_attr attr = {
    .attr_clr = 0,
    .attr_set = MOUNT_ATTR_NODEV | MOUNT_ATTR_NOSUID | MOUNT_ATTR_NOEXEC,
};
mount_setattr(CWD, "/newroot/foobar", AT_RECURSIVE, &attr, sizeof(attr);
if (errno == ENOSYS)
    recursive_remount()

[smcv]

Please do send a pull request if you have a mount_setattr-based code path ready!

[rusty-snake]

Unfortunately not. And my C skills are too limited.

[artli]

when we're doing a --ro-bind it's certainly security-sensitive that we successfully remount all of the filesystems below the requested mount point as read-only. If we fail to make one of them read-only, it's better to fail than to leave it read/write in the sandbox.

Yeah, that sounds totally reasonable; in this case though the specific error is that the mount point is just not found, which is hopefully different enough from "the mount point exists but we failed to mount it" that we can continue? I can see how this is tricky from the security standpoint if we cannot easily pinpoint the cause of the error.

For my use case, I resorted to heuristically detecting this condition by parsing bwrap's stderr, which is obviously a sad hack.

[smcv]

I can see how this is tricky from the security standpoint if we cannot easily pinpoint the cause of the error

Yeah, sorry, if there is any doubt, we have to "fail closed". Sysadmins and users are trusting bubblewrap to provide the security boundary that was asked for.

If you can put together a PR to handle this more gracefully (either by using mount_setattr or by cleverer error handling), I'd consider it (time permitting), but you would have to be able to justify how it isn't a security vulnerability.