audit: EXECVE record - removed bogus newline

audit: EXECVE record - removed bogus newline

[Jiri Pirko]

EXECVE records contain a newline after every argument. auditd converts
"\n" to " " so you cannot see newlines even in raw logs, but they're
there nevertheless. If you're not using auditd, you need to work round
them. These '\n' chars are can be easily replaced by spaces when
creating record in kernel. Note there is no need for trailing '\n' in
an audit record. 

record before this patch:
"type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\"\na1=\"a\"\na2=\"b\"\na3=\"c\"\n"

record after this patch:
"type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\" a1=\"a\" a2=\"b\" a3=\"c\""


Signed-off-by: Jiri Pirko <jpirko@redhat.com>
---
 kernel/auditsc.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8cbddff..c7012e0 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1109,7 +1109,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
 		 * so we can be sure nothing was lost.
 		 */
 		if ((i == 0) && (too_long))
-			audit_log_format(*ab, "a%d_len=%zu ", arg_num,
+			audit_log_format(*ab, " a%d_len=%zu", arg_num,
 					 has_cntl ? 2*len : len);
 
 		/*
@@ -1129,7 +1129,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
 		buf[to_send] = '\0';
 
 		/* actually log it */
-		audit_log_format(*ab, "a%d", arg_num);
+		audit_log_format(*ab, " a%d", arg_num);
 		if (too_long)
 			audit_log_format(*ab, "[%d]", i);
 		audit_log_format(*ab, "=");
@@ -1137,7 +1137,6 @@ static int audit_log_single_execve_arg(struct audit_context *context,
 			audit_log_n_hex(*ab, buf, to_send);
 		else
 			audit_log_format(*ab, "\"%s\"", buf);
-		audit_log_format(*ab, "\n");
 
 		p += to_send;
 		len_left -= to_send;
@@ -1165,7 +1164,7 @@ static void audit_log_execve_info(struct audit_context *context,
 
 	p = (const char __user *)axi->mm->arg_start;
 
-	audit_log_format(*ab, "argc=%d ", axi->argc);
+	audit_log_format(*ab, "argc=%d", axi->argc);
 
 	/*
 	 * we need some kernel buffer to hold the userspace args.  Just
-- 
1.5.6.5

Re: audit: EXECVE record - removed bogus newline

[Eric Paris]

On Thu, 2009-01-08 at 15:38 +0100, Jiri Pirko wrote:
> EXECVE records contain a newline after every argument. auditd converts
> "\n" to " " so you cannot see newlines even in raw logs, but they're
> there nevertheless. If you're not using auditd, you need to work round
> them. These '\n' chars are can be easily replaced by spaces when
> creating record in kernel. Note there is no need for trailing '\n' in
> an audit record. 

While I completely agree the \n was my mistake and should be
dropped/fixed can you fix one more thing and look at another?  First
arg_num_len is being miscalculated since I included the \n in that
calculation (might be the only place....) and I remember not wanting to
follow convention and put the space at the beginning of the aX= for some
reason.  If you add thousands of arguments so this is larger than
MAX_EXECVE_AUDIT_LEN do you end up with an extra space somewhere in the
second EXECVE record?  Or maybe it was a single argument that was larger
than the max, can't remember which, but I do remember having a random
extra space (maybe we'd rather have that for consistency?)

-Eric

> 
> record before this patch:
> "type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\"\na1=\"a\"\na2=\"b\"\na3=\"c\"\n"
> 
> record after this patch:
> "type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\" a1=\"a\" a2=\"b\" a3=\"c\""
> 
> 
> Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> ---
>  kernel/auditsc.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8cbddff..c7012e0 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1109,7 +1109,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
>  		 * so we can be sure nothing was lost.
>  		 */
>  		if ((i == 0) && (too_long))
> -			audit_log_format(*ab, "a%d_len=%zu ", arg_num,
> +			audit_log_format(*ab, " a%d_len=%zu", arg_num,
>  					 has_cntl ? 2*len : len);
>  
>  		/*
> @@ -1129,7 +1129,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
>  		buf[to_send] = '\0';
>  
>  		/* actually log it */
> -		audit_log_format(*ab, "a%d", arg_num);
> +		audit_log_format(*ab, " a%d", arg_num);
>  		if (too_long)
>  			audit_log_format(*ab, "[%d]", i);
>  		audit_log_format(*ab, "=");
> @@ -1137,7 +1137,6 @@ static int audit_log_single_execve_arg(struct audit_context *context,
>  			audit_log_n_hex(*ab, buf, to_send);
>  		else
>  			audit_log_format(*ab, "\"%s\"", buf);
> -		audit_log_format(*ab, "\n");
>  
>  		p += to_send;
>  		len_left -= to_send;
> @@ -1165,7 +1164,7 @@ static void audit_log_execve_info(struct audit_context *context,
>  
>  	p = (const char __user *)axi->mm->arg_start;
>  
> -	audit_log_format(*ab, "argc=%d ", axi->argc);
> +	audit_log_format(*ab, "argc=%d", axi->argc);
>  
>  	/*
>  	 * we need some kernel buffer to hold the userspace args.  Just

Re: audit: EXECVE record - removed bogus newline

[Jiri Pirko]

On Thu, 08 Jan 2009 10:33:18 -0500
Eric Paris <eparis@redhat.com> wrote:

> On Thu, 2009-01-08 at 15:38 +0100, Jiri Pirko wrote:
> > EXECVE records contain a newline after every argument. auditd converts
> > "\n" to " " so you cannot see newlines even in raw logs, but they're
> > there nevertheless. If you're not using auditd, you need to work round
> > them. These '\n' chars are can be easily replaced by spaces when
> > creating record in kernel. Note there is no need for trailing '\n' in
> > an audit record. 
> 
> While I completely agree the \n was my mistake and should be
> dropped/fixed can you fix one more thing and look at another?  First
> arg_num_len is being miscalculated since I included the \n in that
> calculation (might be the only place....)
I think that calculation is correct only the comment needs to be changed like this:
-       /* how many digits are in arg_num? 3 is the length of a=\n */
+       /* how many digits are in arg_num? 3 is the length of " a=" */

the count is still 3 - with '\n' or with ' '.


> and I remember not wanting to
> follow convention and put the space at the beginning of the aX= for some
> reason.  If you add thousands of arguments so this is larger than
> MAX_EXECVE_AUDIT_LEN do you end up with an extra space somewhere in the
> second EXECVE record? 
Is this a problem? We can do some workaround but is it necessary?
> Or maybe it was a single argument that was larger
> than the max
This would do the same thing (leading space in record).
> , can't remember which, but I do remember having a random
> extra space (maybe we'd rather have that for consistency?)
Yes that's right - the space might be good if userspace client just
concatenates two records, the space should be good to be there. But I'm
unable to judge this. What do others think?

Jirka
> 
> -Eric
> 
> > 
> > record before this patch:
> > "type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\"\na1=\"a\"\na2=\"b\"\na3=\"c\"\n"
> > 
> > record after this patch:
> > "type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\" a1=\"a\" a2=\"b\" a3=\"c\""
> > 
> > 
> > Signed-off-by: Jiri Pirko <jpirko@redhat.com>
> > ---
> >  kernel/auditsc.c |    7 +++----
> >  1 files changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 8cbddff..c7012e0 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1109,7 +1109,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
> >  		 * so we can be sure nothing was lost.
> >  		 */
> >  		if ((i == 0) && (too_long))
> > -			audit_log_format(*ab, "a%d_len=%zu ", arg_num,
> > +			audit_log_format(*ab, " a%d_len=%zu", arg_num,
> >  					 has_cntl ? 2*len : len);
> >  
> >  		/*
> > @@ -1129,7 +1129,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
> >  		buf[to_send] = '\0';
> >  
> >  		/* actually log it */
> > -		audit_log_format(*ab, "a%d", arg_num);
> > +		audit_log_format(*ab, " a%d", arg_num);
> >  		if (too_long)
> >  			audit_log_format(*ab, "[%d]", i);
> >  		audit_log_format(*ab, "=");
> > @@ -1137,7 +1137,6 @@ static int audit_log_single_execve_arg(struct audit_context *context,
> >  			audit_log_n_hex(*ab, buf, to_send);
> >  		else
> >  			audit_log_format(*ab, "\"%s\"", buf);
> > -		audit_log_format(*ab, "\n");
> >  
> >  		p += to_send;
> >  		len_left -= to_send;
> > @@ -1165,7 +1164,7 @@ static void audit_log_execve_info(struct audit_context *context,
> >  
> >  	p = (const char __user *)axi->mm->arg_start;
> >  
> > -	audit_log_format(*ab, "argc=%d ", axi->argc);
> > +	audit_log_format(*ab, "argc=%d", axi->argc);
> >  
> >  	/*
> >  	 * we need some kernel buffer to hold the userspace args.  Just
> 
> 

Re: audit: EXECVE record - removed bogus newline

[Eric Paris]

On Fri, 2009-01-09 at 13:21 +0100, Jiri Pirko wrote:
> On Thu, 08 Jan 2009 10:33:18 -0500
> Eric Paris <eparis@redhat.com> wrote:
> 
> > On Thu, 2009-01-08 at 15:38 +0100, Jiri Pirko wrote:
> > > EXECVE records contain a newline after every argument. auditd converts
> > > "\n" to " " so you cannot see newlines even in raw logs, but they're
> > > there nevertheless. If you're not using auditd, you need to work round
> > > them. These '\n' chars are can be easily replaced by spaces when
> > > creating record in kernel. Note there is no need for trailing '\n' in
> > > an audit record. 
> > 
> > While I completely agree the \n was my mistake and should be
> > dropped/fixed can you fix one more thing and look at another?  First
> > arg_num_len is being miscalculated since I included the \n in that
> > calculation (might be the only place....)
> I think that calculation is correct only the comment needs to be changed like this:
> -       /* how many digits are in arg_num? 3 is the length of a=\n */
> +       /* how many digits are in arg_num? 3 is the length of " a=" */
> 
> the count is still 3 - with '\n' or with ' '.

You're right.  Sorry, I'd like to see the comment fixed.  That 3 does
matter and I don't want to see someone not realize how important getting
that len right is....

> > and I remember not wanting to
> > follow convention and put the space at the beginning of the aX= for some
> > reason.  If you add thousands of arguments so this is larger than
> > MAX_EXECVE_AUDIT_LEN do you end up with an extra space somewhere in the
> > second EXECVE record? 
> Is this a problem? We can do some workaround but is it necessary?

I'll go ahead and vote I don't care, even though I brought it up.  My
code left an extra space at the end of the record.  Yours puts it at the
beginning.  From the point of view of auditd the spaces don't mean
anything, so I'm ok with your method.  I guess it saves a call to
audit_log_format, and saving a call is benefit enough

Don't care if it's this window or the next, it's been like this for 3
weeks short of a year now  :)

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

Re: audit: EXECVE record - removed bogus newline

[Jiri Pirko]

(updated)
Added hunk that changes the comment, the rest is the same.


EXECVE records contain a newline after every argument. auditd converts
"\n" to " " so you cannot see newlines even in raw logs, but they're
there nevertheless. If you're not using auditd, you need to work round
them. These '\n' chars are can be easily replaced by spaces when
creating record in kernel. Note there is no need for trailing '\n' in
an audit record. 

record before this patch:
"type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\"\na1=\"a\"\na2=\"b\"\na3=\"c\"\n"

record after this patch:
"type=EXECVE msg=audit(1231421801.566:31): argc=4 a0=\"./test\" a1=\"a\" a2=\"b\" a3=\"c\""


Signed-off-by: Jiri Pirko <jpirko@redhat.com>
Acked-by: Eric Paris <eparis@redhat.com>
---
 kernel/auditsc.c |    9 ++++-----
 1 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8cbddff..7a46d62 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1023,7 +1023,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
 {
 	char arg_num_len_buf[12];
 	const char __user *tmp_p = p;
-	/* how many digits are in arg_num? 3 is the length of a=\n */
+	/* how many digits are in arg_num? 3 is the length of " a=" */
 	size_t arg_num_len = snprintf(arg_num_len_buf, 12, "%d", arg_num) + 3;
 	size_t len, len_left, to_send;
 	size_t max_execve_audit_len = MAX_EXECVE_AUDIT_LEN;
@@ -1109,7 +1109,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
 		 * so we can be sure nothing was lost.
 		 */
 		if ((i == 0) && (too_long))
-			audit_log_format(*ab, "a%d_len=%zu ", arg_num,
+			audit_log_format(*ab, " a%d_len=%zu", arg_num,
 					 has_cntl ? 2*len : len);
 
 		/*
@@ -1129,7 +1129,7 @@ static int audit_log_single_execve_arg(struct audit_context *context,
 		buf[to_send] = '\0';
 
 		/* actually log it */
-		audit_log_format(*ab, "a%d", arg_num);
+		audit_log_format(*ab, " a%d", arg_num);
 		if (too_long)
 			audit_log_format(*ab, "[%d]", i);
 		audit_log_format(*ab, "=");
@@ -1137,7 +1137,6 @@ static int audit_log_single_execve_arg(struct audit_context *context,
 			audit_log_n_hex(*ab, buf, to_send);
 		else
 			audit_log_format(*ab, "\"%s\"", buf);
-		audit_log_format(*ab, "\n");
 
 		p += to_send;
 		len_left -= to_send;
@@ -1165,7 +1164,7 @@ static void audit_log_execve_info(struct audit_context *context,
 
 	p = (const char __user *)axi->mm->arg_start;
 
-	audit_log_format(*ab, "argc=%d ", axi->argc);
+	audit_log_format(*ab, "argc=%d", axi->argc);
 
 	/*
 	 * we need some kernel buffer to hold the userspace args.  Just
-- 
1.5.6.5