[apparmor] [patch] logparser.py: improve file vs. network event recognition

[Steve Beattie]

On Thu, Dec 01, 2016 at 12:37:42PM +0100, Christian Boltz wrote:
> Hello,
> 
> Am Mittwoch, 30. November 2016, 17:19:13 CET schrieb Steve Beattie:
> > On Sun, Nov 20, 2016 at 04:52:46PM +0100, Christian Boltz wrote:
> > > sometimes network events come with an operation keyword looking like
> > > file_perm which makes them look like file events. Instead of
> > > ignoring
> > > these events (which was a hotfix to avoid crashes), improve the type
> > > detection.
> [...]
> 
> > I have one concern about this patch:
> > > [ 02-logparser-improve-file-vs-net.diff ]
> > > 
> > > --- utils/apparmor/logparser.py	2016-11-20 16:00:37.374243431 +0100
> > > +++ utils/apparmor/logparser.py	2016-11-20 16:01:19.158060468 
> 
> > > +    def op_type(self, event):
> > >          """Returns the operation type if known, unkown otherwise"""
> > > 
> > > -        operation_type = self.OPERATION_TYPES.get(operation,
> > > 'unknown') -        return operation_type
> > > +
> > > +        if ( event['operation'].startswith('file_') or
> > > event['operation'].startswith('inode_') or event['operation'] in
> > > self.OP_TYPE_FILE_OR_NET ): 
> > > +            # file or network event?
> > > +            if event['family'] and event['protocol'] and
> > > event['sock_type']: 
> > > +                # 'unix' events also use
> > > keywords like 'connect', but protocol is 0 and should therefore be
> > > filtered out 
> > > +                return 'net'
> > > +            elif event['denied_mask']:
> > > +                return 'file'
> > > +            else:
> > > +                raise AppArmorException('unknown file or network
> > > event type')
> > If you raise an exception here, then you're essentially aborting
> > aa-logprof when it runs across a type that it doesn't know about.
> > Given that there is sometimes a lag between what types are emitted
> > and what types are understood, I'm not sure that this leaves a good
> > experience for the user. 
> 
> Understood, but...
> 
> This exception will only be raised for events that come with a keyword 
> we already know (listed in OP_TYPE_FILE_OR_NET, or starting with file_ or 
> inode_) _and_ the event does not contain the details that are expected 
> for file or network events.
> 
> Therefore I'm quite sure this exception will only be triggered if 
> something really strange happens. I really want to get a bugreport about 
> such cases ;-)  (BTW: The error message will include the log line 
> triggering the exception.)
> 
> If you really want, I can change the 2.10 version to "return 'unknown'" 
> for "strange" file or network events so that we can be 100% sure that 
> this patch doesn't cause a regression in 2.10.2 - but IMHO (and 
> according to the test_multi log examples), I consider this unlikely.
> 
> Unknown keywords will run into the "return 'unknown'" branch, which 
> means the event will be checked for other types - or be ignored if no 
> type matches.

Alright. I don't think we need differing behavior for 2.10, so
Acked-by: Steve Beattie <steve at nxnw.org> for the original patch for
trunk and 2.10.

(Please try to remember to add the empty .err files as well when
committing.)

> > Yet, skipping log messages ought to be communicated to the user 
> > somehow.
> 
> That's another can of worms ;-)

Yeah, I know.

-- 
Steve Beattie
<sbeattie at ubuntu.com>
http://NxNW.org/~steve/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20161204/a9ad33ce/attachment.pgp>