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

[Steve Beattie]

On Sun, Nov 20, 2016 at 04:52:46PM +0100, Christian Boltz wrote:
> Hello,
> 
> 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.
> 
> In detail, this means:
> - replace OPERATION_TYPES (which was basically a list of network event
>   keywords) with OP_TYPE_FILE_OR_NET (which is a list of keywords for
>   file and network events)
> - change op_type() parameters to expect the whole event, not only the
>   operation keyword, and rebuild the type detection based on the event
>   details
> - as a side effect, this simplifies the detection for file event
>   operations in parse_event_for_tree()
> - remove workaround code from parse_event_for_tree()
> 
> Also add 4 new testcases with log messages that were ignored before.
> 
> References:
> 
> a) various bugreports about crashes caused by unexpected operation keywords:
>    https://bugs.launchpad.net/apparmor/+bug/1466812
>    https://bugs.launchpad.net/apparmor/+bug/1509030
>    https://bugs.launchpad.net/apparmor/+bug/1540562
>    https://bugs.launchpad.net/apparmor/+bug/1577051
>    https://bugs.launchpad.net/apparmor/+bug/1582374
> 
> b) the summary bug for this patch
>    https://bugs.launchpad.net/apparmor/+bug/1613061
> 
> (that will make quite some --fixes for bzr commit ;-)
> 
> I propose this patch for trunk and 2.10.

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 +0100
> +
> -    def op_type(self, operation):
> +    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. Yet, skipping log messages ought to be communicated to
the user somehow.

> +        else:
> +            return 'unknown'
>  
>      def profile_exists(self, program):
>          """Returns True if profile exists, False otherwise"""
> 


-- 
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/20161130/d8a01739/attachment.pgp>