Re: Report Double Fetch Bug Found in Linux-4.6.1/kernel/auditsc.c

[Richard Guy Briggs <rgb@redhat.com>]

On 2016-06-21 13:31, Andy Lutomirski wrote:
> On Tue, Jun 21, 2016 at 12:59 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> > On Tue, 2016-06-21 at 15:18 -0400, Richard Guy Briggs wrote:
> >> On 2016-06-21 19:20, Ben Hutchings wrote:
> >> > On Tue, 2016-06-21 at 14:14 -0400, Richard Guy Briggs wrote:
> >> > > On 2016-06-21 10:51, Ben Hutchings wrote:
> >> > > > On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote:
> >> > > > > >
> >> > > > > > 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg@redhat.com> 写道：
> >> > > > > >
> >> > > > > > Not that I understand this report, but
> >> > > > > >
> >> > > > > > On 06/20, Richard Guy Briggs wrote:
> >> > > > > > >
> >> > > > > > > This function is only ever called by __audit_free(), which is only ever
> >> > > > > > > called on failure of task creation or on exit of the task, so in neither
> >> > > > > > > case can anything else change it.
> >> > > > > >
> >> > > > > > How so?
> >> > > > > >
> >> > > > > > Another thread or CLONE_VM task or /proc/pid/mem can change the user-space
> >> > > > > > memory in parallel.
> >> > > > > >
> >> > > > > > Oleg.
> >> > > > >
> >> > > > >
> >> > > > > Exactly, by saying “change the data”, I mean the modification from
> >> > > > > malicious users with crafted operations on the user space memory
> >> > > > > directly, rather than the normal operations within the audit
> >> > > > > subsystem in Linux. Moreover, since the copy operations from the user
> >> > > > > space are not protected by any locks or synchronization primitives,
> >> > > > > changing the data under race condition is feasible I think. Besides,
> >> > > > > there isn’t any visible checking step in the code to guarantee the
> >> > > > > consistency between the two copy operations.
> >> > > > >
> >> > > > > Here I would like to figure out what the consequences really are once
> >> > > > > the data is changed between the two copy operations, such as changing
> >> > > > > a none-control string to a control string but process it as a none-
> >> > > > > control string that has no control chars. I think problems will
> >> > > > > happen.
> >> > > >
> >> > > > So far as userland can see, kernel log lines are separated by newlines.
> >> > >
> >> > > Newlines are control characters that would be caught by that filter.
> >> > > That filter catches '"', < 0x21, > 0x7e.
> >> > >
> >> > > > If we fail to escape a newline, that makes it possible to inject
> >> > > > arbitrary log lines into the kernel log, which may be misleading to the
> >> > > > administrator or to software parsing the log.
> >> > >
> >> > > So, this is addressed, but I'm still trying to assess the danger of this
> >> > > repeated call to copy_from_user().
> >> >
> >> > The problem is that newlines can be added to the strings by another
> >> > task between the first pass that checks for control characters and the
> >> > second pass that copies them to the log.
> >>
> >> Understood, so this is the same sort of problem as Pengfei has raised
> >> with respect to double quotes being added.
> >>
> >> How can subsequent accesses of copy_from_user() be locked, or make sure
> >> the entire buffer is copied in one go?
> >
> > I don't believe it can.  And the fact that those strings can be
> > modified before they're logged kind of defeats the purpose of auditing,
> > no?  Seems like it would make more sense to copy the program name from
> > the binprm, log that at this point and don't even attempt to log the
> > arguments.
> 
> Agreed.

I'm starting to come around to that same conclusion.  Any drivers I've
seen that attempt this are either locking a kernel strucutre, which is
within its control (precluding any unreviewed  patches or modules), or
are locking a userspace entity that is willfully co-operating, neither
of which is this case that concerns us here.

> You definintely can't lock the string.  An attacker could put the
> string in MAP_SHARED memory, for example.

Understood.  So the best effort we can do at this point is to copy the
string all at once, not iterating, and don't repass the string a second
time to do the actual work but use the first copy.

Thanks for the sanity check Andy and Ben.

> --Andy

- RGB

--
Richard Guy Briggs <rgb@redhat.com>
Kernel Security Engineering, Base Operating Systems, Red Hat
Remote, Ottawa, Canada
Voice: +1.647.777.2635, Internal: (81) 32635

--
Linux-audit mailing list
Linux-audit@redhat.com
https://www.redhat.com/mailman/listinfo/linux-audit