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