* [PATCH] integrity: get comm using lock to avoid race in string printing @ 2014-04-02 16:19 Richard Guy Briggs 2014-04-02 17:39 ` Mimi Zohar 0 siblings, 1 reply; 12+ messages in thread From: Richard Guy Briggs @ 2014-04-02 16:19 UTC (permalink / raw) To: linux-audit, linux-kernel, linux-security-module; +Cc: Richard Guy Briggs When task->comm is passed directly to audit_log_untrustedstring() without getting a copy or using the task_lock, there is a race that could happen that would output a NULL (\0) in the output string that would effectively truncate the rest of the report text after the comm= field in the audit, losing fields. Use get_task_comm() to get a copy while acquiring the task_lock to prevent this and to prevent the result from being a mixture of old and new values of comm. Signed-off-by: Richard Guy Briggs <rgb@redhat.com> --- security/integrity/integrity_audit.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c index 85253b5..11706a2 100644 --- a/security/integrity/integrity_audit.c +++ b/security/integrity/integrity_audit.c @@ -33,6 +33,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, const char *cause, int result, int audit_info) { struct audit_buffer *ab; + char comm[sizeof(current->comm)]; if (!integrity_audit_info && audit_info == 1) /* Skip info messages */ return; @@ -49,7 +50,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, audit_log_format(ab, " cause="); audit_log_string(ab, cause); audit_log_format(ab, " comm="); - audit_log_untrustedstring(ab, current->comm); + audit_log_untrustedstring(ab, get_task_comm(comm, current)); if (fname) { audit_log_format(ab, " name="); audit_log_untrustedstring(ab, fname); -- 1.7.1 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] integrity: get comm using lock to avoid race in string printing 2014-04-02 16:19 [PATCH] integrity: get comm using lock to avoid race in string printing Richard Guy Briggs @ 2014-04-02 17:39 ` Mimi Zohar 2014-04-02 18:00 ` Steve Grubb 2014-04-02 18:45 ` [PATCH] integrity: get comm using lock to avoid race in string printing Richard Guy Briggs 0 siblings, 2 replies; 12+ messages in thread From: Mimi Zohar @ 2014-04-02 17:39 UTC (permalink / raw) To: Richard Guy Briggs Cc: linux-audit, linux-kernel, linux-security-module, eparis, sgrubb On Wed, 2014-04-02 at 12:19 -0400, Richard Guy Briggs wrote: > When task->comm is passed directly to audit_log_untrustedstring() without > getting a copy or using the task_lock, there is a race that could happen that > would output a NULL (\0) in the output string that would effectively truncate > the rest of the report text after the comm= field in the audit, losing fields. > > Use get_task_comm() to get a copy while acquiring the task_lock to prevent > this and to prevent the result from being a mixture of old and new values of > comm. > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > --- > security/integrity/integrity_audit.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c > index 85253b5..11706a2 100644 > --- a/security/integrity/integrity_audit.c > +++ b/security/integrity/integrity_audit.c > @@ -33,6 +33,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, > const char *cause, int result, int audit_info) > { > struct audit_buffer *ab; > + char comm[sizeof(current->comm)]; > > if (!integrity_audit_info && audit_info == 1) /* Skip info messages */ > return; > @@ -49,7 +50,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, > audit_log_format(ab, " cause="); > audit_log_string(ab, cause); > audit_log_format(ab, " comm="); > - audit_log_untrustedstring(ab, current->comm); > + audit_log_untrustedstring(ab, get_task_comm(comm, current)); > if (fname) { > audit_log_format(ab, " name="); > audit_log_untrustedstring(ab, fname); This change is already being upstreamed as commit 73a6b44 "Integrity: Pass commname via get_task_comm()". thanks, Mimi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] integrity: get comm using lock to avoid race in string printing 2014-04-02 17:39 ` Mimi Zohar @ 2014-04-02 18:00 ` Steve Grubb 2014-04-02 18:12 ` Mimi Zohar 2014-04-02 18:45 ` [PATCH] integrity: get comm using lock to avoid race in string printing Richard Guy Briggs 1 sibling, 1 reply; 12+ messages in thread From: Steve Grubb @ 2014-04-02 18:00 UTC (permalink / raw) To: Mimi Zohar Cc: Richard Guy Briggs, linux-security-module, linux-audit, linux-kernel Hello Mimi, On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > This change is already being upstreamed as commit 73a6b44 "Integrity: > Pass commname via get_task_comm()". While I was looking at Richard's patch, I noticed a few places where cause and op are logged and the string isn't tied together with a _ or -. These are in ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are these fixed upstream? Or should a patch be made? Thanks, -Steve ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] integrity: get comm using lock to avoid race in string printing 2014-04-02 18:00 ` Steve Grubb @ 2014-04-02 18:12 ` Mimi Zohar 2014-04-02 18:18 ` Eric Paris 0 siblings, 1 reply; 12+ messages in thread From: Mimi Zohar @ 2014-04-02 18:12 UTC (permalink / raw) To: Steve Grubb Cc: Richard Guy Briggs, linux-security-module, linux-audit, linux-kernel On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: > Hello Mimi, > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > > This change is already being upstreamed as commit 73a6b44 "Integrity: > > Pass commname via get_task_comm()". > > While I was looking at Richard's patch, I noticed a few places where cause and > op are logged and the string isn't tied together with a _ or -. These are in > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are > these fixed upstream? Or should a patch be made? Nothing has changed in terms of 'cause' and 'op'. I would suggest making the changes in integrity_audit.c: integrity_audit_msg(). thanks, Mimi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] integrity: get comm using lock to avoid race in string printing 2014-04-02 18:12 ` Mimi Zohar @ 2014-04-02 18:18 ` Eric Paris 2014-04-02 18:27 ` Mimi Zohar 0 siblings, 1 reply; 12+ messages in thread From: Eric Paris @ 2014-04-02 18:18 UTC (permalink / raw) To: Mimi Zohar Cc: Richard Guy Briggs, linux-security-module, linux-kernel, linux-audit On Wed, 2014-04-02 at 14:12 -0400, Mimi Zohar wrote: > On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: > > Hello Mimi, > > > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > > > This change is already being upstreamed as commit 73a6b44 "Integrity: > > > Pass commname via get_task_comm()". > > > > While I was looking at Richard's patch, I noticed a few places where cause and > > op are logged and the string isn't tied together with a _ or -. These are in > > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are > > these fixed upstream? Or should a patch be made? > > Nothing has changed in terms of 'cause' and 'op'. I would suggest > making the changes in integrity_audit.c: integrity_audit_msg(). The question is actually, do you know of anyone who is expecting the space, instead of a more 'audit standard' - or _ ? If not, we'll change it. If so, we'll discuss more :) -Eric ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] integrity: get comm using lock to avoid race in string printing 2014-04-02 18:18 ` Eric Paris @ 2014-04-02 18:27 ` Mimi Zohar 2014-04-02 18:52 ` oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing Richard Guy Briggs 0 siblings, 1 reply; 12+ messages in thread From: Mimi Zohar @ 2014-04-02 18:27 UTC (permalink / raw) To: Eric Paris Cc: Richard Guy Briggs, linux-kernel, linux-ima-user, linux-security-module, linux-audit On Wed, 2014-04-02 at 14:18 -0400, Eric Paris wrote: > On Wed, 2014-04-02 at 14:12 -0400, Mimi Zohar wrote: > > On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: > > > Hello Mimi, > > > > > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > > > > This change is already being upstreamed as commit 73a6b44 "Integrity: > > > > Pass commname via get_task_comm()". > > > > > > While I was looking at Richard's patch, I noticed a few places where cause and > > > op are logged and the string isn't tied together with a _ or -. These are in > > > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are > > > these fixed upstream? Or should a patch be made? > > > > Nothing has changed in terms of 'cause' and 'op'. I would suggest > > making the changes in integrity_audit.c: integrity_audit_msg(). > > The question is actually, do you know of anyone who is expecting the > space, instead of a more 'audit standard' - or _ ? If not, we'll change > it. If so, we'll discuss more :) CC'ing linux-ima-user as well. Mimi ^ permalink raw reply [flat|nested] 12+ messages in thread
* oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing 2014-04-02 18:27 ` Mimi Zohar @ 2014-04-02 18:52 ` Richard Guy Briggs 2014-06-14 0:02 ` Richard Guy Briggs 0 siblings, 1 reply; 12+ messages in thread From: Richard Guy Briggs @ 2014-04-02 18:52 UTC (permalink / raw) To: Mimi Zohar Cc: Eric Paris, Steve Grubb, linux-audit, linux-kernel, linux-security-module, linux-ima-user On 14/04/02, Mimi Zohar wrote: > On Wed, 2014-04-02 at 14:18 -0400, Eric Paris wrote: > > On Wed, 2014-04-02 at 14:12 -0400, Mimi Zohar wrote: > > > On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: > > > > Hello Mimi, > > > > > > > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > > > > > This change is already being upstreamed as commit 73a6b44 "Integrity: > > > > > Pass commname via get_task_comm()". > > > > > > > > While I was looking at Richard's patch, I noticed a few places where cause and > > > > op are logged and the string isn't tied together with a _ or -. These are in > > > > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are > > > > these fixed upstream? Or should a patch be made? > > > > > > Nothing has changed in terms of 'cause' and 'op'. I would suggest > > > making the changes in integrity_audit.c: integrity_audit_msg(). That function could massage incoming text fields and convert spaces to hyphens or underscores, but I'd assume the right place to do it would be in the original text. If you suggest the former, it could just be done in audit_log_string(), but then grepping the source for error messages would not be nearly as useful. Is this what you were suggesting? > > The question is actually, do you know of anyone who is expecting the > > space, instead of a more 'audit standard' - or _ ? If not, we'll change > > it. If so, we'll discuss more :) > > CC'ing linux-ima-user as well. Thanks. > Mimi - 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] 12+ messages in thread
* Re: oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing 2014-04-02 18:52 ` oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing Richard Guy Briggs @ 2014-06-14 0:02 ` Richard Guy Briggs 2014-06-14 9:43 ` [Linux-ima-user] " Dmitry Kasatkin 0 siblings, 1 reply; 12+ messages in thread From: Richard Guy Briggs @ 2014-06-14 0:02 UTC (permalink / raw) To: Mimi Zohar Cc: linux-ima-user, linux-kernel, linux-security-module, linux-audit On 14/04/02, Richard Guy Briggs wrote: > On 14/04/02, Mimi Zohar wrote: > > On Wed, 2014-04-02 at 14:18 -0400, Eric Paris wrote: > > > On Wed, 2014-04-02 at 14:12 -0400, Mimi Zohar wrote: > > > > On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: > > > > > Hello Mimi, > > > > > > > > > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > > > > > > This change is already being upstreamed as commit 73a6b44 "Integrity: > > > > > > Pass commname via get_task_comm()". > > > > > > > > > > While I was looking at Richard's patch, I noticed a few places where cause and > > > > > op are logged and the string isn't tied together with a _ or -. These are in > > > > > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are > > > > > these fixed upstream? Or should a patch be made? > > > > > > > > Nothing has changed in terms of 'cause' and 'op'. I would suggest > > > > making the changes in integrity_audit.c: integrity_audit_msg(). > > That function could massage incoming text fields and convert spaces to > hyphens or underscores, but I'd assume the right place to do it would be > in the original text. If you suggest the former, it could just be done > in audit_log_string(), but then grepping the source for error messages > would not be nearly as useful. Is this what you were suggesting? > > > > The question is actually, do you know of anyone who is expecting the > > > space, instead of a more 'audit standard' - or _ ? If not, we'll change > > > it. If so, we'll discuss more :) > > > > CC'ing linux-ima-user as well. > > Thanks. Was there any response from linux-ima-user? > > Mimi > > - RGB - 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] 12+ messages in thread
* Re: [Linux-ima-user] oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing 2014-06-14 0:02 ` Richard Guy Briggs @ 2014-06-14 9:43 ` Dmitry Kasatkin 2014-06-15 1:46 ` Mimi Zohar 0 siblings, 1 reply; 12+ messages in thread From: Dmitry Kasatkin @ 2014-06-14 9:43 UTC (permalink / raw) To: Richard Guy Briggs Cc: Mimi Zohar, linux-ima-user, linux-security-module, linux-audit, linux-kernel@vger.kernel.org On 14 June 2014 03:02, Richard Guy Briggs <rgb@redhat.com> wrote: > On 14/04/02, Richard Guy Briggs wrote: >> On 14/04/02, Mimi Zohar wrote: >> > On Wed, 2014-04-02 at 14:18 -0400, Eric Paris wrote: >> > > On Wed, 2014-04-02 at 14:12 -0400, Mimi Zohar wrote: >> > > > On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: >> > > > > Hello Mimi, >> > > > > >> > > > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: >> > > > > > This change is already being upstreamed as commit 73a6b44 "Integrity: >> > > > > > Pass commname via get_task_comm()". >> > > > > >> > > > > While I was looking at Richard's patch, I noticed a few places where cause and >> > > > > op are logged and the string isn't tied together with a _ or -. These are in >> > > > > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are >> > > > > these fixed upstream? Or should a patch be made? >> > > > >> > > > Nothing has changed in terms of 'cause' and 'op'. I would suggest >> > > > making the changes in integrity_audit.c: integrity_audit_msg(). >> >> That function could massage incoming text fields and convert spaces to >> hyphens or underscores, but I'd assume the right place to do it would be >> in the original text. If you suggest the former, it could just be done >> in audit_log_string(), but then grepping the source for error messages >> would not be nearly as useful. Is this what you were suggesting? >> >> > > The question is actually, do you know of anyone who is expecting the >> > > space, instead of a more 'audit standard' - or _ ? If not, we'll change >> > > it. If so, we'll discuss more :) >> > >> > CC'ing linux-ima-user as well. >> >> Thanks. > > Was there any response from linux-ima-user? > >> > Mimi >> >> - RGB > Hi, I will find this patch and have a look. Thanks, Dmitry > - 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 > > ------------------------------------------------------------------------------ > HPCC Systems Open Source Big Data Platform from LexisNexis Risk Solutions > Find What Matters Most in Your Big Data with HPCC Systems > Open Source. Fast. Scalable. Simple. Ideal for Dirty Data. > Leverages Graph Analysis for Fast Processing & Easy Data Exploration > http://p.sf.net/sfu/hpccsystems > _______________________________________________ > Linux-ima-user mailing list > Linux-ima-user@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/linux-ima-user -- Thanks, Dmitry ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-ima-user] oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing 2014-06-14 9:43 ` [Linux-ima-user] " Dmitry Kasatkin @ 2014-06-15 1:46 ` Mimi Zohar 2014-06-15 2:45 ` Richard Guy Briggs 0 siblings, 1 reply; 12+ messages in thread From: Mimi Zohar @ 2014-06-15 1:46 UTC (permalink / raw) To: Dmitry Kasatkin Cc: Richard Guy Briggs, linux-ima-user, linux-security-module, linux-audit, linux-kernel@vger.kernel.org On Sat, 2014-06-14 at 12:43 +0300, Dmitry Kasatkin wrote: > On 14 June 2014 03:02, Richard Guy Briggs <rgb@redhat.com> wrote: > > On 14/04/02, Richard Guy Briggs wrote: > >> On 14/04/02, Mimi Zohar wrote: > >> > On Wed, 2014-04-02 at 14:18 -0400, Eric Paris wrote: > >> > > On Wed, 2014-04-02 at 14:12 -0400, Mimi Zohar wrote: > >> > > > On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: > >> > > > > Hello Mimi, > >> > > > > > >> > > > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > >> > > > > > This change is already being upstreamed as commit 73a6b44 "Integrity: > >> > > > > > Pass commname via get_task_comm()". > >> > > > > > >> > > > > While I was looking at Richard's patch, I noticed a few places where cause and > >> > > > > op are logged and the string isn't tied together with a _ or -. These are in > >> > > > > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are > >> > > > > these fixed upstream? Or should a patch be made? > >> > > > > >> > > > Nothing has changed in terms of 'cause' and 'op'. I would suggest > >> > > > making the changes in integrity_audit.c: integrity_audit_msg(). > >> > >> That function could massage incoming text fields and convert spaces to > >> hyphens or underscores, but I'd assume the right place to do it would be > >> in the original text. If you suggest the former, it could just be done > >> in audit_log_string(), but then grepping the source for error messages > >> would not be nearly as useful. Is this what you were suggesting? > >> > >> > > The question is actually, do you know of anyone who is expecting the > >> > > space, instead of a more 'audit standard' - or _ ? If not, we'll change > >> > > it. If so, we'll discuss more :) > >> > > >> > CC'ing linux-ima-user as well. > >> > >> Thanks. > > > > Was there any response from linux-ima-user? > > > >> > Mimi > >> > >> - RGB > > > > Hi, > > I will find this patch and have a look. As nobody has responded, I would assume it is safe to change. Mimi ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Linux-ima-user] oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing 2014-06-15 1:46 ` Mimi Zohar @ 2014-06-15 2:45 ` Richard Guy Briggs 0 siblings, 0 replies; 12+ messages in thread From: Richard Guy Briggs @ 2014-06-15 2:45 UTC (permalink / raw) To: Mimi Zohar Cc: Dmitry Kasatkin, linux-ima-user, linux-security-module, linux-audit, linux-kernel@vger.kernel.org On 14/06/14, Mimi Zohar wrote: > On Sat, 2014-06-14 at 12:43 +0300, Dmitry Kasatkin wrote: > > On 14 June 2014 03:02, Richard Guy Briggs <rgb@redhat.com> wrote: > > > On 14/04/02, Richard Guy Briggs wrote: > > >> On 14/04/02, Mimi Zohar wrote: > > >> > On Wed, 2014-04-02 at 14:18 -0400, Eric Paris wrote: > > >> > > On Wed, 2014-04-02 at 14:12 -0400, Mimi Zohar wrote: > > >> > > > On Wed, 2014-04-02 at 14:00 -0400, Steve Grubb wrote: > > >> > > > > Hello Mimi, > > >> > > > > > > >> > > > > On Wednesday, April 02, 2014 01:39:47 PM Mimi Zohar wrote: > > >> > > > > > This change is already being upstreamed as commit 73a6b44 "Integrity: > > >> > > > > > Pass commname via get_task_comm()". > > >> > > > > > > >> > > > > While I was looking at Richard's patch, I noticed a few places where cause and > > >> > > > > op are logged and the string isn't tied together with a _ or -. These are in > > >> > > > > ima/ima_appraise.c line 383, and ima/ima_policy.c lines 333, 657, and 683. Are > > >> > > > > these fixed upstream? Or should a patch be made? > > >> > > > > > >> > > > Nothing has changed in terms of 'cause' and 'op'. I would suggest > > >> > > > making the changes in integrity_audit.c: integrity_audit_msg(). > > >> > > >> That function could massage incoming text fields and convert spaces to > > >> hyphens or underscores, but I'd assume the right place to do it would be > > >> in the original text. If you suggest the former, it could just be done > > >> in audit_log_string(), but then grepping the source for error messages > > >> would not be nearly as useful. Is this what you were suggesting? > > >> > > >> > > The question is actually, do you know of anyone who is expecting the > > >> > > space, instead of a more 'audit standard' - or _ ? If not, we'll change > > >> > > it. If so, we'll discuss more :) > > >> > > > >> > CC'ing linux-ima-user as well. > > >> > > >> Thanks. > > > > > > Was there any response from linux-ima-user? > > > > > >> > Mimi > > >> > > >> - RGB > > > > > > > Hi, > > > > I will find this patch and have a look. > > As nobody has responded, I would assume it is safe to change. Thanks! :) > Mimi - 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] 12+ messages in thread
* Re: [PATCH] integrity: get comm using lock to avoid race in string printing 2014-04-02 17:39 ` Mimi Zohar 2014-04-02 18:00 ` Steve Grubb @ 2014-04-02 18:45 ` Richard Guy Briggs 1 sibling, 0 replies; 12+ messages in thread From: Richard Guy Briggs @ 2014-04-02 18:45 UTC (permalink / raw) To: Mimi Zohar Cc: linux-audit, linux-kernel, linux-security-module, eparis, sgrubb On 14/04/02, Mimi Zohar wrote: > On Wed, 2014-04-02 at 12:19 -0400, Richard Guy Briggs wrote: > > When task->comm is passed directly to audit_log_untrustedstring() without > > getting a copy or using the task_lock, there is a race that could happen that > > would output a NULL (\0) in the output string that would effectively truncate > > the rest of the report text after the comm= field in the audit, losing fields. > > > > Use get_task_comm() to get a copy while acquiring the task_lock to prevent > > this and to prevent the result from being a mixture of old and new values of > > comm. > > > > Signed-off-by: Richard Guy Briggs <rgb@redhat.com> > > --- > > security/integrity/integrity_audit.c | 3 ++- > > 1 files changed, 2 insertions(+), 1 deletions(-) > > > > diff --git a/security/integrity/integrity_audit.c b/security/integrity/integrity_audit.c > > index 85253b5..11706a2 100644 > > --- a/security/integrity/integrity_audit.c > > +++ b/security/integrity/integrity_audit.c > > @@ -33,6 +33,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, > > const char *cause, int result, int audit_info) > > { > > struct audit_buffer *ab; > > + char comm[sizeof(current->comm)]; > > > > if (!integrity_audit_info && audit_info == 1) /* Skip info messages */ > > return; > > @@ -49,7 +50,7 @@ void integrity_audit_msg(int audit_msgno, struct inode *inode, > > audit_log_format(ab, " cause="); > > audit_log_string(ab, cause); > > audit_log_format(ab, " comm="); > > - audit_log_untrustedstring(ab, current->comm); > > + audit_log_untrustedstring(ab, get_task_comm(comm, current)); > > if (fname) { > > audit_log_format(ab, " name="); > > audit_log_untrustedstring(ab, fname); > > This change is already being upstreamed as commit 73a6b44 "Integrity: > Pass commname via get_task_comm()". Excellent. Missed that. Thanks. > thanks, > > Mimi - 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] 12+ messages in thread
end of thread, other threads:[~2014-06-15 2:45 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-04-02 16:19 [PATCH] integrity: get comm using lock to avoid race in string printing Richard Guy Briggs 2014-04-02 17:39 ` Mimi Zohar 2014-04-02 18:00 ` Steve Grubb 2014-04-02 18:12 ` Mimi Zohar 2014-04-02 18:18 ` Eric Paris 2014-04-02 18:27 ` Mimi Zohar 2014-04-02 18:52 ` oraphaned keywords in audit log text [was: Re: [PATCH] integrity: get comm using lock to avoid race in string] printing Richard Guy Briggs 2014-06-14 0:02 ` Richard Guy Briggs 2014-06-14 9:43 ` [Linux-ima-user] " Dmitry Kasatkin 2014-06-15 1:46 ` Mimi Zohar 2014-06-15 2:45 ` Richard Guy Briggs 2014-04-02 18:45 ` [PATCH] integrity: get comm using lock to avoid race in string printing Richard Guy Briggs
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox