[apparmor] GSoC r13, r14, r15 review
Christian Boltz
apparmor at cboltz.de
Mon Jul 8 00:10:55 UTC 2013
Hello,
I just reviewed the changes from r13, r14 and r15. See the attached
file - I added several inline comments in the diff.
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 ;-)
Also note that the last line contains \n\t - this should become a real
line break and tab in the output...
Regards,
Christian Boltz
--
[T-Shirt-Druck] Die meisten Leute haben bedauerlicherweise ein
ImageMagick mit 16-Bit Farbtiefe, da sind 300x300/16 Bit in "Bauchgröße"
schon eine beträchtliche Menge. Hmmm... he, so kann man endlich Kalorien
in Megabytes umrechnen! :-) [Ratti in suse-linux]
-------------- next part --------------
review of r13, r14 and r15
=== modified file 'Testing/severity_test.py'
+sys.path.append('../')
+sys.path.append('../apparmor')
The path "../apparmor" looks superfluous.
=== 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...
+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?
+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
+ 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?
+ 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 fatal_error(message):
+ if DEBUGGING:
+ # Get the traceback to the message
+ tb_stack = traceback.format_list(traceback.extract_stack())
+ tb_stack = ''.join(tb_stack)
+ # Append the traceback to message
+ message = message + '\n' + tb_stack
+ debug_logger.error(message)
+ caller = inspect.stack()[1][3]
+
+ # If caller is SendDataToYast or GetDatFromYast simply exit
+ sys.exit(1)
### doesn't match the comment - it will always exit
### therefore the lines below are never reached
+ # Else tell user what happened
+ UI_Important(message)
+ shutdown_yast()
+ sys.exit(1)
+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)
+ 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 find_executable(bin_path):
+ """Returns the full executable path for the binary given, None otherwise"""
+ full_bin = None
+ if os.path.exists(bin_path):
+ full_bin = get_full_path(bin_path)
+ else:
+ if '/' not in bin:
+ env_bin = which(bin)
### where does "bin" come from? (wrong variable name?)
+ if env_bin:
+ full_bin = get_full_path(env_bin)
+ if full_bin and os.path.exists(full_bin):
+ return full_bin
+ return None
+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
+def name_to_prof_filename(filename):
+ """Returns the profile"""
+ if bin.startswith(profile_dir):
### again, where does "bin" come from?
+ profile = filename.split(profile_dir, 1)[1]
+ return (filename, profile)
+ else:
+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
+def handle_binfmt(profile, fqdbin):
+ reqs = dict()
+
\ No newline at end of file
### nitpicking - the last line contains superfluous spaces
=== 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?
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
=== apparmor/config.py
+class Config:
+ def __init__(self, conf_type):
+ # The type of config file that'll be read and/or written
+ if conf_type != 'shell' or conf_type != 'ini':
### "not shell OR not ini" is always true - shouldn't this be "not shell AND not ini"?
###
### the better way is:
### if conf_type == 'shell' or conf_type == 'ini':
### self.conf_type = conf_type
### self.input_file = None
### else:
### raise AppArmorException("Unknown configuration file type")
-def find_first_file(file_list):
+ def find_first_file(self, file_list):
"""Returns name of first matching file None otherwise"""
- # I don't understand why it searches the CWD, maybe I'll find out about it in some module
filename = None
- if len(file_list):
+ if filename:
### doesn't this change mean the remaining part of the function is dead code?
### (filename is always None)
vim:ft=diff
More information about the AppArmor
mailing list