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

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.