netdev - Re: [PATCH RFC v4 net-next 17/26] tracing: allow eBPF programs to be attached to events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 15 Aug 2014 12:20:04 -0700
From:	Andy Lutomirski <luto@...capital.net>
To:	Alexei Starovoitov <ast@...mgrid.com>
Cc:	"David S. Miller" <davem@...emloft.net>,
	Ingo Molnar <mingo@...nel.org>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Daniel Borkmann <dborkman@...hat.com>,
	Chema Gonzalez <chema@...gle.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	"H. Peter Anvin" <hpa@...or.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Kees Cook <keescook@...omium.org>,
	Linux API <linux-api@...r.kernel.org>,
	Network Development <netdev@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH RFC v4 net-next 17/26] tracing: allow eBPF programs to be
 attached to events

On Fri, Aug 15, 2014 at 12:07 PM, Alexei Starovoitov <ast@...mgrid.com> wrote:
> On Fri, Aug 15, 2014 at 11:53 AM, Andy Lutomirski <luto@...capital.net> wrote:
>> On Fri, Aug 15, 2014 at 10:51 AM, Alexei Starovoitov <ast@...mgrid.com> wrote:
>>> On Fri, Aug 15, 2014 at 10:25 AM, Andy Lutomirski <luto@...capital.net> wrote:
>>>> On Wed, Aug 13, 2014 at 12:57 AM, Alexei Starovoitov <ast@...mgrid.com> wrote:
>>>>> User interface:
>>>>> fd = open("/sys/kernel/debug/tracing/__event__/filter")
>>>>>
>>>>> write(fd, "bpf_123")
>>>>
>>>> I didn't follow all the code flow leading to parsing the "bpf_123"
>>>> string, but if it works the way I imagine it does, it's a security
>>>> problem.  In general, write(2) should never do anything that involves
>>>> any security-relevant context of the caller.
>>>>
>>>> Ideally, you would look up fd 123 in the file table of whomever called
>>>> open.  If that's difficult to implement efficiently, then it would be
>>>> nice to have some check that the callers of write(2) and open(2) are
>>>> the same task and that exec wasn't called in between.
>>>>
>>>> This isn't a very severe security issue because you need privilege to
>>>> open the thing in the first place, but it would still be nice to
>>>> address.
>>>
>>> hmm. you need to be root to open the events anyway.
>>> pretty much the whole tracing for root only, since any kernel data
>>> structures can be printed, stored into maps and so on.
>>> So I don't quite follow your security concern here.
>>>
>>> Even say root opens a tracepoint and does exec() of another
>>> app that uploads ebpf program, gets program_fd and does
>>> write into tracepoint fd. The root app that did this open() is
>>> doing exec() on purpose. It's not like it's exec-ing something
>>> it doesn't know about.
>>
>> As long as everyone who can debugfs/tracing/whatever has all
>> privileges, then this is fine.
>>
>> If not, then it's a minor capability or MAC bypass.  Suppose you only
>> have one capability or, more realistically, limited MAC permissions.
>
> Hard to think of MAC abbreviation other than in networking way... ;)
> MAC bypass... kinda sounds like L3 networking without L2... ;)
>
>> You can still open the tracing file, pass it to an unwitting program
>> with elevated permission (e.g. using selinux's entrypoint mechanism),
>> and trick that program into writing bpf_123.
>
> hmm, but to open tracing file you'd need to be root already...
> otherwise yeah, if non-root could open it and pass it, then it
> would be nasty.
>
>> Admittedly, it's unlikely that fd 123 will be an *eBPF* fd, but the
>> attack is possible.
>>
>> I don't think that fixing this should be a prerequisite for merging,
>> since the risk is so small.  Nonetheless, it would be nice.  (This
>> family of attacks has lead to several root vulnerabilities in the
>> past.)
>
> Ok. I think keeping a track of pid between open and write is kinda
> ugly.

Agreed.

TBH, I would just add a comment to the open implementation saying
that, if unprivileged or less privileged open is allowed, then this
needs to be fixed.

> Should we add some new CAP flag and check it for all file
> ops? Another option is to conditionally make open() of tracing
> files as cloexec...

That won't help.  The same attack can be done with SCM_RIGHTS, and
cloexec can be cleared.

-- 
Andy Lutomirski
AMA Capital Management, LLC
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html