Also add a rank_path() function to severity.py and change rank() to call
rank_path() for paths.
Long-term goal: get rid of the type "guessing" in rank()
Finally add some tests, mostly based on test-severity.py SeverityTest
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 means adding
- self.can_edit - True if editing via '(N)ew' should be possible (will
be False for bare file rules)
- edit_header() - returns the prompt text and the current path
- validate_edit() - checks if the new path matches the original one
- store_edit() - changes the path to the new one (even if it doesn't
match the old one)
self.can_edit and the 3 functions are also added to BaseRule:
- can_edit is False by default
- the functions raise a NotImplementedError
Also add tests for the added code.
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>
Add the glob() and glob_ext() functions to FileRule, and set
self.can_glob and self.can_glob_ext. Also add some tests (just enough to
make sure the FileRule integration works - the globbing is handled
inside AARE,and the AARE tests contain more testcases).
Note that the implementation differs from the original plan (which was
to have globbing in *Ruleset). Therefore add can_glob and can_glob_ext
to BaseRule (both default to False), and add a comment to BaseRuleset
that globbing needs to be removed from all *Ruleset classes.
Acked-by: Steve Beattie <steve@nxnw.org>
As discussed, I added a pointer to the test-aare.py globbing tests in
test-file.py.
glob_path() and glob_path_ext() modify a (path) regex, so move them to
AARE. Also change them to use self.regex instead of the newpath
parameter, and to return a new AARE object.
While on it, also add several tests to test-aare.py.
Note: There are still glob_path() and glob_path_ext() calls in aa.py,
but those calls are in a (since the middle of this patch series) dead
code section. pyflakes will complain about them nevertheless ;-)
Acked-by: Steve Beattie <steve@nxnw.org>
This patch changes handle_children() (which asks about exec events) and
ask_the_questions() (which asks everything else) to FileRule. This
solves the "brain split" introduced by the previous patch.
This means aa-logprof and aa-genprof ask useful questions again, and
store the answers at the right place.
In detail, this means (with '-' line number from the diff)
- (391) handle_binfmt(): use FileRule. Also avoid breakage if glob_common()
returns an empty result.
- (484) profile_storage(): drop profile['allow']['path'] and
profile['deny']['path']
- (510) create_new_profile(): switch to FileRule
- (1190..1432) lots of changes in handle_children():
- drop escaping (done in FileRule)
- don't add events with 'x' perms to prelog
- use is_known_rule() instead of profile_known_exec()
- replace several regexes for the selected CMD_* with more readable
'in' clauses. While on it, drop unused parts of the regex.
- use plain 'ix', 'px' (as str) instead of str_to_mode() format
- call handle_binfmt() for the interpreter in ix, Pix and Cix rules
- (1652) ask_the_questions(): disable the old file-specific code
(not dropped because some features aren't ported to FileRule yet)
- (2336) collapse_log():
- convert file log events to FileRule (and add some workarounds and
TODOs for logparser.py behaviour that needs to change)
- disable the old file-specific code (not dropped because merging of
existing permissions isn't ported to FileRule yet)
- (2403) drop now unused validate_profile_mode() and the regexes it used
- (3374) drop now unused profile_known_exec()
Test changes:
- adjust fake_ldd to handle /bin/bash
- change test-aa.py AaTest_create_new_profile to expect FileRule instead
of a path hasher. Also copy the profiles to the tempdir and load the
abstractions that are needed by the test.
(These tests get skipped on py2 because changing
apparmor.aa.cfg['settings']['ldd'] doesn't work for some unknown reason)
Important: Some nice-to-have features are not yet implemented for
FileRule:
- globbing
- (N)ew (allowing the user to enter a custom path)
- displaying and merging of permissions already existing in the profile
This means: aa-logprof works, but it's not as user-friendly as before.
The next patches will fix that ;-)
Also note that pyflakes will fail for ask_the_questions_OLD_FILE_CODE()
because of undefined symbols (aamode, profile, hat). This will be fixed
when the old code gets dropped in one of the later patches.
Acked-by: Steve Beattie <steve@nxnw.org>
Bug: https://launchpad.net/bugs/1569316
Change aa.py to use FileRule and FileRuleset for parsing and saving
profiles.
In detail, this means:
- add 'file' to the list of rule classes to enable it at various places
- store file rules in aa[profile][hat]['file'] (not 'path' as before)
to be consistent with the FileRule name
- drop the no longer needed delete_path_duplicates() - this is now
handled by FileRuleset like in all other rule classes.
(same change in cleanprofile.py)
- replace usage of RE_PROFILE_BARE_FILE_ENTRY and RE_PROFILE_PATH_ENTRY
with FileRule.match()
- drop write_path_rules() and write_paths() and replace them with the
new write_file() function.
- adjust several code sections to use write_file() and 'file' instead of
'path'
FileRule doesn't drop optional keywords ('allow' and 'file'), therefore
adjust cleanprof_test.out to the changed behaviour. (If someone insists
on dropping optional keywords in aa-cleanprof, that's something for a
future patch.)
Also adjust the list of known failures in test-parser-simple-tests.py -
switching to FileRule avoids several test failures (and introduces a few
new ones ;-)
IMPORTANT:
This patch introduces a "brain split" which means
- parsing and writing the profile and aa-cleanprof use the new location
(aa[profile][hat]['file'])
- aa-logprof and aa-genprof still save data to the old location
(aa[profile][hat]['allow']['path']) and probably ask superfluous
questions because there are no rules existing in the old location
TL;DR: don't try aa-logprof or aa-genprof with only this patch applied.
I know this isn't ideal, but still better than an even bigger and
totally unreadable patch ;-)
Acked-by: Steve Beattie <steve@nxnw.org>
aa-logprof needs to check if an exec rule for a given path exists.
This patch adds a __FileAnyExec class to FileRule, as well as ANY_EXEC
(which should be used externally, similar to ALL), and adjusts several
checks to allow it as a special execute mode.
This will allow to use is_covered() (or aa.py is_known_rule()) to find
out if execute is permitted, which replaces aa.py profile_known_exec()
in one of the following patches.
As usual, also add some tests.
Acked-by: Steve Beattie <steve@nxnw.org>
Note: as discussed, I adjusted the comment for 'pass' around line 240.
Patch 14 will drop the RE_PROFILE_PATH_ENTRY and
RE_PROFILE_BARE_FILE_ENTRY import from apparmor.aa.
This would break test-regex_matches.py, therefore
import these regexes from apparmor.regex.
Acked-by: Steve Beattie <steve@nxnw.org>
The parser accepts duplicated execute permissions as long as they don't
conflict. For example,
/bin/foo pxpxpxpx,
is a valid rule.
This patch changes FileRule to also accept those duplicated permissions,
even if it's unlikely to hit them outside of the parser tests ;-)
Also add some tests to make sure the parsing works as expected.
Acked-by: Steve Beattie <steve@nxnw.org>
RE_PATH expected (simplified) '/.+', however this excludes a plain '/'
that can appear in path rules.
This patch changes the regex so that it also matches '/'.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
After dropping the dead code in handle_children(), there's only one use
of contains() left in log_str_to_mode().
This patch changes log_str_to_mode to use mode_contains() and drops the
now unused contains() function.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The 'exec' handling in handle_children starts with
if do_execute:
if profile_known_exec(...)
continue
which means if profile_known_exec() returns True, the rest of the loop
will be skipped. profile_known_exec() will return True if it finds an
exec rule in the profile or an include (independent of the exec type,
and (thanks to rematchfrag()) even if the path is globbed.
Later in the loop, there are checks for various exec modes - but those
checks can only be reached without an existing x rule, so they'll never
be hit.
This patch removes the dead code in the handle_children() / 'exec' / 'no
existing x rule found' section.
I confirmed that this code is really dead by
a) reading the code and, after being confused
b) two manual aa-logprof runs with coverage enabled - in one of them, I
added some ix, Px and Cx rules, and in the second one, no more exec
rules were needed/asked.
After dropping the dead code, combinedmode and combinedaudit are no
longer used, so we can also drop the code that sets those variables.
Sidenote: this patch drops 2% of the lines in aa.py ;-)
Acked-by: Seth Arnold <seth.arnold@canonical.com>
These classes handle file rules, including file rules with leading
perms, and are meant to replace lots of file rule code in aa.py and
aa-mergeprof.
Note: get_glob() and logprof_header_localvars() don't even look
finalized and will be changed in a later patch. (Some other things will
also be changed or added with later patches - but you probably won't
notice them while reviewing this patch.)
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: Kshitij Gupta <kgupta8592@gmail.com> (with some suggestions for a follow-up patch)
v1.1: remove 'and not deny' from a condition in split_perms() to get
more helpful error messages for rules like "deny /foo pix,"
Acked-by: Steve Beattie <steve@nxnw.org>
_is_covered_list() has a sanity check that raises an exception if both
other_value and other_all evaluate to False. This breaks when using
_is_covered_list() for FileRule.perms which can be empty if exec_perms
are specified.
This patch adds an optional parameter that allows to skip the sanity
check.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
For now, use an additional regex RE_PROFILE_FILE_ENTRY to avoid
breakage of the existing code by the added match groups.
The regex includes support for file rules with leading and trailing
permissions as well as bare file rules.
Note: even with the restriction to the permission letters we actually
use, it's in theory still possible that a future additional rule type or
permission letter might lead to additional matches for other rule types.
Therefore the parsing code should check for all other rule types before
matching for file rules.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
File permissions can be an empty list (if only exec permissions are
specified). This patch adds the optional allow_empty_list parameter so
that the function can handle this case.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
File rules contain some optional details (like leading permissions and
the file keyword) which should be ignored in non-strict mode.
This patch passes through the 'strict' parameter to is_equal_localvars
and adds it as function parameter in all existing rule classes.
It also adjusts test-baserule.py to test with the additional parameter.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The features_struct.size variable is used to hold a buffer size and it
is also passed in as the size parameter to read(). It should be a size_t
instead of an int.
A new helper function, features_buffer_remaining(), is created to handle
the two places where the remaining bytes in the features buffer are
calculated.
This patch also changes the size parameter of load_features_dir() to a
size_t to match the same parameter of load_features_file() as well as
the features_struct.size change described above.
Two casts were needed when comparing signed types to unsigned types.
These casts are safe because the signed value is checked for "< 0"
immediately before the casts.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The load_features_file() function returned an int but calculated the
value by subtracting two pointers. On 64 bit systems, that results in a
64 bit value being represented as a 32 bit type.
Coverity CID #55992
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
In 2011 (r1803), the traceroute profile was changed to also match
/usr/bin/traceroute.db:
/usr/{sbin/traceroute,bin/traceroute.db} {
However, permissions for /usr/bin/traceroute.db were never added.
This patch fixes this.
While on it, also change the /usr/sbin/traceroute permissions from
rmix to the less confusing mrix.
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk, 2.10 and 2.9.
https://launchpad.net/bugs/1628745
The following upstream kernel commit changed the semantics of the exec
permission check in the 4.8 kernel:
commit 9f834ec18defc369d73ccf9e87a2790bfa05bf46
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date: Mon Aug 22 16:41:46 2016 -0700
binfmt_elf: switch to new creds when switching to new mm
That change means that the target profile of an exec transition must
have permission to map the binary being executed. This patch fixes
regression test failures while the exec_stack.sh test is running against
4.8 and newer kernels by granting mapping permission to the target
profile.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
For reasons that aren't entirely clear, the action to set
apparmor.aa.cfg['settings']['ldd'] to './fake_ldd' does not actually
work on python2.7, get_reqs() tries to use /usr/bin/ldd anyway (printing
out the contents of apparmor.aa.cfg['settings']['ldd'] after the set
operation shows it to still contain '/usr/bin/ldd' o.O). Therefore, skip
these two tests when running under python2.7.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
Bug: https://launchpad.net/bugs/1522938
Fixes build error when attempting to build and test the 2.10.95 release
on Ubuntu 14.04:
$ (cd libraries/libapparmor/ && ./autogen.sh && ./configure && \
make && make check) > /dev/null
...
libtool: Version mismatch error. This is libtool 2.4.6 Debian-2.4.6-0.1, but the
libtool: definition of this LT_INIT comes from libtool 2.4.2.
libtool: You should recreate aclocal.m4 with macros from libtool 2.4.6 Debian-2.4.6-0.1
libtool: and run autoconf again.
make[2]: *** [grammar.lo] Error 63
make[1]: *** [all] Error 2
make: *** [all-recursive] Error 1
The --force option is needed to regenerate the libtool file in
libraries/libapparmor/.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
This is the least invasive solution to the problem I'm trying to solve
right now (Evince not starting in GNOME on Wayland, and probably
similar issues for other GNOME applications I suppose).
At some point, we will probably want to source the wayland abstraction
from other desktop environments' abstractions, or simply from the
X one. Let's come back to it once people using these other desktop
environments on Wayland with AppArmor enabled tell us what policy
change is needed to make it work for them.
profile_A//&:ns1://unconfined (mixed)
this is confusing and can even break some trusted helpers. The unconfined
profile has been special cased and now will report enforce when stacking
with unconfined
profile_A//&:ns1://unconfined (enforce)
This patch fixes the regression tests to work with this change
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Bug: https://launchpad.net/bugs/1521400
In Debian, gnome-session (3.20.1-2)'s changelog reads:
If /etc/gnome/defaults.list was modified by the system administrator,
the file is moved to /etc/xdg/gnome-mimeapps.list during the upgrade.
So we want to at least support /etc/xdg/gnome-mimeapps.list. And
while we're at it, let's support *-mimeapps.list instead of just
gnome-mimeapps.list, in case other desktop environments or derivatives
need such customizations.
In Debian, gnome-session (3.20.1-2)'s changelog reads:
If /etc/gnome/defaults.list was modified by the system administrator,
the file is moved to /etc/xdg/gnome-mimeapps.list during the upgrade.
So we want to at least support /etc/xdg/gnome-mimeapps.list. And while
we're at it, let's support *-mimeapps.list instead of just gnome-mimeapps.list,
in case other desktop environments or derivatives need such customizations.
This turned out to be a simple case of misinterpreting the promptUser()
result - it returns the answer and the selected option, and
"surprisingly" something like
('CMD_ADDHAT', 0)
never matched
'CMD_ADDHAT'
;-)
I also noticed that the new hat doesn't get initialized as
profile_storage(), and that the changed profile doesn't get marked as
changed. This is also fixed by this patch.
References: https://bugs.launchpad.net/apparmor/+bug/1538306
Acked-by: Steve Beattie <steve@nxnw.org> for trunk, 2.10 and 2.9
pyflakes3 doesn't check sys.version and therefore complains about
'unicode' being undefined.
This patch defines unicode as alias of str to make pyflakes3 happy, and
as a side effect, simplifies type_is_str().
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk and 2.10.
By calling self.delete() inside the delete_duplicates() loop, the
self.rules list was modified. This resulted in some rules not being
checked and therefore (some, not all) superfluous rules not being
removed.
This patch switches to a temporary variable to loop over, and rebuilds
self.rules with the rules that are not superfluous.
This also fixes some strange issues already marked with a "Huh?" comment
in the tests.
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk and 2.10.
Note that in 2.10 cleanprof_test.* doesn't contain a ptrace rule,
therefore the cleanprof_test.out change doesn't make sense for 2.10.
This is needed to delete kerberos ccache files, for details see
https://bugzilla.opensuse.org/show_bug.cgi?id=990006#c5
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk, 2.10 and 2.9.
Acked-by: Steve Beattie <steve@nxnw.org> for trunk, 2.10 and 2.9.
Network events can come with an operation= that looks like a file event.
Nevertheless, if the event has a typical network parameter (like
net_protocol) set, make sure to store the network-related flags in ev.
This fixes the test failure introduced in my last commit.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com> for trunk, 2.10 and 2.9
We already ignore network events that look like file events (based on
the operation keyword) if they have a request_mask of 'send' or
'receive' to avoid aa-logprof crashes because of "unknown" permissions.
It turned out that both can happen at once, so we should also ignore
this case.
Also add the now-ignored log event as test_multi testcase.
References: https://bugs.launchpad.net/apparmor/+bug/1577051#13
Acked-by: Tyler Hicks <tyhicks@canonical.com> for trunk, 2.10 and 2.9.
patches
0001-0022 are backports of fixes from the 4.8 pull-request
0023-0025 are the out of tree feature patches
Signed-off-by: John Johansen <john.johansen@canonical.com>
https://launchpad.net/bugs/1604872
dbus-user-session uses the file based Unix socket in $XDG_RUNTIME_DIR/bus.
Extend the dbus-session-strict abstraction to also allow that.
Acked-by: Tyler Hicks <tyhicks@canonical.com>
This is needed for winbindd (since samba 4.4.x), but smbd could also need it.
References: https://bugzilla.opensuse.org/show_bug.cgi?id=990006
Acked-by: Seth Arnold <seth.arnold@canonical.com> for trunk, 2.10 and 2.9.
https://launchpad.net/bugs/1584069
This patch adds support for the safe and unsafe exec modes for
change_profile rules. The logic is pretty simple at this point because
the kernel's default for exec modes changed in newer versions.
Therefore, this patch simply retains any specified exec mode in parsed
rules. If an exec mode is not specified in a rule, there is no attempt
to force the usage of "safe" because older kernels do not support it.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: Christian Boltz <apparmor@cboltz.de>
The onexec.sh test has periodically exhibited unexplicable failures that
are possibly due to race conditions when onexec.sh is verifying the
/proc/PID/attr/{current,exec} values of the process under test. This
patch attempts to solve the flaky test failures by removing the need for
IPC to coordinate between the test script and the test program.
The old onexec test program is removed and the transition test program
is used instead. This allows for the test script to tell the transition
test program what its current and exec procattr labels should be via
command line options.
Since IPC is no longer needed, the signal:ALL allow rule can be dropped
from the test profile. A new allow rule is needed to grant reading of
/proc/*/attr/{current,exec} since transition must verify the contents of
these files.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Add optional command line parameters to the transition test program that
can be used to verify a certain label and/or mode that should be found
in /proc/self/attr/exec.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
