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

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

[Pengfei Wang]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

Hello,


I found this Double-Fetch issue in Linux-4.6.1/kernel/auditsc.c when I
was examining the source code, which I think is a bug.


In function audit_log_single_execve_arg(), the whole argument is
fetched from user space twice via copy_from_user(). In the first loop,
it is firstly fetched (line 1038) to verify, aka looking for non-ascii
chars. While in the second loop, the whole argument is fetched again
(line 1105) from user space and used at line 1121 and line 1123
respectively depends on the previous verification.


However, a double fetch problem happens when the user space fetched
data is changed by a concurrently running user thread under race
condition during the verification and the usage, and the data
inconsistency will cause serious problems. In this case, the verified
non-ascii argument from the first loop is likely to be changed to an
ascii one (i.e. containing ‘ “ ’)  which will be used in the second
loop. Then the argument is passed to audit_log_string() as none-ascii,
then move forward in audit_log_n_string() of file audit.c, the string
is enclosed with quote marks as well. Since the string contains
another quote mark in the middle, problems will happen when processing
the string based on quote mark, e.g. the string will be recognized as
a shorter one based on the middle quote mark. I believe other
consequences are also likely to be caused once the none control string
is treated as a control string, or vice versa, which is very likely to
happen under double fetch situations.


I am looking forward to a reply to confirm this, thank you!



Kind regards

Pengfei

[-- Attachment #2: audit.zip --]
[-- Type: application/zip, Size: 34545 bytes --]

[-- Attachment #3: Type: text/plain, Size: 0 bytes --]

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

[Richard Guy Briggs]

On 2016-06-20 14:50, Pengfei Wang wrote:
> Hello,
> 
> I found this Double-Fetch issue in Linux-4.6.1/kernel/auditsc.c when I
> was examining the source code, which I think is a bug.
> 
> In function audit_log_single_execve_arg(), the whole argument is
> fetched from user space twice via copy_from_user(). In the first loop,
> it is firstly fetched (line 1038) to verify, aka looking for non-ascii
> chars. While in the second loop, the whole argument is fetched again
> (line 1105) from user space and used at line 1121 and line 1123
> respectively depends on the previous verification.
> 
> However, a double fetch problem happens when the user space fetched
> data is changed by a concurrently running user thread under race
> condition during the verification and the usage, and the data
> inconsistency will cause serious problems. In this case, the verified
> non-ascii argument from the first loop is likely to be changed to an
> ascii one (i.e. containing ‘ “ ’)  which will be used in the second
> loop. Then the argument is passed to audit_log_string() as none-ascii,
> then move forward in audit_log_n_string() of file audit.c, the string
> is enclosed with quote marks as well. Since the string contains
> another quote mark in the middle, problems will happen when processing
> the string based on quote mark, e.g. the string will be recognized as
> a shorter one based on the middle quote mark. I believe other
> consequences are also likely to be caused once the none control string
> is treated as a control string, or vice versa, which is very likely to
> happen under double fetch situations.

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.

I don't think what you describe will ever happen.

> I am looking forward to a reply to confirm this, thank you!
> 
> Kind regards
> 
> Pengfei

- 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

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

[Oleg Nesterov]

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.

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

[Pengfei Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1301 bytes --]


> 在 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.

Pengfei

[-- Attachment #1.2: Type: text/html, Size: 2701 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 1962 bytes --]

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.
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.

Ben.

-- 
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought.
... I realized that a large part of my life from then on was going to
be spent
in finding mistakes in my own programs. - Maurice Wilkes, 1949

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Richard Guy Briggs]

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().

> Ben.

- 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

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

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 2652 bytes --]

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.

Ben.

-- 
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought.
... I realized that a large part of my life from then on was going to
be spent
in finding mistakes in my own programs. - Maurice Wilkes, 1949

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Richard Guy Briggs]

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?

> Ben.

- 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

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

[Ben Hutchings]

[-- Attachment #1.1: Type: text/plain, Size: 3507 bytes --]

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.

Ben.

-- 
Ben Hutchings
[W]e found...that it wasn't as easy to get programs right as we had
thought.
... I realized that a large part of my life from then on was going to
be spent
in finding mistakes in my own programs. - Maurice Wilkes, 1949

[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: 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@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.

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

--Andy

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

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

[Richard Guy Briggs]

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

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

[Pengfei Wang]

> 在 2016年6月21日，下午9:47，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.

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@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

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

[Paul Moore]

On Wed, Jun 22, 2016 at 5:57 AM, Pengfei Wang <wpengfeinudt@gmail.com> wrote:
> 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!

FYI: I've created a new issue on GitHub to track this:

 * https://github.com/linux-audit/audit-kernel/issues/18

-- 
paul moore
www.paul-moore.com

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

[Richard Guy Briggs]

On 2016-06-21 10:37, 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.

Ok, fair enough.  So if that is the case, then the iterative call to
copy_from_user() within each loop isn't safe either and needs a lock.

> 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.

If it is possible, I agree that it would cause problems.

So, next question, is each loop itself safe?

> Pengfei

- 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