[apparmor] [patch] [15/38] Change handle_children() and ask_the_questions() to FileRule
Christian Boltz
apparmor at cboltz.de
Wed Sep 21 20:28:04 UTC 2016
Hello,
as promised in
Re: [apparmor] [patch] utils/test/test-aa.py: skip tests that break with python2.7
some minutes ago, here's v2:
[patch] [15/38] Change handle_children() and ask_the_questions() to FileRule
This patch changes handle_children() (which asks about exec events) and
ask_the_questions() (which asks everything else) to FileRule. This
solves the "brain split" introduced by the previous patch.
This means aa-logprof and aa-genprof ask useful questions again, and
store the answers at the right place.
In detail, this means (with '-' line number from the diff)
- (391) handle_binfmt(): use FileRule. Also avoid breakage if glob_common()
returns an empty result.
- (484) profile_storage(): drop profile['allow']['path'] and
profile['deny']['path']
- (510) create_new_profile(): switch to FileRule
- (1190..1432) lots of changes in handle_children():
- drop escaping (done in FileRule)
- don't add events with 'x' perms to prelog
- use is_known_rule() instead of profile_known_exec()
- replace several regexes for the selected CMD_* with more readable
'in' clauses. While on it, drop unused parts of the regex.
- use plain 'ix', 'px' (as str) instead of str_to_mode() format
- call handle_binfmt() for the interpreter in ix, Pix and Cix rules
- (1652) ask_the_questions(): disable the old file-specific code
(not dropped because some features aren't ported to FileRule yet)
- (2336) collapse_log():
- convert file log events to FileRule (and add some workarounds and
TODOs for logparser.py behaviour that needs to change)
- disable the old file-specific code (not dropped because merging of
existing permissions isn't ported to FileRule yet)
- (2403) drop now unused validate_profile_mode() and the regexes it used
- (3374) drop now unused profile_known_exec()
Test changes:
- adjust fake_ldd to handle /bin/bash
- change test-aa.py AaTest_create_new_profile to expect FileRule instead
of a path hasher. Also copy the profiles to the tempdir and load the
abstractions that are needed by the test.
Important: Some nice-to-have features are not yet implemented for
FileRule:
- globbing
- (N)ew (allowing the user to enter a custom path)
- displaying and merging of permissions already existing in the profile
This means: aa-logprof works, but it's not as user-friendly as before.
The next patches will fix that ;-)
---
v2 brings two changes to the test-aa.py part of this patch:
- refresh the first hunk so that it can be applied again (broke by
Steve's 'import sys' addition)
- skip the extended AaTest_create_new_profile on py2 because changing
apparmor.aa.cfg['settings']['ldd'] doesn't work for some reason
[ 15-use-FileRule-in-logprof.diff ]
=== modified file ./utils/apparmor/aa.py
--- utils/apparmor/aa.py 2016-02-21 18:10:49.992607186 +0100
+++ utils/apparmor/aa.py 2016-02-21 18:12:19.928054520 +0100
@@ -37,7 +37,7 @@
import apparmor.ui as aaui
from apparmor.aamode import (str_to_mode, mode_to_str,
- mode_to_str_user, mode_contains, AA_OTHER,
+ mode_to_str_user, mode_contains, split_mode,
flatten_mode, owner_flatten_mode)
from apparmor.regex import (RE_PROFILE_START, RE_PROFILE_END, RE_PROFILE_LINK,
@@ -391,14 +391,20 @@
if get_reqs(library):
reqs += get_reqs(library)
reqs_processed[library] = True
- combined_mode = match_prof_incs_to_path(profile, 'allow', library)
- if combined_mode:
- continue
- library = glob_common(library)
- if not library:
- continue
- profile['allow']['path'][library]['mode'] = profile['allow']['path'][library].get('mode', set()) | str_to_mode('mr')
- profile['allow']['path'][library]['audit'] |= profile['allow']['path'][library].get('audit', set())
+ # match_prof_incs_to_path result gets ignored, so just skip it
+ #combined_mode = match_prof_incs_to_path(profile, 'allow', library)
+ #if combined_mode:
+ # continue
+
+ library_rule = FileRule(library, 'mr', None, FileRule.ALL, owner=False, log_event=True)
+
+ if not is_known_rule(profile, 'file', library_rule):
+ globbed_library = glob_common(library)
+ if globbed_library:
+ # glob_common returns a list, just use the first element (typically '/lib/libfoo.so.*')
+ library_rule = FileRule(globbed_library[0], 'mr', None, FileRule.ALL, owner=False)
+
+ profile['file'].add(library_rule)
def get_interpreter_and_abstraction(exec_target):
'''Check if exec_target is a script.
@@ -470,7 +470,6 @@ def profile_storage(profilename, hat, ca
profile['rlimit'] = RlimitRuleset()
profile['signal'] = SignalRuleset()
- profile['allow']['path'] = hasher()
profile['allow']['mount'] = list()
profile['allow']['pivot_root'] = list()
@@ -510,23 +515,15 @@
interpreter_path, abstraction = get_interpreter_and_abstraction(localfile)
if interpreter_path:
- local_profile[localfile]['allow']['path'][localfile]['mode'] = local_profile[localfile]['allow']['path'][localfile].get('mode', str_to_mode('r')) | str_to_mode('r')
-
- local_profile[localfile]['allow']['path'][localfile]['audit'] = local_profile[localfile]['allow']['path'][localfile].get('audit', set())
-
- local_profile[localfile]['allow']['path'][interpreter_path]['mode'] = local_profile[localfile]['allow']['path'][interpreter_path].get('mode', str_to_mode('ix')) | str_to_mode('ix')
-
- local_profile[localfile]['allow']['path'][interpreter_path]['audit'] = local_profile[localfile]['allow']['path'][interpreter_path].get('audit', set())
+ local_profile[localfile]['file'].add(FileRule(localfile, 'r', None, FileRule.ALL, owner=False))
+ local_profile[localfile]['file'].add(FileRule(interpreter_path, None, 'ix', FileRule.ALL, owner=False))
if abstraction:
local_profile[localfile]['include'][abstraction] = True
handle_binfmt(local_profile[localfile], interpreter_path)
else:
-
- local_profile[localfile]['allow']['path'][localfile]['mode'] = local_profile[localfile]['allow']['path'][localfile].get('mode', str_to_mode('mr')) | str_to_mode('mr')
-
- local_profile[localfile]['allow']['path'][localfile]['audit'] = local_profile[localfile]['allow']['path'][localfile].get('audit', set())
+ local_profile[localfile]['file'].add(FileRule(localfile, 'mr', None, FileRule.ALL, owner=False))
handle_binfmt(local_profile[localfile], localfile)
# Add required hats to the profile if they match the localfile
@@ -1190,19 +1187,6 @@
if not profile or not hat or not detail:
continue
- domainchange = 'nochange'
- if typ == 'exec':
- domainchange = 'change'
-
- # Escape special characters
- detail = detail.replace('[', '\[')
- detail = detail.replace(']', '\]')
- detail = detail.replace('+', '\+')
- detail = detail.replace('*', '\*')
- detail = detail.replace('{', '\{')
- detail = detail.replace('}', '\}')
- detail = detail.replace('!', '\!')
-
# Give Execute dialog if x access requested for something that's not a directory
# For directories force an 'ix' Path dialog
do_execute = False
@@ -1215,8 +1203,9 @@
raise AppArmorBug('exec permissions requested for %i(exec_target)s, but mode is %(mode)s instead of exec. This should not happen - please open a bugreport!' % {'exec_target': exec_target, 'mode':mode})
else:
do_execute = True
+ domainchange = 'change'
- if mode:
+ if mode and mode != str_to_mode('x'): # x is already handled in handle_children, so it must not become part of prelog
path = detail
if prelog[aamode][profile][hat]['path'].get(path, False):
@@ -1224,15 +1213,16 @@
prelog[aamode][profile][hat]['path'][path] = mode
if do_execute:
- if profile_known_exec(aa[profile][hat], 'exec', exec_target):
+ exec_event = FileRule(exec_target, None, FileRule.ANY_EXEC, FileRule.ALL, owner=False, log_event=True)
+ if is_known_rule(aa[profile][hat], 'file', exec_event):
continue
p = update_repo_profile(aa[profile][profile])
if to_name:
- if UI_SelectUpdatedRepoProfile(profile, p) and profile_known_exec(aa[profile][hat], 'exec', to_name):
+ if UI_SelectUpdatedRepoProfile(profile, p) and is_known_rule(aa[profile][hat], 'file', exec_event): # we need an exec_event with target=to_name here
continue
else:
- if UI_SelectUpdatedRepoProfile(profile, p) and profile_known_exec(aa[profile][hat], 'exec', exec_target):
+ if UI_SelectUpdatedRepoProfile(profile, p) and is_known_rule(aa[profile][hat], 'file', exec_event): # we need an exec_event with target=exec_target here
continue
context_new = profile
@@ -1244,6 +1234,7 @@
# Log parsing methods will convert it to its profile form
# nx is internally cx/px/cix/pix + to_name
exec_mode = False
+ file_perm = None
if True:
options = cfg['qualifiers'].get(exec_target, 'ipcnu')
@@ -1296,15 +1287,15 @@
# options = '|'.join(options)
seen_events += 1
- regex_options = re.compile('^CMD_(ix|px|cx|nx|pix|cix|nix|px_safe|cx_safe|nx_safe|pix_safe|cix_safe|nix_safe|ux|ux_safe|EXEC_TOGGLE|DENY)$')
+ # ask user about the exec mode to use
ans = ''
- while not regex_options.search(ans):
- ans = q.promptUser()[0].strip()
+ while ans not in ['CMD_ix', 'CMD_px', 'CMD_cx', 'CMD_nx', 'CMD_pix', 'CMD_cix', 'CMD_nix', 'CMD_ux', 'CMD_DENY']: # add '(I)gnore'? (hotkey conflict with '(i)x'!)
+ ans = q.promptUser()[0]
+
if ans.startswith('CMD_EXEC_IX_'):
exec_toggle = not exec_toggle
- q.functions = []
- q.functions += build_x_functions(default, options, exec_toggle)
+ q.functions = build_x_functions(default, options, exec_toggle)
ans = ''
continue
@@ -1330,71 +1322,62 @@
to_name = aaui.UI_GetString(_('Enter profile name to transition to: '), arg)
- regex_optmode = re.compile('CMD_(px|cx|nx|pix|cix|nix)')
if ans == 'CMD_ix':
- exec_mode = str_to_mode('ix')
- elif regex_optmode.search(ans):
- match = regex_optmode.search(ans).groups()[0]
- exec_mode = str_to_mode(match)
- px_default = 'n'
+ exec_mode = 'ix'
+ elif ans in ['CMD_px', 'CMD_cx', 'CMD_pix', 'CMD_cix']:
+ exec_mode = ans.replace('CMD_', '')
px_msg = _("Should AppArmor sanitise the environment when\nswitching profiles?\n\nSanitising environment is more secure,\nbut some applications depend on the presence\nof LD_PRELOAD or LD_LIBRARY_PATH.")
if parent_uses_ld_xxx:
px_msg = _("Should AppArmor sanitise the environment when\nswitching profiles?\n\nSanitising environment is more secure,\nbut this application appears to be using LD_PRELOAD\nor LD_LIBRARY_PATH and sanitising the environment\ncould cause functionality problems.")
- ynans = aaui.UI_YesNo(px_msg, px_default)
+ ynans = aaui.UI_YesNo(px_msg, 'y')
if ynans == 'y':
# Disable the unsafe mode
- exec_mode = exec_mode - (apparmor.aamode.AA_EXEC_UNSAFE | AA_OTHER(apparmor.aamode.AA_EXEC_UNSAFE))
+ exec_mode = exec_mode.capitalize()
elif ans == 'CMD_ux':
- exec_mode = str_to_mode('ux')
+ exec_mode = 'ux'
ynans = aaui.UI_YesNo(_("Launching processes in an unconfined state is a very\ndangerous operation and can cause serious security holes.\n\nAre you absolutely certain you wish to remove all\nAppArmor protection when executing %s ?") % exec_target, 'n')
if ynans == 'y':
ynans = aaui.UI_YesNo(_("Should AppArmor sanitise the environment when\nrunning this program unconfined?\n\nNot sanitising the environment when unconfining\na program opens up significant security holes\nand should be avoided if at all possible."), 'y')
if ynans == 'y':
# Disable the unsafe mode
- exec_mode = exec_mode - (apparmor.aamode.AA_EXEC_UNSAFE | AA_OTHER(apparmor.aamode.AA_EXEC_UNSAFE))
+ exec_mode = exec_mode.capitalize()
else:
ans = 'INVALID'
- regex_options = re.compile('CMD_(ix|px|cx|nx|pix|cix|nix)')
- if regex_options.search(ans):
+ if exec_mode and 'i' in exec_mode:
# For inherit we need r
- if exec_mode & str_to_mode('i'):
- exec_mode |= str_to_mode('r')
+ file_perm = 'r'
else:
if ans == 'CMD_DENY':
- aa[profile][hat]['deny']['path'][exec_target]['mode'] = aa[profile][hat]['deny']['path'][exec_target].get('mode', str_to_mode('x')) | str_to_mode('x')
- aa[profile][hat]['deny']['path'][exec_target]['audit'] = aa[profile][hat]['deny']['path'][exec_target].get('audit', set())
+ aa[profile][hat]['file'].add(FileRule(exec_target, None, 'x', FileRule.ALL, owner=False, log_event=True, deny=True))
changed[profile] = True
# Skip remaining events if they ask to deny exec
if domainchange == 'change':
return None
if ans != 'CMD_DENY':
- prelog['PERMITTING'][profile][hat]['path'][exec_target] = prelog['PERMITTING'][profile][hat]['path'].get(exec_target, exec_mode) | exec_mode
-
- log_dict['PERMITTING'][profile] = hasher()
-
- aa[profile][hat]['allow']['path'][exec_target]['mode'] = aa[profile][hat]['allow']['path'][exec_target].get('mode', exec_mode)
-
- aa[profile][hat]['allow']['path'][exec_target]['audit'] = aa[profile][hat]['allow']['path'][exec_target].get('audit', set())
-
if to_name:
- aa[profile][hat]['allow']['path'][exec_target]['to'] = to_name
+ rule_to_name = to_name
+ else:
+ rule_to_name = FileRule.ALL
+
+ aa[profile][hat]['file'].add(FileRule(exec_target, file_perm, exec_mode, rule_to_name, owner=False, log_event=True))
changed[profile] = True
- if exec_mode & str_to_mode('i'):
+ if 'i' in exec_mode:
interpreter_path, abstraction = get_interpreter_and_abstraction(exec_target)
if interpreter_path:
- aa[profile][hat]['allow']['path'][interpreter_path]['mode'] = aa[profile][hat]['allow']['path'][interpreter_path].get('mode', str_to_mode('ix')) | str_to_mode('ix')
-
- aa[profile][hat]['allow']['path'][interpreter_path]['audit'] = aa[profile][hat]['allow']['path'][interpreter_path].get('audit', set())
+ aa[profile][hat]['file'].add(FileRule(exec_target, 'r', None, FileRule.ALL, owner=False))
+ aa[profile][hat]['file'].add(FileRule(interpreter_path, None, 'ix', FileRule.ALL, owner=False))
if abstraction:
aa[profile][hat]['include'][abstraction] = True
+ handle_binfmt(aa[profile][hat], interpreter_path)
+
# Update tracking info based on kind of change
if ans == 'CMD_ix':
@@ -1409,7 +1394,7 @@
# Check profile exists for px
if not os.path.exists(get_profile_filename(exec_target)):
ynans = 'y'
- if exec_mode & str_to_mode('i'):
+ if 'i' in exec_mode:
ynans = aaui.UI_YesNo(_('A profile for %s does not exist.\nDo you want to create one?') % exec_target, 'n')
if ynans == 'y':
helpers[exec_target] = 'enforce'
@@ -1432,7 +1417,7 @@
if not aa[profile].get(exec_target, False):
ynans = 'y'
- if exec_mode & str_to_mode('i'):
+ if 'i' in exec_mode:
ynans = aaui.UI_YesNo(_('A profile for %s does not exist.\nDo you want to create one?') % exec_target, 'n')
if ynans == 'y':
hat = exec_target
@@ -1652,6 +1638,8 @@
done = False
# END of code (mostly) shared with aa-mergeprof
+def ask_the_questions_OLD_FILE_CODE(): # XXX unused
+ global seen_events
# Process all the path entries.
for path in sorted(log_dict[aamode][profile][hat]['allow']['path'].keys()):
mode = log_dict[aamode][profile][hat]['allow']['path'][path]
@@ -2336,6 +2326,29 @@
for path in prelog[aamode][profile][hat]['path'].keys():
mode = prelog[aamode][profile][hat]['path'][path]
+ user, other = split_mode(mode)
+
+ # logparser.py doesn't preserve 'owner' information, see https://bugs.launchpad.net/apparmor/+bug/1538340
+ # XXX re-check this code after fixing this bug
+ if other:
+ owner = False
+ mode = other
+ else:
+ owner = True
+ mode = user
+
+ # python3 aa-logprof -f <(echo '[55826.822365] audit: type=1400 audit(1454355221.096:85479): apparmor="ALLOWED" operation="file_receive" profile="/usr/sbin/smbd" name="/foo.png" pid=28185 comm="smbd" requested_mask="w" denied_mask="w" fsuid=100 ouid=100')
+ # happens via log_str_to_mode() called in logparser.py parse_event_for_tree()
+ # XXX fix this in the log parsing!
+ if 'a' in mode and 'w' in mode:
+ mode.remove('a')
+
+ file_event = FileRule(path, mode, None, FileRule.ALL, owner=owner, log_event=True)
+
+ if not is_known_rule(aa[profile][hat], 'file', file_event):
+ log_dict[aamode][profile][hat]['file'].add(file_event)
+
+ if False: # # XXX re-implement with FileRule
combinedmode = set()
# Is path in original profile?
if aa[profile][hat]['allow']['path'].get(path, False):
@@ -2403,24 +2416,6 @@
if not is_known_rule(aa[profile][hat], 'signal', signal_event):
log_dict[aamode][profile][hat]['signal'].add(signal_event)
-
-PROFILE_MODE_RE = re.compile('^(r|w|l|m|k|a|ix|ux|px|pux|cx|pix|cix|cux|Ux|Px|PUx|Cx|Pix|Cix|CUx)+$')
-PROFILE_MODE_DENY_RE = re.compile('^(r|w|l|m|k|a|x)+$')
-
-def validate_profile_mode(mode, allow, nt_name=None):
- if allow == 'deny':
- if PROFILE_MODE_DENY_RE.search(mode):
- return True
- else:
- return False
-
- else:
- if PROFILE_MODE_RE.search(mode):
- return True
- else:
- return False
-
-
def is_skippable_file(path):
"""Returns True if filename matches something to be skipped (rpm or dpkg backup files, hidden files etc.)
The list of skippable files needs to be synced with apparmor initscript and libapparmor _aa_is_blacklisted()
@@ -3774,30 +3769,6 @@
return None
return match
-def profile_known_exec(profile, typ, exec_target):
- if typ == 'exec':
- cm = None
- am = None
- m = []
-
- cm, am, m = rematchfrag(profile, 'deny', exec_target)
- if cm & apparmor.aamode.AA_MAY_EXEC:
- return -1
-
- cm, am, m = match_prof_incs_to_path(profile, 'deny', exec_target)
- if cm & apparmor.aamode.AA_MAY_EXEC:
- return -1
-
- cm, am, m = rematchfrag(profile, 'allow', exec_target)
- if cm & apparmor.aamode.AA_MAY_EXEC:
- return 1
-
- cm, am, m = match_prof_incs_to_path(profile, 'allow', exec_target)
- if cm & apparmor.aamode.AA_MAY_EXEC:
- return 1
-
- return 0
-
def is_known_rule(profile, rule_type, rule_obj):
# XXX get rid of get() checks after we have a proper function to initialize a profile
if profile.get(rule_type, False):
=== modified file ./utils/test/fake_ldd
--- utils/test/fake_ldd 2016-02-21 18:10:49.960607383 +0100
+++ utils/test/fake_ldd 2016-02-21 16:05:39.673508607 +0100
@@ -5,7 +5,7 @@
if len(sys.argv) != 2:
raise Exception('wrong number of arguments in fake_ldd')
-if sys.argv[1] == '/AATest/bin/bash':
+if sys.argv[1] == '/AATest/bin/bash' or sys.argv[1] == '/bin/bash':
print(' linux-vdso.so.1 (0x00007ffcf97f4000)')
print(' libreadline.so.6 => /AATest/lib64/libreadline.so.6 (0x00007f2c41324000)')
print(' libtinfo.so.6 => /AATest/lib64/libtinfo.so.6 (0x00007f2c410f9000)')
=== modified file ./utils/test/test-aa.py
--- utils/test/test-aa.py 2016-02-21 18:10:49.960607383 +0100
+++ utils/test/test-aa.py 2016-02-21 16:05:39.673508607 +0100
@@ -14,6 +14,7 @@
from common_test import read_file, write_file
import os
+import shutil
import sys
import apparmor.aa # needed to set global vars in some tests
@@ -104,21 +105,35 @@
('foo bar', (None, None)),
]
def _run_test(self, params, expected):
+ apparmor.aa.cfg['settings']['ldd'] = './fake_ldd'
+ # for some reason, setting the ldd config option does not get
+ # honored in python2.7
+ # XXX KILL when python 2.7 is dropped XXX
+ if sys.version_info[0] < 3:
+ print("Skipping on python < 3.x")
+ return
+
+ self.createTmpdir()
+
+ #copy the local profiles to the test directory
+ self.profile_dir = '%s/profiles' % self.tmpdir
+ shutil.copytree('../../profiles/apparmor.d/', self.profile_dir, symlinks=True)
+
+ # load the abstractions we need in the test
+ apparmor.aa.profiledir = self.profile_dir
+ apparmor.aa.load_include('abstractions/base')
+ apparmor.aa.load_include('abstractions/bash')
+
exp_interpreter_path, exp_abstraction = expected
program = self.writeTmpfile('script', params)
profile = create_new_profile(program)
if exp_interpreter_path:
- self.assertEqual(profile[program][program]['allow']['path'][exp_interpreter_path]['mode'], {'x', '::i', '::x', 'i'} )
- self.assertEqual(profile[program][program]['allow']['path'][exp_interpreter_path]['audit'], set() )
- self.assertEqual(profile[program][program]['allow']['path'][program]['mode'], {'r', '::r'} )
- self.assertEqual(profile[program][program]['allow']['path'][program]['audit'], set() )
- self.assertEqual(set(profile[program][program]['allow']['path'].keys()), {program, exp_interpreter_path} )
+ self.assertEqual(set(profile[program][program]['file'].get_clean()), {'%s ix,' % exp_interpreter_path, '%s r,' % program, '',
+ '/AATest/lib64/libtinfo.so.* mr,', '/AATest/lib64/libc.so.* mr,', '/AATest/lib64/libdl.so.* mr,', '/AATest/lib64/libreadline.so.* mr,', '/AATest/lib64/ld-linux-x86-64.so.* mr,' })
else:
- self.assertEqual(profile[program][program]['allow']['path'][program]['mode'], {'r', '::r', 'm', '::m'} )
- self.assertEqual(profile[program][program]['allow']['path'][program]['audit'], set() )
- self.assertEqual(set(profile[program][program]['allow']['path'].keys()), {program} )
+ self.assertEqual(set(profile[program][program]['file'].get_clean()), {'%s mr,' % program, ''})
if exp_abstraction:
self.assertEqual(set(profile[program][program]['include'].keys()), {exp_abstraction, 'abstractions/base'})
Regards,
Christian Boltz
--
Jedes meiner Postings ist alt genug, um für sich selbst zu sprechen.
Ich übernehme keinerlei Verantwortung für das Verhalten meiner Mails.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20160921/98bd9229/attachment-0001.pgp>
More information about the AppArmor
mailing list