search

ioi / isolate

Sandbox for securely executing untrusted programs
Other
1.05k stars 157 forks source link

Workaround for mount() failure inside chroot #82

Closed ayuusweetfish closed 4 years ago

ayuusweetfish commented 4 years ago

Hi!

I am running isolate in a chroot file system (Alpine mini rootfs) to keep a consistent compilation and execution environment. Isolate changes all mounts on / to private after entering the sandbox, but this operation fails with EACCES in the chroot()'ed environment.

I have figured out a workaround by mounting root before recursively privatizing mounts under it (rather than /), and unmounting it at exit. This still keeps all mounts private, except during sandbox execution when mounted root (/var/local/lib/isolate/0/root) is visible to peers.

With possibly limited knowledge of internals, I would be glad if this change could be reviewed for validity and security, and accepted if appropriate. Thanks!

bblackham commented 4 years ago

I could recreate the issue running isolate in a chroot, but the error I see is EINVAL, which I think comes from this check in do_change_type in the kernel's namespace.c

    if (path->dentry != path->mnt->mnt_root)
        return -EINVAL;

It makes sense that you can't change mount flags on something that isn't actually the mount point.

As for the patch, it looks sound to me, but it does leave open the possibility of a stale tmpfs of "root" being left lying around in unusual circumstances (e.g. if isolate is SIGKILL'd), which even isolate --cleanup won't remove without further patches.

The only other solution I can think of is to push back onto the user of isolate the expectation that / is always a mountpoint. In the chroot case, you could just use a bind mount to chroot into. (In fact, it looks like you can just use mount --bind /my/chroot /my/chroot to basically turn a directory into a mountpoint.

@kawa-yoiko - could you please test the self-bind-mount approach on your chroot to see if it also resolves your issue (without the patched isolate, of course)? If it does, is that an acceptable workaround?

ayuusweetfish commented 4 years ago

Ah sorry it was a typo, I indeed meant EINVAL… (/_ ;)

Thanks for the suggestion, it works and turns out to be a better solution than the patch! It would be great if this is mentioned somewhere in the docs, but anyway this page might serve the need already. Let's close this.

bblackham commented 4 years ago

Great, thanks for confirming! I have pushed a patch to the man page to document this requirement.