[PATCH] lsm: copy comm before calling audit_log to avoid race in string printing

[PATCH] lsm: copy comm before calling audit_log to avoid race in string printing

[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 middle of the output string that would
effectively truncate the rest of the report text after the comm= field in the
audit log message, losing fields.

Using 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 would incur potentially unacceptable overhead, considering that the value
can be influenced by userspace and therefore untrusted anyways.

Copy the value before passing it to audit_log_untrustedstring() ensures that a
local copy is used to calculate the length *and* subsequently printed.  Even if
this value contains a mix of old and new values, it will only calculate and
copy up to the first NULL, preventing the rest of the audit log message being
truncated.

The LSM_AUDIT_DATA_TASK pid= and comm= labels are duplicates of those at the
start of this function with different values.  Rename them to their object
counterparts opid= and ocomm= to disambiguate.  Use a second local copy of comm
to avoid a race between the first and second calls to
audit_log_untrustedstring() with comm.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 security/lsm_audit.c |   17 ++++++++++-------
 1 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 69fdf3b..3323144 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -211,7 +211,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
 static void dump_common_audit_data(struct audit_buffer *ab,
 				   struct common_audit_data *a)
 {
-	struct task_struct *tsk = current;
+	char comm[sizeof(current->comm)];
 
 	/*
 	 * To keep stack sizes in check force programers to notice if they
@@ -220,8 +220,8 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 	 */
 	BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
 
-	audit_log_format(ab, " pid=%d comm=", task_pid_nr(tsk));
-	audit_log_untrustedstring(ab, tsk->comm);
+	audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
+	audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
 
 	switch (a->type) {
 	case LSM_AUDIT_DATA_NONE:
@@ -276,16 +276,19 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		audit_log_format(ab, " ino=%lu", inode->i_ino);
 		break;
 	}
-	case LSM_AUDIT_DATA_TASK:
-		tsk = a->u.tsk;
+	case LSM_AUDIT_DATA_TASK: {
+		struct task_struct *tsk = a->u.tsk;
 		if (tsk) {
 			pid_t pid = task_pid_nr(tsk);
 			if (pid) {
-				audit_log_format(ab, " pid=%d comm=", pid);
-				audit_log_untrustedstring(ab, tsk->comm);
+				char comm[sizeof(tsk->comm)];
+				audit_log_format(ab, " opid=%d ocomm=", pid);
+				audit_log_untrustedstring(ab,
+				    memcpy(comm, tsk->comm, sizeof(comm)));
 			}
 		}
 		break;
+	}
 	case LSM_AUDIT_DATA_NET:
 		if (a->u.net->sk) {
 			struct sock *sk = a->u.net->sk;
-- 
1.7.1

Re: [PATCH] lsm: copy comm before calling audit_log to avoid race in string printing

[Paul Moore]

On Sunday, November 16, 2014 04:44:10 PM 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 middle of the output string that would
> effectively truncate the rest of the report text after the comm= field in
> the audit log message, losing fields.
> 
> Using 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 would incur potentially unacceptable overhead, considering that the
> value can be influenced by userspace and therefore untrusted anyways.
> 
> Copy the value before passing it to audit_log_untrustedstring() ensures that
> a local copy is used to calculate the length *and* subsequently printed. 
> Even if this value contains a mix of old and new values, it will only
> calculate and copy up to the first NULL, preventing the rest of the audit
> log message being truncated.

In general I think this looks good, some minor nits below.  We should get this 
into linux-next soonish.

> The LSM_AUDIT_DATA_TASK pid= and comm= labels are duplicates of those at the
> start of this function with different values.  Rename them to their object
> counterparts opid= and ocomm= to disambiguate.  Use a second local copy of
> comm to avoid a race between the first and second calls to
> audit_log_untrustedstring() with comm.

This probably should have been split into a separate patch, but not a deal 
breaker I suppose.  For the record, is Steve okay with these field names?

> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
> ---
>  security/lsm_audit.c |   17 ++++++++++-------
>  1 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/security/lsm_audit.c b/security/lsm_audit.c
> index 69fdf3b..3323144 100644
> --- a/security/lsm_audit.c
> +++ b/security/lsm_audit.c
> @@ -211,7 +211,7 @@ static inline void print_ipv4_addr(struct audit_buffer
> *ab, __be32 addr, static void dump_common_audit_data(struct audit_buffer
> *ab,
>  				   struct common_audit_data *a)
>  {
> -	struct task_struct *tsk = current;
> +	char comm[sizeof(current->comm)];

As mentioned previously, I think I prefer TASK_COMM_LEN here, but ultimately 
it isn't too important.

>  	/*
>  	 * To keep stack sizes in check force programers to notice if they
> @@ -220,8 +220,8 @@ static void dump_common_audit_data(struct audit_buffer
> *ab, */
>  	BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
> 
> -	audit_log_format(ab, " pid=%d comm=", task_pid_nr(tsk));
> -	audit_log_untrustedstring(ab, tsk->comm);
> +	audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
> +	audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));

Again.

>  	switch (a->type) {
>  	case LSM_AUDIT_DATA_NONE:
> @@ -276,16 +276,19 @@ static void dump_common_audit_data(struct audit_buffer
> *ab, audit_log_format(ab, " ino=%lu", inode->i_ino);
>  		break;
>  	}
> -	case LSM_AUDIT_DATA_TASK:
> -		tsk = a->u.tsk;
> +	case LSM_AUDIT_DATA_TASK: {
> +		struct task_struct *tsk = a->u.tsk;
>  		if (tsk) {
>  			pid_t pid = task_pid_nr(tsk);
>  			if (pid) {
> -				audit_log_format(ab, " pid=%d comm=", pid);
> -				audit_log_untrustedstring(ab, tsk->comm);
> +				char comm[sizeof(tsk->comm)];
> +				audit_log_format(ab, " opid=%d ocomm=", pid);
> +				audit_log_untrustedstring(ab,
> +				    memcpy(comm, tsk->comm, sizeof(comm)));

... and again.

-- 
paul moore
security and virtualization @ redhat

[PATCH] lsm: copy comm before calling audit_log to avoid race in string printing

[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 middle of the output string that would
effectively truncate the rest of the report text after the comm= field in the
audit log message, losing fields.

Using 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 would incur potentially unacceptable overhead, considering that the value
can be influenced by userspace and therefore untrusted anyways.

Copy the value before passing it to audit_log_untrustedstring() ensures that a
local copy is used to calculate the length *and* subsequently printed.  Even if
this value contains a mix of old and new values, it will only calculate and
copy up to the first NULL, preventing the rest of the audit log message being
truncated.

Use a second local copy of comm to avoid a race between the first and second
calls to audit_log_untrustedstring() with comm.

Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Richard Guy Briggs <rgb@redhat.com>
---
 security/lsm_audit.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/security/lsm_audit.c b/security/lsm_audit.c
index 69fdf3b..b526ddc 100644
--- a/security/lsm_audit.c
+++ b/security/lsm_audit.c
@@ -211,7 +211,7 @@ static inline void print_ipv4_addr(struct audit_buffer *ab, __be32 addr,
 static void dump_common_audit_data(struct audit_buffer *ab,
 				   struct common_audit_data *a)
 {
-	struct task_struct *tsk = current;
+	char comm[sizeof(current->comm)];
 
 	/*
 	 * To keep stack sizes in check force programers to notice if they
@@ -220,8 +220,8 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 	 */
 	BUILD_BUG_ON(sizeof(a->u) > sizeof(void *)*2);
 
-	audit_log_format(ab, " pid=%d comm=", task_pid_nr(tsk));
-	audit_log_untrustedstring(ab, tsk->comm);
+	audit_log_format(ab, " pid=%d comm=", task_pid_nr(current));
+	audit_log_untrustedstring(ab, memcpy(comm, current->comm, sizeof(comm)));
 
 	switch (a->type) {
 	case LSM_AUDIT_DATA_NONE:
@@ -276,16 +276,19 @@ static void dump_common_audit_data(struct audit_buffer *ab,
 		audit_log_format(ab, " ino=%lu", inode->i_ino);
 		break;
 	}
-	case LSM_AUDIT_DATA_TASK:
-		tsk = a->u.tsk;
+	case LSM_AUDIT_DATA_TASK: {
+		struct task_struct *tsk = a->u.tsk;
 		if (tsk) {
 			pid_t pid = task_pid_nr(tsk);
 			if (pid) {
+				char comm[sizeof(tsk->comm)];
 				audit_log_format(ab, " pid=%d comm=", pid);
-				audit_log_untrustedstring(ab, tsk->comm);
+				audit_log_untrustedstring(ab,
+				    memcpy(comm, tsk->comm, sizeof(comm)));
 			}
 		}
 		break;
+	}
 	case LSM_AUDIT_DATA_NET:
 		if (a->u.net->sk) {
 			struct sock *sk = a->u.net->sk;
-- 
1.7.1

Re: [PATCH] lsm: copy comm before calling audit_log to avoid race in string printing

[James Morris]

On Tue, 14 Apr 2015, 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 middle of the output string that would
> effectively truncate the rest of the report text after the comm= field in the
> audit log message, losing fields.
> 
> Using 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 would incur potentially unacceptable overhead, considering that the value
> can be influenced by userspace and therefore untrusted anyways.
> 
> Copy the value before passing it to audit_log_untrustedstring() ensures that a
> local copy is used to calculate the length *and* subsequently printed.  Even if
> this value contains a mix of old and new values, it will only calculate and
> copy up to the first NULL, preventing the rest of the audit log message being
> truncated.
> 
> Use a second local copy of comm to avoid a race between the first and second
> calls to audit_log_untrustedstring() with comm.
> 
> Reported-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Richard Guy Briggs <rgb@redhat.com>

Applied.

-- 
James Morris
<jmorris@namei.org>