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

On Wed, May 17, 2023 at 05:10:47PM -0700, Alexei Starovoitov wrote:

On Wed, May 17, 2023 at 9:50 AM Beau Belgrave [off-list ref] wrote:

quoted

quoted

quoted

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?

...

quoted

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".

I guess, if they have access to the binary, etc.
So they need both access to the binary and to the tracefs directory.
We would not give them access like this in any normal setup other than a
developer environment.

Restricting to cap_sysadmin is not the answer, since you want unpriv.

We do not need unpriv to delete events, only to write and create events.

We allow unregistering call-sites, which would still work unpriv with
this requirement.

SElinux is not the answer either.
Since it's unpriv, different processes should not be able to mess with
user events of other processes.

How is this different than uprobes if we give a user access to
/sys/kernel/tracing/dynamic_events? Users can delete those as well. I
don't see a difference here.

In our production environments we are not giving out wide security to
this file.

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.

Thanks,
-Beau