[apparmor] GSoC review r41..45
Christian Boltz
apparmor at cboltz.de
Sun Aug 11 23:50:16 UTC 2013
Hello,
the review for r41..45 is attached (merged into one review).
BTW: Following the moved code was quite interesting[tm], but still
easier than completely reviewing the new aamode.py and logparser.py ;-)
Regards,
Christian Boltz
--
>Seit einiger Zeit ist ftp://mirrors.mathematik.uni-bi*l*f*ld.de down
Offenbar. [...] Aber warum machst du dir Sorgen? Bi*l*f*ld existiert
nicht, wieso sollte es dort dann ein Ftp-Server geben?
[> Al Bogner und David Haller in suse-linux]
-------------- next part --------------
------------------------------------------------------------
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
@@ -244,7 +184,7 @@
return os.path.realpath(path)
def find_executable(bin_path):
- """Returns the full executable path for the binary given, None otherwise"""
+ """Returns the full executable path for the given, None otherwise"""
# was this comment change intentional?
# (hmm, maybe "Returns the full path for the given executable given, None otherwise"?)
@@ -3296,7 +2747,7 @@
profile_data[profile][hat]['initial_comment'] = initial_comment
initial_comment = ''
if filelist[file]['profiles'][profile].get(hat, False):
- raise AppArmorException('Error: Multiple definitions for hat %s in profile %s.' %(hat, profile))
+ pass#raise AppArmorException('Error: Multiple definitions for hat %s in profile %s.' %(hat, profile))
filelist[file]['profiles'][profile][hat] = True
# huh? what's wrong with raising an exception here?
@@ -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_MAY_EXEC = set('x')
...
+AA_EXEC_UNSAFE = set('z')
# '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)
+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'
+AA_LINK_SUBSET = set('ls')
# similar problem (for the 's' part) as with 'z'
=== 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 parse_event(self, msg):
...
+ # Map c (create) to a and d (delete) to w, logprof doesn't support c and d
+ if rmask:
+ rmask = rmask.replace('c', 'a')
+ rmask = rmask.replace('d', 'w')
+ if not validate_log_mode(hide_log_mode(rmask)):
+ pass#fatal_error(_('Log contains unknown mode %s') % rmask)
# I'd at least make that a debug notice or warning
+ if dmask:
+ dmask = dmask.replace('c', 'a')
+ dmask = dmask.replace('d', 'w')
+ if not validate_log_mode(hide_log_mode(dmask)):
+ pass #fatal_error(_('Log contains unknown mode %s') % dmask)
# I'd at least make that a debug notice or warning
+ 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):
...
+ # Convert new null profiles to old single level null profile
+ if '//null-' in e['profile']:
+ e['profile'] = 'null-complain-profile'
+
+ profile = e['profile']
+ hat = None
+
+ if '//' in e['profile']:
+ profile, hat = e['profile'].split('//')[:2]
# this is "some lines above" (see below)
+ # Filter out change_hat events that aren't from learning
+ if e['operation'] == 'change_hat':
+ if aamode != 'HINT' or aamode != 'PERMITTING':
+ return None
# (was "and" before)
# with "or", this will always match (and always return None)
# (you might want to use if aamode not in ... to make it easier to read)
+ profile = e['name']
+ #hat = None
+ if '//' in e['name']:
+ profile, hat = e['name'].split('//')
# some lines above, you added [:2] - is this needed here too?
...
+ 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
More information about the AppArmor
mailing list