[apparmor] [patch] logparser.py: change mask only for path events
Steve Beattie
steve at nxnw.org
Wed Apr 15 15:50:18 UTC 2015
On Fri, Apr 03, 2015 at 04:50:43PM +0200, Christian Boltz wrote:
> Am Dienstag, 31. März 2015 schrieb Seth Arnold:
> > On Sun, Mar 29, 2015 at 11:04:10PM +0200, Christian Boltz wrote:
> > > 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.
>
> Well, actually it does - if 'mode' is a string, it will throw an
> exception for things like
> if mode & str_to_mode('x'):
>
> I just learned that the hard way ;-)
>
> > and the only solution is to exhaustively test the program.
> >
> > Would you say you've tested it exhaustively after this patch?
>
> In the meantime I tend to say yes - luckily the testing for the doubled
> arrow in named exec brought this up :-)
>
> Here's v2, with log_str_to_mode() calls for e['operation'] == 'exec'
> added. This time I even checked logparser.py to make sure there are no
> more sections that use denied_mask or request_mask ;-)
>
>
> [ 30-logparser-change-mask-only-for-path-events.diff ]
Acked-by: Steve Beattie <steve at nxnw.org> for trunk and 2.9.
> === modified file utils/apparmor/logparser.py
> --- utils/apparmor/logparser.py 2015-04-03 16:33:17.913350220 +0200
> +++ utils/apparmor/logparser.py 2015-04-03 16:24:41.914848079 +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
> @@ -112,34 +113,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())
> @@ -254,6 +235,10 @@
> if profile != 'null-complain-profile' and not self.profile_exists(profile):
> return None
> if e['operation'] == 'exec':
> + # convert rmask and dmask to mode arrays
> + e['denied_mask'], e['name2'] = log_str_to_mode(e['profile'], e['denied_mask'], e['name2'])
> + e['request_mask'], e['name2'] = log_str_to_mode(e['profile'], e['request_mask'], e['name2'])
> +
> if e.get('info', False) and e['info'] == 'mandatory profile missing':
> self.add_to_tree(e['pid'], e['parent'], 'exec',
> [profile, hat, aamode, 'PERMITTING', e['denied_mask'], e['name'], e['name2']])
> @@ -267,7 +252,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-04-03 16:33:17.914350162 +0200
> +++ utils/test/test-capability.py 2015-04-03 15:58:16.782390170 +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-04-03 16:33:17.914350162 +0200
> +++ utils/test/test-logparser.py 2015-04-03 15:58:16.783390112 +0200
> @@ -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
>
>
>
> Regards,
>
> Christian Boltz
> --
> > btw: Entnehme ich Deinen Worten, daß XP und Sicherheit
> > in irgendeiner Weise eine Verbindung eingehen können?
> Klar können sie. eine logische XOR-Verbindung.
> [U. Ohse und S. Posner in dasr]
>
>
> --
> AppArmor mailing list
> AppArmor at lists.ubuntu.com
> Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor
--
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: Digital signature
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20150415/38b01c03/attachment.pgp>
More information about the AppArmor
mailing list