Hello, the attached files contain the remaining parts of old reviews.
I always remove things that are done, and also checked the remaining parts in the r40 code (that's why most files have "checked-in-r40") - but even with this suffix, they should be up to date with r47. I hope the number of files doesn't shock you too much - most of the files are quite small ;-) If you have questions or think some things need to be discussed, just ask ;-) Regards, Christian Boltz -- und *echte* Männer benutzen Linux -- wegen der langen Kommandozeilen ("Meine ist länger als deine!"). Dann muss man nicht mehr Krieg spielen, um zu zeigen, wie hart man ist. [S. Lauterkorn in suse-talk]
review of r13, r14 and r15 === added file 'apparmor/aa.py' --- apparmor/aa.py 1970-01-01 00:00:00 +0000 +++ apparmor/aa.py 2013-07-06 13:27:06 +0000 +CONFDIR = '/etc/apparmor' ### It's just a path, but you should keep it at _one_ place (config.py). ### (besides that, CONFDIR is unused in aa.py) +unimplemented_warning = False ### set this to true in debugging mode? ### (right now, unimplemented_warning is unused) +aa = dict() # Profiles originally in sd, replace by aa +original_aa = dict() ### aa and original_aa aren't very self-explaining variable names - can you give them a better name? ##### They basically contain the list of profiles present version and ##### the one at the time of loading ##### ### Nice, but the variable name should represent that. What about profile_list and original_profile_list? +def get_profile_filename(profile): + """Returns the full profile name""" + filename = profile + if filename.startswith('/'): + # Remove leading / + filename = filename[1:] + else: + filename = "profile_" + filename + filename.replace('/', '.') ### should we replace more chars? ### just imagine someone has "~/bin/bad $$ script ? for * stars" ### (ok, extreme example - but you want a profile for this for sure ;-) ### ### I'm not sure about backward compability if we change this (John?) ### but people could also have a profile for /bin/foo in /etc/apparmor.d/mybar ### so we can't really expect the /bin/foo profile in bin.foo #(r45) get_profile_filename is now in logparser.py +def complain(path): + """Sets the profile to complain mode if it exists""" + filename, name = name_to_prof_filename(path) ### see above - you can't rely on having the proifle for /bin/foo in bin.foo ### (IIRC there's even an open bugreport about this) +def enforce(path): + """Sets the profile to complain mode if it exists""" + filename, name = name_to_prof_filename(path) ### same here +def head(file): + """Returns the first/head line of the file""" + first = '' + if os.path.isfile(file): + with open_file_read(file) as f_in: + first = f_in.readline().rstrip() + return first ### should head() raise an error if file doesn't exist? ### (or does open_file_read() do this already?) +def get_output(params): + """Returns the return code output by running the program with the args given in the list""" + program = params[0] + args = params[1:] + ret = -1 + output = [] + # program is executable + if os.access(program, os.X_OK): + try: + # Get the output of the program + output = subprocess.check_output(params) + except OSError as e: + raise AppArmorException("Unable to fork: %s\n\t%s" %(program, str(e))) + # If exit-codes besides 0 + except subprocess.CalledProcessError as e: + output = e.output + output = output.decode('utf-8').split('\n') ### what happens if someone is not using utf-8? + ret = e.returncode + else: + ret = 0 + output = output.decode('utf-8').split('\n') + # Remove the extra empty string caused due to \n if present + if len(output) > 1: + output.pop() + return (ret, output) +def get_reqs(file): + """Returns a list of paths from ldd output""" + pattern1 = re.compile('^\s*\S+ => (\/\S+)') + pattern2 = re.compile('^\s*(\/\S+)') + reqs = [] + ret, ldd_out = get_output(ldd, file) ### ldd is "None" - you should fill it before using it ;-) ### also, I'm not sure if ldd needs to be a global variable ##### The setter method is yet to come. ;-) ##### Will fix it when refractoring the module later as I planned. :-) === added file 'apparmor/common.py' --- apparmor/common.py 1970-01-01 00:00:00 +0000 +++ apparmor/common.py 2013-07-03 23:34:04 +0000 +def cmd_pipe(command1, command2): + '''Try to pipe command1 into command2.''' + try: + sp1 = subprocess.Popen(command1, stdout=subprocess.PIPE) + sp2 = subprocess.Popen(command2, stdin=sp1.stdout) + except OSError as ex: + return [127, str(ex)] + + if sys.version_info[0] >= 3: + out = sp2.communicate()[0].decode('ascii', 'ignore') ### is "ascii" correct here, or should it be the system locale? +def open_file_read(path): + '''Open specified file read-only''' + try: + orig = codecs.open(path, 'r', "UTF-8") # once more - is hardcoding utf-8 a good idea? === apparmor/severity.py @@ -24,18 +26,24 @@ class Severity: line = line.strip() if '+=' in line: line = line.split('+=') + try: + self.severity['VARIABLES'][line[0]] += [i.strip('"') for i in line[1].split()] + except KeyError: + raise AppArmorException("Variable %s was not previously declared, but is being assigned additional values" % line[0]) ### can you add filename and (optionally) line number to the error message? ##### The variable finder method (which is postponed) will fix it. :-) #(r40) line number was added, filename still missing else: line = line.split('=') - self.severity['VARIABLES'][line[0]] = self.severity['VARIABLES'].get(line[0], []) + [i.strip('"') for i in line[1].split()] + if line[0] in self.severity['VARIABLES'].keys(): + raise AppArmorException("Variable %s was previously declared" % line[0]) ### same here - filename and line number would be nice ##### The variable finder method (which is postponed) will fix it. :-) #(r40) line number was added, filename still missing #(r40) still valid vim:ft=diff
=== modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-07-06 13:27:06 +0000 +++ apparmor/aa.py 2013-07-08 22:16:26 +0000 @@ -393,6 +394,263 @@ + if 'perl' in interpreter: + local_profile[localfile]['include']['abstractions/perl'] = 1 + elif 'python' in interpreter: + local_profile[localfile]['include']['abstractions/python'] = 1 + elif 'ruby' in interpreter: + local_profile[localfile]['include']['abstractions/ruby'] = 1 + elif '/bin/bash' in interpreter or '/bin/dash' in interpreter or '/bin/sh' in interpreter: + local_profile[localfile]['include']['abstractions/ruby'] = 1 ### this would be easier readable and easier to maintain if you make it an array like: ### interpreter['perl'] = 'abstractions/perl' ### interpreter['bash'] = 'abstractions/bash' ### interpreter['sh'] = 'abstractions/bash' ### ### besides that, checking the interpreter basename (path stripped off) feels better than using "... in interpreter" ### (even if this means we have to add "python3" explicitely) #(r40) still valid vim:ft=diff
=== modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-07-24 16:42:34 +0000 +++ apparmor/aa.py 2013-07-27 10:02:12 +0000 +def ask_the_question(): [...] + if not include_valid: + continue ### does this mean it's impossible to add an #include for files not in abstractions/ or custom_includes? ### I'd prefer not to forbid that ### (of course, check if the file exists etc.) + for family in sorted(log_dict[aamode][profile][hat]['netdomain'].keys()): + # severity handling for net toggles goes here + for sock_type in sorted(log_dict[aaprofile][profile][hat]['netdomain'][family].keys()): + if profile_known_network(aa[profile][hat], family, sock_type): + continue + default_option = 1 + options = [] + newincludes = matchnetincludes(aa[profile][hat], family, sock_type) + q = hasher() + if newincludes: + options += map(lambda s: '#include <%s>'%s, sorted(set(newincludes))) + if options: + options.append('network %s %s' % (family, sock_type)) + q['options'] = options + q['selected'] = default_option - 1 + + q['headers'] = [gettext('Profile'), combine_name(profile, hat)] + q['headers'] += [gettext('Network Family'), family] + q['headers'] += [gettext('Socket Type'), sock_type] + + audit_toggle = 0 + q['functions'] = ['CMD_ALLOW', 'CMD_DENY', 'CMD_AUDIT_NEW', + 'CMD_ABORT', 'CMD_FINISHED'] + q['default'] = 'CMD_DENY' ### network rules should also allow globbing ### (not sure if "globbing" is the correct wording, but you should get the point) ### (most-permitting rule is just "network" without any additional parameters) #(r40) still valid
=== modified file 'Testing/severity_test.py' --- Testing/severity_test.py 2013-07-18 19:14:55 +0000 +++ Testing/severity_test.py 2013-07-19 22:49:07 +0000 ### BTW: ### # python3 severity_test.py ### severity_test.py:59: ResourceWarning: unclosed file <_io.BufferedReader name='severity_broken.db'> ### [...] === modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-07-17 23:59:54 +0000 +++ apparmor/aa.py 2013-07-19 22:49:07 +0000 @@ -434,40 +434,30 @@ hashbang = head(localfile) if hashbang.startswith('#!'): interpreter = get_full_path(hashbang.lstrip('#!').strip()) - try: - local_profile[localfile]['allow']['path'][interpreter]['audit'] |= 0 - except TypeError: - local_profile[localfile]['allow']['path'][interpreter]['audit'] = 0 + + local_profile[localfile]['allow']['path'][localfile]['mode'] = local_profile[localfile]['allow']['path'][localfile].get('mode', str_to_mode('r')) | str_to_mode('r') ### that looks already better than the try/except ### nevertheless it will cause some superfluous typing ### the ideal syntax would be something like ### local_profile.add('allow', 'path', '/bin/foo', 'r', no_audit) # no_audit could be an alias of "false" ### (then let all the magic happen inside local_profile, where you need the code only once) @@ -894,5 +885,183 @@ def handle_children(profile, hat, root): entries = root[:] ### the code uses entry[:3], entry[:5] etc. quite often, so a comment explaining the structure ### (or pointing to the explanation elsewhere) would be very useful for entry in entries: + elif type == 'unknown_hat': [...] + if ans == 'CMD_ADDHAT': + hat = uhat + aa[profile][hat]['flags'] = aa[profile][profile]['flags'] + elif ans == 'CMD_USEDEFAULT': + hat = default_hat + elif ans == 'CMD_DENY': + return None ### shouldn't CMD_DENY add a "deny" rule? (or is this handled elsewhere?) + elif type == 'path' or type == 'exec': [...] + # Give Execute dialog if x access requested for something that's not a directory + # For directories force an 'ix' Path dialog + do_execute = False + exec_target = detail + + if mode & str_to_mode('x'): + if os.path.isdir(exec_target): ### this test might fail if someone sends you his log and you try to update a profile based on the log ### (does the log contain directories as "/foo/bar/"? If yes, testing for the trailing "/" might work) + if mode & AA_MAY_LINK: + regex_link = re.compile('^from (.+) to (.+)$') ### I can imagine some funny filenames to break that regex ;-) ### for example "/home/cb/I'm from germany and go to the next train station.txt" ### (not sure if this is too relevant in real world, and what's the best way to prevent it) #(r40) still valid
=== modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-07-27 10:02:12 +0000 +++ apparmor/aa.py 2013-07-28 02:59:59 +0000 @@ -2264,3 +2302,678 @@ +def save_profiles(): + while ans != 'CMD_SAVE_CHANGES': + ans, arg = UI_PromptUser(q) + if ans == 'CMD_VIEW_CHANGES': + which = changed[arg] + oldprofile = serialize_profile(original_aa[which], which) + newprofile = serialize_profile(aa[which], which) + + display_changes(oldprofile, newprofile) + + for profile_name in changed_list: + writeprofile_ui_feedback(profile_name) + reload_base(profile_name) ### just an idea - does it make sense to allow saving only some of the changed profiles?
=== added file 'apparmor/ui.py' --- apparmor/ui.py 1970-01-01 00:00:00 +0000 +++ apparmor/ui.py 2013-07-17 15:08:13 +0000 @@ -0,0 +1,255 @@ +def init_localisation(): + locale.setlocale(locale.LC_ALL, '') + #cur_locale = locale.getlocale() + filename = 'res/messages_%s.mo' % locale.getlocale()[0][0:2] ### [0:2] means locales like pt_BR won't be used (only pt, which might be different). ### I doubt this is intentional. +def UI_YesNo(text, default): + if DEBUGGING: + debug_logger.debug('UI_YesNo: %s: %s %s' %(UI_mode, text, default)) + ans = default + if UI_mode == 'text': + yes = '(Y)es' + no = '(N)o' + usrmsg = 'PromptUser: Invalid hotkey for' + yeskey = 'y' + nokey = 'n' ### can you honor translations please? ### for example, in german it should be (j)a / (n)ein + sys.stdout.write('\n' + text + '\n') + if default == 'y': ### better: if default == yeskey - avoids problems when using translations + sys.stdout.write('\n[%s] / %s\n' % (yes, no)) + else: + sys.stdout.write('\n%s / [%s]\n' % (yes, no)) + ans = readkey() + if ans: + ans = ans.lower() + else: + ans = default + else: + SendDataTooYast({ + 'type': 'dialog-yesno', + 'question': text + }) + ypath, yarg = GetDataFromYast() + ans = yarg['answer'] + if not ans: + ans = default + return ans ### ans can be anything, not just yeskey and nokey ### if something else (= invalid key) is entered, the function should ask again ### (see UI_YesNoCancel which does exactly that) +def UI_YesNoCancel(text, default): + if DEBUGGING: + debug_logger.debug('UI_YesNoCancel: %s: %s %s' % (UI_mode, text, default)) + + if UI_mode == 'text': + yes = '(Y)es' + no = '(N)o' + cancel = '(C)ancel' + yeskey = 'y' + nokey = 'n' + cancelkey = 'c' ### should honor translations +CMDS = { + 'CMD_ALLOW': '(A)llow', + 'CMD_OTHER': '(M)ore', ### do all those options honor translations? === added file 'apparmor/yasti.py' --- apparmor/yasti.py 1970-01-01 00:00:00 +0000 +++ apparmor/yasti.py 2013-07-17 15:08:13 +0000 @@ -0,0 +1,82 @@ +def SendDataToYast(data): + for line in sys.stdin: [...] + Return(data) + return True ### two return statements? +def GetDataFromYast(): [...] + Return('true') + return ypath, yarg ### again - two return statements? +def ParseTerm(input): [...] + ycp.y2error('No term parantheses') ### typo: par_e_ntheses #(r40) still valid
=== modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-07-19 22:49:07 +0000 +++ apparmor/aa.py 2013-07-22 23:05:51 +0000 @@ -449,7 +450,7 @@ + # Don't allow hats to cx? + options.replace('c', '') ### child profiles can't be nested, but one child profile can call another ### at the moment the workaround is to do (assuming we are in /bin/foo//bin/bar child profile) "Px -> /bin/foo//bin/baz" ### having an option "use another child profile" (which makes clear it won't be nested as /bin/foo//bin/bar//bin/baz) would be nice + # Add deny to options + options += 'd' + # Define the default option + default = None + if 'p' in options and os.path.exists(get_profile_filename(exec_target)): + default = 'CMD_px' ### while we are at it - displaying a note "target profile exists" would be helpful + elif ans == 'CMD_ux': + exec_mode = str_to_mode('ux') + ynans = UI_YesNo(gettext('Launching processes in an unconfined state is a very\n' + + 'dangerous operation and can cause serious security holes.\n\n' + + 'Are you absolutely certain you wish to remove all\n' + + 'AppArmor protection when executing :') + '%s ?' % exec_target, 'n') ### even if this question is absolutely correct, a "never ask again" option would be welcome ;-) #... ### that all said - this is at least the second time where the code contains interpreter <-> abstraction information ### better store it at one place - for example like this: ### def abstraction_for_interpreter(interpreter) ### # TODO: interpreter_basename = remove ^(/usr)?/bin/ from interpreter ### if interpreter_basename == 'bash' ### return 'abstractions/bash' ### elif interpreter_basename == 'perl' ### [...] #(r40) still valid
=== 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 @@ + def test_string_to_modes(self): + #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) + 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 * #(r40) still wrong, it should be .* instead of .+ (for empty "flags=()") + 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_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_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 #... ### 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) ### affects: ### - elif RE_PROFILE_CAP.search(line): ### - elif RE_PROFILE_PATH_ENTRY.search ### - elif RE_PROFILE_NETWORK.search(line): ... + 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 ;-) ### 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 ;-) + # 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() +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' #(r40) should probably be 'audit ' (space added) ### final note about all write_* functions: ### it looks like inline comments are always dropped, which is not a good idea #(r40) still valid
### general note: ### "sorted()" marks places that need changes to keep the profile order when writing === modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-07-30 14:43:08 +0000 +++ apparmor/aa.py 2013-07-31 14:26:33 +0000 @ -3495,7 +3496,7 @@ 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,') + data.append('%s%s%scapability %s,' %(pre, audit, allowstr)) ### looks more correct than the previous version in r30 ;-) ### but it doesn't handle the general "capability," rule @@ -3506,3 +3507,546 @@ +def write_net_rules(prof_data, depth, allow): + pre = ' ' * depth + data = [] + allowstr = set_allow_str(allow) + + if prof_data[allow].get('netdomain', False): + if prof_data[allow]['netdomain'].get('rule', False) == 'all': + if prof_data[allow]['netdomain']['audit'].get('all', False): + audit = 'audit ' + data.append('%s%snetwork,' %(pre, audit)) + else: + for fam in sorted(prof_data[allow]['netdomain']['rule'].keys()): ### sorted() + if prof_data[allow]['netdomain']['rule'][fam] == True: + if prof_data[allow]['netdomain']['audit'][fam]: + audit = 'audit' + data.append('%s%s%snetwork %s' % (pre, audit, allowstr, fam)) + else: + for typ in sorted(prof_data[allow]['netdomain']['rule'][fam].keys()): ### sorted() +def write_path_rules(prof_data, depth, allow): + pre = ' ' * depth + data = [] + allowstr = set_allow_str(allow) + + if prof_data[allow].get('path', False): + for path in sorted(prof_data[allow]['path'].keys()): ### sorted() ... + user, other = split_mode(mode) + user_audit, other_audit = split_mode(audit) ### would it make sense to split 'mode' into 'ownermode' and 'othermode'? ### that would avoid the need for split_mode later ### (this also implies large code changes, so think about it for some days before deciding if you want to do it ;-) +def write_piece(profile_data, depth, name, nhat, write_flags): + pre = ' ' * depth + data = [] + wname = None + inhat = False + if name == nhat: + wname = name + else: + wname = name + '//' + nhat + name = nhat + inhat = True + + data += write_header(profile_data[name], depth, wname, False, write_flags) + data += write_rules(profile_data[name], depth+1) + + pre2 = ' ' * (depth+1) + # External hat declarations + for hat in filter(lambda x: x != name, sorted(profile_data.keys())): ### sorted() + if profile_data[hat].get('declared', False): + data.append('%s^%s,' %(pre2, hat)) + + if not inhat: + # Embedded hats + for hat in filter(lambda x: x != name, sorted(profile_data.keys())): ### sorted() + if not profile_data[hat]['external'] and not profile_data[hat]['declared']: + data.append('') + if profile_data[hat]['profile']: + data += map(str, write_header(profile_data[hat], depth+1, hat, True, write_flags)) + else: + data += map(str, write_header(profile_data[hat], depth+1, '^'+hat, True, write_flags)) + + data += map(str, write_rules(profile_data[hat], depth+2)) + + data.append('%s}' %pre2) + + data.append('%s}' %pre) + + # External hats + for hat in filter(lambda x: x != name, sorted(profile_data.keys())): ### sorted() + if name == nhat and profile_data[hat].get('external', False): + data.append('') + data += map(lambda x: ' %s' %x, write_piece(profile_data, depth-1, name, nhat, write_flags)) + data.append(' }') + + return data + +def write_profile(profile): + filename = None + if aa[profile][profile].get('filename', False): + filename = aa[profile][profile]['filename'] + else: + filename = get_profile_filename(profile) + + newprof = tempfile.NamedTemporaryFile('rw', suffix='~' ,delete=False) + if os.path.exists(filename): + shutil.copymode(filename, newprof.name) + else: + #permission_600 = stat.S_IRUSR | stat.S_IWUSR # Owner read and write + #os.chmod(newprof.name, permission_600) + pass + + serialize_options = {} + serialize_options['METADATA'] = True + + profile_string = serialize_profile(aa[profile], profile, serialize_options) + newprof.write(profile_string) ### check for errors? + newprof.close() + os.rename(newprof.name, filename) ### check for errors? +def netrules_access_check(netrules, family, sock_type): + if not netrules: + return 0 + all_net = False + all_net_family = False + net_family_sock = False + if netrules['rule'].get('all', False): + all_net = True + if netrules['rule'].get(family, False) == True: + all_net_family = True + if (netrules['rule'].get(family, False) and + type(netrules['rule'][family]) == dict and + netrules['rule'][family][sock_type]): + net_family_sock = True + + if all_net or all_net_family or net_family_sock: + return True + else: + return False ### instead of using some variables, you could just "return True" on first match ### and "return False" at the end of the function ### similar to profile_known_network() does it +def reload_base(bin_path): + if not check_for_apparmor(): + return None + + filename = get_profile_filename(bin_path) + subprocess.call("cat '%s' | %s -I%s -r >/dev/null 2>&1" %(filename, parser ,profile_dir), shell=True) ### useless use of cat - apparmor_parser can read the file itsself ;-) ### besides that - what happens with crazy filenames like /etc/apparmor.d/bin.pi'ng ? ;-) ### hint: create-apparmor.vim.py uses ### (rc, output) = cmd(['make', '-s', '--no-print-directory', 'list_capabilities']) ### if rc != 0: ### sys.stderr.write("make list_capabilities failed: " + output) ### exit(rc) ### which should solve all problems with funny filenames ;-) +def reload(bin_path): + bin_path = find_executable(bin_path) + if not bin: + return None + + return reload_base(bin_path) ### does this mean the profile only gets reloaded if the binary exists? ### doesn't sound like the best idea... +def loadincludes(): + incdirs = get_subdirectories(profile_dir) + + for idir in incdirs: + if is_skippable_dir(idir): + continue + for dirpath, dirname, files in os.walk(profile_dir + '/' + idir): + if is_skippable_dir(dirpath): + continue + for fi in files: + if is_skippable_file(fi): + continue + else: + load_include(dirpath + '/' + fi) ### I remember some checks that only allow abstractions/ - and now we check nearly every dir for possible includes? ### strange[tm] +def glob_common(path): + globs = [] + + if re.search('[\d\.]+\.so$', path) or re.search('\.so\.[\d\.]+$', path): ### [\d\.] shoud probably be [\d.] + libpath = path + libpath = re.sub('[\d\.]+\.so$', '*.so', libpath) + libpath = re.sub('\.so\.[\d\.]+$', '.so.*', libpath) + if libpath != path: + globs.append(libpath) + + for glob in cfg['globs']: + if re.search(glob, path): + globbedpath = path + globbedpath = re.sub(glob, cfg['globs'][glob]) + if globbedpath != path: + globs.append(globbedpath) + + return sorted(set(globs)) ### does sorted() make sense? +def matchregexp(new, old): + if re.search('\{.*(\,.*)*\}', old): ### this regex can be shortened to '\{.*\}' without changing the meaning (the .* matches the removed part) ### (but I'd guess there must be a reason why (\,.*)* was there, so better check if it's buggy - maybe )* should be )+ or something like that?) + return None + + if re.search('\[.+\]', old) or re.search('\*', old) or re.search('\?', old): + + if re.search('\{.*\,.*\}', new): + pass + +######Initialisations###### + +conf = apparmor.config.Config('ini') +cfg = conf.read_config('logprof.conf') + +if cfg['settings'].get('default_owner_prompt', False): + cfg['settings']['default_owner_prompt'] = False + +profile_dir = conf.find_first_dir(cfg['settings']['profiledir']) or '/etc/apparmor.d' +if not os.path.isdir(profile_dir): + raise AppArmorException('Can\'t find AppArmor profiles' ) ### I'd make the error message more verbose and add profile_dir (and maybe also a hint to the profiledir config option in logprof.conf) ### besides that, changing the profile_dir with a commandline argument should be possible +parser = conf.find_first_file(cfg['settings']['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') ### I'd also check for /usr/sbin/apparmor_parser (not sure if we should do it here or in the default logprof.conf) ### The error message should mention logprof.conf / option parser +filename = conf.find_first_file(cfg['settings']['logfiles']) or '/var/log/syslog' +if not os.path.isfile(filename): + raise AppArmorException('Can\'t find system log.') + +ldd = conf.find_first_file(cfg['settings']['ldd']) or '/usr/bin/ldd' +if not os.path.isfile(ldd) or not os.access(ldd, os.EX_OK): + raise AppArmorException('Can\'t find ldd') + +logger = conf.find_first_file(cfg['settings']['logger']) or '/bin/logger' +if not os.path.isfile(logger) or not os.access(logger, os.EX_OK): + raise AppArmorException('Can\'t find logger') ### same for logfiles, ldd and logger - the error message should mention logprof.conf and the config option #(r40) still valid vim:ft=diff
------------------------------------------------------------ revno: 40 committer: Kshitij Gupta <kgupta8...@gmail.com branch nick: apparmor-profile-tools timestamp: Sat 2013-08-10 12:46:22 +0530 message: fixes from rev 32..39 and fixed(?) flags=(..)thing === modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-08-09 11:19:01 +0000 +++ apparmor/aa.py 2013-08-10 07:16:22 +0000 @@ -1751,6 +1748,7 @@ def ask_the_questions(): found = None + print(log_dict) # debug code? for aamode in sorted(log_dict.keys()): # Describe the type of changes if aamode == 'PERMITTING': vim:ft=diff
------------------------------------------------------------ revno: 45 revno: 44 revno: 43 revno: 42 revno: 41 ------------------------------------------------------------ === modified file 'Testing/aa_test.py' --- Testing/aa_test.py 2013-08-09 11:19:01 +0000 +++ Testing/aa_test.py 2013-08-11 13:00:01 +0000 def test_modes_to_string(self): - MODE_TEST = {'x': apparmor.aa.AA_MAY_EXEC, ... + MODE_TEST = {'x': apparmor.aamode.AA_MAY_EXEC, ... def test_string_to_modes(self): - MODE_TEST = {'x': apparmor.aa.AA_MAY_EXEC, ... + MODE_TEST = {'x': apparmor.aamode.AA_MAY_EXEC, # I still think test_modes_to_string() and test_string_to_modes() should share MODE_TEST ;-) === modified file 'apparmor/aa.py' --- apparmor/aa.py 2013-08-10 07:16:22 +0000 +++ apparmor/aa.py 2013-08-11 17:46:05 +0000 @@ -3831,6 +3284,7 @@ def get_include_data(filename): data = [] + filename = profile_dir + '/' + filename if os.path.exists(filename): # includes do not have to be inside the profile_dir # see the IRC log at the end of this file for details # short version: there's also # #include foo - relative to base dir (typically /etc/apparmor.d) # #include /path/to/foo - absolute path # (both with optional quotes around the filename) === added file 'apparmor/aamode.py' --- apparmor/aamode.py 1970-01-01 00:00:00 +0000 +++ apparmor/aamode.py 2013-08-11 17:46:05 +0000 @@ -0,0 +1,268 @@ +AA_EXEC_INHERIT = set('i') +AA_EXEC_UNCONFINED = set('U') +AA_EXEC_PROFILE = set('P') +AA_EXEC_CHILD = set('C') # I'd directly use ix/Ux/Px/Cx, so you don't need to always add AA_MAY_EXEC +AA_EXEC_NT = set('N') # similar problem as with 'z' #[was:] 'z' is currently unused, but nevertheless there's a small risk here (it will break if we ever introduce 'z' permissions, whatever that will be) === modified file 'apparmor/common.py' --- apparmor/common.py 2013-08-10 07:16:22 +0000 +++ apparmor/common.py 2013-08-11 17:46:35 +0000 @@ -194,13 +195,21 @@ self.debug_level = logging.DEBUG if os.getenv('LOGPROF_DEBUG', False): self.debugging = os.getenv('LOGPROF_DEBUG') + try: + self.debugging = int(self.debugging) + except: + self.debugging = False + if self.debugging not in range(1,4): + sys.stderr.out('Environment Variable: LOGPROF_DEBUG contains invalid value: %s' %os.getenv('LOGPROF_DEBUG')) # I'd also allow 0 (for people who fail to unset the variable and set it to 0 instead) ;-) === added file 'apparmor/logparser.py' --- apparmor/logparser.py 1970-01-01 00:00:00 +0000 +++ apparmor/logparser.py 2013-08-11 09:52:07 +0000 @@ -0,0 +1,379 @@ + def add_to_tree(self, loc_pid, parent, type, event): + self.debug_logger.info('add_to_tree: pid [%s] type [%s] event [%s]' % (loc_pid, type, event)) + if not self.pid.get(loc_pid, False): + profile, hat = event[:2] + if parent and self.pid.get(parent, False): + if not hat: + hat = 'null-complain-profile' + arrayref = [] + self.pid[parent].append(arrayref) + self.pid[loc_pid] = arrayref + for ia in ['fork', loc_pid, profile, hat]: + arrayref.append(ia) +# self.pid[parent].append(array_ref) +# self.pid[loc_pid] = array_ref # so you add empty arrayref to self.pid[parent] and set self.pid[loc_pid] = empty arrayref # and then fill arrayref, but don't use it (code commented out) + else: + arrayref = [] + self.log.append(arrayref) + self.pid[loc_pid] = arrayref +# self.log.append(array_ref) +# self.pid[loc_pid] = array_ref # same for arrayref here + def add_event_to_tree(self, e): ... ... + elif e['operation'] == 'clone': + parent , child = e['pid'], e['task'] + if not parent: + parent = 'null-complain-profile' + if not hat: + hat = 'null-complain-profile' #(removed lines are from aa.py) - arrayref = ['fork', child, profile, hat] - if pid.get(parent, False): - pid[parent].append(arrayref) - else: - log += [arrayref] - pid[child] = arrayref + arrayref = [] + if self.pid.get(parent, False): + self.pid[parent].append(arrayref) + else: + self.log.append(arrayref) + self.pid[child].append(arrayref) # hmm, appending empty arrayref (at various places)? + for ia in ['fork', child, profile, hat]: + arrayref.append(ia) # now you fill arrayref, but the code actually using it is disabled: +# if self.pid.get(parent, False): +# self.pid[parent] += [arrayref] +# else: +# self.log += [arrayref] +# self.pid[child] = arrayref + def profile_exists(self, program): + """Returns True if profile exists, False otherwise""" + # Check cache of profiles + if self.existing_profiles.get(program, False): + return True + # Check the disk for profile + prof_path = self.get_profile_filename(program) + #print(prof_path) + if os.path.isfile(prof_path): # might lead to wrong results if a profile has a non-default filename - but OTOH you already checked the cache, so this shouldn't happen # (might be worth to add a "please mail me if you see this" message ;-) + # Add to cache of profile + self.existing_profiles[program] = True + return True + return False ############################################################################################################# IRC log about #include (starting 2013-08-11, times are CEST) [23:30:40] <cboltz> is it possible/allowed to include a file outside the profile dir? [23:30:54] <cboltz> something like #include </home/cb/foo> ? [23:43:56] <jjohansen> cboltz: #include /home/cb/foo [23:44:57] <cboltz> Kshitij will not like that ;-) - in one of his last commits, he blindly prepends profile_dir ;-) [00:04:59] <jjohansen> err is he aware that the search path can be multiple dirs [00:05:45] <jjohansen> #include "foo" is include from current dir [00:05:45] <jjohansen> #include <foo> search through search path [00:06:05] <jjohansen> #include foo include specified file [00:06:42] <jjohansen> but the parser allows you to override the default search location and specify multiple search locations with -I [00:08:53] * cboltz adds a note to the review [00:10:18] <darix> jjohansen: what is the difference between the first and the last include? [00:10:57] <darix> ah i see [00:12:15] <jjohansen> darix: hrmm looking at it nothing really, there are 3 cases in the parser code, but the one path works out to be stripping the quotes off [00:12:54] <jjohansen> so "" will let you have spaces in your file name [00:13:38] <darix> jjohansen: so i cant have a specific filename with spaces [00:13:38] <jjohansen> ha no it won't, the lex regex doesn't allow it O_o [00:13:55] <jjohansen> darix: well you should be able to [00:13:57] <darix> if "" is tasked for "include from current dir" [00:14:05] <darix> jjohansen: with '\ ' [00:14:06] <darix> ? [00:14:52] <jjohansen> darix: with and without quotes are the same backend code and just try to open the file, if you use an abs path or a relative path it should work [00:15:00] <jjohansen> darix: ? [00:15:11] <darix> jjohansen: i just try to understand yout explaination. :) [00:15:21] <darix> i didnt try anything [00:16:03] <cboltz> I'd guess [00:16:12] <jjohansen> darix: there is a bug, the lex code that matches the #include "file" doesn't allow spaces in file, even though its clear that the quotes are there to allow that [00:16:13] <cboltz> #include foo include from current dir [00:16:25] <cboltz> #include /path/to/foo include from absolute path [00:16:34] <jjohansen> cboltz: that would have to be in quotes, otherwise its treated as separate tokens [00:17:13] <cboltz> the foo or the /path/to/foo? or both? [00:18:29] <jjohansen> cboltz: oh no, sorry neither what I meant was either of those if the path was to contain any white space would need to be in quotes [00:18:57] <jjohansen> that is all the quotes are supposed to do is allow for whitespace [00:19:13] <jjohansen> but there is a bug in the flex code currently [00:24:01] <darix> jjohansen: so we basically have 2 cases [00:24:06] <darix> <foo> => searchpath [00:24:53] <darix> foo or "foo" read file foo. if no "/" in the path it is read from the current directory. [00:32:20] <jjohansen> darix: yes [00:32:36] <jjohansen> sorry for the confusion [00:37:56] <cboltz> #include foo really reads from the current directory (whereever that is)? [00:38:32] <cboltz> if it really works that way, it doesn't sound too useful (because it depends on the working directory) and might be worth a warning in the parser [00:38:42] <cboltz> (absolute paths are a different thing, of course) [01:03:21] <darix> cboltz: well you would expect the parser to chdir to /etc/apparmor.d [01:03:53] <cboltz> maybe, but then it would be the same as #include <foo> [01:04:05] <darix> naw [01:04:13] <darix> cboltz: search path can be changed [01:04:24] <darix> and <> goes through the search path [01:04:35] <darix> that /etc/apparmor.d/ is in the search path is just a coincidence [01:04:48] <cboltz> ok, that's a difference (in theory) [01:05:11] <cboltz> I'd guess in practise most people have only /etc/apparmor.d/ in their search path [01:11:24] <jjohansen> cboltz: the parser chdirs to a base directory at its start and it also chdirs while working through the search path [01:12:05] <jjohansen> so " " starts out relative to the base dir or profile, and if you are in the search dir is still correctly relative [01:12:14] <jjohansen> it doesn't use cwd [01:13:06] <cboltz> what/where is the "base dir"? hardcoded /etc/apparmor.d/? [01:13:23] <cboltz> what/where is the "base dir"? hardcoded /etc/apparmor.d/? [01:13:55] <jjohansen> the default base dir is /etc/apparmor.d/ [01:14:23] <cboltz> ok, makes sense [01:15:43] <jjohansen> cboltz: you can set the base dir with -b [01:16:00] <jjohansen> or base in the parser.conf file [01:16:13] <cboltz> ok [01:16:33] <cboltz> looks like this makes handling #include quite interesting ;-) [01:17:02] <cboltz> I'll paste the whole log into the review so that Kshitij can sort it out ;-) ############################################################################################################# vim:ft=diff
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor