Date: Wed, 30 Mar 2022 13:52:09 -0400 From: Mathieu <sigsys@gmail.com> To: Baptiste Daroussin <bapt@FreeBSD.org> Cc: freebsd-hackers@FreeBSD.org Subject: Re: curtain: WIP sandboxing mechanism with pledge()/unveil() support Message-ID: <8df2d609-0a0d-0a25-4918-a27c91c3790a@gmail.com> In-Reply-To: <20220330072210.icontj4n7hcqwtql@aniel.nours.eu> References: <25b5c60f-b9cc-78af-86d7-1cc714232364@gmail.com> <20220330072210.icontj4n7hcqwtql@aniel.nours.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
On 3/30/22 03:22, Baptiste Daroussin wrote: > Hello Mathieu, > First of all, thank you for this amazing work, leveraging the mac framework to > build curtain is imho an excellent idea, I personnally see a curtain like > approach as complementary to a capsicum approach rather than an antagonist > feature, I can see many possible usage of curtains in freebsd in particular in > the port framework! Alright! One nice thing with the MAC approach is that many of the checks were already carefully placed to not deviate too much from expected behavior for applications. At first I tried to do all of the checks in namei() and this often made syscalls fail too early with the wrong errnos and this confuses some programs. When I figured out the right place to add the checks it was almost always right next to a MAC check. Also, if I hadn't used a MAC module and added a new layer instead, you could say that it would have been the fifth (!!!) general access control mechanism in the kernel (after traditional UNIX, Capsicum, jails and MAC). This was starting to feel like a bit much. So the MAC framework, it's not a perfect fit for some of what this module wants to do but it could be worse. Also, if you combine curtains with chroots you kind of get jails. It's chroot escape-proof because unveils deny access to the outer directories like any other. Privileges are restricted. It doesn't have all the features of jails but it's very jail-like. Just to say, the MAC framework was pretty close to being able to implement jails too. > To allow to integrate and permit reviews from developers, I think we can/should > split the review. The first thing will probably me imho to start a review > process of the sysfilt feature, this is probably the part that will need most of > the back and forth discussion given the rest is pretty isolated (mac module, > userland). Yeah I think it's a good idea. I could do some minor cleanups and add some comments while at it before submitting it. I'm still working on this, but the majority of the changes should be in the userland and the module itself now. And getting some feedback on the kernel parts (and eventually knowing that they're in an acceptable state) would help. The module works well enough already that I don't think that there's anything critical that would require extra changes to the rest of the kernel (unless some problem is found). I think the kernel changes could be broken up something like this: 1) Sysfils. Adds annotations to every syscall in syscalls.master (Linuxulator could be done later too). Generalizes the Capsicum flag to a sysfils bitmap both for sysents and ucreds. Changes to the syscall entry checks. Some sysfil checks spread out in the kernel. 2) There are some small modifications that I believe are bug fixes or minor improvements or basically have no effect in the current state of things (but are important for mac_curtain). 3) The new MAC handlers. That's a significant part too and it can be separate from sysfils. This would include the new call sites in vfs_lookup() (and there's some extra logic too). New call sites in SysV/POSIX IPC modules (and some other changes to path handling). There's a function that deals with getdirentries() filtering directly in the MAC framework (the MAC hooks only deal with the dirents one by one) that maybe doesn't belong there. vn_open() got a new flag. 4) Various modifications that are specifically to support mac_curtain. I tried to minimize those but there are some. struct thread/proc need new fields (opaque pointers for unveil tracking/caching). struct file needs a few extra integer fields (I really tried to get rid of those but couldn't figure out a good way). I also added "sysctl_shadow" objects to kern_sysctl.c. These are handles that can outlast sysctl_oids. It's not elegant, but mac_curtain needs it. At first I thought that I could add long-lived references to sysctl_oids and I designed the curtain data structure for that. Then I realized that sysctl_oids get totally nuked when a module unloads. They get unmapped from memory with the rest of the module's data and there's no holding on to them (IIUC). So I added an intermediate data structure between sysctl_oids and curtains. The mallocs with non-sleepable locks in the SysV IPC module with late label initializations are annoying but this could be fixed separately (it might need a bit of code restructuring...). Making the module fplookup-friendly would probably need some changes to the MAC framework itself, this can be done separately too. There are others, but not critical and can be done separately. > Can you isolate the sysfils code and start a review in phabricator? If you need > help for this don't hesitate to ask me ;) Yup, I'm on it. > Again thanks for the huge work. No problem! This is gonna be a lot of work to review so thanks to anyone involved too. > > Best regards, > Bapt
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?8df2d609-0a0d-0a25-4918-a27c91c3790a>