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]
------------------------------------------------------------
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
-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

Reply via email to