6:28 p.m.
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(a)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
6:29 p.m.
New subject: [PATCH] audit: get comm using lock to avoid race in string printing
---
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
12:37 p.m.
New subject: [PATCH] audit: get comm using lock to avoid race in string printing
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)
12:54 p.m.
New subject: [PATCH] audit: get comm using lock to avoid race in string printing
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(a)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
6:29 p.m.
New subject: [PATCH] audit: copy comm to avoid race in string printing
---
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
8:01 a.m.
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
3932
days inactive
3935
days old
linux-audit@lists.linux-audit.osci.io
5 comments
3 participants
participants (3)
-
Richard Guy Briggs -
Stephen Smalley -
Steve Grubb