=== added file 'Testing/aa_test.py' --- Testing/aa_test.py 1970-01-01 00:00:00 +0000 +++ Testing/aa_test.py 2013-07-30 14:43:08 +0000 @@ -0,0 +1,88 @@ +class Test(unittest.TestCase): + + + def test_parse_event(self): + event = 'type=AVC msg=audit(1345027352.096:499): apparmor="ALLOWED" operation="rename_dest" parent=6974 profile="/usr/sbin/httpd2-prefork//vhost_balmar" name=2F686F6D652F7777772F62616C6D61722E646174616E6F766F322E64652F68747470646F63732F6A6F6F6D6C612F696D616765732F6B75656368656E2F666F746F20322E6A7067 pid=20143 comm="httpd2-prefork" requested_mask="wc" denied_mask="wc" fsuid=30 ouid=30' + parsed_event = apparmor.aa.parse_event(event) + print(parsed_event) ### please check for the expected parsed path etc. (for all 3 tests in test_parse_event) ### BTW: it would be nice to use vhost_foo and a random path from your system (anything with a space in it will do) to avoid getting customer names in the testcase ;-) ### (doesn't really hurt in the encoded form, but I'd like to avoid having the human-readable path in the testcase) + def test_modes_to_string(self): + self.assertEqual(apparmor.aa.mode_to_str(32270), 'rwPCUx') + + MODE_TEST = {'x': apparmor.aa.AA_MAY_EXEC, + 'w': apparmor.aa.AA_MAY_WRITE, ### you might want to store this list somewhere[tm] and also use it in test_string_to_modes() + def test_string_to_modes(self): + + #self.assertEqual(apparmor.aa.str_to_mode('wc'), 32270) + + MODE_TEST = {'x': apparmor.aa.AA_MAY_EXEC, + 'w': apparmor.aa.AA_MAY_WRITE, ### use the shared list from test_modes_to_string() to avoid duplication + #while MODE_TEST: + # string,mode = MODE_TEST.popitem() + # self.assertEqual(apparmor.aa.str_to_mode(string), mode) + + #self.assertEqual(apparmor.aa.str_to_mode('C'), 2048) ### looks like a ToDo note ;-) === modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-07-28 02:59:59 +0000 +++ apparmor/aa.py 2013-07-30 14:43:08 +0000 @@ -2977,3 +2976,533 @@ +def parse_profile_data(data, file, do_include): ... ### general note: ### please check utils/vim/create-apparmor.vim.py and apparmor.vim.in for a way to make the regexes easier to handle, easier readable and less error prone ### it probably makes sense to move them in a shared module to avoid duplication - at least if they are compatible with vim's regex syntax (I didn't check every detail) ### we also have the "allow" keyword as counterpart of "deny" since some days - so all regexes which support "deny" should contain (allow\s+|deny\s+)? ### (again, have a look at apparmor.vim.in and create-apparmor.vim.py - adding "allow" will be a one line change there ;-) + RE_PROFILE_START = re.compile('^\s*(("??\/.+?"??)|(profile\s+("??.+?"??)))\s+ ((flags=)?\((.+)\)\s+)* \{\s*(#.*)?$') ### (spaces around flags= added for readability) ### the flags= part is wrong, see my r28-29 comment / IRC log around line 615 ### basically, flags=(...) or (...) is allowed only once, so it should be ? instead of * + RE_PROFILE_END = re.compile('^\s*\}\s*(#.*)?$') + RE_PROFILE_CAP = re.compile('^\s*(audit\s+)?(deny\s+)?capability\s+(\S+)\s*,\s*(#.*)?$') + RE_PROFILE_SET_CAP = re.compile('^\s*set capability\s+(\S+)\s*,\s*(#.*)?$') ### "set capability" was removed some releases ago - no need to support it + RE_PROFILE_LINK = re.compile('^\s*(audit\s+)?(deny\s+)?link\s+(((subset)|(<=))\s+)?([\"\@\/].*?"??)\s+->\s*([\"\@\/].*?"??)\s*,\s*(#.*)?$') ### "owner" is missing in the regex ### I've never seen the <= part in link rules - John, is this valid syntax? + RE_PROFILE_CHANGE_PROFILE = re.compile('^\s*change_profile\s+->\s*("??.+?"??),(#.*)?$') + RE_PROFILE_ALIAS = re.compile('^\s*alias\s+("??.+?"??)\s+->\s*("??.+?"??)\s*,(#.*)?$') + RE_PROFILE_RLIMIT = re.compile('^\s*set\s+rlimit\s+(.+)\s+<=\s*(.+)\s*,(#.*)?$') ### some rlimit rules do not contain <= - from apparmor.vim.in: ### /\v^\s*set\s+rlimit\s+(nofile|nproc|rtprio)\s+[0-9]+@@EOL@@/ + RE_PROFILE_BOOLEAN = re.compile('^\s*(\$\{?[[:alpha:]][[:alnum:]_]*\}?)\s*=\s*(true|false)\s*,?\s*(#.*)?$') + RE_PROFILE_VARIABLE = re.compile('^\s*(@\{?[[:alpha:]][[:alnum:]_]+\}?)\s*\+?=\s*(.+?)\s*,?\s*(#.*)?$') + RE_PROFILE_CONDITIONAL = re.compile('^\s*if\s+(not\s+)?(\$\{?[[:alpha:]][[:alnum:]_]*\}?)\s*\{\s*(#.*)?$') + RE_PROFILE_CONDITIONAL_VARIABLE = re.compile('^\s*if\s+(not\s+)?defined\s+(@\{?[[:alpha:]][[:alnum:]_]+\}?)\s*\{\s*(#.*)?$') + RE_PROFILE_CONDITIONAL_BOOLEAN = re.compile('^\s*if\s+(not\s+)?defined\s+(\$\{?[[:alpha:]][[:alnum:]_]+\}?)\s*\{\s*(#.*)?$') ### I've never seen/used conditional syntax, but according to the tests in ### parser/tst/simple_tests/conditional/* it's supported ;-) ### this matches only "if", but "else" and "else if" are also supported ### (this is not the most urgent thing to add/fix ;-) + RE_PROFILE_PATH_ENTRY = re.compile('^\s*(audit\s+)?(deny\s+)?(owner\s+)?([\"\@\/].*?)\s+(\S+)(\s+->\s*(.*?))?\s*,\s*(#.*)?$') ### [\"\@\/] looks wrong - inside [...] escaping with \ shouldn't be needed (it makes \ an allowed char, which would be wrong) + RE_PROFILE_NETWORK = re.compile('^\s*(audit\s+)?(deny\s+)?network(.*)\s*(#.*)?$') + RE_PROFILE_CHANGE_HAT = re.compile('^\s*\^(\"??.+?\"??)\s*,\s*(#.*)?$') + RE_PROFILE_HAT_DEF = re.compile('^\s*\^(\"??.+?\"??)\s+ ((flags=)?\((.+)\)\s+)* \{\s*(#.*)?$') ### another wrong flags= part, see above for details ### (again, spaces around flags= added for readability) ... + elif RE_PROFILE_END.search(line): + # If profile ends and we're not in one + if not profile: + raise AppArmorException('Syntax Error: Unexpected End of Profile reached in file: %s line: %s' % (file, lineno+1)) ### note that there are other blocks that also end with } - for example "if" conditionals + elif RE_PROFILE_CAP.search(line): ... + allow = 'allow' + if matches[1]: + allow = 'deny' ### needs an update for the "allow" keyword (and a way to keep the keyword when writing the profile) ### (keeping the "allow" keyword is something to keep the user happy, but not technically needed because "allow" is the default behaviour) + elif RE_PROFILE_SET_CAP.search(line): + matches = RE_PROFILE_SET_CAP.search(line).groups() + + if not profile: + raise AppArmorException('Syntax Error: Unexpected capability entry found in file: %s line: %s' % (file, lineno+1)) + + capability = matches[0] + profile_data[profile][hat]['set_capability'][capability] = True ### "set capability" is no longer supported ... + elif RE_PROFILE_LINK.ssearch(line): + matches = RE_PROFILE_LINK.search(line).groups() + + if not profile: + raise AppArmorException('Syntax Error: Unexpected link entry found in file: %s line: %s' % (file, lineno+1)) + + audit = False + if matches[0]: + audit = True + + allow = 'allow' + if matches[1]: + allow = 'deny' ### needs an update for the "allow" keyword (and a way to keep the keyword when writing the profile) ... + if audit: + profile_data[profile][hat][allow]['link'][link]['audit'] = profile_data[profile][hat][allow]['link'][link].get('audit', 0) | AA_LINK_SUBSET ### just wondering - what does AA_LINK_SUBSET have to do with the audit flag? + else: + profile_data[profile][hat][allow]['link'][link]['audit'] = 0 ... + elif RE_PROFILE_RLIMIT.search(line): ### does this section need any changes for rlimit rules not containing <= ? + elif RE_PROFILE_CONDITIONAL.search(line): + # Conditional Boolean + pass + + elif RE_PROFILE_CONDITIONAL_VARIABLE.search(line): + # Conditional Variable defines + pass + + elif RE_PROFILE_CONDITIONAL_BOOLEAN.search(line): + # Conditional Boolean defined + pass ### looks like ToDo notes ;-) + elif RE_PROFILE_PATH_ENTRY.search(line): ... + allow = 'allow' + if matches[1]: + allow = 'deny' ### another place that needs to support "allow" + elif RE_PROFILE_NETWORK.search(line): ... + allow = 'allow' + if matches[1]: + allow = 'deny' ### another place that needs to support "allow" + if re.search('\s+(\S+)\s+(\S+)\s*,\s*(#.*)?$', network): + nmatch = re.search(network, '\s+(\S+)\s+(\S+)\s*,\s*(#.*)?$').groups() ### should this be re.search('...regex...', network).groups() ? ### besides that, please store the regex in a variable to avoid duplication + fam, typ = nmatch[:2] + profile_data[profile][hat][allow]['netdomain']['rule'][fam][typ] = True + profile_data[profile][hat][allow]['netdomain']['audit'][fam][typ] = audit + elif re.search('\s+(\S+)\s*,\s*(#.*)?$', network): + fam = re.search('\s+(\S+)\s*,\s*(#.*)?$', network).groups()[0] ### this regex is also worth a variable ### BTW: I'd do a two-step match. Basically: ### ^\s*network(\s+.*)\s*,\s*(#.*$) ### ^^^^^^^ ### and then use this part for the detail work. This will result in much easier to read code ;-) + profile_data[profile][hat][allow]['netdomain']['rule'][fam] = True + profile_data[profile][hat][allow]['netdomain']['audit'][fam] = audit + else: + profile_data[profile][hat][allow]['netdomain']['rule']['all'] = True + profile_data[profile][hat][allow]['netdomain']['audit']['all'] = audit # True + elif RE_PROFILE_HAT_DEF.search(line): + # An embedded hat syntax definition starts + matches = RE_PROFILE_HAT_DEF.search(line).groups() + if not profile: + raise AppArmorException('Syntax Error: Unexpected hat definition found in file: %s line: %s' % (file, lineno+1)) + + in_contained_hat = True + hat = matches[0] + hat = strip_quotes(hat) + flags = matches[3] + + profile_data[profile][hat]['flags'] = flags + profile_data[profile][hat]['declared'] = False + #profile_data[profile][hat]['allow']['path'] = hasher() + #profile_data[profile][hat]['allow']['netdomain'] = hasher() + + if initial_comment: + profile_data[profile][hat]['initial_comment'] = initial_comment + initial_comment = '' + + filelist[file]['profiles'][profile][hat] = True ### a check if the hat already exists might be useful to avoid duplicate hat names (which might get merged on write, but I doubt that's intended behaviour) ### interestingly, the parser does _not_ complain about duplicate hats. ### John, is this a bug or intentional? + # Below is not required I'd say ### hmm, not sure - John? + if not do_include: + for hatglob in cfg['required_hats'].keys(): + for parsed_prof in sorted(parsed_profiles): + if re.search(hatglob, parsed_prof): + for hat in cfg['required_hats'][hatglob].split(): + if not profile_data[parsed_prof].get(hat, False): + profile_data[parsed_prof][hat] = hasher() + # End of file reached but we're stuck in a profile + if profile and not do_include: + raise AppArmorException('Syntax Error: Reached end of file %s while inside profile %s' % (file, profile)) ### the error message should mention something like "missing '}'" to make the problem clear +def separate_vars(vs): ### a comment what this function does would be helpful (I had to look up the call chain) +def store_list_var(var, list_var, value): ### same here - a comment describing what the function does would be nice +def write_header(profile_data, depth, name, embedded_hat, write_flags): + pre = ' ' * depth + data = [] + name = quote_if_needed(name) + + if (not embedded_hat and re.search('^[^\/]|^"[^\/]', name)) or (embedded_hat and re.search('^[^^]' ,name)): ### the \ in '^[^\/]|^"[^\/]' looks wrong (no escaping needed inside [...]) +def write_single(profile_data, depth, allow, name, prefix, tail): + pre = ' ' * depth + data = [] + ref, allow = set_ref_allow(profile_data, allow) + + if ref.get(name, False): + for key in sorted(re[name].keys()): ### we should discuss if we want to keep writing in sorted() order (which can be helpful, but also annoying) ### or if we want to keep the original order of a profile whenever possible ### (see discussion about writing config files) ### -> topic for the next meeting? +def set_ref_allow(profile_data, allow): + if allow: + if allow == 'deny': + return profile_data[allow], 'deny ' + else: + return profile_data[allow], '' ### we need a method to keep the "allow" keyword if the profile already contains it ### (just to avoid annoying users) +def write_pair(profile_data, depth, allow, name, prefix, sep, tail, fn): + pre = ' ' * depth + data = [] + ref, allow = set_ref_allow(profile_data, allow) + + if ref.get(name, False): + for key in sorted(re[name].keys()): ### sorted() again - see above +def write_rlimits(prof_data, depth): + return write_pair(prof_data, depth, '', 'rlimit', 'set rlimit ', ' <= ', ',', quote_if_needed) ### needs an update for the rlimit rules that do not contain <= +def write_list_vars(prof_data, depth): + return write_pair(prof_data, depth, '', 'lvar', '', ' = ', '', var_transform) ### I didn't see a function to write variable additions (+=) +def write_cap_rules(prof_data, depth, allow): + pre = ' ' * depth + data = [] + allowstr = '' + if allow == 'deny': + allowstr = 'deny' ### another place that needs to keep the "allow" keyword + if prof_data[allow].get('capability', False): + for cap in sorted(prof_data[allow]['capability'].keys()): ### sorted again - see above + audit = '' + if prof_data[allow]['capability'][cap].get('audit', False): + audit = 'audit' + if prof_data[allow]['capability'][cap].get('set', False): + data.append(pre + audit + allowstr + 'capability,') ### this looks like it writes a general 'capability,' rule ### but it should write 'capability ' + cap + ',' ### (except if the general 'capability,' rule is wanted, of course ;-) + data.append('') + + return data + +def write_capabilities(prof_data, depth): + data = write_single(prof_data, depth, '', 'set_capability', 'set capability ', ',') ### "set capability" was dropped several releases ago (and won't come back) ### just drop support for it ### final note about all write_* functions: ### it looks like inline comments are always dropped, which is not a good idea