[PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Eric Paris]

Remove the limitation on argv size.  The audit system now logs arguments 8k at a
time so the attempt to keep the size of the execve args smaller than one netlink
message is no longer a requirement.

Signed-off-by: Eric Paris <eparis@redhat.com>

---
 kernel/auditsc.c |   10 ----------
 kernel/sysctl.c  |   11 -----------
 2 files changed, 0 insertions(+), 21 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index f9f61db..6627fce 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1876,8 +1876,6 @@ int __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode
 	return 0;
 }
 
-int audit_argv_kb = 32;
-
 int audit_bprm(struct linux_binprm *bprm)
 {
 	struct audit_aux_data_execve *ax;
@@ -1886,14 +1884,6 @@ int audit_bprm(struct linux_binprm *bprm)
 	if (likely(!audit_enabled || !context || context->dummy))
 		return 0;
 
-	/*
-	 * Even though the stack code doesn't limit the arg+env size any more,
-	 * the audit code requires that _all_ arguments be logged in a single
-	 * netlink skb. Hence cap it :-(
-	 */
-	if (bprm->argv_len > (audit_argv_kb << 10))
-		return -E2BIG;
-
 	ax = kmalloc(sizeof(*ax), GFP_KERNEL);
 	if (!ax)
 		return -ENOMEM;
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 53a456e..88e5d06 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -77,7 +77,6 @@ extern int percpu_pagelist_fraction;
 extern int compat_log;
 extern int maps_protect;
 extern int sysctl_stat_interval;
-extern int audit_argv_kb;
 
 /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
 static int maxolduid = 65535;
@@ -347,16 +346,6 @@ static ctl_table kern_table[] = {
 		.mode		= 0644,
 		.proc_handler	= &proc_dointvec,
 	},
-#ifdef CONFIG_AUDITSYSCALL
-	{
-		.ctl_name	= CTL_UNNUMBERED,
-		.procname	= "audit_argv_kb",
-		.data		= &audit_argv_kb,
-		.maxlen		= sizeof(int),
-		.mode		= 0644,
-		.proc_handler	= &proc_dointvec,
-	},
-#endif
 	{
 		.ctl_name	= KERN_CORE_PATTERN,
 		.procname	= "core_pattern",

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Peter Zijlstra]

Hi Eric,

Thanks for ridding us of this wart!

On Tue, 2007-10-02 at 17:29 -0400, Eric Paris wrote:
> Remove the limitation on argv size.  The audit system now logs arguments 8k at a
> time so the attempt to keep the size of the execve args smaller than one netlink
> message is no longer a requirement.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>

> ---
>  kernel/auditsc.c |   10 ----------
>  kernel/sysctl.c  |   11 -----------
>  2 files changed, 0 insertions(+), 21 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index f9f61db..6627fce 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1876,8 +1876,6 @@ int __audit_ipc_set_perm(unsigned long qbytes, uid_t uid, gid_t gid, mode_t mode
>  	return 0;
>  }
>  
> -int audit_argv_kb = 32;
> -
>  int audit_bprm(struct linux_binprm *bprm)
>  {
>  	struct audit_aux_data_execve *ax;
> @@ -1886,14 +1884,6 @@ int audit_bprm(struct linux_binprm *bprm)
>  	if (likely(!audit_enabled || !context || context->dummy))
>  		return 0;
>  
> -	/*
> -	 * Even though the stack code doesn't limit the arg+env size any more,
> -	 * the audit code requires that _all_ arguments be logged in a single
> -	 * netlink skb. Hence cap it :-(
> -	 */
> -	if (bprm->argv_len > (audit_argv_kb << 10))
> -		return -E2BIG;
> -
>  	ax = kmalloc(sizeof(*ax), GFP_KERNEL);
>  	if (!ax)
>  		return -ENOMEM;
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 53a456e..88e5d06 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -77,7 +77,6 @@ extern int percpu_pagelist_fraction;
>  extern int compat_log;
>  extern int maps_protect;
>  extern int sysctl_stat_interval;
> -extern int audit_argv_kb;
>  
>  /* this is needed for the proc_dointvec_minmax for [fs_]overflow UID and GID */
>  static int maxolduid = 65535;
> @@ -347,16 +346,6 @@ static ctl_table kern_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= &proc_dointvec,
>  	},
> -#ifdef CONFIG_AUDITSYSCALL
> -	{
> -		.ctl_name	= CTL_UNNUMBERED,
> -		.procname	= "audit_argv_kb",
> -		.data		= &audit_argv_kb,
> -		.maxlen		= sizeof(int),
> -		.mode		= 0644,
> -		.proc_handler	= &proc_dointvec,
> -	},
> -#endif
>  	{
>  		.ctl_name	= KERN_CORE_PATTERN,
>  		.procname	= "core_pattern",
> 
> 

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Eric Paris]

On Tue, 2007-10-02 at 17:29 -0400, Eric Paris wrote:
> Remove the limitation on argv size.  The audit system now logs arguments 8k at a
> time so the attempt to keep the size of the execve args smaller than one netlink
> message is no longer a requirement.
> 
> Signed-off-by: Eric Paris <eparis@redhat.com>

I think I need to pull this patch. (uggh)  Turns out that one argument
is allowed to be 32 pages long.  Which is almost guaranteed to OOM a
running i686 in audit_expand when we try to get that much memory.  My
1/2 patch in this series doesn't address the SINGLE argument that is
huge case.  Just that a list of arguments is huge.

My belief is that the solution to this problem is to allow audit to
break individual arguments down to a size <8k.  I guess my syntax would
be something like

a0[0]=(first 8k of a single huge argument)
a0[1]=(second 8k of a single huge argument)
....

My 1/2 patch in this series keeps lists or arguments <8k and if people
are ok with that syntax of messages getting sent to userspace we can
audit single message >8k.

If this is a good syntax I'll send a patch for it.  Note that I doubt
there are very many programs that can handle a single argument that big,
but the kernel allows it so we need some syntax to support it....

I guess the other options might be to change this audit_bprm hook to
only limit execve args the way it does now if audit is running AND we
are auditing execve, but i'd rather fix this 'right' rather than keep
around this little wart....

who has a problem with that syntax?  will userspace puke?

-Eric

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Steve Grubb]

On Friday 05 October 2007 11:11:27 Eric Paris wrote:
> My belief is that the solution to this problem is to allow audit to
> break individual arguments down to a size <8k.  I guess my syntax would
> be something like
>
> a0[0]=(first 8k of a single huge argument)
> a0[1]=(second 8k of a single huge argument)

Sure go ahead. Also be sure to test with something that has spaces in the args 
to see what happens when the argument gets encoded. I think this will be so 
rare that no one will ever see it in practice. Either getopt or the shell 
will probably limit the argument size.

I don't recall if the MAX size limit was a define in the previous patch. If 
not, I'd suggest making it a define. I can make the audit buffers bigger at 
some point, but we'll have to recompile everything that links with libaudit. 
So, I'd want to hold off until there is a soname number bump just to make 
sure everything gets recompiled. So, a define would allow us to easily raise 
the kernel side after user space has been changed for a while.

-Steve

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Klaus Weidner]

On Fri, Oct 05, 2007 at 11:11:27AM -0400, Eric Paris wrote:
> My belief is that the solution to this problem is to allow audit to
> break individual arguments down to a size <8k.  I guess my syntax would
> be something like
> 
> a0[0]=(first 8k of a single huge argument)
> a0[1]=(second 8k of a single huge argument)
[...]
> who has a problem with that syntax?  will userspace puke?

I'm a bit worried about special audit record formats that aren't
generally seen in normal operation, since that's an obstacle to
testability. The ASCII audit format encourages an ad-hoc parsing
approach, and it's likely that tools other than the shipped ones won't be
able to handle this and will break unexpectedly, possibly offering
avenues to hide events with unusual records. (I know that people are
supposed to use the parsing library, but they aren't being forced to.)

Would there be a clean way to handle this kind of reassembly in auditd to
ensure that the on-disk record will continue to be in the currently
documented format? Or is there a way to strongly encourage people to keep
their hands off the raw audit logs and use documented interfaces that
take care of the conversions?

-Klaus

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Steve Grubb]

On Monday 08 October 2007 15:45:57 Klaus Weidner wrote:
> On Fri, Oct 05, 2007 at 11:11:27AM -0400, Eric Paris wrote:
> > My belief is that the solution to this problem is to allow audit to
> > break individual arguments down to a size <8k.  I guess my syntax would
> > be something like
> >
> > a0[0]=(first 8k of a single huge argument)
> > a0[1]=(second 8k of a single huge argument)
>
> [...]
>
> > who has a problem with that syntax?  will userspace puke?
>
> I'm a bit worried about special audit record formats that aren't
> generally seen in normal operation, since that's an obstacle to
> testability.

Well, I take this one to be the same as PATH records. Sometimes you get 1, 
sometimes 2, sometimes 3.

> The ASCII audit format encourages an ad-hoc parsing approach, and it's
> likely that tools other than the shipped ones won't be able to handle this
> and will break unexpectedly, possibly offering avenues to hide events with
> unusual records. (I know that people are supposed to use the parsing
> library, but they aren't being forced to.)

I also doubt people doing adhoc parsing know the encoding scheme either. So, 
they will hit a problem sooner or later.


> Would there be a clean way to handle this kind of reassembly in auditd to
> ensure that the on-disk record will continue to be in the currently
> documented format?

Well, the record size limit affects realtime interface programs too. They 
would all need to be recompiled to handle a bigger record.

> Or is there a way to strongly encourage people to keep their hands off the
> raw audit logs and use documented interfaces that take care of the
> conversions?

I suppose I could add a note to the daemon's man page. SE Linux has given the 
logs a special type and as long as you are not in an unconfined domain, you 
shouldn't be able to access it.

-Steve

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Linda Knippers]

Steve Grubb wrote:
> On Monday 08 October 2007 15:45:57 Klaus Weidner wrote:
>> On Fri, Oct 05, 2007 at 11:11:27AM -0400, Eric Paris wrote:
>>> My belief is that the solution to this problem is to allow audit to
>>> break individual arguments down to a size <8k.  I guess my syntax would
>>> be something like
>>>
>>> a0[0]=(first 8k of a single huge argument)
>>> a0[1]=(second 8k of a single huge argument)
>> [...]
>>
>>> who has a problem with that syntax?  will userspace puke?
>> I'm a bit worried about special audit record formats that aren't
>> generally seen in normal operation, since that's an obstacle to
>> testability.
> 
> Well, I take this one to be the same as PATH records. Sometimes you get 1, 
> sometimes 2, sometimes 3.

But for any given system call, wouldn't you always get the same number
of PATH records?

I think its important to know how many of the argument records are
expected, which I think would applies to both the number of arguments
as well as the number of records for any particular argument.

With PATH records, there's an item count in the SYSCALL record and
each PATH record says which one it is, so its possible to verify that
you've gotten them all.  I don't see the same type of information for
the arguments so its not possible to know if you've got a complete
audit trail.
> 
>> The ASCII audit format encourages an ad-hoc parsing approach, and it's
>> likely that tools other than the shipped ones won't be able to handle this
>> and will break unexpectedly, possibly offering avenues to hide events with
>> unusual records. (I know that people are supposed to use the parsing
>> library, but they aren't being forced to.)
> 
> I also doubt people doing adhoc parsing know the encoding scheme either. So, 
> they will hit a problem sooner or later.

Our tests do their own parsing and I expect we'll continue to do that.

-- ljk

> 
> 
>> Would there be a clean way to handle this kind of reassembly in auditd to
>> ensure that the on-disk record will continue to be in the currently
>> documented format?
> 
> Well, the record size limit affects realtime interface programs too. They 
> would all need to be recompiled to handle a bigger record.
> 
>> Or is there a way to strongly encourage people to keep their hands off the
>> raw audit logs and use documented interfaces that take care of the
>> conversions?
> 
> I suppose I could add a note to the daemon's man page. SE Linux has given the 
> logs a special type and as long as you are not in an unconfined domain, you 
> shouldn't be able to access it.
> 
> -Steve
> 
> --
> Linux-audit mailing list
> Linux-audit@redhat.com
> https://www.redhat.com/mailman/listinfo/linux-audit

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Steve Grubb]

On Monday 08 October 2007 18:45:15 Linda Knippers wrote:
> > Well, I take this one to be the same as PATH records. Sometimes you get
> > 1, sometimes 2, sometimes 3.
>
> But for any given system call, wouldn't you always get the same number
> of PATH records?

Maybe, sometimes you get a socket address record, too/instead of. The point is 
that you have no idea how many of them you are going to get without some 
analysis.

> With PATH records, there's an item count in the SYSCALL record and
> each PATH record says which one it is, so its possible to verify that
> you've gotten them all. 

The way these get split, there is no way to know ahead of time how many you 
are going to get.  These are being split as they are being output. The item 
count displayed in syscall is incremented as each aux record data is added to 
the context. So, there's no performance cost for displaying this.

We could add an item parameter, but that only gives you sequence information. 
But you could infer the sequence information by the argument's number - it 
continually increments. If a record ends and a347 and the next one begins at 
a895, then you are missing one or more records.

But even then, I don't think that's possible unless someone's tampered with 
the logs. If any allocation can't be done, the syscall is failed. So, the 
only question is what happens if the netlink queue has a problem sending to 
user space? Well, you get a syslog message and the kernel follows the  
failure action set by the admin - just as it would for any event.

> I don't see the same type of information for the arguments so its not
> possible to know if you've got a complete audit trail.

When it moves on to another record type, you've got them all.

-Steve

Re: [PATCH 2/2] Audit: remove the limit on execve arguments when audit is running

[Linda Knippers]

Steve Grubb wrote:
> On Monday 08 October 2007 18:45:15 Linda Knippers wrote:
>>> Well, I take this one to be the same as PATH records. Sometimes you get
>>> 1, sometimes 2, sometimes 3.
>> But for any given system call, wouldn't you always get the same number
>> of PATH records?
> 
> Maybe, sometimes you get a socket address record, too/instead of. The point is 
> that you have no idea how many of them you are going to get without some 
> analysis.

In the case of arguments, don't you know argc?  You could emit argc= before
a0.

>> With PATH records, there's an item count in the SYSCALL record and
>> each PATH record says which one it is, so its possible to verify that
>> you've gotten them all. 
> 
> The way these get split, there is no way to know ahead of time how many you 
> are going to get.  These are being split as they are being output. The item 
> count displayed in syscall is incremented as each aux record data is added to 
> the context. So, there's no performance cost for displaying this.

I care about knowing how many arguments there are so that I know if I've got
them all, not so much how many records get emitted.  However, in the case
where a single argument is split across multiple records, I think it would
be good to know that I've gotten 1 of 3, 2 of 3, 3 of 3, etc, or the total
length of the argument.
> 
> We could add an item parameter, but that only gives you sequence information. 
> But you could infer the sequence information by the argument's number - it 
> continually increments. If a record ends and a347 and the next one begins at 
> a895, then you are missing one or more records.

Unless I'm missing the last records for a syscall, in which case all I know
is that there aren't any more in the log.
> 
> But even then, I don't think that's possible unless someone's tampered with 
> the logs. If any allocation can't be done, the syscall is failed. So, the 
> only question is what happens if the netlink queue has a problem sending to 
> user space? Well, you get a syslog message and the kernel follows the  
> failure action set by the admin - just as it would for any event.
> 
>> I don't see the same type of information for the arguments so its not
>> possible to know if you've got a complete audit trail.
> 
> When it moves on to another record type, you've got them all.

Not necessarily.

-- ljk
> 
> -Steve