Re: Closing the BPF map permission loophole

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Sep 28, 2022 at 1:54 AM Lorenz Bauer <oss@xxxxxx> wrote:
>
> On Thu, 15 Sep 2022, at 11:30, Lorenz Bauer wrote:
> > Hi list,
> >
> > Here is a summary of the talk I gave at LPC '22 titled "Closing the BPF
> > map permission loophole", with slides at [0].
>
> I've put this topic on the agenda of the 2022-10-06 BPF office hours to get some maintainer attention. Details are here: https://docs.google.com/spreadsheets/d/1LfrDXZ9-fdhvPEp_LHkxAMYyxxpwBXjywWa0AejEveU/edit
>
> Best

So after the office hours I had an offline whiteboard discussion with
Alexei explaining more precisely what I was proposing, and it became
apparent that some of the things I was proposing weren't exactly
clear, and thus people were left confused about the solution I was
talking about. So I'll try to summarize it a bit and add some more
specifics. Hopefully that will help, because I still believe we can
solve this problem moving forward.

But first, two notes.

1) Backporting this is going to be hard, and I don't think that should
be the goal, it's going to be too intrusive, probably.

2) It turned out that we currently don't store user-space-side
read/write permissions on struct bpf_map itself, and we'd need to do
that as a preliminary step here. Below I just assume that struct
bpf_map records all the bpf-side and user-side read/write permissions.

So, the overall idea is that instead of fetching struct bpf_map point
for all kinds of FD-based operations (bpf_obj_get, map_fd_by_id, even
bpf_map_create) we are always working with a view of a map, and that
"view" is a separate struct/object, something like:

struct bpf_map_view {
    struct bpf_map *map;
    /* BPF_F_RDONLY | BPF_F_WRONLY, but we can later also add
       BPF-side flags: BPF_F_RDONLY_PROG | BPF_F_WRONLY_PROG
    */
    int access_flags;
}

So whenever we work with map by FD, we get struct bpf_map_view (i.e.,
we store struct bpf_map_view inside file->private and
inode->i_private). The semantics of view->access_flags is that it is
superimposed on top of bpf_map->map_flags (specifically its
BPF_F_RDONLY | BPF_F_WRONLY parts, later also BPF_F_RDONLY_PROG |
BPF_F_WRONLY_PROG). This means that if struct bpf_map is R/W, but our
current bpf_map_view says BPF_F_RDONLY, then only read-only access is
allowed through that FD. On the other hand, if bpf_map itself is only
BPF_F_RDONLY, but we somehow go bpf_map_view with BPF_F_RDONLY |
BPF_F_WRONLY (e.g., due to chmod loophole), then it doesn't matter,
it's still BPF_F_RDONLY, no write access. We can try preventing such
situations in some circumstances, but as we showed with chmod() it's
impossible to prevent in general.

So, just to hopefully make it a bit clearer, let's discuss a use case
that a bunch of people had in mind. Root/CAP_BPF user created R/W
bpf_map, but wants to pin it in one BPFFS path as R/W, so that it can
be later opened as R/W and modified. This BPFFS path will be
restricted through FS permissions to only allow it to be opened by a
privileged user/group. But, that same original root/CAP_BPF user would
like to also create a read-only BPFFS pinning of that same map, and
let unprivileged user(s) to open and work with that map, but only
perform read-only operations (e.g., BPF_MAP_LOOKUP_ELEM command).

Let's see how that works in this new bpf_map_view model.

1. root/CAP_BPF user does BPF_MAP_CREATE operation, struct bpf_map is
created with map_flags BPF_F_RDONLY | BPF_F_WRONLY. Also, immediately
struct bpf_map_view is created with same BPF_F_RDONLY | BPF_F_WRONLY
access_flags. struct bpf_map_view keeps reference on struct bpf_map,
struct bpf_map_view is assigned to struct file->private, new FD is
returned to user-space.

2. That same root/CAP_BPF user does BPF_OBJ_PIN and specifies that
they want R/W pinning (through file_flags). Kernel clones/copies
struct bpf_map_view (I think bpf_map_view shouldn't be shared between
files/inodes, each file/inode has its own personal copy; but we can
work out the details later), sets (keeps in this case) its
access_flags as  BPF_F_RDONLY | BPF_F_WRONLY. After that they are free
to chown/chmod as necessary to prevent unprivileged user from doing
anything with that BPFFS file, if necessary.

3. Now, for the read-only pinning. User does another BPF_OBJ_PIN using
original R/W map FD, but now they specify file_flags to only allow
BPF_F_RDONLY (I'm too lazy to check what exact flag we pass there to
communicate this intent, it's not that important for this discussion).
At this point, kernel creates a new struct bpf_map_view, pointing to
struct bpf_map, but this time access_flags have only BPF_F_RDONLY set.
Then we proceed to creating an inode, its i_private is assigned this
new R/O bpf_map_view. The user should chmod/chown pinned BPFFS file
appropriately to allow unprivileged users to BPF_OBJ_GET it.


Now, let's assume we are unprivileged user who wants to work with that
pinned BPF map. When we do BPF_OBJ_GET on that read-only pinned file,
kernel fetches struct bpf_map_view from inode->i_private which has
access_flags as BPF_F_RDONLY. That's it, there is no way we can do
update on that map, kernel will reject that even though struct bpf_map
itself allows BPF_F_WRONLY.

Note, though, that once we checked everything, as we create a new
struct file and return new FD to user-space, that new struct file will
have *yet another copy* of struct bpf_map_view, cloned from inode's
bpf_map_view (recall that I was proposing to have 1-to-1 mapping
between file/inode and bpf_map_view).


Let's now assume we are sneaky bastards and chmod that second pinned
BPFFS file to allow r/w file permissions. When we do BPF_OBJ_GET,
again, we'll fetch struct bpf_map_view which enforce BPF_F_RDONLY
(only), despite file itself having writable permissions. We can argue
if we should reject such BPF_OBJ_GET command or silently "downgrade"
to read-only view, that's beside the point.

Hopefully this is a bit clearer.

One last note. When we are talking about BPF_OBJ_GET, we are actually
going to be dealing with 4 layers of read and write permissions:
  1) struct bpf_map's "inherent" permissions
  2) struct bpf_map_view's access_flags
  3) struct file's FS read/write permissions
  4) union bpf_attr's file_flags specified for BPF_OBJ_GET

While that's a lot, we always intersect them and keep only the most
restrictive combination. So if at any of the layers we have read-only
permissions, resulting *new struct bpf_map_view* will only specify
BPF_F_RDONLY. E.g., if at layers 1, 2, and 4 we allow BPF_F_WRONLY,
but BPFFS file permission (layer #3 above) at that moment is
read-only, we should be only getting read-only view of BPF map.

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.

Alright, that's what I think I had to say. Please point out scenarios
that won't be covered by this bpf_map_view model. Thanks.



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux