Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf

From: Andy Lutomirski <luto@kernel.org>
To: Kees Cook <keescook@chromium.org>
Cc: Andy Lutomirski <luto@kernel.org>,
	Song Liu <songliubraving@fb.com>,
	"linux-security@vger.kernel.org" <linux-security@vger.kernel.org>,
	Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Kernel Team <Kernel-team@fb.com>,
	Lorenz Bauer <lmb@cloudflare.com>, Jann Horn <jannh@google.com>,
	Greg KH <gregkh@linuxfoundation.org>,
	Linux API <linux-api@vger.kernel.org>
Subject: Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
Date: Tue, 2 Jul 2019 14:32:08 -0700	[thread overview]
Message-ID: <CALCETrXTta26CTtEDnzvtd03-WOGdXcnsAogP8JjLkcj4-mHvg@mail.gmail.com> (raw)
In-Reply-To: <201907021115.DCD56BBABB@keescook>

On Tue, Jul 2, 2019 at 2:04 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Jul 01, 2019 at 06:59:13PM -0700, Andy Lutomirski wrote:
> > I think I'm understanding your motivation.  You're not trying to make
> > bpf() generically usable without privilege -- you're trying to create
> > a way to allow certain users to access dangerous bpf functionality
> > within some limits.
> >
> > That's a perfectly fine goal, but I think you're reinventing the
> > wheel, and the wheel you're reinventing is quite complicated and
> > already exists.  I think you should teach bpftool to be secure when
> > installed setuid root or with fscaps enabled and put your policy in
> > bpftool.  If you want to harden this a little bit, it would seem
> > entirely reasonable to add a new CAP_BPF_ADMIN and change some, but
> > not all, of the capable() checks to check CAP_BPF_ADMIN instead of the
> > capabilities that they currently check.
>
> If finer grained controls are wanted, it does seem like the /dev/bpf
> path makes the most sense. open, request abilities, use fd. The open can
> be mediated by DAC and LSM. The request can be mediated by LSM. This
> provides a way to add policy at the LSM level and at the tool level.
> (i.e. For tool-level controls: leave LSM wide open, make /dev/bpf owned
> by "bpfadmin" and bpftool becomes setuid "bpfadmin". For fine-grained
> controls, leave /dev/bpf wide open and add policy to SELinux, etc.)
>
> With only a new CAP, you don't get the fine-grained controls. (The
> "request abilities" part is the key there.)

Sure you do: the effective set.  It has somewhat bizarre defaults, but
I don't think that's a real problem.  Also, this wouldn't be like
CAP_DAC_READ_SEARCH -- you can't accidentally use your BPF caps.

I think that a /dev capability-like object isn't totally nuts, but I
think we should do it well, and this patch doesn't really achieve
that.  But I don't think bpf wants fine-grained controls like this at
all -- as I pointed upthread, a fine-grained solution really wants
different treatment for the different capable() checks, and a bunch of
them won't resemble capabilities or /dev/bpf at all.