Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Peter Zijlstra]

On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> It's also worth noting that fine-grained control via a scoped mechanism
> would likely only be used to implement *more restrictions* on Android,
> not to make the feature less aggressive.

> It's desirable for perf events to be disabled by default for non-root
> across the board on Android.

Right, but this is Android. The knob seems to now also live in Debian
(and derived) distros. And there it is utter crap.

It completely defeats having perf for a fairly large segment of
corporate developers who do not get to have root on their own machines
(which is stupid policy but whatever).

It similarly defeats development of self profiling JITs and whatnot.

A capability would allow people to run perf (or another sanctioned
binary), even though in general they cannot do sys_perf_event_open().

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Peter Zijlstra]

On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> It's also worth noting that fine-grained control via a scoped mechanism
> would likely only be used to implement *more restrictions* on Android,
> not to make the feature less aggressive.

> It's desirable for perf events to be disabled by default for non-root
> across the board on Android.

Right, but this is Android. The knob seems to now also live in Debian
(and derived) distros. And there it is utter crap.

It completely defeats having perf for a fairly large segment of
corporate developers who do not get to have root on their own machines
(which is stupid policy but whatever).

It similarly defeats development of self profiling JITs and whatnot.

A capability would allow people to run perf (or another sanctioned
binary), even though in general they cannot do sys_perf_event_open().

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Wed, 2016-10-19 at 07:26 -0300, Arnaldo Carvalho de Melo wrote:
> 
> But self profiling JITs would be useful for non-developers, on Android
> (anywhere, really), and for that it would require being able to at
> least, well, self profile, using sys_perf_event_open() by a normal
> process, limited to profiling itself, no?
> 
> This not being possible, self profiling will use some other means, its
> like sys_perf_event_open() never existed for them.
> 
> - Arnaldo

It would defeat the purpose of the security feature if it was exposed to
apps on Android. There are other ways for Chromium (including WebView)
and the ART JIT to profile. AFAIK, they've never actually considered
using perf for their JIT profiling. It wasn't something that anyone
cared about when this was implemented.

Chromium would *certainly* never use perf for this. They use a much
tighter sandbox than the Android app sandbox. It doesn't have system
calls like open(), let alone perf events. Their seccomp-bpf usage is to
reduce kernel attack surface, since it has proven to be the easiest way
out of a sandbox. They don't even allow futex() without filtering based 
on the parameters anymore. That proved to be a major
issue.

The only case where untrusted apps declaring the privileges they need
actually works out is when the privileges are high-level controls over
access to a user's personal information. That way, they can be exposed
to the user with the ability to reject access when it's first needed or
toggle it off later. The strength of the app sandbox isn't something
that end users can be responsible for. Permissions also don't work if
apps bypass them with local privilege escalation vulnerabilities.

Even for the base system, no perf access is better than having it used
anywhere. The difference is only that the base system can actually be
trusted to declare what it needs, but those components can be exploited
so it's still best if they are trusted as little as possible at runtime.

I could see Android completely removing unprivileged access down the
road too, not as a form of access control (apps already run as unique
uid/gid pairs, etc.) but to remove a non-trivial form of kernel attack
surface. The perf API isn't being singled out here. It just happened to
be the first case where a kernel API was restricted to developer usage
on Android. Should expect more of this.

If you want perf exposed, make it secure. Stop writing kernel code in a
memory unsafe language and start using isolation. Not going to happen in
all likelihood, so kernel code will be increasingly walled off instead
since it's the biggest liability on the system. Fixing bugs on a case by
case basis doesn't work for systems that need even basic security.

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Wed, 2016-10-19 at 07:26 -0300, Arnaldo Carvalho de Melo wrote:
> 
> But self profiling JITs would be useful for non-developers, on Android
> (anywhere, really), and for that it would require being able to at
> least, well, self profile, using sys_perf_event_open() by a normal
> process, limited to profiling itself, no?
> 
> This not being possible, self profiling will use some other means, its
> like sys_perf_event_open() never existed for them.
> 
> - Arnaldo

It would defeat the purpose of the security feature if it was exposed to
apps on Android. There are other ways for Chromium (including WebView)
and the ART JIT to profile. AFAIK, they've never actually considered
using perf for their JIT profiling. It wasn't something that anyone
cared about when this was implemented.

Chromium would *certainly* never use perf for this. They use a much
tighter sandbox than the Android app sandbox. It doesn't have system
calls like open(), let alone perf events. Their seccomp-bpf usage is to
reduce kernel attack surface, since it has proven to be the easiest way
out of a sandbox. They don't even allow futex() without filtering based 
on the parameters anymore. That proved to be a major
issue.

The only case where untrusted apps declaring the privileges they need
actually works out is when the privileges are high-level controls over
access to a user's personal information. That way, they can be exposed
to the user with the ability to reject access when it's first needed or
toggle it off later. The strength of the app sandbox isn't something
that end users can be responsible for. Permissions also don't work if
apps bypass them with local privilege escalation vulnerabilities.

Even for the base system, no perf access is better than having it used
anywhere. The difference is only that the base system can actually be
trusted to declare what it needs, but those components can be exploited
so it's still best if they are trusted as little as possible at runtime.

I could see Android completely removing unprivileged access down the
road too, not as a form of access control (apps already run as unique
uid/gid pairs, etc.) but to remove a non-trivial form of kernel attack
surface. The perf API isn't being singled out here. It just happened to
be the first case where a kernel API was restricted to developer usage
on Android. Should expect more of this.

If you want perf exposed, make it secure. Stop writing kernel code in a
memory unsafe language and start using isolation. Not going to happen in
all likelihood, so kernel code will be increasingly walled off instead
since it's the biggest liability on the system. Fixing bugs on a case by
case basis doesn't work for systems that need even basic security.

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Wed, 2016-10-19 at 10:41 +0100, Mark Rutland wrote:
> On Mon, Oct 17, 2016 at 10:54:33AM -0400, Daniel Micay wrote:
> > On Mon, 2016-10-17 at 14:44 +0100, Mark Rutland wrote:
> > > It's also my understanding that for Android, perf_event_paranoid
> > > is
> > > lowered when the user enables developer mode (rather than only
> > > when an
> > > external debugger is attached); is that correct?
> > 
> > It's exposed as a "system property" marked as writable by the shell
> > user, so the Android Debug Bridge shell can lower it. The debugging
> > tools learned how to toggle it off automatically when they're used.
> > It
> > intentionally isn't a persist. prefixed property so the setting also
> > goes away on reboot.
> > 
> > ADB (incl. the shell user) isn't available until developer mode is
> > enabled + ADB is toggled on in the developer settings, and then it
> > still
> > requires whitelisting keys.
> 
> Ah; so I'd misunderstood somewhat.
> 
> I was under the (clearly mistaken) impression that this was lowered
> when
> developer mode was enabled, rather than only when it was both enabled
> and ADB was connected, for example.
> 
> Thanks for clearing that up!

ADB provides a shell as the 'shell' user, and that user has the ability
to toggle the sysctl. So profiling tools were able to be taught to do
that automatically. It's the only way that the 'shell' user is actually
exposed. For example, a terminal app just runs in the untrusted_app
SELinux domain as a unique unprivileged uid/gid pair, not as the much
more privileged ADB 'shell' domain.

So it doesn't actually get toggled off you use ADB to do something else.

ADB itself is pretty much comparable to SSH, but over USB (i.e. key-
based way of getting a shell).

The 'shell' user has tools like 'run-as' to be able to run things as
various apps (if they are marked debuggable), so in theory it could be
finer-grained and act only there, for the app being debugged. It would
be really hard to cover all use cases and maybe things other than apps
though (although in an Android 'user' build, the base system itself
isn't very debuggable, you really need 'userdebug' or 'eng' which isn't
what ships on end user devices).

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Wed, 2016-10-19 at 10:41 +0100, Mark Rutland wrote:
> On Mon, Oct 17, 2016 at 10:54:33AM -0400, Daniel Micay wrote:
> > On Mon, 2016-10-17 at 14:44 +0100, Mark Rutland wrote:
> > > It's also my understanding that for Android, perf_event_paranoid
> > > is
> > > lowered when the user enables developer mode (rather than only
> > > when an
> > > external debugger is attached); is that correct?
> > 
> > It's exposed as a "system property" marked as writable by the shell
> > user, so the Android Debug Bridge shell can lower it. The debugging
> > tools learned how to toggle it off automatically when they're used.
> > It
> > intentionally isn't a persist. prefixed property so the setting also
> > goes away on reboot.
> > 
> > ADB (incl. the shell user) isn't available until developer mode is
> > enabled + ADB is toggled on in the developer settings, and then it
> > still
> > requires whitelisting keys.
> 
> Ah; so I'd misunderstood somewhat.
> 
> I was under the (clearly mistaken) impression that this was lowered
> when
> developer mode was enabled, rather than only when it was both enabled
> and ADB was connected, for example.
> 
> Thanks for clearing that up!

ADB provides a shell as the 'shell' user, and that user has the ability
to toggle the sysctl. So profiling tools were able to be taught to do
that automatically. It's the only way that the 'shell' user is actually
exposed. For example, a terminal app just runs in the untrusted_app
SELinux domain as a unique unprivileged uid/gid pair, not as the much
more privileged ADB 'shell' domain.

So it doesn't actually get toggled off you use ADB to do something else.

ADB itself is pretty much comparable to SSH, but over USB (i.e. key-
based way of getting a shell).

The 'shell' user has tools like 'run-as' to be able to run things as
various apps (if they are marked debuggable), so in theory it could be
finer-grained and act only there, for the app being debugged. It would
be really hard to cover all use cases and maybe things other than apps
though (although in an Android 'user' build, the base system itself
isn't very debuggable, you really need 'userdebug' or 'eng' which isn't
what ships on end user devices).

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Arnaldo Carvalho de Melo]

Em Wed, Oct 19, 2016 at 12:01:26PM +0200, Peter Zijlstra escreveu:
> On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> > It's also worth noting that fine-grained control via a scoped mechanism
> > would likely only be used to implement *more restrictions* on Android,
> > not to make the feature less aggressive.
 
> > It's desirable for perf events to be disabled by default for non-root
> > across the board on Android.
 
> Right, but this is Android. The knob seems to now also live in Debian
> (and derived) distros. And there it is utter crap.
 
> It completely defeats having perf for a fairly large segment of
> corporate developers who do not get to have root on their own machines
> (which is stupid policy but whatever).
 
> It similarly defeats development of self profiling JITs and whatnot.
 
> A capability would allow people to run perf (or another sanctioned
> binary), even though in general they cannot do sys_perf_event_open().

But self profiling JITs would be useful for non-developers, on Android
(anywhere, really), and for that it would require being able to at
least, well, self profile, using sys_perf_event_open() by a normal
process, limited to profiling itself, no?

This not being possible, self profiling will use some other means, its
like sys_perf_event_open() never existed for them.

- Arnaldo

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Arnaldo Carvalho de Melo]

Em Wed, Oct 19, 2016 at 12:01:26PM +0200, Peter Zijlstra escreveu:
> On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> > It's also worth noting that fine-grained control via a scoped mechanism
> > would likely only be used to implement *more restrictions* on Android,
> > not to make the feature less aggressive.
 
> > It's desirable for perf events to be disabled by default for non-root
> > across the board on Android.
 
> Right, but this is Android. The knob seems to now also live in Debian
> (and derived) distros. And there it is utter crap.
 
> It completely defeats having perf for a fairly large segment of
> corporate developers who do not get to have root on their own machines
> (which is stupid policy but whatever).
 
> It similarly defeats development of self profiling JITs and whatnot.
 
> A capability would allow people to run perf (or another sanctioned
> binary), even though in general they cannot do sys_perf_event_open().

But self profiling JITs would be useful for non-developers, on Android
(anywhere, really), and for that it would require being able to at
least, well, self profile, using sys_perf_event_open() by a normal
process, limited to profiling itself, no?

This not being possible, self profiling will use some other means, its
like sys_perf_event_open() never existed for them.

- Arnaldo

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Mark Rutland]

On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> It's also worth noting that fine-grained control via a scoped
> mechanism would likely only be used to implement *more restrictions*
> on Android, not to make the feature less aggressive. It's desirable
> for perf events to be disabled by default for non-root across the
> board on Android.  The part that's imperfect is that when a developer
> uses a profiling tool, unprivileged usage is automatically enabled
> across the board until reboot. Ideally, it would be enabled only for
> the scope where it's needed. 

Sure; understood.

> It would be very tricky to implement though, especially without adding
> friction, and it would only have value for protecting devices being
> used for development. It really doesn't seem to be worth the trouble,
> especially since it doesn't persist on reboot. It's only a temporary
> security hole and only for developer devices.

I can see that for Android this isn't much of a win. It is beneficial
elsewhere, and covers a larger set of use-cases.

If perf were a filesystem object, we'd only allow access by a given
'perf' group, and that would be sufficient to avoid most of that
friction (IIUC). I wonder what we can do that's similar.

Thanks,
Mark.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Mark Rutland]

On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> It's also worth noting that fine-grained control via a scoped
> mechanism would likely only be used to implement *more restrictions*
> on Android, not to make the feature less aggressive. It's desirable
> for perf events to be disabled by default for non-root across the
> board on Android.  The part that's imperfect is that when a developer
> uses a profiling tool, unprivileged usage is automatically enabled
> across the board until reboot. Ideally, it would be enabled only for
> the scope where it's needed. 

Sure; understood.

> It would be very tricky to implement though, especially without adding
> friction, and it would only have value for protecting devices being
> used for development. It really doesn't seem to be worth the trouble,
> especially since it doesn't persist on reboot. It's only a temporary
> security hole and only for developer devices.

I can see that for Android this isn't much of a win. It is beneficial
elsewhere, and covers a larger set of use-cases.

If perf were a filesystem object, we'd only allow access by a given
'perf' group, and that would be sufficient to avoid most of that
friction (IIUC). I wonder what we can do that's similar.

Thanks,
Mark.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Mark Rutland]

On Mon, Oct 17, 2016 at 10:54:33AM -0400, Daniel Micay wrote:
> On Mon, 2016-10-17 at 14:44 +0100, Mark Rutland wrote:
> > It's also my understanding that for Android, perf_event_paranoid is
> > lowered when the user enables developer mode (rather than only when an
> > external debugger is attached); is that correct?
> 
> It's exposed as a "system property" marked as writable by the shell
> user, so the Android Debug Bridge shell can lower it. The debugging
> tools learned how to toggle it off automatically when they're used. It
> intentionally isn't a persist. prefixed property so the setting also
> goes away on reboot.
> 
> ADB (incl. the shell user) isn't available until developer mode is
> enabled + ADB is toggled on in the developer settings, and then it still
> requires whitelisting keys.

Ah; so I'd misunderstood somewhat.

I was under the (clearly mistaken) impression that this was lowered when
developer mode was enabled, rather than only when it was both enabled
and ADB was connected, for example.

Thanks for clearing that up!

Thanks,
Mark.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Mark Rutland]

On Mon, Oct 17, 2016 at 10:54:33AM -0400, Daniel Micay wrote:
> On Mon, 2016-10-17 at 14:44 +0100, Mark Rutland wrote:
> > It's also my understanding that for Android, perf_event_paranoid is
> > lowered when the user enables developer mode (rather than only when an
> > external debugger is attached); is that correct?
> 
> It's exposed as a "system property" marked as writable by the shell
> user, so the Android Debug Bridge shell can lower it. The debugging
> tools learned how to toggle it off automatically when they're used. It
> intentionally isn't a persist. prefixed property so the setting also
> goes away on reboot.
> 
> ADB (incl. the shell user) isn't available until developer mode is
> enabled + ADB is toggled on in the developer settings, and then it still
> requires whitelisting keys.

Ah; so I'd misunderstood somewhat.

I was under the (clearly mistaken) impression that this was lowered when
developer mode was enabled, rather than only when it was both enabled
and ADB was connected, for example.

Thanks for clearing that up!

Thanks,
Mark.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Peter Zijlstra]

On Wed, Oct 19, 2016 at 07:26:02AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Oct 19, 2016 at 12:01:26PM +0200, Peter Zijlstra escreveu:
> > On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> > > It's also worth noting that fine-grained control via a scoped mechanism
> > > would likely only be used to implement *more restrictions* on Android,
> > > not to make the feature less aggressive.
>  
> > > It's desirable for perf events to be disabled by default for non-root
> > > across the board on Android.
>  
> > Right, but this is Android. The knob seems to now also live in Debian
> > (and derived) distros. And there it is utter crap.
>  
> > It completely defeats having perf for a fairly large segment of
> > corporate developers who do not get to have root on their own machines
> > (which is stupid policy but whatever).
>  
> > It similarly defeats development of self profiling JITs and whatnot.
>  
> > A capability would allow people to run perf (or another sanctioned
> > binary), even though in general they cannot do sys_perf_event_open().
> 
> But self profiling JITs would be useful for non-developers, on Android
> (anywhere, really), and for that it would require being able to at
> least, well, self profile, using sys_perf_event_open() by a normal
> process, limited to profiling itself, no?
> 
> This not being possible, self profiling will use some other means, its
> like sys_perf_event_open() never existed for them.

Right, so with capabilities, we could grant the binary the capability to
use sys_perf_event_open().

That would still leave developers of said JIT in a tight spot, because
there'd be no way to set the capability on their freshly compiled
binary.

They'd have to be granted the capability to the user, using pam_cap.
Which would involve corp. IT doing something sensible, ergo, this'll
never happen :-(.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Peter Zijlstra]

On Wed, Oct 19, 2016 at 07:26:02AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Wed, Oct 19, 2016 at 12:01:26PM +0200, Peter Zijlstra escreveu:
> > On Tue, Oct 18, 2016 at 05:15:01PM -0400, Daniel Micay wrote:
> > > It's also worth noting that fine-grained control via a scoped mechanism
> > > would likely only be used to implement *more restrictions* on Android,
> > > not to make the feature less aggressive.
>  
> > > It's desirable for perf events to be disabled by default for non-root
> > > across the board on Android.
>  
> > Right, but this is Android. The knob seems to now also live in Debian
> > (and derived) distros. And there it is utter crap.
>  
> > It completely defeats having perf for a fairly large segment of
> > corporate developers who do not get to have root on their own machines
> > (which is stupid policy but whatever).
>  
> > It similarly defeats development of self profiling JITs and whatnot.
>  
> > A capability would allow people to run perf (or another sanctioned
> > binary), even though in general they cannot do sys_perf_event_open().
> 
> But self profiling JITs would be useful for non-developers, on Android
> (anywhere, really), and for that it would require being able to at
> least, well, self profile, using sys_perf_event_open() by a normal
> process, limited to profiling itself, no?
> 
> This not being possible, self profiling will use some other means, its
> like sys_perf_event_open() never existed for them.

Right, so with capabilities, we could grant the binary the capability to
use sys_perf_event_open().

That would still leave developers of said JIT in a tight spot, because
there'd be no way to set the capability on their freshly compiled
binary.

They'd have to be granted the capability to the user, using pam_cap.
Which would involve corp. IT doing something sensible, ergo, this'll
never happen :-(.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Tue, 2016-10-18 at 13:48 -0700, Kees Cook wrote:
> On Mon, Oct 17, 2016 at 6:44 AM, Mark Rutland 
> wrote:
> > Hi,
> > 
> > Attempt to revive discussions below...
> > 
> > On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
> > > When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> > > all access to performance events by users without CAP_SYS_ADMIN.
> > > 
> > > This new level of restriction is intended to reduce the attack
> > > surface of the kernel. Perf is a valuable tool for developers but
> > > is generally unnecessary and unused on production systems. Perf
> > > may
> > > open up an attack vector to vulnerable device-specific drivers as
> > > recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> > > CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> > > restriction allows for a safe default to be set on production
> > > systems
> > > while leaving a simple means for developers to grant access [1].
> > > 
> > > This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> > > Spengler. It is based on a patch by Ben Hutchings [2]. Ben's
> > > patches
> > > have been modified and split up to address on-list feedback.
> > > 
> > > kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> > > Android [3].
> > 
> > While people weren't particularly happy with this global toggle
> > approach, my understanding from face-to-face discussions at LSS2016
> > was
> > that people were happy with a more scoped restriction (e.g. using
> > capabilities or some other access control mechanism), but no-one had
> > the
> > time to work on that.
> > 
> > Does that match everyone's understanding, or am I mistaken?
> 
> That's correct: some kind of finer-grain control would be preferred to
> the maintainer, but no one has had time to work on it. (The =3 sysctl
> setting present in Android, Debian, and Ubuntu satisfies most people.)
> 
> -Kees

It's also worth noting that fine-grained control via a scoped mechanism
would likely only be used to implement *more restrictions* on Android,
not to make the feature less aggressive. It's desirable for perf events
to be disabled by default for non-root across the board on Android. The
part that's imperfect is that when a developer uses a profiling tool,
unprivileged usage is automatically enabled across the board until
reboot. Ideally, it would be enabled only for the scope where it's
needed. It would be very tricky to implement though, especially without
adding friction, and it would only have value for protecting devices
being used for development. It really doesn't seem to be worth the
trouble, especially since it doesn't persist on reboot. It's only a
temporary security hole and only for developer devices.

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Tue, 2016-10-18 at 13:48 -0700, Kees Cook wrote:
> On Mon, Oct 17, 2016 at 6:44 AM, Mark Rutland 
> wrote:
> > Hi,
> > 
> > Attempt to revive discussions below...
> > 
> > On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
> > > When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> > > all access to performance events by users without CAP_SYS_ADMIN.
> > > 
> > > This new level of restriction is intended to reduce the attack
> > > surface of the kernel. Perf is a valuable tool for developers but
> > > is generally unnecessary and unused on production systems. Perf
> > > may
> > > open up an attack vector to vulnerable device-specific drivers as
> > > recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> > > CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> > > restriction allows for a safe default to be set on production
> > > systems
> > > while leaving a simple means for developers to grant access [1].
> > > 
> > > This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> > > Spengler. It is based on a patch by Ben Hutchings [2]. Ben's
> > > patches
> > > have been modified and split up to address on-list feedback.
> > > 
> > > kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> > > Android [3].
> > 
> > While people weren't particularly happy with this global toggle
> > approach, my understanding from face-to-face discussions at LSS2016
> > was
> > that people were happy with a more scoped restriction (e.g. using
> > capabilities or some other access control mechanism), but no-one had
> > the
> > time to work on that.
> > 
> > Does that match everyone's understanding, or am I mistaken?
> 
> That's correct: some kind of finer-grain control would be preferred to
> the maintainer, but no one has had time to work on it. (The =3 sysctl
> setting present in Android, Debian, and Ubuntu satisfies most people.)
> 
> -Kees

It's also worth noting that fine-grained control via a scoped mechanism
would likely only be used to implement *more restrictions* on Android,
not to make the feature less aggressive. It's desirable for perf events
to be disabled by default for non-root across the board on Android. The
part that's imperfect is that when a developer uses a profiling tool,
unprivileged usage is automatically enabled across the board until
reboot. Ideally, it would be enabled only for the scope where it's
needed. It would be very tricky to implement though, especially without
adding friction, and it would only have value for protecting devices
being used for development. It really doesn't seem to be worth the
trouble, especially since it doesn't persist on reboot. It's only a
temporary security hole and only for developer devices.

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Kees Cook]

On Mon, Oct 17, 2016 at 6:44 AM, Mark Rutland  wrote:
> Hi,
>
> Attempt to revive discussions below...
>
> On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
>> When kernel.perf_event_paranoid is set to 3 (or greater), disallow
>> all access to performance events by users without CAP_SYS_ADMIN.
>>
>> This new level of restriction is intended to reduce the attack
>> surface of the kernel. Perf is a valuable tool for developers but
>> is generally unnecessary and unused on production systems. Perf may
>> open up an attack vector to vulnerable device-specific drivers as
>> recently demonstrated in CVE-2016-0805, CVE-2016-0819,
>> CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
>> restriction allows for a safe default to be set on production systems
>> while leaving a simple means for developers to grant access [1].
>>
>> This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
>> Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
>> have been modified and split up to address on-list feedback.
>>
>> kernel.perf_event_paranoid=3 is the default on both Debian [2] and
>> Android [3].
>
> While people weren't particularly happy with this global toggle
> approach, my understanding from face-to-face discussions at LSS2016 was
> that people were happy with a more scoped restriction (e.g. using
> capabilities or some other access control mechanism), but no-one had the
> time to work on that.
>
> Does that match everyone's understanding, or am I mistaken?

That's correct: some kind of finer-grain control would be preferred to
the maintainer, but no one has had time to work on it. (The =3 sysctl
setting present in Android, Debian, and Ubuntu satisfies most people.)

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Kees Cook]

On Mon, Oct 17, 2016 at 6:44 AM, Mark Rutland  wrote:
> Hi,
>
> Attempt to revive discussions below...
>
> On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
>> When kernel.perf_event_paranoid is set to 3 (or greater), disallow
>> all access to performance events by users without CAP_SYS_ADMIN.
>>
>> This new level of restriction is intended to reduce the attack
>> surface of the kernel. Perf is a valuable tool for developers but
>> is generally unnecessary and unused on production systems. Perf may
>> open up an attack vector to vulnerable device-specific drivers as
>> recently demonstrated in CVE-2016-0805, CVE-2016-0819,
>> CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
>> restriction allows for a safe default to be set on production systems
>> while leaving a simple means for developers to grant access [1].
>>
>> This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
>> Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
>> have been modified and split up to address on-list feedback.
>>
>> kernel.perf_event_paranoid=3 is the default on both Debian [2] and
>> Android [3].
>
> While people weren't particularly happy with this global toggle
> approach, my understanding from face-to-face discussions at LSS2016 was
> that people were happy with a more scoped restriction (e.g. using
> capabilities or some other access control mechanism), but no-one had the
> time to work on that.
>
> Does that match everyone's understanding, or am I mistaken?

That's correct: some kind of finer-grain control would be preferred to
the maintainer, but no one has had time to work on it. (The =3 sysctl
setting present in Android, Debian, and Ubuntu satisfies most people.)

-Kees

-- 
Kees Cook
Nexus Security

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Mon, 2016-10-17 at 14:44 +0100, Mark Rutland wrote:
> Hi,
> 
> Attempt to revive discussions below...
> 
> On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
> > When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> > all access to performance events by users without CAP_SYS_ADMIN.
> > 
> > This new level of restriction is intended to reduce the attack
> > surface of the kernel. Perf is a valuable tool for developers but
> > is generally unnecessary and unused on production systems. Perf may
> > open up an attack vector to vulnerable device-specific drivers as
> > recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> > CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> > restriction allows for a safe default to be set on production
> > systems
> > while leaving a simple means for developers to grant access [1].
> > 
> > This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> > Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
> > have been modified and split up to address on-list feedback.
> > 
> > kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> > Android [3].
> 
> While people weren't particularly happy with this global toggle
> approach, my understanding from face-to-face discussions at LSS2016
> was
> that people were happy with a more scoped restriction (e.g. using
> capabilities or some other access control mechanism), but no-one had
> the
> time to work on that.
> 
> Does that match everyone's understanding, or am I mistaken?
> 
> It's also my understanding that for Android, perf_event_paranoid is
> lowered when the user enables developer mode (rather than only when an
> external debugger is attached); is that correct?

It's exposed as a "system property" marked as writable by the shell
user, so the Android Debug Bridge shell can lower it. The debugging
tools learned how to toggle it off automatically when they're used. It
intentionally isn't a persist. prefixed property so the setting also
goes away on reboot.

ADB (incl. the shell user) isn't available until developer mode is
enabled + ADB is toggled on in the developer settings, and then it still
requires whitelisting keys.

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Daniel Micay]

On Mon, 2016-10-17 at 14:44 +0100, Mark Rutland wrote:
> Hi,
> 
> Attempt to revive discussions below...
> 
> On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
> > When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> > all access to performance events by users without CAP_SYS_ADMIN.
> > 
> > This new level of restriction is intended to reduce the attack
> > surface of the kernel. Perf is a valuable tool for developers but
> > is generally unnecessary and unused on production systems. Perf may
> > open up an attack vector to vulnerable device-specific drivers as
> > recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> > CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> > restriction allows for a safe default to be set on production
> > systems
> > while leaving a simple means for developers to grant access [1].
> > 
> > This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> > Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
> > have been modified and split up to address on-list feedback.
> > 
> > kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> > Android [3].
> 
> While people weren't particularly happy with this global toggle
> approach, my understanding from face-to-face discussions at LSS2016
> was
> that people were happy with a more scoped restriction (e.g. using
> capabilities or some other access control mechanism), but no-one had
> the
> time to work on that.
> 
> Does that match everyone's understanding, or am I mistaken?
> 
> It's also my understanding that for Android, perf_event_paranoid is
> lowered when the user enables developer mode (rather than only when an
> external debugger is attached); is that correct?

It's exposed as a "system property" marked as writable by the shell
user, so the Android Debug Bridge shell can lower it. The debugging
tools learned how to toggle it off automatically when they're used. It
intentionally isn't a persist. prefixed property so the setting also
goes away on reboot.

ADB (incl. the shell user) isn't available until developer mode is
enabled + ADB is toggled on in the developer settings, and then it still
requires whitelisting keys.

signature.asc
Description: This is a digitally signed message part

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Mark Rutland]

Hi,

Attempt to revive discussions below...

On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
> When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> all access to performance events by users without CAP_SYS_ADMIN.
> 
> This new level of restriction is intended to reduce the attack
> surface of the kernel. Perf is a valuable tool for developers but
> is generally unnecessary and unused on production systems. Perf may
> open up an attack vector to vulnerable device-specific drivers as
> recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> restriction allows for a safe default to be set on production systems
> while leaving a simple means for developers to grant access [1].
> 
> This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
> have been modified and split up to address on-list feedback.
> 
> kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> Android [3].

While people weren't particularly happy with this global toggle
approach, my understanding from face-to-face discussions at LSS2016 was
that people were happy with a more scoped restriction (e.g. using
capabilities or some other access control mechanism), but no-one had the
time to work on that.

Does that match everyone's understanding, or am I mistaken?

It's also my understanding that for Android, perf_event_paranoid is
lowered when the user enables developer mode (rather than only when an
external debugger is attached); is that correct?

Thanks,
Mark.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Mark Rutland]

Hi,

Attempt to revive discussions below...

On Wed, Jul 27, 2016 at 07:45:46AM -0700, Jeff Vander Stoep wrote:
> When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> all access to performance events by users without CAP_SYS_ADMIN.
> 
> This new level of restriction is intended to reduce the attack
> surface of the kernel. Perf is a valuable tool for developers but
> is generally unnecessary and unused on production systems. Perf may
> open up an attack vector to vulnerable device-specific drivers as
> recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> restriction allows for a safe default to be set on production systems
> while leaving a simple means for developers to grant access [1].
> 
> This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
> have been modified and split up to address on-list feedback.
> 
> kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> Android [3].

While people weren't particularly happy with this global toggle
approach, my understanding from face-to-face discussions at LSS2016 was
that people were happy with a more scoped restriction (e.g. using
capabilities or some other access control mechanism), but no-one had the
time to work on that.

Does that match everyone's understanding, or am I mistaken?

It's also my understanding that for Android, perf_event_paranoid is
lowered when the user enables developer mode (rather than only when an
external debugger is attached); is that correct?

Thanks,
Mark.

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Kees Cook]

On Wed, Jul 27, 2016 at 7:45 AM, Jeff Vander Stoep  wrote:
> When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> all access to performance events by users without CAP_SYS_ADMIN.
>
> This new level of restriction is intended to reduce the attack
> surface of the kernel. Perf is a valuable tool for developers but
> is generally unnecessary and unused on production systems. Perf may
> open up an attack vector to vulnerable device-specific drivers as
> recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> restriction allows for a safe default to be set on production systems
> while leaving a simple means for developers to grant access [1].
>
> This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
> have been modified and split up to address on-list feedback.
>
> kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> Android [3].
>
> [1] Making perf available to developers on Android:
> https://android-review.googlesource.com/#/c/234400/
> [2] Original patch by Ben Hutchings:
> https://lkml.org/lkml/2016/1/11/587
> [3] https://android-review.googlesource.com/#/c/234743/
>
> Signed-off-by: Jeff Vander Stoep 

Thanks for splitting this up! It'll be nice to have this delta out of
Debian and Android.

Reviewed-by: Kees Cook 

-Kees

> ---
>  Documentation/sysctl/kernel.txt | 1 +
>  include/linux/perf_event.h  | 5 +
>  kernel/events/core.c| 4 
>  3 files changed, 10 insertions(+)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index ffab8b5..fac9798 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -665,6 +665,7 @@ users (without CAP_SYS_ADMIN).  The default value is 2.
>  >=0: Disallow raw tracepoint access by users without CAP_IOC_LOCK
>  >=1: Disallow CPU event access by users without CAP_SYS_ADMIN
>  >=2: Disallow kernel profiling by users without CAP_SYS_ADMIN
> +>=3: Disallow all event access by users without CAP_SYS_ADMIN
>
>  ==
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 8ed43261..1e2080f 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1156,6 +1156,11 @@ static inline bool perf_paranoid_kernel(void)
> return sysctl_perf_event_paranoid > 1;
>  }
>
> +static inline bool perf_paranoid_any(void)
> +{
> +   return sysctl_perf_event_paranoid > 2;
> +}
> +
>  extern void perf_event_init(void);
>  extern void perf_tp_event(u16 event_type, u64 count, void *record,
>   int entry_size, struct pt_regs *regs,
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 356a6c7..52bd100 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -353,6 +353,7 @@ static struct srcu_struct pmus_srcu;
>   *   0 - disallow raw tracepoint access for unpriv
>   *   1 - disallow cpu events for unpriv
>   *   2 - disallow kernel profiling for unpriv
> + *   3 - disallow all unpriv perf event use
>   */
>  int sysctl_perf_event_paranoid __read_mostly = 2;
>
> @@ -9296,6 +9297,9 @@ SYSCALL_DEFINE5(perf_event_open,
> if (flags & ~PERF_FLAG_ALL)
> return -EINVAL;
>
> +   if (perf_paranoid_any() && !capable(CAP_SYS_ADMIN))
> +   return -EACCES;
> +
> err = perf_copy_attr(attr_uptr, );
> if (err)
> return err;
> --
> 2.8.0.rc3.226.g39d4020
>



-- 
Kees Cook
Chrome OS & Brillo Security

Re: [kernel-hardening] [PATCH 1/2] security, perf: allow further restriction of perf_event_open

[Kees Cook]

On Wed, Jul 27, 2016 at 7:45 AM, Jeff Vander Stoep  wrote:
> When kernel.perf_event_paranoid is set to 3 (or greater), disallow
> all access to performance events by users without CAP_SYS_ADMIN.
>
> This new level of restriction is intended to reduce the attack
> surface of the kernel. Perf is a valuable tool for developers but
> is generally unnecessary and unused on production systems. Perf may
> open up an attack vector to vulnerable device-specific drivers as
> recently demonstrated in CVE-2016-0805, CVE-2016-0819,
> CVE-2016-0843, CVE-2016-3768, and CVE-2016-3843. This new level of
> restriction allows for a safe default to be set on production systems
> while leaving a simple means for developers to grant access [1].
>
> This feature is derived from CONFIG_GRKERNSEC_PERF_HARDEN by Brad
> Spengler. It is based on a patch by Ben Hutchings [2]. Ben's patches
> have been modified and split up to address on-list feedback.
>
> kernel.perf_event_paranoid=3 is the default on both Debian [2] and
> Android [3].
>
> [1] Making perf available to developers on Android:
> https://android-review.googlesource.com/#/c/234400/
> [2] Original patch by Ben Hutchings:
> https://lkml.org/lkml/2016/1/11/587
> [3] https://android-review.googlesource.com/#/c/234743/
>
> Signed-off-by: Jeff Vander Stoep 

Thanks for splitting this up! It'll be nice to have this delta out of
Debian and Android.

Reviewed-by: Kees Cook 

-Kees

> ---
>  Documentation/sysctl/kernel.txt | 1 +
>  include/linux/perf_event.h  | 5 +
>  kernel/events/core.c| 4 
>  3 files changed, 10 insertions(+)
>
> diff --git a/Documentation/sysctl/kernel.txt b/Documentation/sysctl/kernel.txt
> index ffab8b5..fac9798 100644
> --- a/Documentation/sysctl/kernel.txt
> +++ b/Documentation/sysctl/kernel.txt
> @@ -665,6 +665,7 @@ users (without CAP_SYS_ADMIN).  The default value is 2.
>  >=0: Disallow raw tracepoint access by users without CAP_IOC_LOCK
>  >=1: Disallow CPU event access by users without CAP_SYS_ADMIN
>  >=2: Disallow kernel profiling by users without CAP_SYS_ADMIN
> +>=3: Disallow all event access by users without CAP_SYS_ADMIN
>
>  ==
>
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 8ed43261..1e2080f 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -1156,6 +1156,11 @@ static inline bool perf_paranoid_kernel(void)
> return sysctl_perf_event_paranoid > 1;
>  }
>
> +static inline bool perf_paranoid_any(void)
> +{
> +   return sysctl_perf_event_paranoid > 2;
> +}
> +
>  extern void perf_event_init(void);
>  extern void perf_tp_event(u16 event_type, u64 count, void *record,
>   int entry_size, struct pt_regs *regs,
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index 356a6c7..52bd100 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -353,6 +353,7 @@ static struct srcu_struct pmus_srcu;
>   *   0 - disallow raw tracepoint access for unpriv
>   *   1 - disallow cpu events for unpriv
>   *   2 - disallow kernel profiling for unpriv
> + *   3 - disallow all unpriv perf event use
>   */
>  int sysctl_perf_event_paranoid __read_mostly = 2;
>
> @@ -9296,6 +9297,9 @@ SYSCALL_DEFINE5(perf_event_open,
> if (flags & ~PERF_FLAG_ALL)
> return -EINVAL;
>
> +   if (perf_paranoid_any() && !capable(CAP_SYS_ADMIN))
> +   return -EACCES;
> +
> err = perf_copy_attr(attr_uptr, );
> if (err)
> return err;
> --
> 2.8.0.rc3.226.g39d4020
>



-- 
Kees Cook
Chrome OS & Brillo Security