[apparmor] GSoC review r48..51

Christian Boltz apparmor at cboltz.de
Thu Aug 22 20:39:54 UTC 2013


Hello,

Am Donnerstag, 22. August 2013 schrieb Christian Boltz:
> the review for r48, 49, 50 and 51 is attached.

... or not :-/

Let me try a second time ;-)


Regards,

Christian Boltz
-- 
# GO AWAY !
# YOU DO NOT WANT TO SEE THIS SCRIPT !!!
[from /opt/kde3/share/apps/krpmview/setup_temp_source]
-------------- next part --------------
merged review for:
------------------------------------------------------------
revno: 51
  First set of tools in their alpha release, logprof and genprof are pre-bleeding edge so dont hurt yourself or worse your distro.
------------------------------------------------------------
revno: 50
  Semmingly working writer from old profile
------------------------------------------------------------
revno: 49
  first partially working iteration of new profile writer from old profile
------------------------------------------------------------
revno: 48
  working commit prior to writer code alterations

  
=== added file 'Tools/aa-audit.py'
--- Tools/aa-audit.py	1970-01-01 00:00:00 +0000
+++ Tools/aa-audit.py	2013-08-21 05:56:09 +0000
@@ -0,0 +1,54 @@
+#!/usr/bin/python
+import sys
+import os
+import argparse
+
+import apparmor.aa as apparmor
+
+parser = argparse.ArgumentParser(description='Switch the given programs to audit mode')
+parser.add_argument('-d', type=str, help='path to profiles')
+parser.add_argument('program', type=str, nargs='+', help='name of program to hswitch to audit mode')

# typo: switch (not _h_switch)

+args = parser.parse_args()
+
+profiling = args.program
+profiledir = args.d
+
+if profiledir:
+    apparmor.profile_dir = apparmor.get_full_path(profiledir)
+    if not os.path.isdir(apparmor.profile_dir):
+        raise apparmor.AppArmorException("Can't find AppArmor profiles in %s." %profiledir)

# wrong error message, should be "%s is not a directory"

+    if os.path.exists(program):
+        apparmor.read_profiles()
+        filename = apparmor.get_profile_filename(program)
+        
+        if not os.path.isfile(filename) or apparmor.is_skippable_file(filename):
+            continue
+        
+        sys.stdout.write(_('Setting %s to audit mode.\n')%program)
+        
+        apparmor.set_profile_flags(filename, 'audit')

# does this mean that existing flags (for example 'complain') are lost?
# (if yes, then complain() and enforce() in aa.py share the same bug)
# (hint: a function add_profile_flag() and remove_profile_flag() might be useful)

+        cmd_info = apparmor.cmd(['cat', filename, '|', parser, '-I%s'%apparmor.profile_dir, '-R 2>&1', '1>/dev/null'])

# The parser can read the file itsself - no need to use "cat"

+        if cmd_info[0] != 0:
+            raise apparmor.AppArmorException(cmd_info[1])
+    else:
+        if '/' not in p:
+            apparmor.UI_Info(_("Can't find %s in the system path list. If the name of the application is correct, please run 'which %s' as a user with correct PATH environment set up in order to find the fully-qualified path.")%(p, p))

# maybe add something like "Please give the full path as parameter"?

# I also have a feature request - add a commandline option to remove the audit flag
# (basically "aa-noaudit", but I doubt we need to make it a separate command)

# another feature request - aa-disable should get a similar option (basically aa-enable)

=== added file 'Tools/aa-autodep.py'
--- Tools/aa-autodep.py	1970-01-01 00:00:00 +0000
+++ Tools/aa-autodep.py	2013-08-21 05:56:09 +0000
@@ -0,0 +1,53 @@
+parser = argparse.ArgumentParser(description='Disable the profile for the given programs')

# wrong description ;-)

+parser.add_argument('-d', type=str, help='path to profiles')
+parser.add_argument('program', type=str, nargs='+', help='name of program to have profile disabled')
+args = parser.parse_args()
+
+profiledir = args.d
+profiling = args.program
+
+if profiledir:
+    apparmor.profile_dir = apparmor.get_full_path(profiledir)
+    if not os.path.isdir(apparmor.profile_dir):
+        raise apparmor.AppArmorException("Can't find AppArmor profiles in %s." %profiledir)

# wrong error message - should be "%s is not a directory"

+for p in profiling:
+    else:
+        if '/' not in p:
+            apparmor.UI_Info(_("Can't find %s in the system path list. If the name of the application is correct, please run 'which %s' as a user with correct PATH environment set up in order to find the fully-qualified path.")%(p, p))

# maybe add "and use the full path as parameter for aa-genprof" or something like that?

=== added file 'Tools/aa-complain.py'
--- Tools/aa-complain.py	1970-01-01 00:00:00 +0000
+++ Tools/aa-complain.py	2013-08-21 05:56:09 +0000

# exactly the same code as aa-audit (with s/audit/complain)
# and therefore the same notes apply

# This means you have multiple copies of the same code, with only 5 lines differing.
# The better solution would be to make aa-complain, aa-enforce, aa-audit etc. one-liners calling 
# a common module that does the handling
# (or make a symlink aa-complain -> aa-audit and decide behaviour based on $0 - but I'd prefer using a common module)

=== added file 'Tools/aa-disable.py'
--- Tools/aa-disable.py	1970-01-01 00:00:00 +0000
+++ Tools/aa-disable.py	2013-08-21 05:56:09 +0000
# see aa-audit and aa-complain for the general notes.

+disabledir = apparmor.profile_dir+'/disable'
+if not os.path.isdir(disabledir):
+    raise apparmor.AppArmorException("Can't find AppArmor disable directorys %s." %disabledir)

# typo: "directory" (not "directory_s_")

+        bname = os.path.basename(filename)
+        if not bname:
+            apparmor.AppArmorException(_('Unable to find basename for %s.')%filename)

# missing "raise"?



=== added file 'Tools/aa-enforce.py'
--- Tools/aa-enforce.py	1970-01-01 00:00:00 +0000
+++ Tools/aa-enforce.py	2013-08-21 05:56:09 +0000
# see aa-audit for comments


=== added file 'Tools/aa-genprof.py'
--- Tools/aa-genprof.py	1970-01-01 00:00:00 +0000
+++ Tools/aa-genprof.py	2013-08-21 05:56:09 +0000
@@ -0,0 +1,148 @@
+def last_audit_entry_time():
+    out = subprocess.check_output(['tail', '-1', '/var/log/audit/audit.log'], shell=True)

# hey, we have a '-f' argument ;-)

+parser.add_argument('-f', type=str, help='path to logfile')
...
+filename = args.f

# filename is not used anywhere

+aa_mountpoint = apparmor.check_for_apparmor()
+if not aa_mountpoint:
+    raise apparmor.AppArmorException(_('AppArmor seems to have not been started. Please enable AppArmor and try again.'))

# the first sentence looks a bit complicated. I'm not a native english speaker, but what about "It seems AppArmor was not started."?
# (should be the same as in aa-logprof)

+if profiledir:
+    apparmor.profile_dir = apparmor.get_full_path(profiledir)
+    if not os.path.isdir(apparmor.profile_dir):
+        raise apparmor.AppArmorException("Can't find AppArmor profiles in %s." %profiledir)

# wrong error message - should be "not a directory"

...
+if not program or not os.path.exists(program):
+    if '/' not in profiling:
+        raise apparmor.AppArmorException(_("Can't find %s in the system path list. If the name of the application is correct, please run 'which %s' in another window in order to find the fully-qualified path.") %(profiling, profiling))

# maybe add "and use the full path as parameter for aa-genprof" or something like that?

+syslog = True
+logmark = ''
+done_profiling = False
+
+if os.path.exists('/var/log/audit/audit.log'):
+    syslog = False

# not sure if "audit.log exists" is the best way to choose the logfile
# but I have to admit that I don't have a better method ;-)
# @John: any better ideas?


+for p in sorted(apparmor.helpers.keys()):

# just curious - why sorted() ?

+    if apparmor.helpers[p] == 'enforce':
+        enforce(p)
+        reload(p)
+
+apparmor.UI_Info(_('\nReloaded AppArmor profiles in enforce mode.'))
+apparmor.UI_Info(_('\nPlease consider contributing your new profile!\nSee the following wiki page for more information:\nhttp://wiki.apparmor.net/index.php/Profiles\n'))
+apparmor.UI_Info(_('Finished generating profile for %s.')%program)
+sys.exit(0)

=== modified file 'Tools/aa-logprof.py'
--- Tools/aa-logprof.py	2013-08-11 09:52:07 +0000
+++ Tools/aa-logprof.py	2013-08-21 05:56:09 +0000
@@ -1,15 +1,30 @@
-sys.path.append('../')

# just an information for everybody reading this diff and trying the new tools:
# it's finally time to symlink apparmor/ into /usr/lib/python2.7/site-packages and /usr/lib64/python3.3/site-packages ;-)


+parser.add_argument('-f', type=str, help='path to logfile')
...
+filename = args.f

# nice, but it would be even better if you use that filename later ;-)
# (currently there's no code to handle it)

+aa_mountpoint = apparmor.check_for_apparmor()
+if not aa_mountpoint:
+    raise apparmor.AppArmorException(_('AppArmor seems to have not been started. Please enable AppArmor and try again.'))

# the first sentence looks a bit complicated. I'm not a native english speaker, but what about "It seems AppArmor was not started."?

+if profiledir:
+    apparmor.profiledir = apparmor.get_full_path(profiledir)
+    if not os.path.isdir(apparmor.profiledir):
+        raise apparmor.AppArmorException("Can't find AppArmor profiles in %s." %profiledir)

# This error message is wrong - it should just say "%s is not a directory"

...
+sys.exit(0)

# not sure if exitcode 0 is always correct ;-)

=== added file 'Tools/aa-unconfined.py'
--- Tools/aa-unconfined.py	1970-01-01 00:00:00 +0000
+++ Tools/aa-unconfined.py	2013-08-21 05:56:09 +0000

+aa_mountpoint = apparmor.check_for_apparmor()
+if not aa_mountpoint:
+    raise apparmor.AppArmorException(_('AppArmor seems to have not been started. Please enable AppArmor and try again.'))

# the first sentence looks a bit complicated. I'm not a native english speaker, but what about "It seems AppArmor was not started."?
# (should be the same as in aa-logprof)
    
+    cmdline = apparmor.cmd(['cat', '/proc/%s/cmdline'%pid])[1]

# looks like a useless use of cat - what about just reading from that file?

+    pname = cmdline.split('\0')[0]
+    if '/' in pname and pname != prog:
+        pname = '(%s)'%pname
+    else:
+        pname = ''
+    if not attr:
+        if re.search('^(/usr)?/bin/(python|perl|bash)', prog):

# this list looks incomplete - what about sh etc?

+            cmdline = re.sub('\0', ' ', cmdline)
+            cmdline = re.sub('\s+$', '', cmdline).strip()
+            sys.stdout.write(_('%s %s (%s) not confined\n')%(pid, prog, cmdline))
+        else:
+            if pname and pname[-1] == ')':
+                pname += ' '
+            sys.stdout.write(_('%s %s %snot confined\n')%(pid, prog, pname))
+    else:
+        if re.search('^(/usr)?/bin/(python|perl|bash)', prog):

# this list looks incomplete - what about sh etc?


=== modified file 'apparmor/aa.py'
--- apparmor/aa.py	2013-08-12 19:13:20 +0000
+++ apparmor/aa.py	2013-08-21 05:56:09 +0000
@@ -516,6 +513,16 @@
+def get_profile_flags(filename):
+    flags = 'enforce'
+    with open_file_read(filename) as f_in:
+        for line in f_in:
+            if RE_PROFILE_START.search(line):
+                flags = RE_PROFILE_START.search(line).groups()[6]
+                return flags

# to make things more interesting: in theory, a file can contain more than one profile...

+    return flags
            
# instead of returning 'enforce' as default, you should raise an exception if the file does not contain RE_PROFILE_START


@ -1@@ -1720,15 +1736,17 @@
                                         if matchregexp(path, entry):
                                             if mode_contains(mode, aa[profile][hat]['allow']['path'][entry]['mode']):
-                                                aa[profile][hat]['allow']['path'].pop(entry)
-                                                deleted += 1
+                                                deleted.append(entry)
+                                    for entry in deleted:
+                                        aa[profile][hat]['allow']['path'].pop(entry)
+                                    deleted = len(deleted)

# deleted contains a nice diff, but you are throwing it away in the last line

@@ -1951,26 +1969,28 @@
 def delete_cap_duplicates(profilecaps, inccaps):
...
+        for capname in deleted:
+            profilecaps.pop(capname)
+    
+    return len(deleted)

# deleted contains a nice diff, but you are throwing it away in the last line
 
 def delete_path_duplicates(profile, incname, allow):
...
+    for entry in deleted:
+        profile[allow]['path'].pop(entry)
+    return len(deleted)
 
# deleted contains a nice diff, but you are throwing it away in the last line

@@ -2223,6 +2262,26 @@
 
+def display_changes_with_comments(oldprofile, newprofile):
+    """Compare the new profile with the existing profile inclusive of all the comments"""
+    if not os.path.exists(oldprofile):
+        raise AppArmorException("Can't find existing profile %s to compare changes." %oldprofile)
+    if UI_mode == 'yast':
+        #To-Do
+        pass

# in yast mode, you'll also need to create the diff - only displaying it is different

+    else:
+        newtemp = tempfile.NamedTemporaryFile('w')
+        newtemp.write(newprofile)
+        newtemp.flush()
+        
+        difftemp = tempfile.NamedTemporaryFile('w')
+        
+        subprocess.call('diff -u -p %s %s > %s' %(oldprofile, newtemp.name, difftemp.name), shell=True)
+        
+        newtemp.close()
+        subprocess.call('less %s' %difftemp.name, shell=True)

# this "less" call is probably the only thing that will be different in yast mode

+        difftemp.close()

@@ -3157,6 +3215,535 @@
     
+def serialize_profile_from_old_profile(profile_data, name, options):

+    with open_file_read(prof_filename) as f_in:
+        profile = None
+        hat = None
+        write_methods = {'alias': write_alias,
+                         'lvar': write_list_vars,
+                         'include': write_includes,
+                         'rlimit': write_rlimits,
+                         'capability': write_capabilities,
+                         'netdomain': write_netdomain,
+                         'link': write_links,
+                         'path': write_paths,
+                         'change_profile': write_change_profile,
+                         }
+        prof_correct = True
+        segments = {
+                    'alias': False,
+                    'lvar': False,
+                    'include': False,
+                    'rlimit': False,
+                    'capability': False,
+                    'netdomain': False,
+                    'link': False,
+                    'path': False,
+                    'change_profile': False,
+                    'include_local_started': False,
+        }
+        #data.append('reading prof')
+        for line in f_in:
+            correct = True
+            line = line.rstrip('\n')
+            #data.append(' ')#data.append('read: '+line)
+            if RE_PROFILE_START.search(line):
                
+            elif RE_PROFILE_END.search(line):

# just for the records: } can also be the end of a hat or end of a rule block
# you'll need to handle that

+                # DUMP REMAINDER OF PROFILE
+                if profile:

+                    data += write_alias(write_prof_data[name], depth)
+                    data += write_list_vars(write_prof_data[name], depth)
+                    data += write_includes(write_prof_data[name], depth)
+                    data += write_rlimits(write_prof_data, depth)
+                    data += write_capabilities(write_prof_data[name], depth)
+                    data += write_netdomain(write_prof_data[name], depth)
+                    data += write_links(write_prof_data[name], depth)
+                    data += write_paths(write_prof_data[name], depth)
+                    data += write_change_profile(write_prof_data[name], depth)

# can't you just loop over write_methods?

...                   
+                
+            elif RE_PROFILE_CAP.search(line):
+                matches = RE_PROFILE_CAP.search(line).groups()
+                audit = False
+                if matches[0]:
+                    audit = matches[0]
+                
+                allow = 'allow'
+                if matches[1] and matches[1].strip() == 'deny':
+                    allow = 'deny'
+                
+                capability = matches[2]

# will this also work for a line just containing "capability,"?

+                if not write_prof_data[hat][allow]['capability'][capability].get('set', False):
+                    correct = False
+                if not write_prof_data[hat][allow]['capability'][capability].get(audit, False) == audit:
+                    correct = False
+                
+                if correct:
+                    if not segments['capability'] and True in segments.values():
+                        for segs in list(filter(lambda x: segments[x], segments.keys())):
+                            depth = len(line) - len(line.lstrip())
+                            data += write_methods[segs](write_prof_data[name], int(depth/2))
+                            segments[segs] = False
+                            if write_prof_data[name]['allow'].get(segs, False): write_prof_data[name]['allow'].pop(segs)
+                            if write_prof_data[name]['deny'].get(segs, False): write_prof_data[name]['deny'].pop(segs)
+                    segments['capability'] = True
+                    write_prof_data[hat][allow]['capability'].pop(capability)
+                    data.append(line)
+                    
+                    #write_prof_data[hat][allow]['capability'][capability].pop(audit)
+                    
+                    #Remove this line

;-)

+                else:
+                    # To-Do

# a warning (maybe even an error?) would be nice (for all rule types, not just for capabilities)

            
            
+            elif RE_PROFILE_HAT_DEF.search(line):
+                matches = RE_PROFILE_HAT_DEF.search(line).groups()   
+                in_contained_hat = True
+                hat = matches[0]
+                hat = strip_quotes(hat)
+                flags = matches[3]
+                if not write_prof_data[hat]['flags'] == flags:
+                    correct = False
+                if not write_prof_data[hat]['declared'] == False:
+                    correct = False
+                if not write_filelist['profile'][profile][hat]:
+                    correct = False
+                if correct:
+                    data.append(line)
+                else:
+                    #To-Do
+                    pass
+            else:

# This "else" basically means "unknown line"
# that's ok for comments and whitespace-only lines
# but everything else should raise an error

+                if correct:
+                    data.append(line)
+                else:
+                    #To-Do
+                    pass



=== modified file 'apparmor/ui.py'
--- apparmor/ui.py	2013-08-12 19:13:20 +0000
+++ apparmor/ui.py	2013-08-21 05:56:09 +0000
@@ -261,7 +292,7 @@
 def confirm_and_finish():
-    sys.stdout.stdout('FINISHING\n')
+    sys.stdout.write('FINISHING..\n')

# typically there are 3 dots - "FINISHING...\n"

=== added file 'apparmor/writeprofile.py'

# that's an empty file...



vim:ft=diff


More information about the AppArmor mailing list