Re: [PATCH v3 bpf-next 00/21] bpf: Sysctl hook

[Andrey Ignatov <rdna@fb.com>]

Jann Horn <jannh@google.com> [Tue, 2019-04-09 13:42 -0700]:
> On Tue, Apr 9, 2019 at 10:26 PM Andrey Ignatov <rdna@fb.com> wrote:
> > The patch set introduces new BPF hook for sysctl.
> >
> > It adds new program type BPF_PROG_TYPE_CGROUP_SYSCTL and attach type
> > BPF_CGROUP_SYSCTL.
> >
> > BPF_CGROUP_SYSCTL hook is placed before calling to sysctl's proc_handler so
> > that accesses (read/write) to sysctl can be controlled for specific cgroup
> > and either allowed or denied, or traced.
> 
> Don't look at the credentials of "current" in a read or write handler.
> Consider what happens if, for example, someone inside a cgroup opens a
> sysctl file and passes the file descriptor to another process outside
> the cgroup over a unix domain socket, and that other process then
> writes to it. Either do your access check on open, or use the
> credentials that were saved during open() in the read/write handler.

This way this someone inside cgroup should already have control over
something running as root [1] outside of this cgroup, i.e. the game is
already lost, even without this hook.

[1] Since proc_sys_read() / proc_sys_write() check sysctl_perm() before
    execution reaches the hook.

This patch set doesn't look at credentials at all and relies on what
checks were already done at sys_open time or in proc_sys_call_handler()
before execution reaches the hook.

> > The hook has access to sysctl name, current sysctl value and (on write
> > only) to new sysctl value via corresponding helpers. New sysctl value can
> > be overridden by program. Both name and values (current/new) are
> > represented as strings same way they're visible in /proc/sys/. It is up to
> > program to parse these strings.
> 
> But even if a filter is installed that prevents all access to a
> sysctl, you can still read it by installing your own filter that, when
> a read is attempted the next time, dumps the value into a map or
> something like that, right?

No. This can be controlled by cgroup hierarchy and appropriate attach
flags, same way as with any other cgroup-bpf hook.

E.g. imagine there is a cgroup hierarchy:
  root/slice/container/

and container application runs in root/slice/container/ in a cgroup
namespace (CLONE_NEWCGROUP) that makes visible only "container/" part of
the hierarchy, i.e. from inside container application can't even see
"root/slice/".

Administrator can then attach sysctl hook to "root/slice/" with attach
flag NONE (bpf_attr.attach_flags = 0) what means nobody down the
hierarchy can override the program attached by administrator.

> > To help with parsing the most common kind of sysctl value, vector of
> > integers, two new helpers are provided: bpf_strtol and bpf_strtoul with
> > semantic similar to user space strtol(3) and strtoul(3).
> >
> > The hook also provides bpf_sysctl context with two fields:
> > * @write indicates whether sysctl is being read (= 0) or written (= 1);
> > * @file_pos is sysctl file position to read from or write to, can be
> >   overridden.
> >
> > The hook allows to make better isolation for containerized applications
> > that are run as root so that one container can't change a sysctl and affect
> > all other containers on a host, make changes to allowed sysctl in a safer
> > way and simplify sysctl tracing for cgroups.
> 
> Why can't you use a user namespace and isolate things properly that
> way? That would be much cleaner, wouldn't it?

I'm not sure I understand how user namespace helps here. From my
understanding it can only completely deny access to sysctl and can't do
fine-grained control for specific sysctl knobs. It also can't make
allow/deny decision based on sysctl value being written.

Basically user namespace is all or nothing. This sysctl hook provides a
way to implement fine-grained access control for sysctl knobs based on
sysctl name or value being written or whatever else policy administrator
can come up with.

-- 
Andrey Ignatov