mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
From: Richard Guy Briggs <rgb@redhat.com>
To: linux-audit@redhat.com, LKML <linux-kernel@vger.kernel.org>
Cc: Steve Grubb <sgrubb@redhat.com>, Eric Paris <eparis@redhat.com>
Subject: race in audit_log_untrusted_string for task_struct::comm
Date: Sat, 15 Mar 2014 19:28:46 -0400	[thread overview]
Message-ID: <20140315232846.GE27244@madcap2.tricolour.ca> (raw)

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

             reply	other threads:[~2014-03-15 23:28 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-03-15 23:28 Richard Guy Briggs [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140315232846.GE27244@madcap2.tricolour.ca \
    --to=rgb@redhat.com \
    --cc=eparis@redhat.com \
    --cc=linux-audit@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=sgrubb@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®