Re: [RFC][PATCH] net/bpfilter: Remove this broken and apparently unmantained

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Fri, Jun 26, 2020 at 01:58:35PM +0900, Tetsuo Handa wrote:
> On 2020/06/26 10:51, Alexei Starovoitov wrote:
> > On Thu, Jun 25, 2020 at 06:36:34PM -0700, Linus Torvalds wrote:
> >> On Thu, Jun 25, 2020 at 12:34 PM David Miller <davem@davemloft.net> wrote:
> >>>
> >>> It's kernel code executing in userspace.  If you don't trust the
> >>> signed code you don't trust the signed code.
> >>>
> >>> Nothing is magic about a piece of code executing in userspace.
> >>
> >> Well, there's one real issue: the most likely thing that code is going
> >> to do is execute llvm to generate more code.
> 
> Wow! Are we going to allow execution of such complicated programs?

No. llvm was _never_ intended to be run from the blob.
bpfilter was envisioned as self contained binary. If it needed to do
optimizations on generated bpf code it would have to do them internally.

> I was hoping that fork_usermode_blob() accepts only simple program
> like the content of "hello64" generated by

pretty much. statically compiled elf that is self contained.

> For example, a usermode process started by fork_usermode_blob() which was initially
> containing
> 
> ----------
> while (read(0, &uid, sizeof(uid)) == sizeof(uid)) {
>     if (uid == 0)
>         write(1, "OK\n", 3);
>     else
>         write(1, "NG\n", 3);
> }
> ----------
> 
> can be somehow tampered like
> 
> ----------
> while (read(0, &uid, sizeof(uid)) == sizeof(uid)) {
>     if (uid != 0)
>         write(1, "OK\n", 3);
>     else
>         write(1, "NG\n", 3);
> }
> ----------
> 
> due to interference from the rest of the system, how can we say "we trust kernel
> code executing in userspace" ?

I answered this already in the previous email.
Use security_ptrace_access_check() LSM hook to make sure that no other process
can tamper with blob's memory when it's running as user process.
In the future it would be trivial to add a new ptrace flag to
make sure that blob's memory is not ptraceable from the start.

> My question is: how is the byte array (which was copied from kernel space) kept secure/intact
> under "root can poke into kernel or any process memory." environment? It is obvious that
> we can't say "we trust kernel code executing in userspace" without some mechanism.

Already answered.

> Currently fork_usermode_blob() is not providing security context for the byte array to be
> executed. We could modify fork_usermode_blob() to provide security context for LSMs, but
> I'll be more happy if we can implement that mechanism without counting on in-tree LSMs, for
> SELinux is too complicated to support.

I'm pretty sure it was answered in the upthread by selinux folks.
Quick recap: we can add security labels, sha, strings, you_name_it to the blob that
lsm hooks can track.
We can also add another LSM hook to fork_usermode_blob(), so if tomoyo is so worried
about blobs it would be able to reject all of them without too much work.

> 
> > I think that's Tetsuo's point about lack of LSM hooks is kernel_sock_shutdown().
> > Obviously, kernel_sock_shutdown() can be called by kernel only.
> 
> I can't catch what you mean. The kernel code executing in userspace uses syscall
> interface (e.g. SYSCALL_DEFINE2(shutdown, int, fd, int, how) path), doesn't it?

yes.

> > I suspect he's imaging a hypothetical situation where kernel bits of kernel module
> > interact with userblob bits of kernel module.
> > Then another root process tampers with memory of userblob.
> 
> Yes, how to protect the memory of userblob is a concern. The memory of userblob can
> interfere (or can be interfered by) the rest of the system is a problem.

answered.

> > I think this is trivially enforceable without creating new features.
> > Existing security_ptrace_access_check() LSM hook can prevent tampering with
> > memory of userblob.
> 
> There is security_ptrace_access_check() LSM hook, but no zero-configuration
> method is available.

huh?
tomoyo is not using that hook, but selinux and many other LSMs do.
please learn from others.

> > security label can carry that execution context.
> 
> If files get a chance to be associated with appropriate pathname and
> security label.

I can easily add a fake pathname to the blob, but it won't help tomoyo.
That's what I was saying all along.
pathname based security provides false sense of security.

I'm pretty sure this old blog has been read by many folks who
are following this thread, but it's worth reminding again:
https://securityblog.org/2006/04/19/security-anti-pattern-path-based-access-control/
I cannot agree more with Joshua.
Here is a quote:
"The most obvious problem with this is that not all objects are files and thus do not have paths."

> >> My personally strongest argument for remoiving this kernel code is
> >> that it's been there for a couple of years now, and it has never
> >> actually done anything useful, and there's no actual sign that it ever
> >> will, or that there is a solid plan in place for it.
> > 
> > you probably missed the detailed plan:
> > https://lore.kernel.org/bpf/20200609235631.ukpm3xngbehfqthz@ast-mbp.dhcp.thefacebook.com/
> > 
> > The project #3 is the above is the one we're working on right now.
> > It should be ready to post in a week.
> 
> I got a question on project #3. Given that "cat /sys/fs/bpf/my_ipv6_route"
> produces the same human output as "cat /proc/net/ipv6_route", how security
> checks which are done for "cat /proc/net/ipv6_route" can be enforced for
> "cat /sys/fs/bpf/my_ipv6_route" ? Unless same security checks (e.g. permission
> to read /proc/net/ipv6_route ) is enforced, such bpf usage sounds like a method
> for bypassing existing security mechanisms.

Standard file permissions. Nothing to do with bpf.