If a user specifies a non-existing file to merge into the profiles
(`aa-mergeprof /file/not/found`), this results in a backtrace showing an
AppArmorBug because that file unsurprisingly doesn't end up in the
active_profiles filelist.
Handle this more gracefully by adding a read_error_fatal parameter to
read_profile() that, if set, forwards the exception. With that,
aa-mergeprof doesn't try to list the profiles in this non-existing file.
Note that all other callers of read_profile() continue to ignore read
errors, because aborting just because a single file in /etc/apparmor.d/
(for example a broken symlink) isn't readable would be a bad idea.
... instead of converting log_dict to traditional [profile][hat] layout
in do_logprof_pass().
A nice side effect is that we get sorting the main profile before its
hats for free and can remove the sorting code.
Also update a comment in ask_rule_questions().
Finally, adjust aa-mergeprof so that it hands over a merged log_dict (using
split_to_merged())
Since this option is mostly meant for testing, it will not show up in
--help.
aa-notify was the only tool that honored the __AA_CONFDIR env variable.
It still does if --configdir is not given.
Note: Since we now pass confdir= to init_aa() (in most cases None),
setting the default needs to be moved inside the function.
On arch
make -C profiles check-logprof
fails with
*** Checking profiles from ./apparmor.d against logprof
ERROR: Can't find AppArmor profiles in /etc/apparmor.d
make: *** [Makefile:113: check-logprof] Error 1
make: Leaving directory '/build/apparmor/src/apparmor-2.13.3/profiles'
because /etc/apparmor.d/ is not available in the build environment
and aa-logprofs --dir argument, is not being passed to init_aa()
but used to update profiles_dir after the fact.
Fix this by passing profiledir as an argument to init_aa()
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/36
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/663
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Christian Boltz <apparmor@cboltz.de>
Everything handled in 'filelist' gets handled in active_profiles now.
Note: the 'elif' branch in delete_all_duplicates() was probably never
hit because `if include.get(...)` always matched. The only possible
exception might be non-existing include files, but those cause a 'file
not found' error anyway.
... because this is now done via IncludeRule, and keeping the separate
code would mean asking twice.
Note that the user interface changes slightly.
The old workflow was
1 - #include <foo>
2 - #include <bar>
3 - #include <baz>
[select 2 and (A)dd, then get the next question for the remainder]
1 - #include <foo>
2 - #include <baz>
[(A)dd another one, or (I)gnore the remainder]
The new workflow will ask separately for each abstraction, and you can
(A)dd or (I)gnore it. This is probably easier to understand because it's
basically a yes/no question.
For now, that means aa-mergeprof will ask for `abi` and `include if
exists` rules (currently hardcoded).
This needs storing of `active_profiles` in the Prof object - the
preamble `abi` and `include if exists` rules are stored there.
Since several functions expect an `include` dict, add an empty one to
ProfileList to prevent lots of errors and breakage. It can be removed
again when handling of `include` rules gets moved to IncludeRule.
Technical stuff first:
Replace existing_profiles (a dict with the filenames for both active and
inactive profiles) with active_profiles and extra_profiles which are
ProfileList()s and store the active profiles and those in the extra
directory separately. Thanks to ProfileList, now also the relation
between attachments and filenames is easily available.
Also replace all usage of existing_profiles with active_profiles and
extra_profiles, and adjust it to the ProfileList syntax everywhere.
With this change, several bugs in aa-complain and the other minitools
get fixed:
- aa-complain etc. never found profiles that have a profile name
(the attachment wasn't checked)
- even if the profile name was given as parameter to aa-complain, it
first did "which $parameter" so it never matched on named profiles
- profile names with alternations (without attachment specification)
also never matched because the old code didn't use AARE.
References: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=882047#92
(search for "As usual" ;-)
Just for completeness - the matching still doesn't honor/expand
variables in the profile name.
Split get_profile_filename() into
- get_profile_filename_from_profile_name() (parameter: a profile name)
- get_profile_filename_from_attachment() (parameter: an attachment)
Currently both functions call get_profile_filename_orig() (formerly
get_profile_filename()) so the behaviour doesn't change yet.
The most important part of this commit is changing all
get_profile_filename() calls to use one of the new functions to make
clear if they specify a profile or an attachment/executable as
parameter.
As promised, the is_attachment parameter starts to get used in this
patch ;-)
Note: The get_new parameter (which I'll explain in the patch actually
using it) is set to True in all calls to the new functions.
The long term plan is to get rid of it in most cases (hence defaulting
to False), but that will need more testing.
For now we only allow quoted absolute paths without spaces in the name
due to:
- 1738877: include rules don't handle files with spaces in the name
- 1738879: include rules don't handle absolute paths without quotes in
some versions of parser
- 1738880: include rules don't handle relative paths in some versions of
the parser
This is a preparation patch to use for JSON mode of conveying
diff filename. In this patch we move diff generation functions to UI.
In the process, I have cleaned up the code to reduce code and enable reuse.
Remove unused function get_profile_diff().
Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
Acked-by: Christian Boltz <apparmor@cboltz.de>
[cboltz] Also adjust aa-mergeprof to the new function name/location
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Introduce an apparmor.aa.init_aa() method and move the initialization
code of the apparmor.aa module into it. Note that this change will break
any external users of apparmor.aa because global variables that were
previously initialized when importing apparmor.aa will not be
initialized unless a call to the new apparmor.aa.init_aa() method is
made.
The main purpose of this change is to allow the utils tests to be able
to set a non-default location for configuration files. Instead of
hard-coding the location of logprof.conf and other utils related
configuration files to /etc/apparmor/, this patch allows it to be
configured by calling apparmor.aa.init_aa(confdir=PATH).
This allows for the make check target to use the in-tree config file,
profiles, and parser by default. A helper method, setup_aa(), is added
to common_test.py that checks for an environment variable containing a
non-default configuration directory path prior to calling
apparmor.aa.init_aa(). All test scripts that use apparmor.aa are updated
to call setup_aa().
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Suggested-by: Christian Boltz <apparmor@cboltz.de>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: Christian Boltz <apparmor@cboltz.de>
Replace most of aa-mergeprof ask_merge_questions() with a call to
aa.py ask_the_questions() (which is, besides some small exceptions that
are not relevant for aa-mergeprof, in sync with the dropped code).
The remaining part gets renamed to ask_merge_questions() to avoid
confusion with the function name in aa.py. Also drop the (now
superfluous) parameter.
aa.py ask_the_questions() needs to allow 'merge' as aamode.
While on it, replace the fatal_error() call for unknown aamode with
raising an AppArmorBug.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The function is an exact copy of the code in aa-mergeprof (except
removing the 'self' function parameter and changing the whitespace
level)
Also add a ask_conflict_mode() call to aa.py ask_the_questions().
This is needed for aa-mergeprof, and won't hurt in aa-logprof mode
because handle_children() already handles all exec events.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Bug: https://launchpad.net/bugs/1522938
Set log_dict['merge'] = other.aa and aamode = 'merge', and use
log_dict[aamode] everywhere.
This brings aa-mergeprof ask_the_questions() closer to the code in aa.py.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
3-way-merge was never really implemented.
This patch drops all traces of it to make the code more readable and
easier to maintain.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
As discussed a while ago, switch the utils (including their tests) to
use python3 by default. While on it, drop usage of "env" to always get
the system python3 instead of a random one that happens to live
somewhere in $PATH.
In practise, this patch doesn't change much - AFAIK openSUSE, Debian and
Ubuntu already patch aa-* to use python3.
Also add a note to README to officially deprecate Python 2.x.
(I won't break Python 2.x support intentionally - unless some future
change gives me a very good reason to finally drop Python 2.x support.)
Acked-by: Seth Arnold <seth.arnold@canonical.com>
(since 2016-08-23, but the commit had to wait for the FileRule series
because it touches test-file.py)
If a merged profile contains additional hats or subprofiles, the "old"
aa-mergeprof silently created them as additional hasher elements (partly
buggy, because subprofiles would end up as '^/subprofile' instead of
'profile /subprofile'). After switching to FileRule, aa-mergeprof crashes
on new hats or subprofiles.
This patch adds code to ask the user if the new hat or subprofile should
be added - which means this patch replaces two bugs (crash + silently
adding subprofiles and hats) with a new feature ;-)
The new questions also add a new text CMD_ADDSUBPROFILE in ui.py.
Finally, the new "button" combinations get added to test-translations.py.
If you want to test, try to aa-mergeprof this profile (the subprofile
and hat are dummies, nothing ping would really require):
#include <tunables/global>
/{usr/,}bin/ping {
#include <abstractions/base>
#include <abstractions/consoles>
#include <abstractions/nameservice>
capability net_raw,
capability setuid,
network inet raw,
network inet6 raw,
/{,usr/}bin/ping mixr,
/etc/modules.conf r,
^hat {
/bin/hat r,
/bin/bash px,
}
profile /subprofile {
/bin/subprofile r,
/bin/bash px,
}
# Site-specific additions and overrides. See local/README for details.
#include <local/bin.ping>
}
Note that this patch is not covered by unittests, but it passed all my
manual tests.
Acked-by: Steve Beattie <steve@nxnw.org>
Bug: https://launchpad.net/bugs/1507469
aa-mergeprof empties 'includes' when running reset_aa(). The result is
KeyError: 'abstractions/newly_added_abstraction'
if an include file gets added because it isn't part of 'includes' at
this time. Note that you'll need to add another rule after adding the
include to trigger checking the includes for superfluous rules.
This fixes the regression found by Steve - which isn't really a
regression, "just" one more thing that got more visible with the new
code. Before, it was just an ill-addressed hasher that didn't complain ;-)
Acked-by: Steve Beattie <steve@nxnw.org>
When an user adds a new rule to a profile, cleanup / delete existing
rules that are covered by the new rule, and report the number of deleted
rules.
Acked-by: Steve Beattie <steve@nxnw.org>
Replace the old (hasher-based) conflict_mode() with the new
(FileRule-based) ask_conflict_mode() function. If it detects conflicting
exec rules, it asks the user which one to keep.
Also call ask_conflict_mode() from ask_the_questions() so that it is
actually used.
Note: This patch isn't covered by unittests, but I did some manual
testing to make sure it works as expected.
Acked-by: Steve Beattie <steve@nxnw.org>
The clear_common() call was disabled because it crashed in
delete_path_duplicates(). With the switch to FileRule, this function
no longer exists and therefore it can't crash ;-)
This patch re-enables the clear_common() call to avoid asking
superfluous questions.
References: https://bugs.launchpad.net/apparmor/+bug/1382236
Acked-by: Steve Beattie <steve@nxnw.org>
Add set_options_audit_mode() to switch the audit mode in all options
offered by aa-logprof and aa-mergeprof, not only the "original" rule
(in aa-logprof, this means the non-globbed rule_obj).
As usual, add some tests to ensure the function works as expected.
Acked-by: Steve Beattie <steve@nxnw.org>
aa.py:
- add propose_file_rules() - will propose matching paths from existing
rules in the profile or one of the includes
- save user_globs if user selects '(N)ew' (will be re-used when
proposing rules)
- change user_globs to a dict so that it can carry the human-readable
path and an AARE object for it
- change order_globs() to ensure the original path (given as parameter)
is always the last item in the resulting list
- add a ruletype switch to ask_the_questions() so that it uses
propose_file_rules() for file events (I don't like this
ruletype-specific solution too much, but everything else would make
things even more complicated)
Also keep aa-mergeprof ask_the_questions() in sync with aa.py.
In FileRule, add original_perms (might be set by propose_file_rules())
Finally, add some tests to ensure propose_file_rules() does what it promises.
Acked-by: Steve Beattie <steve@nxnw.org>
This brings back the edit option for the path of file rules.
Also add it to aa-mergeprof to keep ask_the_questions() in sync.
Note: aa-mergeprof will ask about path mismatchs basically always.
That's because AARE is too careful on the matching - something to be
fixed in a later patch.
Acked-by: Steve Beattie <steve@nxnw.org>
This change also needs some other changes in ask_the_questions():
- set q.options and q.selected inside the loop (because glob() and
glob_ext() add another option)
- set 'selection' outside the if block to avoid doing it in nearly every
if branch
- make sure to add the selected rule, not just rule_obj (which doesn't
contain a modified, for example globbed, rule)
- skip 'deny' if an #include is selected
- re-add handling for CMD_GLOB and CMD_GLOB_EXT (was lost when switching
to FileRule)
- add selection_to_rule_obj() helper function
- add glob and glob with ext buttons in available_buttons() if
rule_obj.can_glob or rule_obj.can_glob_ext
Also apply the changes in ask_the_questions() to aa-mergeprof to keep it
in sync with aa.py, and disable the old path handling in aa-mergeprof.
Note: in its current state, aa-mergeprof will ask for some "superfluous"
file permissions, and doesn't check for 'x' conflicts. One of the
following patches will fix that.
Acked-by: Steve Beattie <steve@nxnw.org>
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.
Having a list of rule types/classes at several places is annoying and
error-prone. This patch centralizes the list in aa.py.
This also means ask_the_question() in aa.py will now (in theory) support
'change_profile' and 'rlimit'. In practise, that doesn't change anything
because logparser.py doesn't support change_profile events yet - and
rlimit doesn't cause any log events.
Also add some long overdue copyright headers.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Instead of always showing a backtrace,
- for AppArmorException (used for profile syntax errors etc.), print only
the exceptions value because a backtrace is superfluous and would
confuse users.
- for other (unexpected) exceptions, print backtrace and save detailed
information in a file in /tmp/ (including variable content etc.) to
make debugging easier.
This is done by adding the apparmor.fail module which contains a custom
exception handler (using cgitb, except for AppArmorException).
Also change all python aa-* tools to use the new exception handler.
Note: aa-audit did show backtraces only if the --trace option was given.
This is superfluous with the improved exception handling, therefore this
patch removes the --trace option. (The other aa-* tools never had this
option.)
If you want to test the behaviour of the new exception handler, you can
use this script:
#!/usr/bin/python
from apparmor.common import AppArmorException, AppArmorBug
from apparmor.fail import enable_aa_exception_handler
enable_aa_exception_handler()
# choose one ;-)
raise AppArmorException('Harmless example failure')
#raise AppArmorBug('b\xe4d bug!')
#raise Exception('something is broken!')
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The function is basically a wrapper around a regex, so regex.py is a
much better home.
While on it, rename the regex to RE_INCLUDE, change it to named matches,
use RE_EOL to handle comments and compile it outside the function, which
should result in a (small) performance improvement.
Also rewrite re_match_include(), let it check for empty include
filenames ("#include <>") and let it raise AppArmorException in that
case.
Finally, adjust code calling it to the new location, and add some tests
for re_match_include()
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
Add two variable references (aa and changed) in aa-mergeprof
ask_the_questions() so that the code can use the short name and be more
in sync with aa.py ask_the_questions().
With this patch applied, the "for ruletype in ['capability', 'network']:"
block is in sync, with the exception of the sections that intentionally
differ:
- the check for the profile mode
- the default button selection based on profile mode
- the seen_events counter
The patch also includes some minor whitespace fixes.
Acked-by: Steve Beattie <steve@nxnw.org>
This allows to drop the "apparmor.aa." prefix in ask_the_question() to
get the code more in sync with aa.py ask_the_question().
Acked-by: Steve Beattie <steve@nxnw.org>
Move the code to set q.headers, q.functions and q.default for network
and capability rules inside the "while not done" loop. This ensures to
always have valid headers (for example, after changing the audit
qualifier, the severity was "lost" before) and avoids some duplicated
code.
Also drop a useless "if True:" condition and change the whitespace of
the following lines.
Acked-by: Steve Beattie <steve@nxnw.org>
