Hello, Those were a few lenghty commits, great to hear you worked on them. Thanks. :-)
On Mon, Jul 8, 2013 at 5:40 AM, Christian Boltz <appar...@cboltz.de> wrote: > Hello, > > I just reviewed the changes from r13, r14 and r15. See the attached > file - I added several inline comments in the diff. > Response is embedded below. :-) > > Besides that, I got a nice traceback: > > # python3 severity_test.py > E. > ====================================================================== > ERROR: testInvalid (__main__.Test) > ---------------------------------------------------------------------- > Traceback (most recent call last): > File "../apparmor/severity.py", line 76, in __init__ > resource, severity = line.split() > ValueError: need more than 1 value to unpack > > During handling of the above exception, another exception occurred: > > Traceback (most recent call last): > File "severity_test.py", line 20, in testInvalid > broken = severity.Severity('severity_broken.db') > File "../apparmor/severity.py", line 79, in __init__ > raise AppArmorException("No severity value present in file: > %s\n\t[Line %s]: %s" % (dbname, lineno, line)) > apparmor.common.AppArmorException: 'No severity value present in file: > severity_broken.db\n\t[Line 14]: CAP_SYS_MODULE' > > ---------------------------------------------------------------------- > Ran 2 tests in 0.541s > > FAILED (errors=1) > > I somehow doubt this is intentional - the test should catch this > exception ;-) > It was inside the try except block of test, I got it out to try how exception looked. Turns out \n\t aren't working yet. :-\ > Also note that the last line contains \n\t - this should become a real > line break and tab in the output... > Response to review of r13, r14 and r15, removed the fixed bugs from the diff. My responses begin with ##### hope they're not hard to find ;-) === 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 +# Setup logging incase of debugging is enabled +if os.getenv('LOGPROF_DEBUG', False): + import logging, atexit + DEBUGGING = True + logprof_debug = os.environ['LOGPROF_DEBUG'] + logging.basicConfig(filename=logprof_debug, level=logging.DEBUG) ### taking the filename from an env variable? Not sure if this is a good idea security-wise... ##### The user (not a casual user ofcourse) would set an environment variable if debugging, ##### Any other you'd want to set debugging? +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) ##### Unused but only as of now! ;-) +unimplemented_warning = False ##### Some variables are there just as placeholder for later to avoid corrupting the global namespace later ### 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 +def check_for_LD_XXX(file): + """Returns True if specified program contains references to LD_PRELOAD or + LD_LIBRARY_PATH to give the PX/UX code better suggestions""" ### the x in Px/Ux is always lowercase ##### Fixed + found = False + if not os.path.isfile(file): + return False + size = os.stat(file).st_size + # Limit to checking files under 10k for the sake of speed ### hmm, performance vs. reliable results. ### what about a higher limit? ##### I'm open to suggestions for a higher value :-) + if size >10000: + return False + with open_file_read(file) as f_in: + for line in f_in: + if 'LD_PRELOAD' in line or 'LD_LIBRARY_PATH' in line: + found = True + return found +def get_full_path(original_path): + """Return the full path after resolving any symlinks""" + path = original_path + link_count = 0 + if not '/' in path: + path = os.getcwd() + '/' + path ### this might fail with things like "./foo" (contains / and therefore will not be converted to an absolute path) ### - maybe use if not path.startswith('/'): ### - also cleanup the resulting path - /bin/../bin/ping doesn't look nice ### (os.path.abspath() might help) ##### ./ point ##### The cleanup of resulting path exists with the return statement via os.path.realpath() + while os.path.islink(path): + link_count += 1 + if link_count > 64: + fatal_error("Followed too many links while resolving %s" % (original_path)) + direc, file = os.path.split(path) + link = os.readlink(path) + # If the link an absolute path + if link.startswith('/'): + path = link + else: + # Link is relative path + path = direc + '/' + link + return os.path.realpath(path) +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 ##### It would be interesting to have custom profile names, storing/using them. ##### Tell me where you'll store them and I'll fetch them ;-) +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) ##### Look above +def enforce(path): + """Sets the profile to complain mode if it exists""" + filename, name = name_to_prof_filename(path) ### same here ##### Look above +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?) ##### os.path.isfile() checks that and returns an empty line, maybe a warning could be logged. :) +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? ##### The program's output needs to be utf-8 encoded else they get some weird stuff, on encoding look below. + 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. :-) +def handle_binfmt(profile, fqdbin): + reqs = dict() + \ No newline at end of file ### nitpicking - the last line contains superfluous spaces ##### I had a system crash so it was an abrupt push, there's plenty of code to follow. :-) === 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? ##### From aa-enforce, havent used it yet. However, utf-8 is default for Python3, so maybe we can use it? +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? ##### Again from aa-enforce, and we can discuss using utf-8 or system locale. === 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. :-) 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 ##### Same as above vim:ft=diff -- AppArmor mailing list AppArmor@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/apparmor