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-ker...@i-love.sakura.ne.jp>
> Signed-off-by: Richard Guy Briggs <r...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
