> 在 2016年6月21日，下午9:47，Richard Guy Briggs <[email protected]> 写道： > > On 2016-06-21 13:31, Andy Lutomirski wrote: >> On Tue, Jun 21, 2016 at 12:59 PM, Ben Hutchings <[email protected]> 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 <[email protected]> 写道： >>>>>>>>> >>>>>>>>> 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 <[email protected]> > Kernel Security Engineering, Base Operating Systems, Red Hat > Remote, Ottawa, Canada > Voice: +1.647.777.2635, Internal: (81) 32635 -- Linux-audit mailing list [email protected] https://www.redhat.com/mailman/listinfo/linux-audit
