[apparmor] [patch] logparser.py: change mask only for path events
Seth Arnold
seth.arnold at canonical.com
Wed Apr 1 03:02:47 UTC 2015
On Sun, Mar 29, 2015 at 11:04:10PM +0200, Christian Boltz wrote:
> Hello,
>
> this patch moves the code that does the c -> a and d -> w replacement
> in denied_mask and requested_mask so that it only runs for path events,
> but not for other events (like dbus and ptrace). The validate_log_mode()
> and log_str_to_mode() calls are also moved.
>
> Technically, this means moving code from parse_event() to the path
> section in add_event_to_tree().
>
> This also means aa-logprof no longer crashes if it hits a ptrace or
> dbus event in the log.
>
> The "if dmask:" and "if rmask:" checks are removed - if a path event
> doesn't have these two, it is totally broken and worth a aa-logprof
> crash ;-)
>
> Also adjust the parse_event() tests to expect the "raw" mask instead of
> a set.
>
>
> This patch fixes
> https://bugs.launchpad.net/apparmor/+bug/1426651 and
> https://bugs.launchpad.net/apparmor/+bug/1243932
>
>
> I manually tested that
> - c and d log events are still converted to a and w
> - ptrace events no longer crash aa-logprof
>
> Note: add_event_to_tree() is not covered by tests.
This is a hard one; the code changes all seem plausible enough and they do
fix very real bugs that are annoying users today, but the changes to the
datatypes that result in the test changes really worries me. Python won't
tell us if something is left somewhere in the program that requires them
to be sets rather than strings. and the only solution is to exhaustively
test the program.
Would you say you've tested it exhaustively after this patch?
I'm inclined to ack it anyway, since if it mostly works it'll be an
improvement, but I don't want to be hasty.
Thanks
> [ 30-logparser-change-mask-only-for-path-events.diff ]
>
> === modified file utils/apparmor/logparser.py
> --- utils/apparmor/logparser.py 2015-03-29 21:23:50.441294500 +0200
> +++ utils/apparmor/logparser.py 2015-03-29 22:25:37.256978665 +0200
> @@ -112,34 +112,14 @@
> ev['task'] = event.task
> ev['info'] = event.info
> ev['error_code'] = event.error_code
> - dmask = event.denied_mask
> - rmask = event.requested_mask
> + ev['denied_mask'] = event.denied_mask
> + ev['request_mask'] = event.requested_mask
> ev['magic_token'] = event.magic_token
> if ev['operation'] and self.op_type(ev['operation']) == 'net':
> ev['family'] = event.net_family
> ev['protocol'] = event.net_protocol
> ev['sock_type'] = event.net_sock_type
> LibAppArmor.free_record(event)
> - # Map c (create) to a and d (delete) to w, logprof doesn't support c and d
> - if rmask:
> - rmask = rmask.replace('c', 'a')
> - rmask = rmask.replace('d', 'w')
> - if not validate_log_mode(hide_log_mode(rmask)):
> - raise AppArmorException(_('Log contains unknown mode %s') % rmask)
> - if dmask:
> - dmask = dmask.replace('c', 'a')
> - dmask = dmask.replace('d', 'w')
> - if not validate_log_mode(hide_log_mode(dmask)):
> - raise AppArmorException(_('Log contains unknown mode %s') % dmask)
> - #print('parse_event:', ev['profile'], dmask, ev['name2'])
> - mask, name = log_str_to_mode(ev['profile'], dmask, ev['name2'])
> -
> - ev['denied_mask'] = mask
> - ev['name2'] = name
> -
> - mask, name = log_str_to_mode(ev['profile'], rmask, ev['name2'])
> - ev['request_mask'] = mask
> - ev['name2'] = name
>
> if not ev['time']:
> ev['time'] = int(time.time())
> @@ -267,7 +247,25 @@
> e['operation'] in ['open', 'truncate', 'mkdir', 'mknod', 'chmod', 'rename_src',
> 'rename_dest', 'unlink', 'rmdir', 'symlink_create', 'link',
> 'sysctl', 'getattr', 'setattr', 'xattr'] ):
> - #print(e['operation'], e['name'])
> +
> + # Map c (create) to a and d (delete) to w (logging is more detailed than the profile language)
> + rmask = e['request_mask']
> + rmask = rmask.replace('c', 'a')
> + rmask = rmask.replace('d', 'w')
> + if not validate_log_mode(hide_log_mode(rmask)):
> + raise AppArmorException(_('Log contains unknown mode %s') % rmask)
> +
> + dmask = e['denied_mask']
> + dmask = dmask.replace('c', 'a')
> + dmask = dmask.replace('d', 'w')
> + if not validate_log_mode(hide_log_mode(dmask)):
> + raise AppArmorException(_('Log contains unknown mode %s') % dmask)
> +
> + # convert rmask and dmask to mode arrays
> + e['denied_mask'], e['name2'] = log_str_to_mode(e['profile'], dmask, e['name2'])
> + e['request_mask'], e['name2'] = log_str_to_mode(e['profile'], rmask, e['name2'])
> +
> + # check if this is an exec event
> is_domain_change = False
> if e['operation'] == 'inode_permission' and (e['denied_mask'] & AA_MAY_EXEC) and aamode == 'PERMITTING':
> following = self.peek_at_next_log_entry()
> === modified file utils/test/test-capability.py
> --- utils/test/test-capability.py 2015-03-03 19:52:15.160640000 +0100
> +++ utils/test/test-capability.py 2015-03-29 22:41:54.412007617 +0200
> @@ -102,8 +102,8 @@
> parsed_event = parser.parse_event(event)
>
> self.assertEqual(parsed_event, {
> - 'request_mask': set(),
> - 'denied_mask': set(),
> + 'request_mask': None,
> + 'denied_mask': None,
> 'error_code': 0,
> 'magic_token': 0,
> 'parent': 0,
> === modified file utils/test/test-logparser.py
> --- utils/test/test-logparser.py 2015-03-03 22:16:55.549199117 +0100
> +++ utils/test/test-logparser.py 2015-03-29 22:41:02.093109280 +0200
> @@ -1,5 +1,6 @@
> # ----------------------------------------------------------------------
> # Copyright (C) 2013 Kshitij Gupta <kgupta8592 at gmail.com>
> +# Copyright (C) 2015 Christian Boltz <apparmor at cboltz.de>
> #
> # This program is free software; you can redistribute it and/or
> # modify it under the terms of version 2 of the GNU General Public
> @@ -26,7 +26,7 @@
> self.assertEqual(parsed_event['name'], '/home/www/foo.bar.in/httpdocs/apparmor/images/test/image 1.jpg')
> self.assertEqual(parsed_event['profile'], '/usr/sbin/httpd2-prefork//vhost_foo')
> self.assertEqual(parsed_event['aamode'], 'PERMITTING')
> - self.assertEqual(parsed_event['request_mask'], set(['w', 'a', '::w', '::a']))
> + self.assertEqual(parsed_event['request_mask'], 'wc')
>
> self.assertIsNotNone(ReadLog.RE_LOG_v2_6_audit.search(event))
> self.assertIsNone(ReadLog.RE_LOG_v2_6_syslog.search(event))
> @@ -37,7 +37,7 @@
> self.assertEqual(parsed_event['name'], '/home/foo/.bash_history')
> self.assertEqual(parsed_event['profile'], 'foo bar')
> self.assertEqual(parsed_event['aamode'], 'PERMITTING')
> - self.assertEqual(parsed_event['request_mask'], set(['r', 'w', 'a','::r' , '::w', '::a']))
> + self.assertEqual(parsed_event['request_mask'], 'rw')
>
> self.assertIsNotNone(ReadLog.RE_LOG_v2_6_audit.search(event))
> self.assertIsNone(ReadLog.RE_LOG_v2_6_syslog.search(event))
> @@ -49,7 +49,7 @@
> self.assertEqual(parsed_event['name'], '/dev/tty')
> self.assertEqual(parsed_event['profile'], '/home/cb/linuxtag/apparmor/scripts/hello')
> self.assertEqual(parsed_event['aamode'], 'PERMITTING')
> - self.assertEqual(parsed_event['request_mask'], set(['r', 'w', 'a', '::r', '::w', '::a']))
> + self.assertEqual(parsed_event['request_mask'], 'rw')
>
> self.assertIsNone(ReadLog.RE_LOG_v2_6_audit.search(event))
> self.assertIsNotNone(ReadLog.RE_LOG_v2_6_syslog.search(event))
> @@ -61,7 +61,7 @@
> self.assertEqual(parsed_event['name'], '/usr/bin/')
> self.assertEqual(parsed_event['profile'], '/home/simi/bin/aa-test')
> self.assertEqual(parsed_event['aamode'], 'PERMITTING')
> - self.assertEqual(parsed_event['request_mask'], set(['r', '::r']))
> + self.assertEqual(parsed_event['request_mask'], 'r')
>
> self.assertIsNone(ReadLog.RE_LOG_v2_6_audit.search(event))
> self.assertIsNotNone(ReadLog.RE_LOG_v2_6_syslog.search(event))
> @@ -75,7 +75,7 @@
> 'aamode': 'ERROR', # aamode for disconnected paths overridden aamode in parse_event()
> 'active_hat': None,
> 'attr': None,
> - 'denied_mask': {'r', '::r'},
> + 'denied_mask': 'r',
> 'error_code': 13,
> 'info': 'Failed name lookup - disconnected path',
> 'magic_token': 0,
> @@ -85,7 +85,7 @@
> 'parent': 0,
> 'pid': 25333,
> 'profile': '/sbin/klogd',
> - 'request_mask': {'r', '::r'},
> + 'request_mask': 'r',
> 'resource': 'Failed name lookup - disconnected path',
> 'task': 0,
> 'time': 1424425690
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150331/14b63b31/attachment-0001.pgp>
More information about the AppArmor
mailing list