Thanks to reading the wrong directory in read_inactive_profiles()
(profile_dir instead of extra_profile_dir), aa-genprof never asked about
using a profile from the extra_profile_dir.
Sounds like an easy fix, right? ;-)
After fixing this (last chunk), several other errors popped up, one
after the other:
- get_profile() missed a required parameter in a serialize_profile() call
- when saving the profile, it was written to extra_profile_dir, not to
profile_dir where it (as a now-active profile) should be. This is
fixed by removing the filename from existing_profiles{} so that it can
pick up the default name.
- CMD_FINISHED (when asking if the extra profile should be used or a new
one) behaved exactly like CMD_CREATE_PROFILE, but this is surprising
for the user. Remove it to avoid confusion.
- displaying the extra profile was only implemented in YaST mode
- get_pager() returned None, not an actual pager. Since we have 'less'
hardcoded at several places, also return it in get_pager()
Finally, also remove CMD_FINISHED from the get_profile() test in
test-translations.py.
(test-translations.py is only in trunk, therefore this part of the patch
is obviously trunk-only.)
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk
Acked-by: John Johansen <john.johansen@canonical.com> for trunk + a 50% ACK for 2.10 and 2.9
Acked-by: Kshitij Gupta <kgupta8592@gmail.com> for trunk, 2.10 and 2.9
This patch includes several changes and fixes in change_profile highlighting:
- allow audit and deny keywords
- allow bare change_profile rules
- allow change_profile rules without '-> ...' part
- allow usage of the new 'safe' and 'unsafe' keywords
- ensure the exec condition starts with / or @
Acked-by: Seth Arnold <seth.arnold@canonical.com>
This commit touches up the .po files that generate warnings
when msgfmt processes them to create .mo files, at least with gettext
0.19.7-2ubuntu3 in Ubuntu 16.04 LTS. Example warning types cleaned up
include:
ce.po:7: warning: header field 'Last-Translator' still has the initial default value
ce.po:7: warning: header field 'Language' missing in header
de.po:6: warning: header field 'Language-Team' still has the initial default value
This commit also fixes up po files where the Report-Msgid-Bugs-To:
field had not been updated, setting it with the email address
'AppArmor list <apparmor@lists.ubuntu.com>'
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: Christian Boltz <apparmor@cboltz.de>
Those events are actually network events, so ideally we should map them
as such. Unfortunately this requires bigger changes, so here is a hotfix
that ignores those events and thus avoids crashing aa-logprof.
References: https://bugs.launchpad.net/apparmor/+bug/1577051https://bugs.launchpad.net/apparmor/+bug/1582374
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk, 2.10 and 2.9
In detail, this means:
- handle ptrace events in logparser.py
- "translate" those events in aa.py - from log (logparser.py readlog())
to prelog (handle_children()) to log_dict (collapse_log()))
- finally ask the user about the ptrace in ask_the_questions()
(no code change needed there)
Note that these changes are not covered by tests, however they worked in
a manual test with the log examples in the libapparmor testsuite.
Unfortunately there's no example log for eavesdrop, so it might be a
good idea to a) add such a log line and b) test with it
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Note: as discussed on #apparmor, I changed the mapping of peer_profile so
that it ends up in peer=(label=...) instead of the wrong peer=(name=...).
"Everywhere" means aa-mergeprof and aa-cleanprof. In theory also
aa-logprof, but that needs some code that parses dbus log events ;-)
Also add some dbus rules to the aa-cleanprof test profiles to ensure
superfluous dbus rules get deleted.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
DBUS_Rule (in rules.py) was added in r2424 as a "this is how it should
look like" proof of concept, but was never used.
We have a "real" class for dbus rules now, so we can drop the proof of
concept class.
Also remove a commented, old version of RE_DBUS_ENTRY from aa.py
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Change aa.py to use DbusRule and DbusRuleset in profile_storage,
parse_profile_data() and write_dbus. This also means we can drop the
now unused parse_dbus_rule() and write_dbus_rules() functions.
Raw_DBUS_Rule in rules.py is now also unused and can be dropped.
Also shorten the list of known-failing tests in
test-parser-simple-tests.py. Even if the list of removals doesn't look
too long, the generated_dbus/* removals mean 1989 tests now cause the
expected failures.
OTOH, I had to add 4 tests to the known-failing list:
- 3 tests with a "wrong" order of the conditionals which the parser
accepts (which is slightly surprising, because usually we enforce the
order of rule parts)
- one test fails because the path in the path= conditional doesn't start
with / or a variable. Instead, it starts with an alternation, which
wouldn't be allowed in file rules.
Those 4 failures need more investigation, but shouldn't block this
patchset.
Finally, adjust test-regex_matches.py to import RE_PROFILE_DBUS from
apparmor.regex instead of apparmor.aa.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The tests include the two tests from test-dbus_parse.py, therefore
delete this file.
As usual, we have 100% coverage :-)
Also addd an explicit str() conversion to common_test.py to avoid
TypeError: not all arguments converted during string formatting
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Those classes will be used to parse and handle dbus rules.
They understand the syntax of dbus rules.
Note that get_clean() doesn't output superfluos things, so
dbus ( send ),
will become
dbus send,
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Some dbus rule conditionals come with optional parenthesis. Instead of
making the regex even more complicated, use a small function to strip
those parenthesis.
Also add some tests for strip_parenthesis() to test-regex.py.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
As a preparation for the DbusRule class, add a <details> match group
to RE_PROFILE_DBUS.
Also adjust test-regex_matches.py for the added group.
Note: RE_PROFILE_DBUS is only used in aa.py, and only matches[0..2]
are used. 0 and 1 are audit and allow/deny and 2 is and stays the whole
rule (except audit and allow/deny). Therefore no aa.py changes are
needed.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Variables can be used in several rule types (from the existing *Rule
classes: change_profile, dbus, ptrace, signal). It seems nobody uses
variables with those rules, otherwise we'd have received a bugreport ;-)
I noticed this while working on FileRule, where usage of variables is
more common. The file code in bzr (not using a *Rule class) already
loads the variables, so old versions don't need changes for file rule
handling.
However, 2.10 already has ChangeProfileRule and therefore also needs
this fix.
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk and 2.10.
While running test-translations.py with the fixed german translations,
I noticed that I still get errors about hotkey conflicts
It turned out that test-translations.py reads the system-wide
apparmor-utils.mo in addition to the in-tree translations.
(I have the 2.11 beta1 translations installed, which contain hotkey
conflicts for the german translations).
This is surprising because test-translations.py explicitely sets the
locale path. Interestingly, this happens only 4 times (checked with a
temp profile with audit for those files) while test-translations.py has
9 tests).
(Any idea if this behaviour is normal or a bug?)
This patch adds LC_ALL=C to the make check and make coverage commandline
so that the system-wide translations don't get used.
I checked with a modified de.po that in-tree hotkey conflicts still get
detected.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
This test builds and installs the apparmor-utils translations into a
tempdir, and then checks if there's any hotkey conflict in one of the
languages. This is based on a manually maintained list of "buttons" that
are displayed at the same time.
To make things a bit easier to test, add CMD_CANCEL to ui.py CMDS[].
Also replace hardcoded usage of '(Y)es', '(N)o' and '(C)ancel' with
CMDS['CMD_YES'], CMDS['CMD_NO'] and CMDS['CMD_CANCEL'].
Acked-by: Seth Arnold <seth.arnold@canonical.com>
exec choices are stored in transitions[], but that's never used
(and I don't see a need for it), therefore stop storing it.
Note: hat choices (CMD_ADDHAT, CMD_USEDEFAULT and CMD_DENY) get still
stored in transitions[], and that information is used if the same hat
name appears again.
Acked-by: Steve Beattie <steve@nxnw.org>
Automated infrastructure management tools, such as Chef, Puppet, and so
on, could use a way to check AppArmor status that is both high-level
(meaning it does not rely on kernel interfaces in /proc) and machine-
readable (meaning it does not require the complexity of parsing output
of tools originally intended for human consumption).
Adding a JSON variant of the standard aa-status output achieves both.
Bug: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1480492
If python3-apparmor is not installed, aa-status aborts due to the added
import to handle fancier exception handling failing. This patch makes
aa-status(8) work even in that case, falling back to normal python
exceptions, to keep its required dependencies as small as possible.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: Christian Boltz <apparmor@cboltz.de>
write_prof_data[hat] is correct (it only contains one profile, see bug 1528139),
write_prof_data[profile][hat] is not and returns an empty (sub)hasher.
This affects RE_PROFILE_START and RE_PROFILE_BARE_FILE_ENTRY.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com> for trunk, 2.9 and 2.10
a) change log_dict to profile_storage()
Change collapse_log() to initialize log_dict[aamode][profile][hat]
as profile_storage() instead of a hasher().
This also means path events need to go into
log_dict[aamode][profile][hat]['allow']['path']
instead of
log_dict[aamode][profile][hat]['path']
to match the profile_storage() layout.
b) Simplify log translation
The translation from logparser.py's output to *Rule events was more ugly
than needed. This patch removes one step.
Instead of translating log_dict to log_obj in ask_the_questions(), add
*Rule objects to log_dict and adjust ask_the_questions() to use log_dict
instead of log_obj.
This also means log_obj in ask_the_questions() is now superfluous and
can be removed.
c) Other small changes:
- use is_known_rule() instead of .is_covered() for capability events,
which means included files are also checked now.
- remove the "if rule_obj.log_event != aamode:" check, because
a) it depends on the content of *Rule.log_event (which means it
ignores events with log_event != 'ALLOWING' or 'REJECTING'
b) it's superfluous because the whole code section is wrapped in a
"for aamode in sorted(log.dict.keys())" which means we have
separate loops for enforce and complain mode already
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
If the program specified as get_output param isn't executable or doesn't
exist at all, get_output() returns with ret = -1.
Raising an exception looks like a better option, especially because
other possible exec failures already raise an exception ("Unable to
fork").
Note: get_output is only used by get_reqs() which also does the
os.access() check for x permissions (and raises an exception), so in
practise raising an exception in get_output() doesn't change anything.
This change also allows to rewrite and simplify get_output() quite a bit.
Another minor change (and fix) is in the removal of the last line. The
old code removed the last line if output contained at least two items.
This had two not-so-nice effects:
- an empty output resulted in [''] instead of []
- if a command didn't add a \n on the last line, this line was deleted
nevertheless
The patch changes that to always remove the last line if it is empty,
which fixes both issues mentioned above.
Also add a test to ensure the exception is really raised, and adjust the
test that expects an empty stdout.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
To make these tests independent from the underlaying system, add a
fake_ldd script that provides hardcoded ldd output for the "known"
executables and libraries.
To avoid interferences with the real system (especially symlinks), all
paths in fake_ldd have '/AATest' prepended.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
To ensure aa-cleanprof works as expected (and writing the rules works
as expected), add some rules for every rule class to the cleanprof.in
and cleanprof.out test profiles.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
According to a discussion with John on IRC, denied_mask="x" can only
happen for 'exec' log events. This patch raises an exception if John
is wrong ;-)
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
This should happen rarely, but nevertheless it can happen - and since
AppArmor needs the symlink target in the profile, we have to resolve all
symlinks.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
If a profile file contains multiple profiles and one of those profiles
contains a rule managed by a *Ruleset class,
serialize_profile_from_old_profile() crashes with an AttributeError.
This happens because profile_data / write_prof_data contain only one
profile with its hats, which explodes if a file contains multiple
profiles, as reported in lp#1528139
Fixing this would need lots of
write_prof_data[hat] -> write_prof_data[profile][hat]
changes (and of course also a change in the calling code) or, better
option, a full rewrite of serialize_profile_from_old_profile().
Unfortunately I don't have the time to do the rewrite at the moment (I
have other things on my TODO list), and changing write_prof_data[hat] ->
write_prof_data[profile][hat] is something that might introduce more
breakage, so I'm not too keen to do that.
Therefore this patch wraps the serialize_profile_from_old_profile() call
in try/except. If it fails, the diff will include an error message and
recommend to use 'View Changes b/w (C)lean profiles' instead, which is
known to work.
Note: I know using an error message as 'newprofile' isn't an usual way
to display an error message, but I found it more intuitive than
displaying it as a warning (without $PAGER).
References: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1528139
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk and 2.10
parser/tst/simple_tests/profile/profile_ns_bad8.sd was added in r3376
(trunk) / r3312 (2.10 branch) and contains the profile name ':ns/t'
which misses the terminating ':' for the namespace.
Unfortunately the tools don't understand namespaces yet and just use the
full profile name. This also means this test doesn't fail as expected
when tested against the utils code.
This patch adds profile_ns_bad8.sd to the exception list of
test-parser-simple-tests.py.
Acked-by: Steve Beattie <steve@nxnw.org> for trunk and 2.10.
https://launchpad.net/bugs/1546455
Don't filter out AF_UNSPEC from the list of valid protocol families so
that the parser will accept rules such as 'network unspec,'.
There are certain syscalls, such as socket(2), where the LSM hooks are
called before the protocol family is validated. In these cases, AppArmor
was emitting denials even though socket(2) will eventually fail. There
may be cases where AF_UNSPEC sockets are accepted and we need to make
sure that we're mediating those appropriately.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Suggested-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
[cboltz: Add 'unspec' to the network domain keywords of the utils]
If a profile file contains multiple profiles, aa-mergeprof crashes on
saving in write_profile() because the second profile in the file is not
listed in 'changed'. (This happens only if the second profile didn't
change.)
This patch first checks if 'changed' contains the profile before
pop()ing it.
Reproducer: copy utils/test/cleanprof_test.in to your profile directory
and run aa-mergeprof utils/test/cleanprof_test.out. Then just press
's' to save the profile.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com> for trunk, 2.10 and 2.9
If autodep() is called with a pname starting with / (which can happen
for (N)amed exec depending on the user input), this pname is mapped to
bin_name.
This might look like a good idea, however if the given pname doesn't
exist as file on-disk, autodep() returns None instead of a (mostly
empty) profile. (Reproducer: choose (N)amed, enter "/foo/bar")
Further down the road, this results in two things:
a) the None result gets written as empty profile file (with only a "Last
modified" line)
b) a crash if someone chooses to add an abstraction to the None, because
None doesn't support the delete_duplicates() method for obvious
reasons ;-)
Unfortunately this patch also introduces a regression - aa-logprof now
fails to follow the exec and doesn't ask about the log events for the
exec target anymore. However this doesn't really matter because of a) -
asking and saving to /dev/null vs. not asking isn't a real difference ;-)
Actually the patch slightly improves things - it creates a profile for
the exec target, but only with the depmod() defaults (abstractions/base)
and always in complain mode.
I'd prefer a patch that also creates a complete profile for the exec
target, but that isn't as easy as fixing the issues mentioned above and
therefore is something for a future fix. To avoid we forget it, I opened
https://bugs.launchpad.net/apparmor/+bug/1545155
Note: 2.9 "only" writes an empty file and doesn't crash - but writing
an empty profile is still an improvement.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com> for trunk, 2.10 and 2.9
According to the discussion with John on IRC, exec log events for
directories should never happen, therefore let handle_children()
raise an exception.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
