[apparmor] GSoC review r52 and r53
Christian Boltz
apparmor at cboltz.de
Mon Aug 26 18:39:13 UTC 2013
Hello,
the review for r52 and r53 is attached.
For the flags handling, I have an interesting question:
Should we prefer to add the flags (for example "complain") to the
profile as we do now, or should we prefer to create a symlink in
/etc/apparmor.d/force-complain/ ?
The flags=(...) in the profile has the advantage that people are used to
it, OTOH creating a symlink means we don't need to modify the profile.
Opinions?
(We'll have to contunue supporting both ways, the question is what
aa-complain, aa-audit etc. should do.)
Regards,
Christian Boltz
--
Der fünfte apokalyptische Reiter der Digitalen Inkompatibilität wird
dich mit den hornigen Hufen des Workarounds niedertrampeln, und niemand
wird in der Nacht deine Fehlermeldungen lesen.
Wir aber werden an einem warmen Feuer sitzen, in dem deine Sourcen
verbrennen, und uns der Kommandozeile erfreuen. So.
[Ratti in fontlinge-devel]
-------------- next part --------------
------------------------------------------------------------
revno: 52
committer: Kshitij Gupta <kgupta8592 at gmail.com
branch nick: apparmor-profile-tools
timestamp: Mon 2013-08-26 00:23:59 +0530
message:
Merged aa-audit, aa-autodep, aa-complain, aa-disable, aa-enforce to share the
common code into a tools.py module. Added -r/--remove feature to aa-complain,
aa-enforce, aa-audit and -r/--revert feature to aa-disable. Some other fixes
from review 48..51
------------------------------------------------------------
revno: 53
committer: Kshitij Gupta <kgupta8592 at gmail.com
branch nick: apparmor-profile-tools
timestamp: Mon 2013-08-26 00:41:15 +0530
message:
aa-cleanprof tool
=== modified file 'Tools/aa-audit.py'
--- Tools/aa-audit.py 2013-08-21 05:56:09 +0000
+++ Tools/aa-audit.py 2013-08-25 18:53:59 +0000
+audit.check_profile_dir()
# $tool.check_profile_dir should be part of apparmor/tools.py
# besides that, I like the new mini-tools :-)
=== modified file 'Tools/aa-autodep.py'
--- Tools/aa-autodep.py 2013-08-21 05:56:09 +0000
+++ Tools/aa-autodep.py 2013-08-25 18:53:59 +0000
-parser = argparse.ArgumentParser(description='Disable the profile for the given programs')
+parser = argparse.ArgumentParser(description='')
# a description would be nice ;-)
+autodep.check_profile_dir()
# $tool.check_profile_dir should be part of apparmor/tools.py
=== added file 'Tools/aa-cleanprof.py'
--- Tools/aa-cleanprof.py 1970-01-01 00:00:00 +0000
+++ Tools/aa-cleanprof.py 2013-08-25 18:53:59 +0000
# aa-cleanprof should also use apparmor/tools.py
# (the code looks quite similar, it even has the wrong error message if the profiledir doesn't exist ;-)
+ if os.path.exists(program):
# This check means aa-cleanprof works only if you have the program on your computer -
# you can't cleanup "unused" profiles.
# That's not the best idea ;-)
# you should check for "profile for program exists" instead.
=== modified file 'Tools/aa-complain.py'
--- Tools/aa-complain.py 2013-08-21 05:56:09 +0000
+++ Tools/aa-complain.py 2013-08-25 18:53:59 +0000
+complain.check_profile_dir()
# $tool.check_profile_dir should be part of apparmor/tools.py
=== modified file 'Tools/aa-disable.py'
--- Tools/aa-disable.py 2013-08-21 05:56:09 +0000
+++ Tools/aa-disable.py 2013-08-25 18:53:59 +0000
+disable.check_profile_dir()
+disable.check_disable_dir()
# $tool.check_profile_dir should be part of apparmor/tools.py
# (and also check_disable_dir() (wrapped with an if condition) to have all code at the same place)
=== modified file 'Tools/aa-enforce.py'
--- Tools/aa-enforce.py 2013-08-21 05:56:09 +0000
+++ Tools/aa-enforce.py 2013-08-25 18:53:59 +0000
+parser.add_argument('-r', '--remove', action='store_true', help='remove enforce mode')
# maybe "switch to complain mode" would be a better help text
+enforce.check_profile_dir()
# $tool.check_profile_dir should be part of apparmor/tools.py
# (did I already mention that I love copy&paste reviews? ;-)
+args = parser.parse_args()
+
+enforce = aa_tools('enforce', args)
# "enforce" is not a real flag - basically it translates to "not complain".
# live would be easier in apparmor/tools.py if you do
#
# args.remove = not args.remove # invert the "remove" flag
# enforce = aa_tools('complain', args)
=== modified file 'Tools/aa-unconfined.py'
--- Tools/aa-unconfined.py 2013-08-21 05:56:09 +0000
+++ Tools/aa-unconfined.py 2013-08-25 18:53:59 +0000
@@ -48,22 +48,22 @@
if not attr:
- if re.search('^(/usr)?/bin/(python|perl|bash)', prog):
- cmdline = re.sub('\0', ' ', cmdline)
+ if re.search('^(/usr)?/bin/(python|perl|bash|sh)', prog):
# this still doesn't cover the "etc." part of "sh etc." ;-)
# (hint: apparmor/aa.py contains some more shells, and it would be a good idea to have the list of shells at _one_ place)
# besides that, the regex will also cover /bin/pythonhunter and /usr/bin/shit ;-)
+ cmdline = re.sub('\x00', ' ', cmdline)
cmdline = re.sub('\s+$', '', cmdline).strip()
+ if 'perl' in cmdline:
+ print(cmdline)
# debug code?
...
else:
- if re.search('^(/usr)?/bin/(python|perl|bash)', prog):
+ if re.search('^(/usr)?/bin/(python|perl|bash|sh)', prog):
# this still doesn't cover the "etc." part of "sh etc." ;-) (see above for details)
=== modified file 'apparmor/aa.py'
--- apparmor/aa.py 2013-08-21 05:56:09 +0000
+++ apparmor/aa.py 2013-08-25 18:53:59 +0000
@@ -225,15 +227,15 @@
if not prof_filename :
fatal_error("Can't find %s" % path)
UI_Info('Setting %s to complain mode.' % name)
- set_profile_flags(prof_filename, 'complain')
+ change_profile_flags(prof_filename, 'complain')
# change_profile_flags should require (as third parameter) if it has to add or to remove the flag
def enforce(path):
"""Sets the profile to enforce mode if it exists"""
prof_filename, name = name_to_prof_filename(path)
if not prof_filename :
fatal_error("Can't find %s" % path)
UI_Info('Setting %s to enforce mode' % name)
- set_profile_flags(prof_filename, '')
+ change_profile_flags(prof_filename, 'complain')
# change_profile_flags should require (as third parameter) if it has to add or to remove the flag
@@ -515,14 +517,45 @@
...
+def change_profile_flags(filename, flag, remove=False):
# the third parameter should be non-optional - if it has to add or to remove the flag
# (the current way of toggling the flag could cause funny behaviour - for example aa-complain could accidently remove the complain flag ;-)
# you should name the third parameter "set_flag" ("remove" already contains a "not" which might cause confusion)
+ old_flags = get_profile_flags(filename)
+ newflags = []
+ if old_flags != '':
+ # Flags maybe white-space and/or , separated
+ old_flags = old_flags.split(',')
# nice comment, but you only split at , ? ;-)
+ if type(old_flags) == type([]):
+ for i in old_flags:
+ newflags += i.split()
+ else:
+ newflags = old_flags.split()
+ #newflags = [lambda x:x.strip(), oldflags]
+ if flag in newflags:
+ if remove:
+ newflags.remove(flag)
+ else:
+ if not remove:
+ if flag == '':
+ if 'complain' in newflags:
+ newflags.remove('complain')
# special handling for complain? not a good idea.
+ else:
+ newflags.append(flag)
# better:
# if set_flag:
# if flag not in newflags:
# newflags.append(flag)
# else:
# if flag in newflags:
# newflags.remove(flag)
+ newflags = ','.join(newflags)
+
+ set_profile_flags(filename, newflags)
=== added file 'apparmor/tools.py'
--- apparmor/tools.py 1970-01-01 00:00:00 +0000
+++ apparmor/tools.py 2013-08-25 18:53:59 +0000
@@ -0,0 +1,128 @@
+class aa_tools:
+ def __init__(self, tool_name, args):
+ self.name = tool_name
+ self.profiledir = args.d
+ self.profiling = args.program
# check_profile_dir() call would fit here
+ if tool_name in ['audit', 'complain', 'enforce']:
+ self.remove = args.remove
+ elif tool_name == 'disable':
+ self.revert = args.revert
+ self.disabledir = apparmor.profile_dir+'/disable'
# check_disable_dir() call would fit here
# BTW: profile_dir + '/disable' could end up in /some/dir//disable - does check_disable_dir work with double slashes?
+ def act(self):
+ for p in self.profiling:
+ if not p:
+ continue
+
+ program = None
+ if os.path.exists(p):
+ program = apparmor.get_full_path(p).strip()
+ else:
+ which = apparmor.which(p)
+ if which:
+ program = apparmor.get_full_path(which)
+
+ if os.path.exists(program):
# This check means aa-* works only if you have the program on your computer -
# you can't do anything with "unused" profiles.
# That's not the best idea ;-)
# you should check for "profile for program exists" instead.
+ apparmor.read_profiles()
+ filename = apparmor.get_profile_filename(program)
+
+ if not os.path.isfile(filename) or apparmor.is_skippable_file(filename):
+ continue
# I'd at least expect a warning "no profile for %s, skipping" here
+ if self.name == 'enforce':
+ apparmor.UI_Info(_('Setting %s to enforce mode.\n')%program)
+ apparmor.change_profile_flags(filename, '', self.remove)
# looks wrong - I'd expect to see the "complain" flag and "not self.remove"
# (besides that, this section could be superfluous if you like my proposal for aa-enforce.py)
# Note: you should add a section for complain that adds or removes the force-complain symlink
# and we should discuss if we prefer the complain flag or the symlink ;-)
+ #apparmor.set_profile_flags(filename, '')
+ self.remove_symlinks(filename)
# without checking for the --remove parameter?
# see comment for remove_symlinks
+ elif self.name == 'disable':
+ apparmor.UI_Info(_('Disabling %s.\n')%program)
# will look funny if --revert was given ;-)
+ if not self.revert:
+ self.disable_profile(filename)
+ else:
+ self.remove_disable_link(filename)
+ else:
+ apparmor.UI_Info(_('Setting %s to %s mode.\n')%(program, self.name))
+ apparmor.change_profile_flags(filename, self.name, self.remove)
+ #apparmor.set_profile_flags(filename, self.name)
# using "else" of course makes things easier
# nevertheless, checking against the list of known flags might be a good idea
# (I'm quite sure you have this list somewhere already ;-)
+ cmd_info = apparmor.cmd(['cat', filename, '|', apparmor.parser, '-I%s'%apparmor.profile_dir, '-R 2>&1', '1>/dev/null'])
# still a useless use of cat ;-) - the parser can read the file itsself
+ 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))
# add something like "Please give the full path as parameter"?
# (same text as in aa-genprof.py)
+ else:
+ apparmor.UI_Info(_("%s does not exist, please double-check the path.")%p)
+ sys.exit(1)
+ def remove_symlinks(self, filename):
+ # Remove symlink from profile_dir/force-complain
# bad function name - it should be "remove_complain_symlinks"
# better name it "set_enforce_mode" and include removing the complain flag in the function
# hmm, that reminds me to enforce() in aa.py ;-) - you should merge it into it
+ complainlink = filename
+ complainlink = re.sub('^%s'%apparmor.profile_dir, '%s/force-complain'%apparmor.profile_dir, complainlink)
+ if os.path.exists(complainlink):
+ os.remove(complainlink)
+
+ # remove symlink in profile_dir/disable
+ disablelink = filename
+ disablelink = re.sub('^%s'%apparmor.profile_dir, '%s/disable'%apparmor.profile_dir, disablelink)
+ if os.path.exists(disablelink):
+ os.remove(disablelink)
# if profile_dir/disable does not exist, this will delete the profile
# you should check
# - if re.sub did change something
# - if disablelink is a symlink (and not a normal file)
# you'll probably end up with two functions:
# - delete_symlink(subdir, filename) # to delete a symlink in subdir (for example "disable")
# - create_symlink(subdir, filename) # to add a symlink in subdir
+ def remove_disable_link(self, filename):
+ # Remove the file from disable dir
# better function name would be "enable_profile"
+ bname = os.path.basename(filename)
+ if not bname:
+ raise apparmor.AppArmorException(_('Unable to find basename for %s.')%filename)
+
+ link = '%s/%s'%(self.disabledir, bname)
+ if os.path.exists(link):
+ os.remove(link)
# should check if link is really a symlink - or use the proposed delete_symlink function ;-)
+ def disable_profile(self, filename):
+ bname = os.path.basename(filename)
+ if not bname:
+ raise apparmor.AppArmorException(_('Unable to find basename for %s.')%filename)
+
+ link = '%s/%s'%(self.disabledir, bname)
+ if not os.path.exists(link):
+ try:
+ os.symlink(filename, link)
+ except:
+ raise apparmor.AppArmorException('Could not create %s symlink to %s.'%(link, filename))
# looks like a good template for create_symlink() ;-)
vim:ft=diff
More information about the AppArmor
mailing list