Hello,

see the attachment for the r24 review.


Regards,

Christian Boltz
-- 
<Dominian> There is always room for improvement
<Dominian> to seek perfection is to drive yourself insane.
<Dominian> except suseROCKs, he's already insane.
[from #opensuse-project]
=== modified file 'apparmor/aa.py'
--- apparmor/aa.py	2013-07-19 22:49:07 +0000
+++ apparmor/aa.py	2013-07-22 23:05:51 +0000
@@ -449,7 +450,7 @@
                 local_profile[localfile]['include']['abstractions/python'] = True
             elif 'ruby' in interpreter:

### hmm, just using "ruby" might lead to false positives
### better check for the full path or at least the full basename

                 local_profile[localfile]['include']['abstractions/ruby'] = True
-            elif interpreter in ['/bin/bash', '/bin/dash', '/bin/sh']:
+            elif re.search('/bin/(bash|dash|sh)', interpreter):

### if you use a regex, don't forget ^...$  ;-)



                     
+                        # If profiled program executes itself only 'ix' option
+                        if exec_target == profile:
+                            options = 'i'

### ix is a good default, but there may be (rare) cases where a program executes itsself with different options etc.
### so I'd still allow the other x options

+                        # Don't allow hats to cx?
+                        options.replace('c', '')

### child profiles can't be nested, but one child profile can call another
### at the moment the workaround is to do (assuming we are in /bin/foo//bin/bar child profile) "Px -> /bin/foo//bin/baz"
### having an option "use another child profile" (which makes clear it won't be nested as /bin/foo//bin/bar//bin/baz) would be nice

+                        # Add deny to options
+                        options += 'd'
+                        # Define the default option
+                        default = None
+                        if 'p' in options and os.path.exists(get_profile_filename(exec_target)):
+                            default = 'CMD_px'

### while we are at it - displaying a note "target profile exists" would be helpful




+                            elif ans == 'CMD_ux':
+                                exec_mode = str_to_mode('ux')
+                                ynans = UI_YesNo(gettext('Launching processes in an unconfined state is a very\n' +
+                                                        'dangerous operation and can cause serious security holes.\n\n' +
+                                                        'Are you absolutely certain you wish to remove all\n' +
+                                                        'AppArmor protection when executing :') + '%s ?' % exec_target, 'n')

### even if this question is absolutely correct, a "never ask again" option would be welcome ;-)

+                            if exec_mode & str_to_mode('i'):
+                                if 'perl' in exec_target:
+                                    aa[profile][hat]['include']['abstractions/perl'] = True

### oh, does executing /bin/perlentaucher need abstractions/perl?
### (the code will happily add it ;-)

+                                elif '/bin/bash' in exec_path or '/bin/sh' in exec_path:

### similar question here for /usr/bin/bin/shorewall ;-)
### (in other words: using "in" can have quite some side effects)

+                                    if 'perl' in interpreter:
+                                        aa[profile][hat]['include']['abstractions/perl'] = True
+                                    elif '/bin/bash' in interpreter or '/bin/sh' in interpreter:
+                                        aa[profile][hat]['include']['abstractions/bash'] = True

### some more "in"...

### that all said - this is at least the second time where the code contains interpreter <-> abstraction information
### better store it at one place - for example like this:
###     def abstraction_for_interpreter(interpreter)
###         # TODO: interpreter_basename = remove ^(/usr)?/bin/ from interpreter
###         if interpreter_basename == 'bash'
###             return 'abstractions/bash'
###         elif interpreter_basename == 'perl'
###             [...]



=== modified file 'apparmor/severity.py'
--- apparmor/severity.py	2013-07-19 22:49:07 +0000
+++ apparmor/severity.py	2013-07-22 23:05:51 +0000
@@ -184,7 +184,7 @@
     
     def load_variables(self, prof_path):
         """Loads the variables for the given profile"""
-        regex_include = re.compile('include <(\S*)>')
+        regex_include = re.compile('^#?include\s*<(\S*)>')

### nearly correct ;-) - you should allow whitespace at the beginning of the line

         if os.path.isfile(prof_path):
             with open_file_read(prof_path) as f_in:
                 for line in f_in:

-- 
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at: 
https://lists.ubuntu.com/mailman/listinfo/apparmor

Reply via email to