[apparmor] Fwd: GSoC r13, r14, r15 review
Kshitij Gupta
kgupta8592 at gmail.com
Mon Jul 8 20:11:32 UTC 2013
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 <apparmor at 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
More information about the AppArmor
mailing list