[RFC][PATCH] - auditing cmdline

public inbox for linux-audit@redhat.com
 help / color / mirror / Atom feed

* [RFC][PATCH] - auditing cmdline
@ 2014-01-07 20:43 William Roberts
  2014-01-07 20:44 ` [PATCH] squash: review-updates William Roberts
  0 siblings, 1 reply; 8+ messages in thread
From: William Roberts @ 2014-01-07 20:43 UTC (permalink / raw)
  To: sds, rgb, linux-audit

Just some updates from the review that I have gathered.
1. Possibility of accessing invalid memory on res being 0.
2. Do not switch ordering of fields.

Does anyone hate using the boolean expression to get my
increment value? Removes some branches. Based on my
loose understanding that boolean operators are guarenteed
to be 0 or 1. Although I am never a big fan of stuff like
this in general. Should I just do the branches?

http://c-faq.com/bool/bool2.html

[PATCH] squash: review-updates

^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH] squash: review-updates
  2014-01-07 20:43 [RFC][PATCH] - auditing cmdline William Roberts
@ 2014-01-07 20:44 ` William Roberts
  2014-01-08 16:01   ` William Roberts
                     ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: William Roberts @ 2014-01-07 20:44 UTC (permalink / raw)
  To: sds, rgb, linux-audit; +Cc: William Roberts

Signed-off-by: William Roberts <wroberts@tresys.com>
---
 kernel/auditsc.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index a4c2003..9ba1f2a 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
 		if (!buf)
 			goto out;
 		res = get_cmdline(tsk, buf, PATH_MAX);
-		/* Ensure NULL terminated */
-		if (buf[res-1] != '\0')
-			buf[res-1] = '\0';
+		if (res == 0) {
+			kfree(buf);
+			goto out;
+		}
+		/*
+		 * Ensure NULL terminated but don't clobber the end
+		 * unless the buffer is full. Worst case you end up
+		 * with 2 null bytes ending it. By doing it this way
+		 * one avoids additional branching. One checking if the
+		 * end is null and another to check if their should be
+		 * an increment before setting the null byte.
+		 */
+		res += res < PATH_MAX;
+		buf[res-1] = '\0';
 		context->cmdline = buf;
 	}
 	msg = context->cmdline;
@@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
 			 context->name_count);
 
 	audit_log_task_info(ab, tsk);
-	audit_log_cmdline(ab, tsk, context);
 	audit_log_key(ab, context->filterkey);
+	audit_log_cmdline(ab, tsk, context);
 	audit_log_end(ab);
 
 	for (aux = context->aux; aux; aux = aux->next) {
-- 
1.7.9.5

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH] squash: review-updates
  2014-01-07 20:44 ` [PATCH] squash: review-updates William Roberts
@ 2014-01-08 16:01   ` William Roberts
  2014-01-08 16:10   ` Richard Guy Briggs
  2014-01-10 21:08   ` Eric Paris
  2 siblings, 0 replies; 8+ messages in thread
From: William Roberts @ 2014-01-08 16:01 UTC (permalink / raw)
  To: Stephen Smalley, Richard Guy Briggs, linux-audit@redhat.com
  Cc: William Roberts

On Tue, Jan 7, 2014 at 12:44 PM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> Signed-off-by: William Roberts <wroberts@tresys.com>
> ---
>  kernel/auditsc.c |   19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index a4c2003..9ba1f2a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
>                 if (!buf)
>                         goto out;
>                 res = get_cmdline(tsk, buf, PATH_MAX);
> -               /* Ensure NULL terminated */
> -               if (buf[res-1] != '\0')
> -                       buf[res-1] = '\0';
> +               if (res == 0) {
> +                       kfree(buf);
> +                       goto out;
> +               }
> +               /*
> +                * Ensure NULL terminated but don't clobber the end
> +                * unless the buffer is full. Worst case you end up
> +                * with 2 null bytes ending it. By doing it this way
> +                * one avoids additional branching. One checking if the
> +                * end is null and another to check if their should be
> +                * an increment before setting the null byte.
> +                */
> +               res += res < PATH_MAX;
> +               buf[res-1] = '\0';
>                 context->cmdline = buf;
>         }
>         msg = context->cmdline;
> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
>                          context->name_count);
>
>         audit_log_task_info(ab, tsk);
> -       audit_log_cmdline(ab, tsk, context);
>         audit_log_key(ab, context->filterkey);
> +       audit_log_cmdline(ab, tsk, context);
>         audit_log_end(ab);
>
>         for (aux = context->aux; aux; aux = aux->next) {
> --
> 1.7.9.5
>


FYI for those wondering. Page 205 and 206, Sections A7.9 and A7.10 in
the K+R C book 2nd edition explicitly states that relational and
equality operators return a 0 or 1.

-- 
Respectfully,

William C Roberts

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] squash: review-updates
  2014-01-07 20:44 ` [PATCH] squash: review-updates William Roberts
  2014-01-08 16:01   ` William Roberts
@ 2014-01-08 16:10   ` Richard Guy Briggs
  2014-01-10 21:08   ` Eric Paris
  2 siblings, 0 replies; 8+ messages in thread
From: Richard Guy Briggs @ 2014-01-08 16:10 UTC (permalink / raw)
  To: William Roberts; +Cc: William Roberts, linux-audit

On 14/01/07, William Roberts wrote:
> Signed-off-by: William Roberts <wroberts@tresys.com>
> ---
>  kernel/auditsc.c |   19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index a4c2003..9ba1f2a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
>  		if (!buf)
>  			goto out;
>  		res = get_cmdline(tsk, buf, PATH_MAX);
> -		/* Ensure NULL terminated */
> -		if (buf[res-1] != '\0')
> -			buf[res-1] = '\0';
> +		if (res == 0) {
> +			kfree(buf);
> +			goto out;
> +		}
> +		/*
> +		 * Ensure NULL terminated but don't clobber the end
> +		 * unless the buffer is full. Worst case you end up
> +		 * with 2 null bytes ending it. By doing it this way
> +		 * one avoids additional branching. One checking if the
> +		 * end is null and another to check if their should be
> +		 * an increment before setting the null byte.
> +		 */
> +		res += res < PATH_MAX;
> +		buf[res-1] = '\0';
>  		context->cmdline = buf;
>  	}
>  	msg = context->cmdline;
> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
>  			 context->name_count);
>  
>  	audit_log_task_info(ab, tsk);
> -	audit_log_cmdline(ab, tsk, context);
>  	audit_log_key(ab, context->filterkey);
> +	audit_log_cmdline(ab, tsk, context);
>  	audit_log_end(ab);
>  
>  	for (aux = context->aux; aux; aux = aux->next) {

I'm fine with this.

- RGB

--
Richard Guy Briggs <rbriggs@redhat.com>
Senior Software Engineer, Kernel Security, AMER ENG Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635, Alt: +1.613.693.0684x3545

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] squash: review-updates
  2014-01-07 20:44 ` [PATCH] squash: review-updates William Roberts
  2014-01-08 16:01   ` William Roberts
  2014-01-08 16:10   ` Richard Guy Briggs
@ 2014-01-10 21:08   ` Eric Paris
  2014-01-10 21:33     ` Eric Paris
  2014-01-10 21:37     ` William Roberts
  2 siblings, 2 replies; 8+ messages in thread
From: Eric Paris @ 2014-01-10 21:08 UTC (permalink / raw)
  To: William Roberts; +Cc: rgb, William, Roberts, linux-audit

If you know the buf len, you can just use audit_log_n_untrusted_string()
I think....

On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
> Signed-off-by: William Roberts <wroberts@tresys.com>
> ---
>  kernel/auditsc.c |   19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index a4c2003..9ba1f2a 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
>  		if (!buf)
>  			goto out;
>  		res = get_cmdline(tsk, buf, PATH_MAX);
> -		/* Ensure NULL terminated */
> -		if (buf[res-1] != '\0')
> -			buf[res-1] = '\0';
> +		if (res == 0) {
> +			kfree(buf);
> +			goto out;
> +		}
> +		/*
> +		 * Ensure NULL terminated but don't clobber the end
> +		 * unless the buffer is full. Worst case you end up
> +		 * with 2 null bytes ending it. By doing it this way
> +		 * one avoids additional branching. One checking if the
> +		 * end is null and another to check if their should be
> +		 * an increment before setting the null byte.
> +		 */
> +		res += res < PATH_MAX;
> +		buf[res-1] = '\0';
>  		context->cmdline = buf;
>  	}
>  	msg = context->cmdline;
> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
>  			 context->name_count);
>  
>  	audit_log_task_info(ab, tsk);
> -	audit_log_cmdline(ab, tsk, context);
>  	audit_log_key(ab, context->filterkey);
> +	audit_log_cmdline(ab, tsk, context);
>  	audit_log_end(ab);
>  
>  	for (aux = context->aux; aux; aux = aux->next) {

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] squash: review-updates
  2014-01-10 21:08   ` Eric Paris
@ 2014-01-10 21:33     ` Eric Paris
  2014-01-10 21:37     ` William Roberts
  1 sibling, 0 replies; 8+ messages in thread
From: Eric Paris @ 2014-01-10 21:33 UTC (permalink / raw)
  To: William Roberts; +Cc: rgb, William, Roberts, linux-audit

On Fri, 2014-01-10 at 16:08 -0500, Eric Paris wrote:
> If you know the buf len, you can just use audit_log_n_untrusted_string()
> I think....

admittedly audit_log_n_untrusted_string() will log ALL of n.  So you
could use strnlen() as the argument to audit_log_n_untrusted_string()
(or create another helper, I don't care)

-Eric
> 
> On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
> > Signed-off-by: William Roberts <wroberts@tresys.com>
> > ---
> >  kernel/auditsc.c |   19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index a4c2003..9ba1f2a 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
> >  		if (!buf)
> >  			goto out;
> >  		res = get_cmdline(tsk, buf, PATH_MAX);
> > -		/* Ensure NULL terminated */
> > -		if (buf[res-1] != '\0')
> > -			buf[res-1] = '\0';
> > +		if (res == 0) {
> > +			kfree(buf);
> > +			goto out;
> > +		}
> > +		/*
> > +		 * Ensure NULL terminated but don't clobber the end
> > +		 * unless the buffer is full. Worst case you end up
> > +		 * with 2 null bytes ending it. By doing it this way
> > +		 * one avoids additional branching. One checking if the
> > +		 * end is null and another to check if their should be
> > +		 * an increment before setting the null byte.
> > +		 */
> > +		res += res < PATH_MAX;
> > +		buf[res-1] = '\0';
> >  		context->cmdline = buf;
> >  	}
> >  	msg = context->cmdline;
> > @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
> >  			 context->name_count);
> >  
> >  	audit_log_task_info(ab, tsk);
> > -	audit_log_cmdline(ab, tsk, context);
> >  	audit_log_key(ab, context->filterkey);
> > +	audit_log_cmdline(ab, tsk, context);
> >  	audit_log_end(ab);
> >  
> >  	for (aux = context->aux; aux; aux = aux->next) {
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] squash: review-updates
  2014-01-10 21:08   ` Eric Paris
  2014-01-10 21:33     ` Eric Paris
@ 2014-01-10 21:37     ` William Roberts
  2014-01-10 21:42       ` William Roberts
  1 sibling, 1 reply; 8+ messages in thread
From: William Roberts @ 2014-01-10 21:37 UTC (permalink / raw)
  To: Eric Paris; +Cc: Richard Guy Briggs, William Roberts, linux-audit@redhat.com

I think your right....

On Fri, Jan 10, 2014 at 1:08 PM, Eric Paris <eparis@redhat.com> wrote:
> If you know the buf len, you can just use audit_log_n_untrusted_string()
> I think....
>
> On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
>> Signed-off-by: William Roberts <wroberts@tresys.com>
>> ---
>>  kernel/auditsc.c |   19 +++++++++++++++----
>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>> index a4c2003..9ba1f2a 100644
>> --- a/kernel/auditsc.c
>> +++ b/kernel/auditsc.c
>> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
>>               if (!buf)
>>                       goto out;
>>               res = get_cmdline(tsk, buf, PATH_MAX);
>> -             /* Ensure NULL terminated */
>> -             if (buf[res-1] != '\0')
>> -                     buf[res-1] = '\0';
>> +             if (res == 0) {
>> +                     kfree(buf);
>> +                     goto out;
>> +             }
>> +             /*
>> +              * Ensure NULL terminated but don't clobber the end
>> +              * unless the buffer is full. Worst case you end up
>> +              * with 2 null bytes ending it. By doing it this way
>> +              * one avoids additional branching. One checking if the
>> +              * end is null and another to check if their should be
>> +              * an increment before setting the null byte.
>> +              */
>> +             res += res < PATH_MAX;
>> +             buf[res-1] = '\0';
>>               context->cmdline = buf;
>>       }
>>       msg = context->cmdline;
>> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
>>                        context->name_count);
>>
>>       audit_log_task_info(ab, tsk);
>> -     audit_log_cmdline(ab, tsk, context);
>>       audit_log_key(ab, context->filterkey);
>> +     audit_log_cmdline(ab, tsk, context);
>>       audit_log_end(ab);
>>
>>       for (aux = context->aux; aux; aux = aux->next) {
>
>



-- 
Respectfully,

William C Roberts

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] squash: review-updates
  2014-01-10 21:37     ` William Roberts
@ 2014-01-10 21:42       ` William Roberts
  0 siblings, 0 replies; 8+ messages in thread
From: William Roberts @ 2014-01-10 21:42 UTC (permalink / raw)
  To: Eric Paris; +Cc: Richard Guy Briggs, William Roberts, linux-audit@redhat.com

hmm if I recall too the result from calling the underlying vma
functions was the width of the field, not
always necessarily the null byte, like you said, doing the combo you
suggest corrects that.

However, strnlen is pricey id like to avoid. The code I have at least
avoids branching, which is good.
And is then directly known to be safe to pass to
audit_log_untrusted_string(), which aborts printing
on the first null byte.

On Fri, Jan 10, 2014 at 1:37 PM, William Roberts
<bill.c.roberts@gmail.com> wrote:
> I think your right....
>
> On Fri, Jan 10, 2014 at 1:08 PM, Eric Paris <eparis@redhat.com> wrote:
>> If you know the buf len, you can just use audit_log_n_untrusted_string()
>> I think....
>>
>> On Tue, 2014-01-07 at 12:44 -0800, William Roberts wrote:
>>> Signed-off-by: William Roberts <wroberts@tresys.com>
>>> ---
>>>  kernel/auditsc.c |   19 +++++++++++++++----
>>>  1 file changed, 15 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
>>> index a4c2003..9ba1f2a 100644
>>> --- a/kernel/auditsc.c
>>> +++ b/kernel/auditsc.c
>>> @@ -1292,9 +1292,20 @@ static void audit_log_cmdline(struct audit_buffer *ab, struct task_struct *tsk,
>>>               if (!buf)
>>>                       goto out;
>>>               res = get_cmdline(tsk, buf, PATH_MAX);
>>> -             /* Ensure NULL terminated */
>>> -             if (buf[res-1] != '\0')
>>> -                     buf[res-1] = '\0';
>>> +             if (res == 0) {
>>> +                     kfree(buf);
>>> +                     goto out;
>>> +             }
>>> +             /*
>>> +              * Ensure NULL terminated but don't clobber the end
>>> +              * unless the buffer is full. Worst case you end up
>>> +              * with 2 null bytes ending it. By doing it this way
>>> +              * one avoids additional branching. One checking if the
>>> +              * end is null and another to check if their should be
>>> +              * an increment before setting the null byte.
>>> +              */
>>> +             res += res < PATH_MAX;
>>> +             buf[res-1] = '\0';
>>>               context->cmdline = buf;
>>>       }
>>>       msg = context->cmdline;
>>> @@ -1333,8 +1344,8 @@ static void audit_log_exit(struct audit_context *context, struct task_struct *ts
>>>                        context->name_count);
>>>
>>>       audit_log_task_info(ab, tsk);
>>> -     audit_log_cmdline(ab, tsk, context);
>>>       audit_log_key(ab, context->filterkey);
>>> +     audit_log_cmdline(ab, tsk, context);
>>>       audit_log_end(ab);
>>>
>>>       for (aux = context->aux; aux; aux = aux->next) {
>>
>>
>
>
>
> --
> Respectfully,
>
> William C Roberts



-- 
Respectfully,

William C Roberts

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2014-01-10 21:42 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-01-07 20:43 [RFC][PATCH] - auditing cmdline William Roberts
2014-01-07 20:44 ` [PATCH] squash: review-updates William Roberts
2014-01-08 16:01   ` William Roberts
2014-01-08 16:10   ` Richard Guy Briggs
2014-01-10 21:08   ` Eric Paris
2014-01-10 21:33     ` Eric Paris
2014-01-10 21:37     ` William Roberts
2014-01-10 21:42       ` William Roberts

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox