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
