On Thu, 2022-09-22 at 15:47 +0100, Lorenz Bauer wrote:
> On Wed, 21 Sep 2022, at 17:32, Roberto Sassu wrote:
> > I saw your fix #2, even if I didn't fully understand it. What do
> > you
> > think instead about converting the fd modes to map flags (e.g.
> > BPF_F_RDONLY -> BPF_RDONLY_PROG), and we rely on the existing
> > verifier
> > behavior for the _PROG counterparts? In this way, it will be the
> > verifier enforcing the decision made by security_bpf_map().
>
> Thanks for that idea, I think something like it was floated during
> the discussion after my talk as well but I forgot about it. I gave it
> a shot, and it turns out okay actually. The biggest draw back is that
> this approach requires commit 591fe9888d78 ("bpf: add program side
> {rd, wr}only support for maps") which appeared after BPF_F_RDONLY.
Yes, true. Not sure if it makes sense to backport it to stable versions
(probably not). Or alternatively, for older versions we could ensure
that the fd is for read/write, even if as you said, there is a risk of
breakage of existing applications. It seems unlikely that this could
happen, as libbpf still does not support requesting a read-only fd,
although one could create an ad-hoc function to set the appropriate
parameters for the bpf() system call.
Actually, if it may help, I could send my version of the fix I
developed to validate the idea. I also wrote the tests.
> Problem #3: Read-only fds can be transmuted into read-write fds
> > > via
> > > object pinning
> >
> > Maybe I'm missing something, but I consider pinning more like
> > adding a
> > new reference to an eBPF object (like the ID).
> >
> > You are still subject to access control decision by
> > security_bpf_map(),
> > as for BPF_MAP_GET_FD_BY_ID().
>
> I had a look, security_bpf_map() is only called from
> bpf_map_new_fd(), which in turn is invoked from GET_FD_BY_ID,
> MAP_CREATE and OBJ_GET. Checking at this point is too late, since
> OBJ_PIN + chmod allow escalating privileges. Can you explain your
> idea some more?
The ability to access the path of a pinned map does not give you the
ability to access the map itself. You still need to pass
security_bpf_map(). With SELinux, you would need a rule like:
allow <ctx of subject accessing the pinned map> <ctx of the map
creator>: bpf { map_read map_write };
The inode is not passed to security_bpf_map(), so likely it is not
taken into account for the security decision.
What you say, I think it applies to map iterators. The ability to
access the path of an iterator gives you the ability to make changes to
the map without further checks.
My further question, we could discuss it separately, is: by updating
the verifier to check fd modes, and by checking the fd modes for map
iterators, are we able to intercept any possible attempt from user
space to modify a map?
> Now, the security model is limited to two permissions (read,
> > write). If
> > we want to add a new one to decide whether or not a new reference
> > can
> > be added, we could revisit this.
>
> Maybe, but that would preclude back porting any fixes. I'll write up
> another summary in a bit that shows that this problem goes back all
> the way to the introduction of BPF_F_RDONLY, etc.
Definitely, if possible, the fix should be backportable.
Roberto
