Re: [PATCH] tracing/user_events: Run BPF program if attached

[Beau Belgrave]

On Thu, Jun 01, 2023 at 11:46:13AM +0200, Christian Brauner wrote:
> On Wed, May 17, 2023 at 05:10:47PM -0700, Alexei Starovoitov wrote:
> > On Wed, May 17, 2023 at 9:50 AM Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > >
> > > > > Looks like user events were designed with intention to be unprivileged.
> > > > > When I looked at kernel/trace/trace_events_user.c I assumed root.
> > > > > I doubt other people reviewed it from security perspective.
> > > > >
> > > > > Recommending "chmod a+rw /sys/kernel/tracing/user_events_data" doesn't sound like a good idea.
> > > > >
> > > > > For example, I think the following is possible:
> > > > > fd = open("/sys/kernel/tracing/user_events_data")
> > > > > ioclt(fd, DIAG_IOCSDEL)
> > > > >   user_events_ioctl_del
> > > > >      delete_user_event(info->group, name);
> > > > >
> > > > > 'info' is different for every FD, but info->group is the same for all users/processes/fds,
> > > > > because only one global init_group is created.
> > > > > So one user can unregister other user event by knowing 'name'.
> > > > > A security hole, no?
> > 
> > ...
> > 
> > > Regarding deleting events, only users that are given access can delete
> > > events. They must know the event name, just like users with access to
> > > delete files must know a path (and have access to it). Since the
> > > write_index and other details are per-process, unless the user has
> > > access to either /sys/kernel/tracing/events/user_events/* or
> > > /sys/kernel/tracing/user_events_status, they do not know which names are
> > > being used.
> > >
> > > If that is not enough, we could require CAP_SYSADMIN to be able to
> > > delete events even when they have access to the file. Users can also
> > > apply SELinux policies per-file to achieve further isolation, if
> > > required.
> > 
> > Whether /sys/kernel/tracing/user_events_status gets g+rw
> > or it gets a+rw (as your documentation recommends)
> > it is still a security issue.
> > The "event name" is trivial to find out by looking at the source code
> > of the target process or just "string target_binary".
> > Restricting to cap_sysadmin is not the answer, since you want unpriv.
> > SElinux is not the answer either.
> > Since it's unpriv, different processes should not be able to mess with
> > user events of other processes.
> > It's a fundamental requirement of any kernel api.
> > This has to be fixed before any bpf discussion.
> > If it means that you need to redesign user_events do it now and
> > excuses like "it's uapi now, so we cannot fix it" are not going to fly.
> 
> Looking at this a little because I have a few minutes.
> What's all this unused code?
> 

These are stubs to integrate namespace support. I've been working on a
series that adds a tracing namespace support similiar to the IMA
namespace work [1]. That series is ending up taking more time than I
anticipated.

> static inline struct user_event_group
> *user_event_group_from_user_ns(struct user_namespace *user_ns)
> {
>         if (user_ns == &init_user_ns)
>                 return init_group;
> 
>         return NULL;
> }
> 
> static struct user_event_group *current_user_event_group(void)
> {
>         struct user_namespace *user_ns = current_user_ns();
>         struct user_event_group *group = NULL;
> 
>         while (user_ns) {
>                 group = user_event_group_from_user_ns(user_ns);
> 
>                 if (group)
>                         break;
> 
>                 user_ns = user_ns->parent;
>         }
> 
>         return group;
> }
> 
> User namespaces form strict hierarchies so you always end up at
> init_user_ns no matter where you start from in the hierarchy. Return the
> init_group and delete that code above.
> 

This is a good point, I'll delete this code and bring it back as part of
the namespace support patch series when appropriate.

> static char *user_event_group_system_name(struct user_namespace *user_ns)
> {
>         char *system_name;
>         int len = sizeof(USER_EVENTS_SYSTEM) + 1;
> 
>         if (user_ns != &init_user_ns) {
>                 /*
>                  * Unexpected at this point:
>                  * We only currently support init_user_ns.
>                  * When we enable more, this will trigger a failure so log.
>                  */
>                 pr_warn("user_events: Namespace other than init_user_ns!\n");
>                 return NULL;
>         }
> 
> Your delegation model is premised on file permissions of a single file
> in global tracefs. It won't work with user namespaces so let's not give
> the false impression that this is on the table.
> 

Users that are given access to the single file still should be able to
be isolated for each other. The series I'm working on does this by
changing the system name of user_events on a per-namespace basis.

This prevents one namespace from messing with another, such as deleting
their events, etc. I'll reach out to you for some time to discuss this
direction to ensure I'm not off base.

> Plus, all of this is also called in a single place during
> trace_events_user_init() which is called from fs_initcall() so you
> couldn't even pass a different user namespace if you wanted to because
> only init_user_ns exists.

In later series this is also called upon file open of user_events_data
to find the right group, etc. I'll get this code removed for now and
bring it back later.

Thanks,
-Beau

1. https://lore.kernel.org/linux-kernel/20230206140253.3755945-1-stefanb@xxxxxxxxxxxxx/