[PATCH v2 0/2] Fix auditsc DoS and mark it BROKEN

[PATCH v2 0/2] Fix auditsc DoS and mark it BROKEN

[Andy Lutomirski]

CONFIG_AUDITSYSCALL is awful.  Patch 2 enumerates some reasons.

Patch 1 fixes a nasty DoS and possible information leak.  It should
be applied and backported.

Patch 2 is optional.  I leave it to other peoples' judgment.

Andy Lutomirski (2):
  auditsc: audit_krule mask accesses need bounds checking
  audit: Move CONFIG_AUDITSYSCALL into staging and update help text

Andy Lutomirski (2):
  auditsc: audit_krule mask accesses need bounds checking
  audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

 init/Kconfig     | 13 ++++++++-----
 kernel/auditsc.c | 27 ++++++++++++++++++---------
 2 files changed, 26 insertions(+), 14 deletions(-)

-- 
1.9.3

[PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Andy Lutomirski]

Fixes an easy DoS and possible information disclosure.

This does nothing about the broken state of x32 auditing.

Cc: stable@vger.kernel.org
Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 kernel/auditsc.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f251a5e..7ccd9db 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
 	return AUDIT_BUILD_CONTEXT;
 }
 
+static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
+{
+	int word, bit;
+
+	if (val > 0xffffffff)
+		return false;
+
+	word = AUDIT_WORD(val);
+	if (word >= AUDIT_BITMASK_SIZE)
+		return false;
+
+	bit = AUDIT_BIT(val);
+
+	return rule->mask[word] & bit;
+}
+
 /* At syscall entry and exit time, this filter is called if the
  * audit_state is not low enough that auditing cannot take place, but is
  * also not high enough that we already know we have to write an audit
@@ -745,11 +761,8 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
 
 	rcu_read_lock();
 	if (!list_empty(list)) {
-		int word = AUDIT_WORD(ctx->major);
-		int bit  = AUDIT_BIT(ctx->major);
-
 		list_for_each_entry_rcu(e, list, list) {
-			if ((e->rule.mask[word] & bit) == bit &&
+			if (audit_in_mask(&e->rule, ctx->major) &&
 			    audit_filter_rules(tsk, &e->rule, ctx, NULL,
 					       &state, false)) {
 				rcu_read_unlock();
@@ -769,20 +782,16 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
 static int audit_filter_inode_name(struct task_struct *tsk,
 				   struct audit_names *n,
 				   struct audit_context *ctx) {
-	int word, bit;
 	int h = audit_hash_ino((u32)n->ino);
 	struct list_head *list = &audit_inode_hash[h];
 	struct audit_entry *e;
 	enum audit_state state;
 
-	word = AUDIT_WORD(ctx->major);
-	bit  = AUDIT_BIT(ctx->major);
-
 	if (list_empty(list))
 		return 0;
 
 	list_for_each_entry_rcu(e, list, list) {
-		if ((e->rule.mask[word] & bit) == bit &&
+		if (audit_in_mask(&e->rule, ctx->major) &&
 		    audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
 			ctx->current_state = state;
 			return 1;
-- 
1.9.3

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Eric Paris]

On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
> Fixes an easy DoS and possible information disclosure.
> 
> This does nothing about the broken state of x32 auditing.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  kernel/auditsc.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f251a5e..7ccd9db 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>  	return AUDIT_BUILD_CONTEXT;
>  }
>  
> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
> +{
> +	int word, bit;
> +
> +	if (val > 0xffffffff)
> +		return false;

Why is this necessary?  

> +
> +	word = AUDIT_WORD(val);
> +	if (word >= AUDIT_BITMASK_SIZE)
> +		return false;

Since this covers it and it extensible...

> +
> +	bit = AUDIT_BIT(val);
> +
> +	return rule->mask[word] & bit;

Returning an int as a bool creates worse code than just returning the
int.  (although in this case if the compiler chooses to inline it might
be smart enough not to actually convert this int to a bool and make
worse assembly...)   I'd suggest the function here return an int.  bools
usually make the assembly worse...

Otherwise I'd give it an ACK...

> +}
> +
>  /* At syscall entry and exit time, this filter is called if the
>   * audit_state is not low enough that auditing cannot take place, but is
>   * also not high enough that we already know we have to write an audit
> @@ -745,11 +761,8 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
>  
>  	rcu_read_lock();
>  	if (!list_empty(list)) {
> -		int word = AUDIT_WORD(ctx->major);
> -		int bit  = AUDIT_BIT(ctx->major);
> -
>  		list_for_each_entry_rcu(e, list, list) {
> -			if ((e->rule.mask[word] & bit) == bit &&
> +			if (audit_in_mask(&e->rule, ctx->major) &&
>  			    audit_filter_rules(tsk, &e->rule, ctx, NULL,
>  					       &state, false)) {
>  				rcu_read_unlock();
> @@ -769,20 +782,16 @@ static enum audit_state audit_filter_syscall(struct task_struct *tsk,
>  static int audit_filter_inode_name(struct task_struct *tsk,
>  				   struct audit_names *n,
>  				   struct audit_context *ctx) {
> -	int word, bit;
>  	int h = audit_hash_ino((u32)n->ino);
>  	struct list_head *list = &audit_inode_hash[h];
>  	struct audit_entry *e;
>  	enum audit_state state;
>  
> -	word = AUDIT_WORD(ctx->major);
> -	bit  = AUDIT_BIT(ctx->major);
> -
>  	if (list_empty(list))
>  		return 0;
>  
>  	list_for_each_entry_rcu(e, list, list) {
> -		if ((e->rule.mask[word] & bit) == bit &&
> +		if (audit_in_mask(&e->rule, ctx->major) &&
>  		    audit_filter_rules(tsk, &e->rule, ctx, n, &state, false)) {
>  			ctx->current_state = state;
>  			return 1;

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Andy Lutomirski]

On Wed, May 28, 2014 at 7:23 PM, Eric Paris <eparis@redhat.com> wrote:
> On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
>> Fixes an easy DoS and possible information disclosure.
>>
>> This does nothing about the broken state of x32 auditing.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> ---
>>  kernel/auditsc.c | 27 ++++++++++++++++++---------
>>  1 file changed, 18 insertions(+), 9 deletions(-)
>>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index f251a5e..7ccd9db 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>>       return AUDIT_BUILD_CONTEXT;
>>  }
>>
>> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
>> +{
>> +     int word, bit;
>> +
>> +     if (val > 0xffffffff)
>> +             return false;
>
> Why is this necessary?

To avoid an integer overflow.  Admittedly, this particular overflow
won't cause a crash, but it will cause incorrect results.

>
>> +
>> +     word = AUDIT_WORD(val);
>> +     if (word >= AUDIT_BITMASK_SIZE)
>> +             return false;
>
> Since this covers it and it extensible...
>
>> +
>> +     bit = AUDIT_BIT(val);
>> +
>> +     return rule->mask[word] & bit;
>
> Returning an int as a bool creates worse code than just returning the
> int.  (although in this case if the compiler chooses to inline it might
> be smart enough not to actually convert this int to a bool and make
> worse assembly...)   I'd suggest the function here return an int.  bools
> usually make the assembly worse...

I'm ambivalent.  The right assembly would use flags on x86, not an int
or a bool, and I don't know what the compiler will do.

--Andy

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Eric Paris]

On Wed, 2014-05-28 at 19:27 -0700, Andy Lutomirski wrote:
> On Wed, May 28, 2014 at 7:23 PM, Eric Paris <eparis@redhat.com> wrote:
> > On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
> >> Fixes an easy DoS and possible information disclosure.
> >>
> >> This does nothing about the broken state of x32 auditing.
> >>
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> >> ---
> >>  kernel/auditsc.c | 27 ++++++++++++++++++---------
> >>  1 file changed, 18 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> >> index f251a5e..7ccd9db 100644
> >> --- a/kernel/auditsc.c
> >> +++ b/kernel/auditsc.c
> >> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
> >>       return AUDIT_BUILD_CONTEXT;
> >>  }
> >>
> >> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
> >> +{
> >> +     int word, bit;
> >> +
> >> +     if (val > 0xffffffff)
> >> +             return false;
> >
> > Why is this necessary?
> 
> To avoid an integer overflow.  Admittedly, this particular overflow
> won't cause a crash, but it will cause incorrect results.

You know this code pre-dates git?  I admit, I'm shocked no one ever
noticed it before!  This is ANCIENT.  And clearly broken.

I'll likely ask Richard to add a WARN_ONCE() in both this place, and
below in word > AUDIT_BITMASK_SIZE so we might know if we ever need a
larger bitmask to store syscall numbers....

It'd be nice if lib had a efficient bitmask implementation...

> >
> >> +
> >> +     word = AUDIT_WORD(val);
> >> +     if (word >= AUDIT_BITMASK_SIZE)
> >> +             return false;
> >
> > Since this covers it and it extensible...
> >
> >> +
> >> +     bit = AUDIT_BIT(val);
> >> +
> >> +     return rule->mask[word] & bit;
> >
> > Returning an int as a bool creates worse code than just returning the
> > int.  (although in this case if the compiler chooses to inline it might
> > be smart enough not to actually convert this int to a bool and make
> > worse assembly...)   I'd suggest the function here return an int.  bools
> > usually make the assembly worse...
> 
> I'm ambivalent.  The right assembly would use flags on x86, not an int
> or a bool, and I don't know what the compiler will do.

Also, clearly X86_X32 was implemented without audit support, so we
shouldn't config it in.  What do you think of this?

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 25d2c6f..fa852e2 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -129,7 +129,7 @@ config X86
 	select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
 	select HAVE_CC_STACKPROTECTOR
 	select GENERIC_CPU_AUTOPROBE
-	select HAVE_ARCH_AUDITSYSCALL
+	select HAVE_ARCH_AUDITSYSCALL if !X86_X32
 
 config INSTRUCTION_DECODER
 	def_bool y

Re: [PATCH v2 1/2] auditsc: audit_krule mask accesses need bounds checking

[Andy Lutomirski]

On Wed, May 28, 2014 at 7:43 PM, Eric Paris <eparis@redhat.com> wrote:
> On Wed, 2014-05-28 at 19:27 -0700, Andy Lutomirski wrote:
>> On Wed, May 28, 2014 at 7:23 PM, Eric Paris <eparis@redhat.com> wrote:
>> > On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
>> >> Fixes an easy DoS and possible information disclosure.
>> >>
>> >> This does nothing about the broken state of x32 auditing.
>> >>
>> >> Cc: stable@vger.kernel.org
>> >> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
>> >> ---
>> >>  kernel/auditsc.c | 27 ++++++++++++++++++---------
>> >>  1 file changed, 18 insertions(+), 9 deletions(-)
>> >>
>> >> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> >> index f251a5e..7ccd9db 100644
>> >> --- a/kernel/auditsc.c
>> >> +++ b/kernel/auditsc.c
>> >> @@ -728,6 +728,22 @@ static enum audit_state audit_filter_task(struct task_struct *tsk, char **key)
>> >>       return AUDIT_BUILD_CONTEXT;
>> >>  }
>> >>
>> >> +static bool audit_in_mask(const struct audit_krule *rule, unsigned long val)
>> >> +{
>> >> +     int word, bit;
>> >> +
>> >> +     if (val > 0xffffffff)
>> >> +             return false;
>> >
>> > Why is this necessary?
>>
>> To avoid an integer overflow.  Admittedly, this particular overflow
>> won't cause a crash, but it will cause incorrect results.
>
> You know this code pre-dates git?  I admit, I'm shocked no one ever
> noticed it before!  This is ANCIENT.  And clearly broken.
>
> I'll likely ask Richard to add a WARN_ONCE() in both this place, and
> below in word > AUDIT_BITMASK_SIZE so we might know if we ever need a
> larger bitmask to store syscall numbers....
>

Not there, please!  It would make sense to check when rules are
*added*, but any user can trivially invoke the filter with pretty much
any syscall nr.

> It'd be nice if lib had a efficient bitmask implementation...
>
>> >
>> >> +
>> >> +     word = AUDIT_WORD(val);
>> >> +     if (word >= AUDIT_BITMASK_SIZE)
>> >> +             return false;
>> >
>> > Since this covers it and it extensible...
>> >
>> >> +
>> >> +     bit = AUDIT_BIT(val);
>> >> +
>> >> +     return rule->mask[word] & bit;
>> >
>> > Returning an int as a bool creates worse code than just returning the
>> > int.  (although in this case if the compiler chooses to inline it might
>> > be smart enough not to actually convert this int to a bool and make
>> > worse assembly...)   I'd suggest the function here return an int.  bools
>> > usually make the assembly worse...
>>
>> I'm ambivalent.  The right assembly would use flags on x86, not an int
>> or a bool, and I don't know what the compiler will do.
>
> Also, clearly X86_X32 was implemented without audit support, so we
> shouldn't config it in.  What do you think of this?
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 25d2c6f..fa852e2 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -129,7 +129,7 @@ config X86
>         select HAVE_IRQ_EXIT_ON_IRQ_STACK if X86_64
>         select HAVE_CC_STACKPROTECTOR
>         select GENERIC_CPU_AUTOPROBE
> -       select HAVE_ARCH_AUDITSYSCALL
> +       select HAVE_ARCH_AUDITSYSCALL if !X86_X32

I'm fine with that, but I hope that distros will choose x32 over
auditsc, at least until someone makes enabling auditsc be less
intrusive.

Or someone could fix it, modulo the syscall nr 2048 thing.  x32
syscall nrs are *huge*.  So are lots of other architectures', a few of
which probably claim to support auditsc.

--Andy

[PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Andy Lutomirski]

Here are some issues with the code:
 - It thinks that syscalls have four arguments.
 - It's a performance disaster.
 - It assumes that syscall numbers are between 0 and 2048.
 - It's unclear whether it's supposed to be reliable.
 - It's broken on things like x32.
 - It can't support ARM OABI.
 - Its approach to freeing memory is terrifying.

Signed-off-by: Andy Lutomirski <luto@amacapital.net>
---
 init/Kconfig | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/init/Kconfig b/init/Kconfig
index 9d3585b..24d4b53 100644
--- a/init/Kconfig
+++ b/init/Kconfig
@@ -296,13 +296,16 @@ config HAVE_ARCH_AUDITSYSCALL
 	bool
 
 config AUDITSYSCALL
-	bool "Enable system-call auditing support"
-	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL
+	bool "Enable system-call auditing support (not recommended)"
+	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL && BROKEN
 	default y if SECURITY_SELINUX
 	help
-	  Enable low-overhead system-call auditing infrastructure that
-	  can be used independently or with another kernel subsystem,
-	  such as SELinux.
+	  Enable system-call auditing infrastructure that can be used
+	  independently or with another kernel subsystem, such as
+	  SELinux.
+
+	  AUDITSYSCALL has serious performance and correctness issues.
+	  Use it with extreme caution.
 
 config AUDIT_WATCH
 	def_bool y
-- 
1.9.3

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Eric Paris]

NAK

On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
> Here are some issues with the code:
>  - It thinks that syscalls have four arguments.

Not true at all.  It records the registers that would hold the first 4
entries on syscall entry, for use later if needed, as getting those
later on some arches is not feasible (see ia64).  It makes no assumption
about how many syscalls a function has.

>  - It's a performance disaster.

Only if you enable it.  If you don't use audit it is a single branch.
Hardly a disaster.

>  - It assumes that syscall numbers are between 0 and 2048.

There could well be a bug here.  Not questioning that.  Although that
would be patch 1/2

>  - It's unclear whether it's supposed to be reliable.

Unclear to whom?

>  - It's broken on things like x32.
>  - It can't support ARM OABI.

Some arches aren't supported?  And that makes it BROKEN?  

>  - Its approach to freeing memory is terrifying.

What?

None of your reasons hold water.  Bugs need to be fixed.  Try reporting
them...  This is just stupid.

> Signed-off-by: Andy Lutomirski <luto@amacapital.net>
> ---
>  init/Kconfig | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/init/Kconfig b/init/Kconfig
> index 9d3585b..24d4b53 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -296,13 +296,16 @@ config HAVE_ARCH_AUDITSYSCALL
>  	bool
>  
>  config AUDITSYSCALL
> -	bool "Enable system-call auditing support"
> -	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL
> +	bool "Enable system-call auditing support (not recommended)"
> +	depends on AUDIT && HAVE_ARCH_AUDITSYSCALL && BROKEN
>  	default y if SECURITY_SELINUX
>  	help
> -	  Enable low-overhead system-call auditing infrastructure that
> -	  can be used independently or with another kernel subsystem,
> -	  such as SELinux.
> +	  Enable system-call auditing infrastructure that can be used
> +	  independently or with another kernel subsystem, such as
> +	  SELinux.
> +
> +	  AUDITSYSCALL has serious performance and correctness issues.
> +	  Use it with extreme caution.
>  
>  config AUDIT_WATCH
>  	def_bool y

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Andy Lutomirski]

On Wed, May 28, 2014 at 7:09 PM, Eric Paris <eparis@redhat.com> wrote:
> NAK
>
> On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
>> Here are some issues with the code:
>>  - It thinks that syscalls have four arguments.
>
> Not true at all.  It records the registers that would hold the first 4
> entries on syscall entry, for use later if needed, as getting those
> later on some arches is not feasible (see ia64).  It makes no assumption
> about how many syscalls a function has.

What about a5 and a6?

>
>>  - It's a performance disaster.
>
> Only if you enable it.  If you don't use audit it is a single branch.
> Hardly a disaster.

It forces all syscalls into the slow path and it can do crazy things
like building audit contexts just in case actual handling of the
syscall triggers an audit condition so that the exit path can log the
syscall.  That's way worse than a single branch.

Try it: benchmark getpid on Fedora and then repeat the experiment with
syscall auditing fully disabled.  The difference is striking.

>
>>  - It assumes that syscall numbers are between 0 and 2048.
>
> There could well be a bug here.  Not questioning that.  Although that
> would be patch 1/2

Even with patch 1, it still doesn't handle large syscall numbers -- it
just assumes they're not audited.

>
>>  - It's unclear whether it's supposed to be reliable.
>
> Unclear to whom?

To me.

If some inode access or selinux rule triggers an audit, is the auditsc
code guaranteed to write an exit record?  And see below...

>
>>  - It's broken on things like x32.
>>  - It can't support ARM OABI.
>
> Some arches aren't supported?  And that makes it BROKEN?

It acts like x32 is supported.  OABI is fixable.  Admittedly, OABI not
being supported is fine, now that it's correctly disabled.

>
>>  - Its approach to freeing memory is terrifying.
>
> What?
>
> None of your reasons hold water.  Bugs need to be fixed.  Try reporting
> them...  This is just stupid.

...for reference, I've *tried* to fix the performance issues.  I've
run into all kinds of problems.

The actual context code is incredibly tangled.  It's unclear whether
it would be permissible for various rule combinations to suppress exit
records triggered by selinux.  Any effort to actually deal with this
stuff will have to deal with the fact that the audit code builds up
lots of state and frees it on syscall exit.  That means that the exit
hook needs to be actually invoked, otherwise we leak.  I was never
able to convince myself that the current code is correct wrt kernel
threads.

In summary, the code is a giant mess.  The way it works is nearly
incomprehensible.  It contains at least one severe bug.  I'd love to
see it fixed, but for now, distributions seem to think that enabling
CONFIG_AUDITSYSCALL is a reasonable thing to do, and I'd argue that
it's actually a terrible choice for anyone who doesn't actually need
syscall audit rules.  And I don't know who needs these things.

I've heard an argument that selinux benefits from this, since the
syscall exit log helps with diagnosing denials.  I think that's
absurd.  I've never found anything wrong with the denial record itself
that would be helped by seeing the syscall log.  (And there's always
syscall_get_xyz, which would cover this case splendidly with about an
order of magnitude less code.)

As I said, I'm not going to push hard for this code to be marked
BROKEN.  But I think it's rather broken, and I'm definitely not
volunteering to fix it.

--Andy

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Eric Paris]

On Wed, 2014-05-28 at 19:40 -0700, Andy Lutomirski wrote:
> On Wed, May 28, 2014 at 7:09 PM, Eric Paris <eparis@redhat.com> wrote:
> > NAK
> >
> > On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
> >> Here are some issues with the code:
> >>  - It thinks that syscalls have four arguments.
> >
> > Not true at all.  It records the registers that would hold the first 4
> > entries on syscall entry, for use later if needed, as getting those
> > later on some arches is not feasible (see ia64).  It makes no assumption
> > about how many syscalls a function has.
> 
> What about a5 and a6?

On the couple of syscalls where a5 and a6 had any state that was
actually wanted by someone (mainly just the fd on mmap) audit collects
it later in the actual syscall.

> >>  - It assumes that syscall numbers are between 0 and 2048.
> >
> > There could well be a bug here.  Not questioning that.  Although that
> > would be patch 1/2
> 
> Even with patch 1, it still doesn't handle large syscall numbers -- it
> just assumes they're not audited.

That's because we haven't had large syscall numbers.  That's the whole
point of an arch doing select HAVE_ARCH_AUDITSYSCALL.  If they don't
meet the requirements, they shouldn't be selecting it....

> >>  - It's unclear whether it's supposed to be reliable.
> >
> > Unclear to whom?
> 
> To me.
> 
> If some inode access or selinux rule triggers an audit, is the auditsc
> code guaranteed to write an exit record?  And see below...

This is an honest question:  Do you want to discuss these things, or
would you be happier if I shut up, fix the bugs you found, and leave
things be?  I don't want to have an argument, I'm happy to have a
discussion if you think that will be beneficial...

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Andy Lutomirski]

On Wed, May 28, 2014 at 7:54 PM, Eric Paris <eparis@redhat.com> wrote:
> On Wed, 2014-05-28 at 19:40 -0700, Andy Lutomirski wrote:
>> On Wed, May 28, 2014 at 7:09 PM, Eric Paris <eparis@redhat.com> wrote:
>> > NAK
>> >
>> > On Wed, 2014-05-28 at 18:44 -0700, Andy Lutomirski wrote:
>> >> Here are some issues with the code:
>> >>  - It thinks that syscalls have four arguments.
>> >
>> > Not true at all.  It records the registers that would hold the first 4
>> > entries on syscall entry, for use later if needed, as getting those
>> > later on some arches is not feasible (see ia64).  It makes no assumption
>> > about how many syscalls a function has.
>>
>> What about a5 and a6?
>
> On the couple of syscalls where a5 and a6 had any state that was
> actually wanted by someone (mainly just the fd on mmap) audit collects
> it later in the actual syscall.
>
>> >>  - It assumes that syscall numbers are between 0 and 2048.
>> >
>> > There could well be a bug here.  Not questioning that.  Although that
>> > would be patch 1/2
>>
>> Even with patch 1, it still doesn't handle large syscall numbers -- it
>> just assumes they're not audited.
>
> That's because we haven't had large syscall numbers.  That's the whole
> point of an arch doing select HAVE_ARCH_AUDITSYSCALL.  If they don't
> meet the requirements, they shouldn't be selecting it....
>
>> >>  - It's unclear whether it's supposed to be reliable.
>> >
>> > Unclear to whom?
>>
>> To me.
>>
>> If some inode access or selinux rule triggers an audit, is the auditsc
>> code guaranteed to write an exit record?  And see below...
>
> This is an honest question:  Do you want to discuss these things, or
> would you be happier if I shut up, fix the bugs you found, and leave
> things be?  I don't want to have an argument, I'm happy to have a
> discussion if you think that will be beneficial...
>

I'm happy to discuss these things.  I'd be happy if you fix things.  I
think that there are two major issues affecting users of auditsc and
that either those issues should be fixed or users of auditsc should
understand and accept these issues.

Issue 1: syscall numbering.  The syscall filters are unlikely to work
well on lots of architectures.

Issue 2: performance.  Until someone fixes it (which is probably
hard), turning this thing on hurts a lot.  Oleg added a hack awhile
ago that lets you do 'auditctl -a task,never' to get rid of the
performance hit for new tasks, but that rather defeats the point.

The scariness of the code can be lived with, but holy cow it's scary.

I'm just not going to fix the issues myself.  I tried and gave up.

--Andy

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Steve Grubb]

On Wednesday, May 28, 2014 07:40:57 PM Andy Lutomirski wrote:
> >>  - It assumes that syscall numbers are between 0 and 2048.
> >>
> > There could well be a bug here.  Not questioning that.  Although that
> > would be patch 1/2
> 
> Even with patch 1, it still doesn't handle large syscall numbers -- it
> just assumes they're not audited.

All syscalls must be auditable. Meaning that if an arch goes above 2048, then 
we'll need to do some math to get it to fall back within the range.


> >>  - It's unclear whether it's supposed to be reliable.
> >>
> > Unclear to whom?
> 
> To me.
> 
> If some inode access or selinux rule triggers an audit, is the auditsc
> code guaranteed to write an exit record?  And see below...

It should or indicate that it could not.

-Steve

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Andy Lutomirski]

On Thu, May 29, 2014 at 6:05 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Wednesday, May 28, 2014 07:40:57 PM Andy Lutomirski wrote:
>> >>  - It assumes that syscall numbers are between 0 and 2048.
>> >>
>> > There could well be a bug here.  Not questioning that.  Although that
>> > would be patch 1/2
>>
>> Even with patch 1, it still doesn't handle large syscall numbers -- it
>> just assumes they're not audited.
>
> All syscalls must be auditable. Meaning that if an arch goes above 2048, then
> we'll need to do some math to get it to fall back within the range.
>

Off the top of my head, x32, some ARM variants, and some MIPS variants
have syscalls above 2048.  auditsc has been disabled on the offending
ARM variant (because I noticed that the same issue affects seccomp,
and no one particularly wants to fix it), but I suspect that auditsc
is enabled but completely broken on whichever MIPS ABI has the issue.
I haven't checked.

TBH, I think that it's silly to let the auditsc design be heavily
constrained by ia64 considerations -- I still think that the syscall
entry hooks could be removed completely with some effort on most
architectures and that performance would improve considerably.  For
users who don't have syscall filter rules, performance might even
improve on ia64 -- I don't care how expensive syscall_get_args is, the
actual printk/auditd thing will dominate in cases where anything is
written.

I wonder whether the syscall restart mechanism can be used to
thoroughly confused auditsc.  I don't really know how syscall restarts
work.

My point is: I don't know what guarantees auditsc is supposed to
provide, nor do I know how to evaluate whether the code is correct.
For users who aren't bound by Common Criteria or related things, I
suspect that syscall auditing (as opposed to, say, LSM-based auditing)
will provide dubious value at best.  Keep in mind that many syscalls
take pointer arguments, and the auditsc code can't really do anything
sensible with them.

>
>> >>  - It's unclear whether it's supposed to be reliable.
>> >>
>> > Unclear to whom?
>>
>> To me.
>>
>> If some inode access or selinux rule triggers an audit, is the auditsc
>> code guaranteed to write an exit record?  And see below...
>
> It should or indicate that it could not.
>

--Andy

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Steve Grubb]

On Thursday, May 29, 2014 09:04:10 AM Andy Lutomirski wrote:
> On Thu, May 29, 2014 at 6:05 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> > On Wednesday, May 28, 2014 07:40:57 PM Andy Lutomirski wrote:
> >> >>  - It assumes that syscall numbers are between 0 and 2048.
> >> >>
> >> > There could well be a bug here.  Not questioning that.  Although that
> >> > would be patch 1/2
> >> 
> >> Even with patch 1, it still doesn't handle large syscall numbers -- it
> >> just assumes they're not audited.
> > 
> > All syscalls must be auditable. Meaning that if an arch goes above 2048,
> > then we'll need to do some math to get it to fall back within the range.
>
> Off the top of my head, x32, some ARM variants, and some MIPS variants
> have syscalls above 2048.

That could be fixed by putting a subtraction in place to get the bit mask to 
map correctly. User space audit rules would need to fix that also.


> auditsc has been disabled on the offending
> ARM variant (because I noticed that the same issue affects seccomp,
> and no one particularly wants to fix it), but I suspect that auditsc
> is enabled but completely broken on whichever MIPS ABI has the issue.
> I haven't checked.
> 
> TBH, I think that it's silly to let the auditsc design be heavily
> constrained by ia64 considerations -- I still think that the syscall
> entry hooks could be removed completely with some effort on most
> architectures and that performance would improve considerably.  For
> users who don't have syscall filter rules, performance might even
> improve on ia64 -- I don't care how expensive syscall_get_args is, the
> actual printk/auditd thing will dominate in cases where anything is
> written.

The last time I heard of benchmarking being done, audit's performance hit was 
negligible. That was about 4 years ago and there has been a whole lot of code 
churn since then. But it should in general be the cost of an 'if' statement 
unless auditing is enabled. If it is enabled, then you necessarily get the 
full treatment in case you trigger an event.


> I wonder whether the syscall restart mechanism can be used to
> thoroughly confused auditsc.  I don't really know how syscall restarts
> work.
> 
> My point is: I don't know what guarantees auditsc is supposed to
> provide, nor do I know how to evaluate whether the code is correct.

There is an audit test suite that is used to verify that its working. Its not 
an exhaustive test suite, but it provides good coverage.


> For users who aren't bound by Common Criteria or related things, I
> suspect that syscall auditing (as opposed to, say, LSM-based auditing)
> will provide dubious value at best.

No, it works pretty good. You can see who is accessing what files pretty 
clearly.

> Keep in mind that many syscalls take pointer arguments, and the auditsc code
> can't really do anything sensible with them.

All security relevant information is collected in auxiliary records if they 
are not directly readable as a syscall argument. This is a basic requirement. 
We are not required to capture all possible arguments. If you know of any 
security relevant parameters not captured, please let us know.

-Steve

Re: [PATCH v2 2/2] audit: Mark CONFIG_AUDITSYSCALL BROKEN and update help text

[Andy Lutomirski]

On Thu, May 29, 2014 at 9:25 AM, Steve Grubb <sgrubb@redhat.com> wrote:
> On Thursday, May 29, 2014 09:04:10 AM Andy Lutomirski wrote:
>> On Thu, May 29, 2014 at 6:05 AM, Steve Grubb <sgrubb@redhat.com> wrote:
>> > On Wednesday, May 28, 2014 07:40:57 PM Andy Lutomirski wrote:
>> >> >>  - It assumes that syscall numbers are between 0 and 2048.
>> >> >>
>> >> > There could well be a bug here.  Not questioning that.  Although that
>> >> > would be patch 1/2
>> >>
>> >> Even with patch 1, it still doesn't handle large syscall numbers -- it
>> >> just assumes they're not audited.
>> >
>> > All syscalls must be auditable. Meaning that if an arch goes above 2048,
>> > then we'll need to do some math to get it to fall back within the range.
>>
>> Off the top of my head, x32, some ARM variants, and some MIPS variants
>> have syscalls above 2048.
>
> That could be fixed by putting a subtraction in place to get the bit mask to
> map correctly. User space audit rules would need to fix that also.
>
>
>> auditsc has been disabled on the offending
>> ARM variant (because I noticed that the same issue affects seccomp,
>> and no one particularly wants to fix it), but I suspect that auditsc
>> is enabled but completely broken on whichever MIPS ABI has the issue.
>> I haven't checked.
>>
>> TBH, I think that it's silly to let the auditsc design be heavily
>> constrained by ia64 considerations -- I still think that the syscall
>> entry hooks could be removed completely with some effort on most
>> architectures and that performance would improve considerably.  For
>> users who don't have syscall filter rules, performance might even
>> improve on ia64 -- I don't care how expensive syscall_get_args is, the
>> actual printk/auditd thing will dominate in cases where anything is
>> written.
>
> The last time I heard of benchmarking being done, audit's performance hit was
> negligible. That was about 4 years ago and there has been a whole lot of code
> churn since then. But it should in general be the cost of an 'if' statement
> unless auditing is enabled. If it is enabled, then you necessarily get the
> full treatment in case you trigger an event.
>

The actual rule matching is reasonably inexpensive, but the killer is
the fact that every system call is forced into the slow path.  Without
audit, this cost is paid by seccomp users and tasks that are being
ptraced.  With audit, every single syscall for every single task pays
the full cost of a slow path system call.

This is extra sad because, AFAICS, most of the syscall entry audit
code shouldn't be necessary even for audit users.

>> For users who aren't bound by Common Criteria or related things, I
>> suspect that syscall auditing (as opposed to, say, LSM-based auditing)
>> will provide dubious value at best.
>
> No, it works pretty good. You can see who is accessing what files pretty
> clearly.

Right, but I don't think it's the *syscall* part that's really
necessary here.  The fact that someone opened /etc/passwd is
interesting.  The fact that someone sys_opened (const char *)0x123456
is not.

>
>> Keep in mind that many syscalls take pointer arguments, and the auditsc code
>> can't really do anything sensible with them.
>
> All security relevant information is collected in auxiliary records if they
> are not directly readable as a syscall argument. This is a basic requirement.
> We are not required to capture all possible arguments. If you know of any
> security relevant parameters not captured, please let us know.
>

a5 and a6.  The cmsg data in sendmsg (which is very security-relevant
-- think of SCM_RIGHTS and SCM_CREDENTIALS).  The destination for
sendto.  The whole payload of netlink messages.  I don't know how many
of these are actually captures.

Plus, except for things like setuid and sigreturn, capturing
parameters on entry shouldn't be needed anyway.  Capturing at log time
should be fine.

If this magic capturing only happened for users who need it for
compliance reasons, that would be one thing.  But it happens for every
single system call on Fedora.

--Andy