Closing the BPF map permission loophole

[Roberto Sassu]

On Mon, 2022-12-12 at 19:19 +0100, Roberto Sassu wrote:
> On Mon, 2022-12-12 at 09:07 -0800, Alexei Starovoitov wrote:
> > On Mon, Dec 12, 2022 at 8:11 AM Roberto Sassu
> > <roberto.sassu at huaweicloud.com> wrote:
> > > On Mon, 2022-11-07 at 13:11 +0100, Roberto Sassu wrote:
> > > 
> > > [...]
> > > 
> > > > > > > P.S. We can extend this to BPF-side BPF_F_RDONLY_PROG |
> > > > > > > BPF_F_WRONLY_PROG as well, it's just that we'll need to define how
> > > > > > > user will control that. E.g., FS read-only permission, does it
> > > > > > > restrict both user-space and BPF-view, or just user-space view? We can
> > > > > > > certainly extend file_flags to allow users to get BPF-side read-only
> > > > > > > and user-space-side read-write BPF map FD, for example. Obviously, BPF
> > > > > > > verifier would need to know about struct bpf_map_view when accepting
> > > > > > > BPF map FD in ldimm64 and such.
> > > > > > 
> > > > > > I guess, this patch could be used:
> > > > > > 
> > > > > > https://lore.kernel.org/bpf/20220926154430.1552800-3-roberto.sassu@huaweicloud.com/
> > > > > > 
> > > > > > When passing a fd to an eBPF program, the permissions of the user space
> > > > > > side cannot exceed those defined from eBPF program side.
> > > > > 
> > > > > Don't know, maybe. But I can see how BPF-side can be declared r/w for
> > > > > BPF programs, while user-space should be restricted to read-only. I'm
> > > > > a bit hesitant to artificially couple both together.
> > > > 
> > > > Ok. At least what I would do is to forbid write, if you provide a read-
> > > > only fd.
> > > 
> > > Ok, we didn't do too much progress for a while. I would like to resume
> > > the discussion.
> > > 
> > > Can we start from the first point Lorenz mentioned? Given a read-only
> > > map fd, it is not possible to write to the map. Can we make sure that
> > > this properly work?
> > > 
> > > In my opinion, to achieve this particular goal, the map view
> > > abstraction Andrii suggested, should not be necessary.
> > 
> > What do you 'not necessary' ?
> > afair the map view abstraction is only one that actually addresses
> > all the issues.
> 
> For the first issue, map iterators, you need to ensure that the fd is
> read-write if the key/value can be modified.
> 
> For the second issue, fd modes ignored by the verifier, you need to
> restrict operations on the map, to meet the expectactions of whoever
> granted the fd to the requestor (as Lorenz said, if you have a read-
> only fd, you should not be able to write to the map).
> 
> Maybe I missed something, I didn't get how the map view abstraction
> could help better in these cases.

Ok, let me try to complete the solution for the issues Lorenz pointed
out. Here I discuss only the system call side of access.

I was thinking on the meaning of the permissions on the inode of a
pinned eBPF object. Given that the object exists without pinning, this
double check of permissions first on the inode and then on the object
to me looks very confusing.

So, here is a proposal: what if read and write in the context of
pinning don't refer to accessing the eBPF object itself but to the
ability to read the association between inode and eBPF object or to
write/replace the association with a different eBPF object (I guess not
supported now).

We continue to do access control only at the time a requestor asks for
a fd. Currently there is only MAC, but we can add DAC and POSIX ACL too
(Andrii wanted to give read permission to a specific group). The owner
is who created the eBPF object and who can decide (for DAC and ACL) who
can access that object.

The requestor obtains a fd with modes depending on what was granted. Fd
modes (current behavior) give the requestor the ability to do certain
operations. It is responsibility of the function performing the
operation on an eBPF object to check the fd modes first.

It does not matter if the eBPF object is accessed through ID or inode,
access control is solely based on who is accessing the object, who
created it and the object permissions. *_GET_FD_BY_ID and OBJ_GET
operations will have the same access control.

With my new proposal, once an eBPF object is pinned the owner or
whoever can access the inode could do chown/chmod. But this does not
have effect on the permissions of the object. It changes only who can
retrieve the association with the eBPF object itself.

Permissions on the eBPF object could be changed with the bpf() syscall
and with new operations (such as OBJ_CHOWN, OBJ_CHMOD). These
operations are of course subject to access control too.

The last part is who can do pinning. Again, an eBPF object can be
pinned several times by different users. It won't affect who can access
the object, but only who can access the association between inode and
eBPF object.

We can make things very simple: whoever is able to read the association
is granted with the privilege to pin the eBPF object again.

One could ask what happens if a user has only read permission on an
inode created by someone else, but has also write permission on a new
inode the user creates by pinning the eBPF object again (I assume that
changing the association makes sense). Well, that user is the owner of
the inode. If the user wants other users accessing it to see a
different eBPF object, it is the user's decision.

Roberto