* race in audit_log_untrusted_string for task_struct::comm
@ 2014-03-15 23:28 Richard Guy Briggs
2014-03-15 23:29 ` [PATCH] audit: get comm using lock to avoid race in string printing Richard Guy Briggs
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Richard Guy Briggs @ 2014-03-15 23:28 UTC (permalink / raw)
To: linux-audit, LKML; +Cc: Steve Grubb, Eric Paris
Hi,
I'm investigating a race in audit_log_untrusted_string() in the case of
task_struct::comm.
Originally from commit 0a4ff8c2 audit_log_task() currently hands
task_struct::comm directly to audit_log_untrusted_string() which can
race if another task/thread on another CPU modifies it between the time
strlen() is called on it and the time it is processed in
audit_log_n_untrustedstring(). I originally thought it was just a
matter of the value being truncated or mixed between old and new values,
but it appears it is worse than that in that it causes following labels
to be dropped. If it just affected the value of comm that was printed,
I am guessing this wouldn't be a big deal since the user can modify this
value anyway and we never did trust it. however, since it affects the
rest of the line, it has to be addressed.
In commit 219f0817 from 2005, Stephen Smalley used get_task_comm() to
get the value of comm to log to audit_log_untrusted_string() which calls
the task_lock. This is quite safe, but incurs overhead that may not be
found acceptable by some.
Eric subsequently used memcpy() in c2a7780e in 2008 in another area of the
code that stores the value for later rather than use it immediately, so
the race issue was not present there, but there is still the danger of
incomplete or mixed text in that field.
Both are safe in terms of avoiding losing other fields. One might have
half-inconsistent information in it, the other incurs locking costs.
I'm inclined to go get_task_comm() in all 5 locations, but if we care
more about locking overhead, I'll switch to memcpy().
Steve, do we care about the integrity of the comm field?
Eric, is the overhead of task_lock unacceptable?
Linked in this thread (should I be able to convince git send-email to
work with me) please find the get_task_comm() patch and the alternate
memcpy() patch.
- 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] 6+ messages in thread
* [PATCH] audit: get comm using lock to avoid race in string printing
2014-03-15 23:28 race in audit_log_untrusted_string for task_struct::comm Richard Guy Briggs
@ 2014-03-15 23:29 ` Richard Guy Briggs
2014-03-18 17:37 ` Stephen Smalley
2014-03-15 23:29 ` [PATCH] audit: copy comm " Richard Guy Briggs
2014-03-17 13:01 ` race in audit_log_untrusted_string for task_struct::comm Steve Grubb
2 siblings, 1 reply; 6+ messages in thread
From: Richard Guy Briggs @ 2014-03-15 23:29 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs, eparis, sgrubb
---
kernel/audit.c | 5 ++---
kernel/auditsc.c | 9 +++++----
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 9239e5e..5b600c8 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1883,7 +1883,7 @@ EXPORT_SYMBOL(audit_log_task_context);
void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
{
const struct cred *cred;
- char name[sizeof(tsk->comm)];
+ char comm[sizeof(tsk->comm)];
struct mm_struct *mm = tsk->mm;
char *tty;
@@ -1917,9 +1917,8 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
from_kgid(&init_user_ns, cred->fsgid),
tty, audit_get_sessionid(tsk));
- get_task_comm(name, tsk);
audit_log_format(ab, " comm=");
- audit_log_untrustedstring(ab, name);
+ audit_log_untrustedstring(ab, get_task_comm(name, tsk));
if (mm) {
down_read(&mm->mmap_sem);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8fffca8..b789e0c 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2289,7 +2289,7 @@ void __audit_ptrace(struct task_struct *t)
context->target_uid = task_uid(t);
context->target_sessionid = audit_get_sessionid(t);
security_task_getsecid(t, &context->target_sid);
- memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
+ get_task_comm(context->target_comm, t);
}
/**
@@ -2328,7 +2328,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
ctx->target_uid = t_uid;
ctx->target_sessionid = audit_get_sessionid(t);
security_task_getsecid(t, &ctx->target_sid);
- memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
+ get_task_comm(ctx->target_comm, t);
return 0;
}
@@ -2349,7 +2349,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
axp->target_uid[axp->pid_count] = t_uid;
axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
- memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
+ get_task_comm(axp->target_comm[axp->pid_count], t);
axp->pid_count++;
return 0;
@@ -2435,6 +2435,7 @@ static void audit_log_task(struct audit_buffer *ab)
kgid_t gid;
unsigned int sessionid;
struct mm_struct *mm = current->mm;
+ char comm[sizeof(current->comm)];
auid = audit_get_loginuid(current);
sessionid = audit_get_sessionid(current);
@@ -2447,7 +2448,7 @@ static void audit_log_task(struct audit_buffer *ab)
sessionid);
audit_log_task_context(ab);
audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
- audit_log_untrustedstring(ab, current->comm);
+ audit_log_untrustedstring(ab, get_task_comm(comm, current);
if (mm) {
down_read(&mm->mmap_sem);
if (mm->exe_file)
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] audit: copy comm to avoid race in string printing
2014-03-15 23:28 race in audit_log_untrusted_string for task_struct::comm Richard Guy Briggs
2014-03-15 23:29 ` [PATCH] audit: get comm using lock to avoid race in string printing Richard Guy Briggs
@ 2014-03-15 23:29 ` Richard Guy Briggs
2014-03-17 13:01 ` race in audit_log_untrusted_string for task_struct::comm Steve Grubb
2 siblings, 0 replies; 6+ messages in thread
From: Richard Guy Briggs @ 2014-03-15 23:29 UTC (permalink / raw)
To: linux-audit, linux-kernel; +Cc: Richard Guy Briggs, eparis, sgrubb
---
kernel/audit.c | 5 ++---
kernel/auditsc.c | 3 ++-
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/kernel/audit.c b/kernel/audit.c
index 9239e5e..ecb08a5 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1883,7 +1883,7 @@ EXPORT_SYMBOL(audit_log_task_context);
void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
{
const struct cred *cred;
- char name[sizeof(tsk->comm)];
+ char comm[sizeof(tsk->comm)];
struct mm_struct *mm = tsk->mm;
char *tty;
@@ -1917,9 +1917,8 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
from_kgid(&init_user_ns, cred->fsgid),
tty, audit_get_sessionid(tsk));
- get_task_comm(name, tsk);
audit_log_format(ab, " comm=");
- audit_log_untrustedstring(ab, name);
+ audit_log_untrustedstring(ab, memcpy(comm, tsk->comm, sizeof(comm));
if (mm) {
down_read(&mm->mmap_sem);
diff --git a/kernel/auditsc.c b/kernel/auditsc.c
index 8fffca8..dcddadd 100644
--- a/kernel/auditsc.c
+++ b/kernel/auditsc.c
@@ -2434,6 +2434,7 @@ static void audit_log_task(struct audit_buffer *ab)
kuid_t auid, uid;
kgid_t gid;
unsigned int sessionid;
+ char comm[sizeof(current->comm)];
struct mm_struct *mm = current->mm;
auid = audit_get_loginuid(current);
@@ -2447,7 +2448,7 @@ static void audit_log_task(struct audit_buffer *ab)
sessionid);
audit_log_task_context(ab);
audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
- audit_log_untrustedstring(ab, current->comm);
+ audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm));
if (mm) {
down_read(&mm->mmap_sem);
if (mm->exe_file)
--
1.7.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: race in audit_log_untrusted_string for task_struct::comm
2014-03-15 23:28 race in audit_log_untrusted_string for task_struct::comm Richard Guy Briggs
2014-03-15 23:29 ` [PATCH] audit: get comm using lock to avoid race in string printing Richard Guy Briggs
2014-03-15 23:29 ` [PATCH] audit: copy comm " Richard Guy Briggs
@ 2014-03-17 13:01 ` Steve Grubb
2 siblings, 0 replies; 6+ messages in thread
From: Steve Grubb @ 2014-03-17 13:01 UTC (permalink / raw)
To: Richard Guy Briggs; +Cc: linux-audit, LKML, Eric Paris
On Saturday, March 15, 2014 07:28:46 PM Richard Guy Briggs wrote:
> I'm inclined to go get_task_comm() in all 5 locations, but if we care
> more about locking overhead, I'll switch to memcpy().
>
> Steve, do we care about the integrity of the comm field?
In the case of interpreters, its about the only thing we know about the
application being executed. For example, a shell script will have exe=/bin/sh,
so comm= is our only clue.
-Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] audit: get comm using lock to avoid race in string printing
2014-03-15 23:29 ` [PATCH] audit: get comm using lock to avoid race in string printing Richard Guy Briggs
@ 2014-03-18 17:37 ` Stephen Smalley
2014-03-18 17:54 ` Richard Guy Briggs
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Smalley @ 2014-03-18 17:37 UTC (permalink / raw)
To: Richard Guy Briggs, linux-audit, linux-kernel
On 03/15/2014 07:29 PM, Richard Guy Briggs wrote:
> ---
> kernel/audit.c | 5 ++---
> kernel/auditsc.c | 9 +++++----
> 2 files changed, 7 insertions(+), 7 deletions(-)
Doesn't this also need to be fixed (twice) in security/lsm_audit.c?
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 9239e5e..5b600c8 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1883,7 +1883,7 @@ EXPORT_SYMBOL(audit_log_task_context);
> void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> {
> const struct cred *cred;
> - char name[sizeof(tsk->comm)];
> + char comm[sizeof(tsk->comm)];
> struct mm_struct *mm = tsk->mm;
> char *tty;
>
> @@ -1917,9 +1917,8 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> from_kgid(&init_user_ns, cred->fsgid),
> tty, audit_get_sessionid(tsk));
>
> - get_task_comm(name, tsk);
> audit_log_format(ab, " comm=");
> - audit_log_untrustedstring(ab, name);
> + audit_log_untrustedstring(ab, get_task_comm(name, tsk));
>
> if (mm) {
> down_read(&mm->mmap_sem);
> diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> index 8fffca8..b789e0c 100644
> --- a/kernel/auditsc.c
> +++ b/kernel/auditsc.c
> @@ -2289,7 +2289,7 @@ void __audit_ptrace(struct task_struct *t)
> context->target_uid = task_uid(t);
> context->target_sessionid = audit_get_sessionid(t);
> security_task_getsecid(t, &context->target_sid);
> - memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> + get_task_comm(context->target_comm, t);
> }
>
> /**
> @@ -2328,7 +2328,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
> ctx->target_uid = t_uid;
> ctx->target_sessionid = audit_get_sessionid(t);
> security_task_getsecid(t, &ctx->target_sid);
> - memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> + get_task_comm(ctx->target_comm, t);
> return 0;
> }
>
> @@ -2349,7 +2349,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
> axp->target_uid[axp->pid_count] = t_uid;
> axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
> security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
> - memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> + get_task_comm(axp->target_comm[axp->pid_count], t);
> axp->pid_count++;
>
> return 0;
> @@ -2435,6 +2435,7 @@ static void audit_log_task(struct audit_buffer *ab)
> kgid_t gid;
> unsigned int sessionid;
> struct mm_struct *mm = current->mm;
> + char comm[sizeof(current->comm)];
>
> auid = audit_get_loginuid(current);
> sessionid = audit_get_sessionid(current);
> @@ -2447,7 +2448,7 @@ static void audit_log_task(struct audit_buffer *ab)
> sessionid);
> audit_log_task_context(ab);
> audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
> - audit_log_untrustedstring(ab, current->comm);
> + audit_log_untrustedstring(ab, get_task_comm(comm, current);
> if (mm) {
> down_read(&mm->mmap_sem);
> if (mm->exe_file)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] audit: get comm using lock to avoid race in string printing
2014-03-18 17:37 ` Stephen Smalley
@ 2014-03-18 17:54 ` Richard Guy Briggs
0 siblings, 0 replies; 6+ messages in thread
From: Richard Guy Briggs @ 2014-03-18 17:54 UTC (permalink / raw)
To: Stephen Smalley; +Cc: linux-audit, linux-kernel
On 14/03/18, Stephen Smalley wrote:
> On 03/15/2014 07:29 PM, Richard Guy Briggs wrote:
> > ---
> > kernel/audit.c | 5 ++---
> > kernel/auditsc.c | 9 +++++----
> > 2 files changed, 7 insertions(+), 7 deletions(-)
>
> Doesn't this also need to be fixed (twice) in security/lsm_audit.c?
Yes. Tetsuo had sent a tentative patch to James Morris who pushed back
pending review.
I also just found one in security/integrity/integrity_audit.c
> > diff --git a/kernel/audit.c b/kernel/audit.c
> > index 9239e5e..5b600c8 100644
> > --- a/kernel/audit.c
> > +++ b/kernel/audit.c
> > @@ -1883,7 +1883,7 @@ EXPORT_SYMBOL(audit_log_task_context);
> > void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> > {
> > const struct cred *cred;
> > - char name[sizeof(tsk->comm)];
> > + char comm[sizeof(tsk->comm)];
> > struct mm_struct *mm = tsk->mm;
> > char *tty;
> >
> > @@ -1917,9 +1917,8 @@ void audit_log_task_info(struct audit_buffer *ab, struct task_struct *tsk)
> > from_kgid(&init_user_ns, cred->fsgid),
> > tty, audit_get_sessionid(tsk));
> >
> > - get_task_comm(name, tsk);
> > audit_log_format(ab, " comm=");
> > - audit_log_untrustedstring(ab, name);
> > + audit_log_untrustedstring(ab, get_task_comm(name, tsk));
> >
> > if (mm) {
> > down_read(&mm->mmap_sem);
> > diff --git a/kernel/auditsc.c b/kernel/auditsc.c
> > index 8fffca8..b789e0c 100644
> > --- a/kernel/auditsc.c
> > +++ b/kernel/auditsc.c
> > @@ -2289,7 +2289,7 @@ void __audit_ptrace(struct task_struct *t)
> > context->target_uid = task_uid(t);
> > context->target_sessionid = audit_get_sessionid(t);
> > security_task_getsecid(t, &context->target_sid);
> > - memcpy(context->target_comm, t->comm, TASK_COMM_LEN);
> > + get_task_comm(context->target_comm, t);
> > }
> >
> > /**
> > @@ -2328,7 +2328,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
> > ctx->target_uid = t_uid;
> > ctx->target_sessionid = audit_get_sessionid(t);
> > security_task_getsecid(t, &ctx->target_sid);
> > - memcpy(ctx->target_comm, t->comm, TASK_COMM_LEN);
> > + get_task_comm(ctx->target_comm, t);
> > return 0;
> > }
> >
> > @@ -2349,7 +2349,7 @@ int __audit_signal_info(int sig, struct task_struct *t)
> > axp->target_uid[axp->pid_count] = t_uid;
> > axp->target_sessionid[axp->pid_count] = audit_get_sessionid(t);
> > security_task_getsecid(t, &axp->target_sid[axp->pid_count]);
> > - memcpy(axp->target_comm[axp->pid_count], t->comm, TASK_COMM_LEN);
> > + get_task_comm(axp->target_comm[axp->pid_count], t);
> > axp->pid_count++;
> >
> > return 0;
> > @@ -2435,6 +2435,7 @@ static void audit_log_task(struct audit_buffer *ab)
> > kgid_t gid;
> > unsigned int sessionid;
> > struct mm_struct *mm = current->mm;
> > + char comm[sizeof(current->comm)];
> >
> > auid = audit_get_loginuid(current);
> > sessionid = audit_get_sessionid(current);
> > @@ -2447,7 +2448,7 @@ static void audit_log_task(struct audit_buffer *ab)
> > sessionid);
> > audit_log_task_context(ab);
> > audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
> > - audit_log_untrustedstring(ab, current->comm);
> > + audit_log_untrustedstring(ab, get_task_comm(comm, current);
> > if (mm) {
> > down_read(&mm->mmap_sem);
> > if (mm->exe_file)
> >
>
- 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] 6+ messages in thread
end of thread, other threads:[~2014-03-18 17:56 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-15 23:28 race in audit_log_untrusted_string for task_struct::comm Richard Guy Briggs
2014-03-15 23:29 ` [PATCH] audit: get comm using lock to avoid race in string printing Richard Guy Briggs
2014-03-18 17:37 ` Stephen Smalley
2014-03-18 17:54 ` Richard Guy Briggs
2014-03-15 23:29 ` [PATCH] audit: copy comm " Richard Guy Briggs
2014-03-17 13:01 ` race in audit_log_untrusted_string for task_struct::comm Steve Grubb
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).