Add --size option to control size of a --tmpfs

[tomsmeding]

Rationale

bwrap is a sandboxing utility, and one usually sandboxes a process to control its access to resources. One of the resources that one might want to control is disk space usage, but bwrap currently provides no facilities to control disk space usage.

We're using bwrap for a programming language playground where people can submit arbitrary code; we run the code on a server and send the output back. Obviously this needs sandboxing, and among other things, we want to avoid an ENOSPC DOS.

#375 suggested a possible fix for this missing feature: to add a facility to control the size of a created tmpfs.

Fixes #375.

Solution

This adds a new --size option that works similarly to --perms in that it modifies a subsequent --tmpfs option. Like --perms, an error is thrown if the option is not applied to a suitable action; --size is currently only implemented for --tmpfs. Combining --perms and --size is allowed, and the code checks that the options are not given twice for the same action.

~~The argument to the --size option is checked for acceptability (namely being fully alphanumeric, so that it does not escape the size= mount option of tmpfs) in the privileged process that receives privileged-operation requests; this is not done earlier because we need to do it here anyway, due to the untrustedness of the unprivileged code.~~ (made obsolete due to accepting a number of bytes)

Review

Please review this code! I'm not familiar with the bwrap codebase, hence I probably made mistakes. Tests have been added (and they pass) and documentation has been updated, but I didn't build the documentation locally to see if the result is good.

[tomsmeding]

In the force-push:

• Repeated strcmp on "--perms" and "--size" factored out to is_modifier_option()
• Somewhat more self-contained commits

Parsing of tmpfs sizes is not yet done in bwrap, see discussion above.

[tomsmeding]

@smcv In the second force-push:

• Rebase on current main
• Commit sequence has been further cleaned up, with each commit now making a self-contained change
• --size now expects a plain number of bytes, and accepts nothing else. This is tested. This makes the alphanumericness (?) checks in the privileged process unnecessary.
• --size is still accepted in all cases, not only if unprivileged. Your call on this one; it would be easy to change.

Thanks again for the review!

[tomsmeding]

Something I forgot to put in the message above: as is commented in the code, tmpfs seems to make the size 0 if the specified size= is too large. Do we need to do something with that? Is this reliable behaviour?

[tomsmeding]

@smcv Sorry for bumping, but if you are willing, I would love to see this merged. If there is something you would like me to fix beforehand, I can.

[tomsmeding]

@smcv Last bump, sorry -- I think this is ready for final review / merge. I'm willing to make any changes you wish. I'm using this in production and we'd have more trust in using bwrap master than in using my own fork. :)

If you do not wish to merge this, that is fine too, but I would like to know so that I can look for other solutions.

[smcv]

This is on my list, but my list is not short.

[tomsmeding]

Thanks for the reply! Please take your time, no rush. Thanks for the hard work.

[tomsmeding]

@smcv Thanks a lot for the review! I think I've addressed all your comments. The commits are separate for now; after your approval (or before, if you wish) I plan to squash the first (2796bdf) and fourth (bd9f8f4) commit into the initial implementation commit (d712f83), and to squash the second (d53d6ee) and third (c3a1c64) commits into the initial testing commit (f856ddd).

As mentioned in my other comment, I opted to completely disallow --size in privileged mode, since this is the simplest to implement and the easiest to audit. The rest of your comments did not need discussion, I think.

Please let me know if this looks good or if you'd like something else changed as well.

[smcv]

As mentioned in my other comment, I opted to completely disallow --size in privileged mode, since this is the simplest to implement and the easiest to audit

I agree with this approach. If someone else wants this feature in setuid mode, it can be up to them to justify why it's safe.

[smcv]

I'll do another review pass on the whole PR, but I think my only concern is the phrasing of the error message when --size is used in setuid mode.

Yes, this. If you rephrase the error message and squash the commits, I think this is ready.

[tomsmeding]

@smcv Thanks so much! I squashed as planned, and made precisely one additional modification: the --size setuid message as you said. Otherwise there should be no diff between the code before and after the rebase.

Thanks also for the hint on how to manage PRs; my active open source experience is still fairly limited.