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.
>> 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?
> 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.
Thanks
Lorenz
