Add an Option to Limit the Sandbox Child Process Memory #192
Conversation
turbo
commented
Apr 24, 2017
turbo
commented
This change adds an additional parameter --limit-mem VALUE, which when supplied sets the maximum size of the child process's virtual memory (address space) to VALUE megabytes. This affects memory mapping and stack expansion calls.
|
Can one of the admins verify this patch?
|
|
This needs CAP_SYS_RESOURCE to raise the hard limit, which we do not have, at least in the unprivileged user namespace case. However, it will still work for lowering the limit, which is still interesting. It does not fully protect against malicious memory use though, as you can have any number of pids with this much memory each. Of course, you can already get similar functionallitty by using ulimit before you start the bwrap, but that is not as convenient. |
|
If we do this we probably also need to verify that you're not trying to raise the hard limit in the setuid bwrap case. |
Yes, sorry if this was unclear. The intention is to set a lower soft limit, not to raise any limits.
I use this option exactly for this purpose. E.g. an interactive shell or script compiles and runs untrusted code - in my case these are single-threaded python scripts. The limit does a good enough job of killing the process if it tries to allocate more memory, but keeps the calling shell alive. This should maybe be noted in the parameter description.
Good catch. I removed the setuid code in my offline branch, that's why I missed it when modifying my upstream branch. |
| @@ -2216,6 +2237,13 @@ main (int argc, | |||
| prctl (PR_SET_SECCOMP, SECCOMP_MODE_FILTER, &seccomp_prog) != 0) | |||
| die_with_error ("prctl(PR_SET_SECCOMP)"); | |||
|
|
|||
| /* Optionally limit memory. This must occur after the fork and | |||
| * before the exec* call. */ | |||
| if (opt_limit_mem) { | |||
There was a problem hiding this comment.
This needs to come before the seccomp filter - see the rationale in #155
There was a problem hiding this comment.
Also, code style is GNU - braces on a new line.
| * before the exec* call. */ | ||
| if (opt_limit_mem) { | ||
| limit_mem.rlim_cur = limit_mem.rlim_max = opt_limit_mem * 1048576; | ||
| setrlimit(RLIMIT_DATA, &limit_mem); |
There was a problem hiding this comment.
And yeah, we should probably just disable this in the setuid case...unless it seems easy to get the current limit and check it ourself. (Alternatively, maybe we could temporarily drop CAP_SYS_RESOURCE before doing this call? Actually, maybe we're dropping it already?)
There was a problem hiding this comment.
I'd like to go with the safer method first, denying this option in setuid mode. I don't know if there's a way to reliably check the limit for all supported platforms. That seems like something that's less stable and could be done in the future.
Checking the cap is a better way. I'll investigate.
|
Big picture of course - limits like this are really better done via cgroups. I'm not opposed to carrying some rlimit code though. Should we also try to add a knob to limit max pids like systemd's |
IMO that's a good idea. From my perspective (sandboxing CI tasks), the most important limits are:
There are some limits that might be useful for a multi-tenant system, but they are maybe better suited for the orchestration tool that calls bwrap:
et al. |
|
☔ The latest upstream changes (presumably #196) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Can one of the admins verify this patch?
|
