2
0
mirror of https://gitlab.com/apparmor/apparmor synced 2025-09-03 15:55:46 +00:00

[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.
  (These tests get skipped on py2 because changing
  apparmor.aa.cfg['settings']['ldd'] doesn't work for some unknown reason)


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 ;-)

Also note that pyflakes will fail for ask_the_questions_OLD_FILE_CODE()
because of undefined symbols (aamode, profile, hat). This will be fixed
when the old code gets dropped in one of the later patches.


Acked-by: Steve Beattie <steve@nxnw.org>

Bug: https://launchpad.net/bugs/1569316
This commit is contained in:
Christian Boltz
2016-10-01 19:55:58 +02:00
parent aaa244c5ec
commit ee7560d6ef
3 changed files with 101 additions and 125 deletions

View File

@@ -37,7 +37,7 @@ from apparmor.common import (AppArmorException, AppArmorBug, open_file_read, val
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 @@ def handle_binfmt(profile, path):
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.
@@ -468,7 +474,6 @@ def profile_storage(profilename, hat, calledby):
profile['rlimit'] = RlimitRuleset()
profile['signal'] = SignalRuleset()
profile['allow']['path'] = hasher()
profile['allow']['mount'] = list()
profile['allow']['pivot_root'] = list()
@@ -484,23 +489,15 @@ def create_new_profile(localfile, is_stub=False):
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
@@ -1166,19 +1163,6 @@ def handle_children(profile, hat, root):
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
@@ -1191,8 +1175,9 @@ def handle_children(profile, hat, root):
raise AppArmorBug('exec permissions requested for %(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):
@@ -1200,15 +1185,16 @@ def handle_children(profile, hat, root):
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
@@ -1220,6 +1206,7 @@ def handle_children(profile, hat, root):
# 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')
@@ -1272,15 +1259,15 @@ def handle_children(profile, hat, root):
# 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
@@ -1306,71 +1293,62 @@ def handle_children(profile, hat, root):
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':
@@ -1390,7 +1368,7 @@ def handle_children(profile, hat, root):
# 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'
@@ -1408,7 +1386,7 @@ def handle_children(profile, hat, root):
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
@@ -1629,6 +1607,8 @@ def ask_the_questions():
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]
@@ -2314,6 +2294,29 @@ def collapse_log():
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):
@@ -2381,24 +2384,6 @@ def collapse_log():
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()
@@ -3743,30 +3728,6 @@ def matchliteral(aa_regexp, literal):
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):

View File

@@ -5,7 +5,7 @@ import sys
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)')

View File

@@ -14,6 +14,7 @@ from common_test import AATest, setup_all_loops
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
@@ -111,21 +112,35 @@ class AaTest_create_new_profile(AATest):
('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'})