Closing the BPF map permission loophole

[Roberto Sassu]

On Wed, 2022-12-21 at 19:55 -0500, Paul Moore wrote:
> On Wed, Dec 21, 2022 at 4:54 AM Roberto Sassu
> <roberto.sassu at huaweicloud.com> wrote:
> > On 12/20/2022 9:44 PM, Paul Moore wrote:
> > > On Fri, Dec 16, 2022 at 5:24 AM Roberto Sassu
> > > <roberto.sassu at huaweicloud.com> wrote:
> > > > 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.
> > > 
> > > Just to make sure I understand you correctly, you're suggesting that
> > > the access modes assigned to a pinned map's fd are simply what is
> > > requested by the caller, and don't necessarily represent the access
> > > control modes of the underlying map, is that correct?  That seems a
> > 
> > The fd modes don't necessarily represent the access control modes of the
> > inode the map is pinned to. But they surely represent the access control
> > modes of the map object itself.
> > 
> > The access control modes of the inode tell if the requestor is able to
> > retrieve the map from it, before accessing the map is attempted. But,
> > even if the request is granted (i.e. the inode has read permission), the
> > requestor has still to pass access control on the map object, which is
> > separate.
> 
> Okay, good.  That should work.
> 
> > Fd modes are bound to the map access modes, but not necessarily bound to
> > the inode access modes (fd with write mode, on an inode with only read
> > permission). Fd modes are later enforced by map operations by checking
> > the compatibility of the operation (e.g. read-like operation requires fd
> > read mode).
> > 
> > The last point is what it means getting a fd on the inode itself. It is
> > possible, because inodes could have seq_file operations. Thus, one could
> > dump the map content by just reading from the inode.
> 
> Gotcha, yes, that would be bad.
> 
> > Here, I suggest that we still do two separate checks. One is for the
> > open(), done by the VFS, and the other to access the map object. Not
> > having read permission on the inode means that the map content cannot be
> > dumped. But, having read permission on the inode does not imply the
> > ability to do it (still the map object check has to be passed).
> 
> That makes sense to me.

Andrii, Lorenz, what do you think about the new interpretation of the
permissions of the inode of a pinned map?

Thanks

Roberto