Hello, see the attachment for the r24 review.
Regards, Christian Boltz -- <Dominian> There is always room for improvement <Dominian> to seek perfection is to drive yourself insane. <Dominian> except suseROCKs, he's already insane. [from #opensuse-project]
=== 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 @@ local_profile[localfile]['include']['abstractions/python'] = True elif 'ruby' in interpreter: ### hmm, just using "ruby" might lead to false positives ### better check for the full path or at least the full basename local_profile[localfile]['include']['abstractions/ruby'] = True - elif interpreter in ['/bin/bash', '/bin/dash', '/bin/sh']: + elif re.search('/bin/(bash|dash|sh)', interpreter): ### if you use a regex, don't forget ^...$ ;-) + # If profiled program executes itself only 'ix' option + if exec_target == profile: + options = 'i' ### ix is a good default, but there may be (rare) cases where a program executes itsself with different options etc. ### so I'd still allow the other x options + # 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 ;-) + if exec_mode & str_to_mode('i'): + if 'perl' in exec_target: + aa[profile][hat]['include']['abstractions/perl'] = True ### oh, does executing /bin/perlentaucher need abstractions/perl? ### (the code will happily add it ;-) + elif '/bin/bash' in exec_path or '/bin/sh' in exec_path: ### similar question here for /usr/bin/bin/shorewall ;-) ### (in other words: using "in" can have quite some side effects) + if 'perl' in interpreter: + aa[profile][hat]['include']['abstractions/perl'] = True + elif '/bin/bash' in interpreter or '/bin/sh' in interpreter: + aa[profile][hat]['include']['abstractions/bash'] = True ### some more "in"... ### 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' ### [...] === modified file 'apparmor/severity.py' --- apparmor/severity.py 2013-07-19 22:49:07 +0000 +++ apparmor/severity.py 2013-07-22 23:05:51 +0000 @@ -184,7 +184,7 @@ def load_variables(self, prof_path): """Loads the variables for the given profile""" - regex_include = re.compile('include <(\S*)>') + regex_include = re.compile('^#?include\s*<(\S*)>') ### nearly correct ;-) - you should allow whitespace at the beginning of the line if os.path.isfile(prof_path): with open_file_read(prof_path) as f_in: for line in f_in:
-- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor