get_field_str() and interpret_field() bug with multi-word fields

[Eric Paris]

On Tue, 2008-08-12 at 16:57 -0400, John Dennis wrote:
> Eric Paris wrote: 
> > On Tue, 2008-08-12 at 15:58 -0400, John Dennis wrote:
> > 
> >   
> > > So many people have complained about this; I do not understand the
> > > resistance to fixing it. The argument it would break something which
> > > is broken to begin with does not seem like a reasonable justification
> > > to me. The sooner it's fixed the better IMHO.
> > >     
> > 
> > Show me the code and I'll start trying to fix the kernel based on that
> > code as best we can.  But before you start read over the article
> > 
> > Can user-space bugs be kernel regressions?
> > http://lwn.net/Articles/292143/
> > 
> > As soon as you grasp that article send me the code and we'll work
> > together to fix this problem!
> > 
> >   
> 
> Perhaps you should grasp the concept this is not a user space bug but
> a flawed implementation. Anyone with the most basic understanding of
> parsing and protocols would never defend the current implementation
> (the fact it's in the kernel does not suspend the laws of computer
> science and justify it).

Although I don't completely agree with you I do not disagree.  The
problem is that you are simply wrong in your proposed solution.  Any
kernel change MUST be backwards compatible with old userspace.  Period.
End of story.  If you want to break that rule just give up.  Bitch till
you turn blue, any kernel change MUST be backwards compatible with old
userspace.  

Given that fact.  And it's a fact.  I'm willing to start working on
moving forward with a new interface.  Most likely its going to have to
be a little tunable /proc/sys/kernel/old_audit_format which is going to
emit records EXACTLY the way we do it today.  We can add that to the
list of things that will get removed (In about 2+ years.)  I may even be
amenable to accepting a compile time option for old/new kernel output
format.  But in either case the patch set need to address ALL
non-extensible kernel->userspace data flows (AUDIT_SIGNAL_GET or
whatever that message interface is sucks, ioctls suck)  But I'll do/help
write that kind of work.

So as long as you somehow MAINTAIN COMPATIBILITY WITH OLD USERSPACE
please design the better interface.  I'd love to see some patches and as
long as you maintain the one rule I've laid down I'll gladly work with
you.  (There is another rule, you aren't allowed to add even slightly
complex parsers into the kernel, but I don't think you want to do that)

We need to remember a couple of things though.

#1 the kernel does NOT under ANY circumstances validate audit data.  If
userspace sends us crap that doesn't fit the system, too bad, crap goes
out.

#2 is really just #1.  What it really means is that patches that change
security/selinux are not acceptable.  Users of the audit system can do
any darn thing they want.  the audit parsing utilities can't possibly
know what the point of a message from SMACK means and so if SMACK
doesn't follow your convention its not your problem.  Same goes for
SELinux or any other USER of the audit system.  The kernel does not and
WILL NOT validate messages.

-Eric

> 
> Let me give you a simple example, suppose this key/value pair was in
> an audit record:
> 
> foo=00
> 
> How does one know which of the possible values foo has:
> 
> 1) it's the integer zero (but in what radix? does the leading zero
> imply octal or is it just an insignificant digit?)
> 
> 2) it's the hexadecimal encoding of a single character string
> containing one null byte.
> 
> 3) it's the 2 character string "00" consisting of two zero characters.
> 
> The fact is it's ambiguous, it could be any of the above. It's
> ambiguous because the audit stream is an improperly specified
> protocol.
> -- 
> John Dennis <jdennis at redhat.com>