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

在 2016年6月20日，下午8:18，Oleg Nesterov <oleg(a)redhat.com&gt; 写道： 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.

On Tue, 2016-06-21 at 10:37 +0100, Pengfei Wang wrote: > > > > 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg(a)redhat.com&gt; 写道： > > > > 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.

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(a)redhat.com&gt; 写道： > > > > > > > > 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.

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(a)redhat.com&gt; 写道： > > > > > > > > > > > > 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.

在 2016年6月21日，下午9:47，Richard Guy Briggs <rgb(a)redhat.com&gt; 写道： On 2016-06-21 13:31, Andy Lutomirski wrote: > On Tue, Jun 21, 2016 at 12:59 PM, Ben Hutchings <ben(a)decadent.org.uk&gt; 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(a)redhat.com&gt; 写道： >>>>>>>> >>>>>>>> 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(a)redhat.com&gt; Kernel Security Engineering, Base Operating Systems, Red Hat Remote, Ottawa, Canada Voice: +1.647.777.2635, Internal: (81) 32635

> 在 2016年6月20日，下午8:18，Oleg Nesterov <oleg(a)redhat.com&gt; 写道： > > 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