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

[Andy Lutomirski]

On Tue, Jun 21, 2016 at 12:59 PM, Ben Hutchings <ben at 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 at 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.

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

--Andy