On Thu, Jun 13, 2024 at 4:45 AM Jonathan Calmels [off-list ref] wrote:

On Wed, Jun 12, 2024 at 08:54:28PM GMT, John Johansen wrote:

quoted

On 6/12/24 10:29, Paul Moore wrote:

quoted

On Wed, Jun 12, 2024 at 4:15 AM Jonathan Calmels [off-list ref] wrote:

quoted

On Tue, Jun 11, 2024 at 06:38:31PM GMT, Paul Moore wrote:

quoted

On Tue, Jun 11, 2024 at 6:15 PM Jonathan Calmels [off-list ref] wrote:

...

quoted

quoted

quoted

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.

quoted

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.

quoted

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.

quoted

quoted

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