Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Configure a proper chroot jail for NsJail #55

Merged
merged 10 commits into from Feb 5, 2020
Merged

Configure a proper chroot jail for NsJail #55

merged 10 commits into from Feb 5, 2020

Conversation

@MarkKoz
Copy link
Member

MarkKoz commented Dec 29, 2019

Description

devfs and sysfs were problematic since they were being mounted as tmpfs, which is r/w. For example, the Python process could write to cgroups. Now, only what is needed to run Python gets mounted. This boils down to the venv itself and some shared libraries Python needs.

  • Use a config file for NsJail instead of command-line options
  • Map 65534 (nobody) user & group inside the user namespace to 65534 outside the namespace rather than mapping to current uid/guid (which was 0 AKA root)

Additional Discussion

Unprivileged Docker container

I've explored running the container as unprivileged, which would make Docker mount neither devfs nor sysfs. However, so far this has been problematic mainly because the NsJail cgroup needs to be created. That requires /sys to be mounted and privileges to write to it. There are some experimental Docker features which allow for privileges while building, but I couldn't manage to create the cgroup during the build. In retrospect, it's probably because /sys wasn't mounted anyway (would need to double check if it gets mounted during build).

Regardless, it is questionable what advantages running as unprivileged would have when NsJail is already being employed and now a (hopefully) proper chroot jail is configured. Of course, giving privileges when they aren't needed is always something one would rather not do. Doing away with privileges in this case just doesn't seem trivial to accomplish.

Ownership inside NsJail

Inside NsJail, ls shows all mounted files and directories are owned by 65534. Though theoretically everything is mounted as read-only, this is still a bit concerning since the owner has write permissions.

However, the output of ls may be a bit misleading: according to one of the maintainers of NsJail, it is overflowing to 65534 because the root uid/guid is not mapped in the user namespace. If root is mapped to e.g. 12345 inside, then all files show as belonging to 12345 instead, which seems to be correct because everything in the Docker container is owned by root (USER is never specified in the Dockerfiles so I guess Docker defaults to using root when copying from the host).

MarkKoz and others added 6 commits Dec 28, 2019
devfs and sysfs were problematic since they were being mounted as
tmpfs, which is r/w. For example, the Python process could write to
cgroups. Now, only what is needed to run Python gets mounted. This
boils down to the venv itself and some shared libraries Python needs.

* Use a config file for NsJail instead of command-line options
* Map 65534 (nobody) user & group inside the user namespace to 65534
  outside the namespace rather than mapping to current uid/guid (which
  was 0 AKA root)
Co-authored-by: 0xf0f <0x0meta@gmail.com>
This is a test for #53, which fixed numpy failing to import due to
using multiple threads by default.
@MarkKoz MarkKoz added the bug label Dec 29, 2019
MarkKoz and others added 2 commits Jan 11, 2020
@MarkKoz MarkKoz requested review from python-discord/core-developers, eivl and dementati and removed request for python-discord/core-developers Feb 2, 2020
Copy link
Member

SebastiaanZ left a comment

This is the only thing that is standing out to me. I'm still in the process of actually testing it.

snekbox/nsjail.py Outdated Show resolved Hide resolved
Copy link
Member

lemonsaurus left a comment

The code itself looks good. I have no idea what any of this does. I'm sure you've tested it thoroughly and there's very little risk involved if this has a mistake in it, letting it sit any longer is completely pointless.

Co-Authored-By: Sebastiaan Zeeff <33516116+SebastiaanZ@users.noreply.github.com>
@MarkKoz MarkKoz requested a review from python-discord/core-developers as a code owner Feb 5, 2020
@MarkKoz MarkKoz removed request for eivl and dementati Feb 5, 2020
@MarkKoz MarkKoz merged commit 504a6b9 into master Feb 5, 2020
2 checks passed
2 checks passed
Snekbox Build #20200205.2 succeeded
Details
Snekbox (Lint & Test) Lint & Test succeeded
Details
@MarkKoz MarkKoz deleted the proper-chroot branch Feb 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.