[apparmor] GSoC - remaining parts of old reviews
Christian Boltz
apparmor at cboltz.de
Tue Aug 13 22:02:20 UTC 2013
Hello,
the attached files contain the remaining parts of old reviews.
I always remove things that are done, and also checked the remaining
parts in the r40 code (that's why most files have "checked-in-r40") -
but even with this suffix, they should be up to date with r47.
I hope the number of files doesn't shock you too much - most of the
files are quite small ;-)
If you have questions or think some things need to be discussed, just
ask ;-)
Regards,
Christian Boltz
--
und *echte* Männer benutzen Linux -- wegen der langen Kommandozeilen
("Meine ist länger als deine!"). Dann muss man nicht mehr Krieg spielen,
um zu zeigen, wie hart man ist. [S. Lauterkorn in suse-talk]
-------------- next part --------------
review of r13, r14 and r15
=== 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
+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?
##### They basically contain the list of profiles present version and
##### the one at the time of loading
##### ### Nice, but the variable name should represent that. What about profile_list and original_profile_list?
+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
#(r45) get_profile_filename is now in logparser.py
+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
##### The setter method is yet to come. ;-)
##### Will fix it when refractoring the module later as I planned. :-)
=== 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?
##### The variable finder method (which is postponed) will fix it. :-)
#(r40) line number was added, filename still missing
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
##### The variable finder method (which is postponed) will fix it. :-)
#(r40) line number was added, filename still missing
#(r40) still valid
vim:ft=diff
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r16___checked-in-r40
Type: text/x-patch
Size: 1143 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0008.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r26-27___checked-in-r40
Type: text/x-patch
Size: 2277 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0009.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r23___checked-in-r40
Type: text/x-patch
Size: 3107 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0010.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r28-29___checked-in-r40
Type: text/x-patch
Size: 842 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0011.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r17___checked-in-r40
Type: text/x-patch
Size: 2717 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0012.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r24___checked-in-r40
Type: text/x-patch
Size: 2180 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0013.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r30___checked-in-r40
Type: text/x-patch
Size: 7472 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0014.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: review-r31___checked-in-r40
Type: text/x-matlab
Size: 10233 bytes
Desc: not available
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20130814/3670178f/attachment-0015.bin>
-------------- next part --------------
------------------------------------------------------------
revno: 40
committer: Kshitij Gupta <kgupta8592 at gmail.com
branch nick: apparmor-profile-tools
timestamp: Sat 2013-08-10 12:46:22 +0530
message:
fixes from rev 32..39 and fixed(?) flags=(..)thing
=== modified file 'apparmor/aa.py'
--- apparmor/aa.py 2013-08-09 11:19:01 +0000
+++ apparmor/aa.py 2013-08-10 07:16:22 +0000
@@ -1751,6 +1748,7 @@
def ask_the_questions():
found = None
+ print(log_dict)
# debug code?
for aamode in sorted(log_dict.keys()):
# Describe the type of changes
if aamode == 'PERMITTING':
vim:ft=diff
-------------- 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
@@ -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_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'
#[was:] '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)
=== 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 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):
...
...
+ 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