Hello,
Am Freitag, 27. Februar 2015 schrieb Steve Beattie:
> In order to help you in your mission to keep a stable number of
> patches pending review, I noticed a couple of quirks in behavior while
> testing your patch:
You should know that writing something like that is very dangerous and
uncovers quite some issues, not only in the tools.
This mail already contains two patches, just scroll down ;-)
Oh, and you mentioned several things that are not my bug (like missing
functions in libapparmor and missing parser documentation), which I'll
happily hand over to you ;-)
> 1) If the profile is not currently loaded in the kernel when
> aa-disable is run, aa-disable throws a python traceback due to
> the parser returning the error code that it was unsuccessful
> in unloading the profile. This doesn't seem to me to warrant
> aa-disable crashing, perhaps we can handle this situation
> better here?
"crash" isn't the correct term - it raises an exception which isn't
catched ;-)
I'd say the best fix is to change how we handle exceptions.
AppArmorExceptions should just print a single line with the error
message IMHO, while everything else should still print a backtrace.
You probably also remember the discussion about using cgitb - we "just"
need a patch to do it ;-)
Oh, while talking about single line error messages:
# python3 ../../utils/aa-disable -d . sbin.klogd # running as _user_
Disabling /home/cb/apparmor/HEAD-clean/profiles/apparmor.d/sbin.klogd.
Traceback (most recent call last):
File "../../utils/aa-disable", line 30, in <module>
tool.cmd_disable()
File "/home/cb/apparmor/HEAD-clean/utils/apparmor/tools.py", line 146, in
cmd_disable
raise apparmor.AppArmorException(cmd_info[1])
apparmor.common.AppArmorException: "/sbin/apparmor_parser: Sorry. You need root
privileges to run this program.\n\nAppArmor parser version 2.9.1\nCopyright (C)
1999-2008 Novell Inc.\nCopyright 2009-2012 Canonical Ltd.\n\nUsage:
/sbin/apparmor_parser [options] [profile]\n\nOptions:\n--------\n-a,
--add\t\tAdd apparmor definitions [default]\n-r, --replace\t\tReplace apparmor
definitions\n-R, --remove\t\tRemove apparmor definitions\n-C,
--Complain\t\tForce the profile into complain mode\n-B, --binary\t\tInput is
precompiled profile\n-N, --names\t\tDump names of profiles in input.\n-S,
--stdout\t\tDump compiled profile to stdout\n-o n, --ofile n\t\tWrite output to
file n\n-b n, --base n\t\tSet base dir and cwd\n-I n, --Include n\tAdd n to the
search path\n-f n, --subdomainfs n\tSet location of apparmor filesystem\n-m n,
--match-string n Use only features n\n-M n, --features-file n Use only
features in file n\n-n n, --namespace n\tSet Namespace for the profile\n-X,
--readimpliesX\tMap pr
ofile read permissions to mr\n-k, --show-cache\tReport cache hit/miss
details\n-K, --skip-cache\tDo not attempt to load or save cached profiles\n-T,
--skip-read-cache\tDo not attempt to load cached profiles\n-W,
--write-cache\tSave cached profile (force with -T)\n --skip-bad-cache\tDon't
clear cache if out of sync\n --purge-cache\tClear cache regardless of its
state\n --create-cache-dir\tCreate the cache dir if missing\n-L, --cache-loc
n\tSet the location of the profile cache\n-q, --quiet\t\tDon't emit
warnings\n-v, --verbose\t\tShow profile names as they load\n-Q,
--skip-kernel-load\tDo everything except loading into kernel\n-V,
--version\t\tDisplay version info and exit\n-d, --debug \t\tDebug apparmor
definitions\n-p, --preprocess\tDump preprocessed profile\n-D [n],
--dump\t\tDump internal info for debugging\n-O [n], --Optimize\tControl dfa
optimizations\n-h [cmd], --help[=cmd] Display this text or info about
cmd\n--abort-on-error\tAbort processing of profiles on first e
rror\n--skip-bad-cache-rebuild Do not try rebuilding the cache if it is
rejected by the kernel\n--warn n\t\tEnable warnings (see --help=warn)\n"
The parser errors out for obvious reasons, but why does it hide the
"You need root privileges" message behind the full --help details?
IMHO it should _only_ print the "You need root privileges" message.
(BTW: The same happens for apparmor_parser --unknown-option.)
> (Though, aa-disable may not be able to distinguish from the
> results returned by the parser "removing the profile failed due
> to the profile not existing in the kernel's set of loaded policy"
> and "removing the profile failed due to some other kernel error,
> incompatibility between the parser and kernel, etc." Moving the
> policy load to libapparmor and getting the utils to make use of it
> could potentially get us a richer set of failure state information.)
Right. I'm not too keen on parsing the parser output for the error
message. The really correct fix would be to check if the profile is
loaded (a function in libapparmor would be very helpful) and only unload
it if it is really loaded.
I'll wait for the patched libapparmor ;-)
> 2) As a follow up to the above, we don't appear to document the
> return codes of the parser in the apparmor_parser(8) man page. :(
Nice catch - can someone who is familiar with the parser code please do
this?
> 3) This is more of a discussion point than anything, but my test
> environment as passed to the utils via --dir had an outdated
> tunables/ dir and didn't have an abstractions/ directory. This
You uncovered a bug without even noticing it ;-)
(after writing some more paragraphs) Well, actually even two bugs ;-)
All functions in tools.py call apparmor.read_profiles() first.
All functions? No, there's a little Gaulish village ^W^W cmd_disable()
function that doesn't do that, and therefore
- doesn't error out when hitting a broken profile directory
- doesn't find a profile if it doesn't use the default naming scheme
(for example /bin/true profile hiding in bin.false)
Let me propose another patch (for 2.9 and trunk):
[ tools-read-profiles-in-disable.diff ]
=== modified file 'utils/apparmor/tools.py'
--- utils/apparmor/tools.py 2015-02-27 13:19:00 +0000
+++ utils/apparmor/tools.py 2015-02-27 23:43:12 +0000
@@ -125,6 +121,8 @@
sys.exit(1)
def cmd_disable(self):
+ apparmor.read_profiles()
+
for (program, profile) in self.get_next_to_profile():
output_name = profile if program is None else program
(a follow-up patch will move all read_profiles() calls to __init__(),
but I prefer to keep bugfix and cleanup as separate patches)
Surprisingly, aa-disable still won't detect the missing abstractions
after applying this patch. Following down the code shows that
load_include() in aa.py basically does:
if os.path.isfile(...)
[... parse the file ...]
elif os.path.isdir(...)
[... add files to load_includeslist ...]
return 0
Do you miss something? ;-)
Hint: it doesn't error out on non-existing files.
Here's another patch for trunk and 2.9:
[ load_include_not_found_error.diff ]
=== modified file 'utils/apparmor/aa.py'
--- utils/apparmor/aa.py 2015-02-20 20:36:55 +0000
+++ utils/apparmor/aa.py 2015-02-28 00:25:00 +0000
@@ -4390,6 +4398,8 @@
#If the include is a directory means include all subfiles
elif os.path.isdir(profile_dir + '/' + incfile):
load_includeslist += list(map(lambda x: incfile + '/' + x,
os.listdir(profile_dir + '/' + incfile)))
+ else:
+ raise AppArmorException("Include file %s not found" % (profile_dir
+ '/' + incfile) )
return 0
BTW: the return value isn't used anywhere, should we just remove the
"return 0" line?
Oh, and I should probably test if adding to load_includeslist works as
expected, because it's modifying the variable inside the while loop
and we know that python doesn't like that too much...
> caused the parser to abort when trying to operate on my test
> profile not because it couldn't find the abstractions included
> within it, but because the system-wide abstractions that it fell
> back to referenced a policy variable that my test tree's
> tunables/ did not define. This was not what I was expecting, but I'm
> not sure that we've defined what we mean by -d/--dir with the utils;
> my expectation was that it would be the equivalent to setting the
> --base option in the parser, but maybe that was misguided. My bigger
Sounds like a valid and sane assumption - one of my next patches will
pass the directory given in -d as --base to the parser.
BTW: Will this also override --Include (which might have
/etc/apparmor.d/abstractions as default if I get parser.conf right)
or will the parser still search there if the file doesn't exist in the
--base directory?
> question is: are the utils when passed --dir in sync with parser in
> terms of where relative includes will be pulled from?
AFAIK the tools don't fallback to /etc/apparmor.d/ when -d is specified
and only use <directory given with -d>/abstractions.
Regards,
Christian Boltz
--
Sich aktiv an Wikipedia beteiligen habe ich versucht.
Es war grausam. Dagegen ist das Heise-Forum ein Streichelzoo.
[Charly Kuehnast zu http://vvv.koehntopp.de/wpkris/?p=739032]
--
AppArmor mailing list
AppArmor@lists.ubuntu.com
Modify settings or unsubscribe at:
https://lists.ubuntu.com/mailman/listinfo/apparmor
