On Thu, Jun 13, 2024 at 4:45 AM Jonathan Calmels <jcalmels@xxxxxxxx> wrote: > On Wed, Jun 12, 2024 at 08:54:28PM GMT, John Johansen wrote: > > On 6/12/24 10:29, Paul Moore wrote: > > > On Wed, Jun 12, 2024 at 4:15 AM Jonathan Calmels <jcalmels@xxxxxxxx> wrote: > > > > On Tue, Jun 11, 2024 at 06:38:31PM GMT, Paul Moore wrote: > > > > > On Tue, Jun 11, 2024 at 6:15 PM Jonathan Calmels <jcalmels@xxxxxxxx> wrote: > > > > > > ... > > > > > > > > > Arguably, if we do want fine-grained userns policies, we need LSMs to > > > > > > influence the userns capset at some point. > > > > > > > > > > One could always use, or develop, a LSM that offers additional > > > > > controls around exercising capabilities. There are currently four > > > > > in-tree LSMs, including the capabilities LSM, which supply a > > > > > security_capable() hook that is used by the capability-based access > > > > > controls in the kernel; all of these hook implementations work > > > > > together within the LSM framework and provide an additional level of > > > > > control/granularity beyond the existing capabilities. > > > > > > > > Right, but the idea was to have a simple and easy way to reuse/trigger > > > > as much of the commoncap one as possible from BPF. If we're saying we > > > > need to reimplement and/or use a whole new framework, then there is > > > > little value. > > > > > > I can appreciate how allowing direct manipulation of capability bits > > > from a BPF LSM looks attractive, but my hope is that our discussion > > > here revealed that as you look deeper into making it work there are a > > > number of pitfalls which prevent this from being a safe option for > > > generalized systems. > > > > > > > TBH, I don't feel strongly about this, which is why it is absent from > > > > v1. However, as John pointed out, we should at least be able to modify > > > > the blob if we want flexible userns caps policies down the road. > > > > > > As discussed in this thread, there are existing ways to provide fine > > > grained control over exercising capabilities that can be safely used > > > within the LSM framework. I don't want to speak to what John is > > > envisioning, but he should be aware of these mechanisms, and if I > > > recall he did voice a level of concern about the same worries I > > > mentioned. > > > > > > > sorry, I should have been more clear. I envision LSMs being able to > > update their own state in the userns hook. > > > > Basically the portion of the patch that removes const from the > > userns hook. > > Yes, pretty sure we'll need this regardless. > > > An LSM updating the capset is worrysome for all the reasons you > > pointed out, and I think a few more. I haven't had a chance to really > > look at v2 yet, so I didn't want to speak directly on the bpf part of > > the patch without first giving a good once over. > > > I'm happy to discuss ways in which we can adjust the LSM hooks/layer > > > to support different approaches to capability controls, but one LSM > > > directly manipulating the state of another is going to be a no vote > > > from me. > > > > I might not be as hard no as Paul here, I am always willing to listen > > to arguments, but it would have to be a really good argument to > > modify the capset, when there are multiple LSMs in play on a system. > > The way I see it, it's more about enhancing the capability LSM with BPF > hooks and have it modify its own state dynamically, not so much > crosstalk between two distinct LSM frameworks (say one where the BPF > LSM implements a lot of things like capable()). As I mentioned previously, if you want to do something with the capability sets you simply need to do it within the confines of security/commoncap.c. If you're really set on the "MUST BE BPF!" way of life, and you can convince Serge (capabilities maintainer) that it would be a good idea, you could propose a dedicated BPF hook within the capabilities LSM. I'm not sure how wise that would be, but it would resolve a lot of the LSM ordering/stacking issues that we've discussed. > If we think there is no way we can come up with something that's safe > enough, and that the risks outweigh the benefits, fine by me, we can > drop this patch from the series. To be clear, this patch is not acceptable at this point in time. With the understanding that I haven't looked that closely at the rest of the patchset, it looks fairly well contained to the capabilities code which means it is largely up to Serge, not me. I will mention that you should update the audit code to recognize the new capability set, look at kernel/auditsc.c for more information. -- paul-moore.com
