diff --git a/utils/aa-mergeprof b/utils/aa-mergeprof index b0967e49d..d5700dbd2 100755 --- a/utils/aa-mergeprof +++ b/utils/aa-mergeprof @@ -165,7 +165,10 @@ class Merge(object): options = [] for inc in other.filelist[other.filename]['include'].keys(): if not inc in self.user.filelist[self.user.filename]['include'].keys(): - options.append('#include <%s>' %inc) + if inc.startswith('/'): + options.append('#include "%s"' %inc) + else: + options.append('#include <%s>' %inc) default_option = 1 diff --git a/utils/apparmor/aa.py b/utils/apparmor/aa.py index 1e7f4bba2..6dd44371a 100644 --- a/utils/apparmor/aa.py +++ b/utils/apparmor/aa.py @@ -1432,7 +1432,10 @@ def ask_the_questions(log_dict): options = [] for inc in log_dict[aamode][profile][hat]['include'].keys(): if not inc in aa[profile][hat]['include'].keys(): - options.append('#include <%s>' %inc) + if inc.startswith('/'): + options.append('#include "%s"' %inc) + else: + options.append('#include <%s>' %inc) default_option = 1 @@ -1719,6 +1722,8 @@ def valid_include(profile, incname): if incname.startswith('abstractions/') and os.path.isfile(profile_dir + '/' + incname): return True + elif incname.startswith('/') and os.path.isfile(incname): + return True return False @@ -2298,7 +2303,11 @@ def parse_profile_data(data, file, do_include): filelist[file] = hasher() filelist[file]['include'][include_name] = True # If include is a directory - if os.path.isdir(profile_dir + '/' + include_name): + if include_name.startswith('/'): + dir = include_name + else: + dir = profile_dir + '/' + include_name + if os.path.isdir(dir): for file_name in include_dir_filelist(profile_dir, include_name): if not include.get(file_name, False): load_include(file_name) @@ -2568,7 +2577,13 @@ def write_single(prof_data, depth, allow, name, prefix, tail): if ref.get(name, False): for key in sorted(ref[name].keys()): - qkey = quote_if_needed(key) + if name == 'include': + if key.startswith('/'): + qkey = '"%s"' % key + else: + qkey = '<%s>' % quote_if_needed(key) + else: + qkey = quote_if_needed(key) data.append('%s%s%s%s%s' % (pre, allow, prefix, qkey, tail)) if ref[name].keys(): data.append('') @@ -2607,7 +2622,7 @@ def write_pair(prof_data, depth, allow, name, prefix, sep, tail, fn): return data def write_includes(prof_data, depth): - return write_single(prof_data, depth, '', 'include', '#include <', '>') + return write_single(prof_data, depth, '', 'include', '#include ', '') def write_change_profile(prof_data, depth): data = [] @@ -3334,7 +3349,11 @@ def is_known_rule(profile, rule_type, rule_obj): incname = includelist.pop(0) checked.append(incname) - if os.path.isdir(profile_dir + '/' + incname): + if incname.startswith('/'): + dir = incname + else: + dir = profile_dir + '/' + incname + if os.path.isdir(dir): includelist += include_dir_filelist(profile_dir, incname) else: if include[incname][incname].get(rule_type, False): @@ -3362,7 +3381,11 @@ def get_file_perms(profile, path, audit, deny): continue checked.append(incname) - if os.path.isdir(profile_dir + '/' + incname): + if incname.startswith('/'): + dir = incname + else: + dir = profile_dir + '/' + incname + if os.path.isdir(dir): includelist += include_dir_filelist(profile_dir, incname) else: incperms = include[incname][incname]['file'].get_perms_for_path(path, audit, deny) @@ -3443,7 +3466,8 @@ def reload(bin_path): def get_include_data(filename): data = [] - filename = profile_dir + '/' + filename + if not filename.startswith('/'): + filename = profile_dir + '/' + filename if os.path.exists(filename): with open_file_read(filename) as f_in: data = f_in.readlines() @@ -3452,15 +3476,24 @@ def get_include_data(filename): return data def include_dir_filelist(profile_dir, include_name): - '''returns a list of files in the given profile_dir/include_name directory, except skippable files''' + '''returns a list of files in the given profile_dir/include_name directory, + except skippable files. If include_name is an absolute path, ignore + profile_dir. + ''' files = [] - for path in os.listdir(profile_dir + '/' + include_name): + if include_name.startswith('/'): + dir = include_name + else: + dir = profile_dir + '/' + include_name + for path in os.listdir(dir): path = path.strip() if is_skippable_file(path): continue - if os.path.isfile(profile_dir + '/' + include_name + '/' + path): + if os.path.isfile(dir + '/' + path): file_name = include_name + '/' + path - file_name = file_name.replace(profile_dir + '/', '') + # strip off profile_dir for non-absolute paths + if not include_name.startswith('/'): + file_name = file_name.replace(profile_dir + '/', '') files.append(file_name) return files @@ -3469,17 +3502,20 @@ def load_include(incname): load_includeslist = [incname] while load_includeslist: incfile = load_includeslist.pop(0) + incfile_abs = incfile + if not incfile.startswith('/'): + incfile_abs = profile_dir + '/' + incfile if include.get(incfile, {}).get(incfile, False): pass # already read, do nothing - elif os.path.isfile(profile_dir + '/' + incfile): - data = get_include_data(incfile) + elif os.path.isfile(incfile_abs): + data = get_include_data(incfile_abs) incdata = parse_profile_data(data, incfile, True) attach_profile_data(include, incdata) #If the include is a directory means include all subfiles - elif os.path.isdir(profile_dir + '/' + incfile): + elif os.path.isdir(incfile_abs): load_includeslist += include_dir_filelist(profile_dir, incfile) else: - raise AppArmorException("Include file %s not found" % (profile_dir + '/' + incfile) ) + raise AppArmorException("Include file %s not found" % (incfile_abs)) return 0 @@ -3577,4 +3613,3 @@ def init_aa(confdir="/etc/apparmor"): parser = conf.find_first_file(cfg['settings'].get('parser')) or '/sbin/apparmor_parser' if not os.path.isfile(parser) or not os.access(parser, os.EX_OK): raise AppArmorException('Can\'t find apparmor_parser at %s' % (parser)) - diff --git a/utils/apparmor/regex.py b/utils/apparmor/regex.py index 4e3958aa1..2aa267b96 100644 --- a/utils/apparmor/regex.py +++ b/utils/apparmor/regex.py @@ -133,7 +133,7 @@ def parse_profile_start_line(line, filename): return result -RE_INCLUDE = re.compile('^\s*#?include\s*<(?P.*)>' + RE_EOL) +RE_INCLUDE = re.compile('^\s*#?include\s*(<(?P.*)>|"(?P.*)"|(?P[^<>"]*))' + RE_EOL) def re_match_include(line): """Matches the path for include and returns the include path""" @@ -142,10 +142,29 @@ def re_match_include(line): if not matches: return None - if not matches.group('magicpath').strip(): + path = None + if matches.group('magicpath'): + path = matches.group('magicpath').strip() + elif matches.group('unquotedpath'): + # LP: #1738879 - parser doesn't handle unquoted paths everywhere + # path = matches.group('unquotedpath').strip() + raise AppArmorException(_('Syntax error: required quotes missing for #include')) + elif matches.group('quotedpath'): + path = matches.group('quotedpath').strip() + # LP: 1738880 - parser doesn't handle relative paths everywhere, and + # neither do we (see aa.py) + if len(path) > 0 and path[0] != '/': + raise AppArmorException(_('Syntax error: #include cannot be relative path')) + + # if path is empty or the empty string + if path is None or path == "": raise AppArmorException(_('Syntax error: #include rule with empty filename')) - return matches.group('magicpath') + # LP: #1738877 - parser doesn't handle files with spaces in the name + if re.search('\s', path): + raise AppArmorException(_('Syntax error: #include rule filename cannot contain spaces')) + + return path def strip_parenthesis(data): '''strips parenthesis from the given string and returns the strip()ped result. diff --git a/utils/apparmor/severity.py b/utils/apparmor/severity.py index f0d56c056..890a9e5df 100644 --- a/utils/apparmor/severity.py +++ b/utils/apparmor/severity.py @@ -185,7 +185,9 @@ class Severity(object): # If any includes, load variables from them first match = re_match_include(line) if match: - new_path = self.PROF_DIR + '/' + match + new_path = match + if not new_path.startswith('/'): + new_path = self.PROF_DIR + '/' + match self.load_variables(new_path) else: # Remove any comments diff --git a/utils/test/test-aa.py b/utils/test/test-aa.py index c5d171c24..422b64a03 100644 --- a/utils/test/test-aa.py +++ b/utils/test/test-aa.py @@ -859,6 +859,59 @@ class AaTest_propose_file_rules(AATest): proposals = propose_file_rules(profile, rule_obj) self.assertEqual(proposals, expected) + +class AaTest_propose_file_rules_with_absolute_includes(AATest): + tests = [ + # log event path and perms expected proposals + (['/not/found/anywhere', 'r'], ['/not/found/anywhere r,']), + (['/dev/null', 'w'], ['/dev/null rw,']), + (['/some/random/include', 'r'], ['/some/random/include rw,']), + (['/some/other/include', 'w'], ['/some/other/* rw,', '/some/other/inc* rw,', '/some/other/include rw,']), + ] + + def _run_test(self, params, expected): + 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') + + abs_include1 = write_file(self.tmpdir, 'test-abs1', "/some/random/include rw,") + apparmor.aa.load_include(abs_include1) + + abs_include2 = write_file(self.tmpdir, 'test-abs2', "/some/other/* rw,") + apparmor.aa.load_include(abs_include2) + + abs_include3 = write_file(self.tmpdir, 'test-abs3', "/some/other/inc* rw,") + apparmor.aa.load_include(abs_include3) + + profile = apparmor.aa.ProfileStorage('/test', '/test', 'test-aa.py') + profile['include']['abstractions/base'] = False + profile['include'][abs_include1] = False + profile['include'][abs_include2] = False + profile['include'][abs_include3] = False + + rule_obj = FileRule(params[0], params[1], None, FileRule.ALL, owner=False, log_event=True) + proposals = propose_file_rules(profile, rule_obj) + self.assertEqual(proposals, expected) + + +class AaTest_nonexistent_includes(AATest): + def test_bad_includes(self): + tests = [ + "/nonexistent/absolute/path", + "nonexistent/relative/path", + ] + + for i in tests: + with self.assertRaises(AppArmorException): + apparmor.aa.load_include(i) + + setup_aa(apparmor.aa) setup_all_loops(__name__) if __name__ == '__main__': diff --git a/utils/test/test-regex_matches.py b/utils/test/test-regex_matches.py index b93a71495..04033b52a 100644 --- a/utils/test/test-regex_matches.py +++ b/utils/test/test-regex_matches.py @@ -437,17 +437,27 @@ class TestInvalid_parse_profile_start_line(AATest): class Test_re_match_include(AATest): tests = [ - ('#include ', 'abstractions/base' ), + ('#include ', 'abstractions/base' ), # magic path ('#include # comment', 'abstractions/base' ), ('#include#comment', 'abstractions/base' ), (' #include ', 'abstractions/base' ), - ('include ', 'abstractions/base' ), # not supported by parser - # ('include foo', 'foo' ), # XXX not supported in tools yet - # ('include /foo/bar', '/foo/bar' ), # XXX not supported in tools yet - # ('include "foo"', 'foo' ), # XXX not supported in tools yet - # ('include "/foo/bar"', '/foo/bar' ), # XXX not supported in tools yet - (' some #include ', None, ), + ('#include "/foo/bar"', '/foo/bar' ), # absolute path + ('#include "/foo/bar" # comment', '/foo/bar' ), + ('#include "/foo/bar"#comment', '/foo/bar' ), + (' #include "/foo/bar" ', '/foo/bar' ), + ('include ', 'abstractions/base' ), # magic path + ('include # comment', 'abstractions/base' ), + ('include#comment', 'abstractions/base' ), + (' include ', 'abstractions/base' ), + ('include "/foo/bar"', '/foo/bar' ), # absolute path + ('include "/foo/bar" # comment', '/foo/bar' ), + ('include "/foo/bar"#comment', '/foo/bar' ), + (' include "/foo/bar" ', '/foo/bar' ), + + (' some #include ', None, ), # non-matching (' /etc/fstab r,', None, ), + ('/usr/include r,', None, ), + ('/include r,', None, ), ] def _run_test(self, params, expected): @@ -455,8 +465,53 @@ class Test_re_match_include(AATest): class TestInvalid_re_match_include(AATest): tests = [ - ('#include <>', AppArmorException ), + ('#include <>', AppArmorException ), # '#include' ('#include < >', AppArmorException ), + ('#include ""', AppArmorException ), + ('#include " "', AppArmorException ), + ('#include', AppArmorException ), + ('#include ', AppArmorException ), + ('#include "foo"', AppArmorException ), # LP: 1738880 (relative) + ('#include "foo" # comment', AppArmorException ), + ('#include "foo"#comment', AppArmorException ), + (' #include "foo" ', AppArmorException ), + ('#include "foo/bar"', AppArmorException ), + ('#include "foo/bar" # comment', AppArmorException ), + ('#include "foo/bar"#comment', AppArmorException ), + (' #include "foo/bar" ', AppArmorException ), + ('#include foo', AppArmorException ), # LP: 1738879 (no quotes) + ('#include foo/bar', AppArmorException ), + ('#include /foo/bar', AppArmorException ), + ('#include foo bar', AppArmorException ), # LP: 1738877 (space in name) + ('#include foo bar/baz', AppArmorException ), + ('#include "foo bar"', AppArmorException ), + ('#include /foo bar', AppArmorException ), + ('#include "/foo bar"', AppArmorException ), + ('#include "foo bar/baz"', AppArmorException ), + + ('include <>', AppArmorException ), # 'include' + ('include < >', AppArmorException ), + ('include ""', AppArmorException ), + ('include " "', AppArmorException ), + ('include', AppArmorException ), + ('include ', AppArmorException ), + ('include "foo"', AppArmorException ), # LP: 1738880 (relative) + ('include "foo" # comment', AppArmorException ), + ('include "foo"#comment', AppArmorException ), + (' include "foo" ', AppArmorException ), + ('include "foo/bar"', AppArmorException ), + ('include "foo/bar" # comment', AppArmorException ), + ('include "foo/bar"#comment', AppArmorException ), + (' include "foo/bar" ', AppArmorException ), + ('include foo', AppArmorException ), # LP: 1738879 (no quotes) + ('include foo/bar', AppArmorException ), + ('include /foo/bar', AppArmorException ), + ('include foo bar', AppArmorException ), # LP: 1738877 (space in name) + ('include foo bar/baz', AppArmorException ), + ('include "foo bar"', AppArmorException ), + ('include /foo bar', AppArmorException ), + ('include "/foo bar"', AppArmorException ), + ('include "foo bar/baz"', AppArmorException ), ] def _run_test(self, params, expected):