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

[Pengfei Wang]

> 在 2016年6月21日，下午9:47，Richard Guy Briggs <rgb at redhat.com> 写道：
> 
> On 2016-06-21 13:31, Andy Lutomirski wrote:
>> 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.
> 
> 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.

Agreed, buffer the string at the first round and use it instead of recopying it a second time from user space would keep it safe, which is the easiest way I think. Please fix it, thanks!

--Pengfei

> 
> Thanks for the sanity check Andy and Ben.
> 
>> --Andy
> 
> - RGB
> 
> --
> Richard Guy Briggs <rgb at redhat.com>
> Kernel Security Engineering, Base Operating Systems, Red Hat
> Remote, Ottawa, Canada
> Voice: +1.647.777.2635, Internal: (81) 32635