The current behavior of priority rules can be non-intuitive with
higher priority rules completely overriding lower priority rules even in
permissions not held in common. This behavior does have use cases but
its can be very confusing, and does not normal policy behavior
Eg.
priority=0 allow r /**,
priority=1 deny w /**,
will result in no allowed permissions even though the deny rule is
only removing the w permission, beause the higher priority rule
completely over ride lower priority permissions sets (including
none shared permissions).
Instead move to tracking the priority at a per permission level. This
allows the w permission to still override at priority 1, while the
read permission is allowed at priority 0.
The final constructed state will still drop priority for the final
permission set on the state.
Note: this patch updates the equality tests for the cases where
the complete override behavior was being tested for.
The complete override behavior will be reintroduced in a future
patch with a keyword extension, enabling that behavior to be used
for ordered blocks etc.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The original patch adding priority to the set of prefixes failed to
update the prefix dump to include the priority priority field.
Fixes: e3fca60d1 ("parser: add the ability to specify a priority prefix to rules")
Signed-off-by: John Johansen <john.johansen@canonical.com>
The priority field is only used during state construction, and can
even prevent later optimizations like minimization. The parser already
explcitily clears the states priority field as part of the last thing
done during construction so it doesn't prevent minimization
optimizations.
This means the state priority not only wastes storage because it is
unused post construction but if used it could introduce regressions,
or other issues.
The change to the minimization tests just removes looking for the
priority field that is no longer reported.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Like was done for the other MatchFlags switch to using a node type
instead of dynamic_cast as this will result in a performance
improvement.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Introduce the EXPECT_DENIALS environment variable for profile tests.
Each line of EXPECT_DENIALS is a regex that must match an AppArmor
denial for the corresponding test, and conversely.
This ensures that problematic behaviors are correctly blocked and logged.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
Profile for `tar` package.
In order to test this, I've diffed the output of the `tar`'s testsuite with and without the profile:
```
sudo apt build-dep tar
apt source tar
cd tar-*/
./configure
cd tests/
./testsuite > without_profile.log
apparmor_parser ~/tar
./testsuite > with_profile.log
diff without_profile.log with_profile.log # should not output anything
echo $? # should be zero
```
Additionally, [the testsuite available on QRT](https://git.launchpad.net/qa-regression-testing/tree/scripts/test-tar.py) for the `tar` package should continue to pass after loading the profile.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1453
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
A number of tests are failing and since spread does not contain a native
XFAIL facility, we have to maintain a silent-failure feature code
ourselves. A few of those have been fixed since the first iteration of
this patch. The remaining known failures are being fixed.
Later on I would like to separate XFAIL from SKIP so that if a test is
known to exercise kernel feature unavailable on the given system, the
test is just not executed.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1483
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Python 3.13 changes the formatting of long-short option pairs that use a
meta-variable. Up until 3.13 the meta-variable was repeated. Since
Python change [1] the meta-var is only printed once.
[1] https://github.com/python/cpython/pull/103372
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This is so that we get a baseline that passes to enable testing in CI/CD
but also to spark a discussion around what to do with a profile that
indirectly relies on a kernel feature that is not available on a given
system.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
A number of tests are failing and since spread does not contain a native
XFAIL facility, we have to maintain a silent-failure feature code
ourselves. A few of those have been fixed since the first iteration of
this patch. The remaining known failures are being fixed.
Later on I would like to separate XFAIL from SKIP so that if a test is
known to exercise kernel feature unavailable on the given system, the
test is just not executed.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Since kernel commit 8c4b785a86be the class is available to check if
the log belongs to which class. This fixes cases where the logparser
is not able to distinguish between network and file operations.
This issue does not manifest previous to and including apparmor-4.0
because we did not process auditing logs then.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/478
Reported-by: vyomydv vyom.yadav@canonical.com
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The "last" command, which was supplied by util-linux in older Ubuntu
versions, is now supplied by wtmpdb in Oracular and Plucky. Unfortunately,
this changed the output format and broke our column based parsing.
While the wtmpdb upstream has added json support at
https://github.com/thkukuk/wtmpdb/issues/20, we cannot use it because
we need to support systems that do not have this new feature added.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Unfortunately we are affected by the backwards-incompatible change introduced by https://github.com/swig/swig/pull/2907
These wrappers will be needed to fix tests on systems using SWIG 4.3 or later, e.g. Ubuntu Plucky.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
By making the test a file to be included as a helper, we can reuse most of the code for a fuse_overlayfs test without copy-pasting
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
We now have GitLab CI/CD pipeline co-existing with spread, coupled with
image-garden and the cloud-init profile defined for each distribution.
To avoid duplicating list of required dependencies, re-use cloud-init
profile as the reference list of dependencies (superset between build
and test) to install.
In addition to the dependency list, the build_all job now re-uses spread
prepare section in similar fashion. If it builds in spread, it should
build in CI as well.
A small quality-of-life improvement is the shape of a collapsible
section around dependency installation should make reading job logs
easier.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1494
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Zygmunt Krynicki <me@zygoon.pl>
Our ubuntu packaging builds Python-enabled libapparmor's in the directories `libapparmor/libapparmor.python[version_identifier]`. In order for the util's `make check` to pick up on the correct libapparmor during the Ubuntu build process, we need the ability to override its search path. This patch introduces a `LIBAPPARMOR_BASEDIR` variable to allow for that.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1497
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
The first patch fixes a `test-aa-notify.py` `TypeError` when `APPARMOR_NOTIFY` and `__AA_CONFDIR` are both specified, which is something that was broken all this time.
The second patch ensures that `aa-notify` in the test suite is run using the same Python interpreter that the test suite itself is run with, which is necessary for testing the utils under different Pythons.
The third patch does analogous modifications to the minitools tests that launch `aa-audit`, `aa-complain`, etc.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1498
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
This should be a more readable example to follow in other tests. The
toybox test was special given the fact that it is a shell itself, and is
fairly programmable.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Using gdb in batch mode, put a breakpoint on _start and spawn the
process. Then using the built-in python interpreter print the
confinement label on the process and terminate everything.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
We now have GitLab CI/CD pipeline co-existing with spread, coupled with
image-garden and the cloud-init profile defined for each distribution.
To avoid duplicating list of required dependencies, re-use cloud-init
profile as the reference list of dependencies (superset between build
and test) to install.
In addition to the dependency list, the build_all job now re-uses spread
prepare section in similar fashion. If it builds in spread, it should
build in CI as well.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
os.environ returns a string, but the default value is a list, and the concatenation of __AA_CONFDIR assumes a list.
Thus, if APPARMOR_NOTIFY and __AA_CONFDIR were both specified, this would error out.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Tests that interact with the kernel are skipped (tests/regression and
tests/snapd) but everything else is green. Most of the tests are
actually passing. The only exception is the aa-notify test that was
broken by Python 3.13 stdlib change. The fix for that has been posted
separately.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1496
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Zygmunt Krynicki <me@zygoon.pl>
Tests that interact with the kernel are skipped (tests/regression and
tests/snapd) but everything else is green. Most of the tests are
actually passing. The only exception is the aa-notify test that was
broken by Python 3.13 stdlib change. The fix for that has been posted
separately.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Hopefully more and more profiles will come with smoke tests. Since the
pattern of those tests is likely to be very similar (compile profile,
run some programs, remove profile) it will be good to check if the
profile had caused any denials to be logged. Having this at the suite
level should make writing actual tests easier.
The prepare-each and restore-each logic compile the profile, check for
errors and finally remove the profile. The debug-each logic shows the
program name (with full path).
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Those fell under the radar during the initial push to expose all of
the tests to spread.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The openSUSE project has decided to switch to security=selinux by
default. For the purpose of continuing to test AppArmor on the
distribution, alter the cloud-init profile to switch to booting with
security=apparmor.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
There is no other use of this yaml fragment in the project so inline it
for simplicity.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The "only" feature has been deprecated for a while. The standard
replacement is the rules:if feature.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This makes the snapd/mount-control test pass on all the currently tested systems. Note that there's a somewhat complex problem with the new mount APIs (https://lwn.net/Articles/753473/) from 2018 that are now being used on, for example, Debian 13.
I will need to make similar changes to the profiles generated by snapd, so any insight on what to do there is strongly appreciated.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1479
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
In aa-notify, notifications are now merged by default to reduce the risk
of flooding.
Additionally, we now use an exponential backoff algorithm for the
merging time period. If there is several notications within a time
period, it doubles, up to a maximum. The time period shrinks if there is
no notification. The time period is reset if the user clicks on a
notifiation
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1468
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
lastlog2 is the 2038-safe replacement for wtmp, and in the meantime
became part of util-linux.
This commit switches from trying to parse the lastlog2 output to
directly reading lastlog2.db with sqlite3.
Adjust get_last_login_timestamp() to use the lastlog2 database
(/var/lib/lastlog/lastlog2.db) if it exists, and adjust
get_last_login_timestamp_lastlog2() to actually do that.
(If lastlog2.db doesn't exist, aa-notify will read wtmp as usual.)
Unfortunately lastlog2 doesn't have a way to get machine-readable output
(for example json), therefore - after trying and failing to parse the
lastlog2 output - directly read from lastlog2.db. Let's hope the format
never changes ;-)
Fixes: https://bugzilla.opensuse.org/show_bug.cgi?id=1228378
Fixes: https://bugzilla.opensuse.org/show_bug.cgi?id=1216660
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/372
lastlog2 is the 2038-safe replacement for wtmp, and in the meantime
became part of util-linux.
Adjust get_last_login_timestamp() to use lastlog2 if it exists, and add
get_last_login_timestamp_lastlog2() to actually do that.
(If lastlog2 doesn't exist, aa-notify will read wtmp as usual.)
Unfortunately lastlog2 doesn't have a way to get machine-readable output
(for example json), therefore we have to parse the output that is meant
for humans. Let's hope the format never changes ;-)
(The alternative would have been to use squlite3 to once more read the
data behind the official program's back, but that was already a bad idea
for wtmp, therefore I decided against it.)
Fixes: https://bugzilla.opensuse.org/show_bug.cgi?id=1228378
Fixes: https://bugzilla.opensuse.org/show_bug.cgi?id=1216660
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/372
... and add a wrapper function with the old name
Also rename the tests to the new name, and create a copy with the
original name. The copy will be adjusted to also check/expect lastlog2
results in a later commit.
Add a basic overview of the ordering of the backend of the compiler
and which stages specific dump info lines up with.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The BASH_XTRACEFD variable can be used to redirect "set -x" traces
to a dedicated file. We can use it to split the execution trace
(what has actually happened) from the failure messages.
On a failing test this does provide improved clarity when debugging
interactively with "spread -debug". On non-interactive runs the now
shorter error list is also implicitly printed.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1481
Approved-by: Christian Boltz <apparmor@cboltz.de>
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Christian Boltz <apparmor@cboltz.de>
The BASH_XTRACEFD variable can be used to redirect "set -x" traces
to a dedicated file. We can use it to split the execution trace
(what has actually happened) from the failure messages.
On a failing test this does provide improved clarity when debugging
interactively with "spread -debug". On non-interactive runs the now
shorter error list is also implicitly printed.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The package is required by the file_unbindable_mount regression test.
To properly re-generate affected images please update image-garden
to version containing 9714dc45d0ef06862ffe7037193dc43386db48ea
(Tie .user-data and .meta-data to MAKEFILE_LIST).
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
In addition allow linking to libeconf, generalize locale paths to cover
values other than C.UTF-8 and allow reading system-wide locale.alias and
gconv modules.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This is not the best of fixes but it seems that on Debian 13, with new
libmount calling fsopen/fsconfig/move_mount, the current apparmor mount
rule is insufficient to allow the call to go through.
The key problems are:
- the fstype is not visible to LSM
- the source directory is an empty string
- the mount is moved to final position
I don't know the extent of "new" mount API coverage by LSM hooks but
I think we should either synthesize new permissions from old rules,
.e.g match each of the system calls against what the mount class
expression, or somehow allow the exceptions better.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Construction of the chfa can reorder states from what the numbering
given during the hfa constuctions because of reordering for better
compression, dead state removal to ensure better packing etc.
This however means the dfa dump is difficult (it is possible using
multiple dumpes) to match up to the chfa that the kernel is
using. Make this easier by making the dfa dump be able to take
the remapping as input, and provide an option to dump the
chfa equivalent hfa.
Renumbered states will show up as {new <== {orig}} in the dump
Eg.
```
--D dfa-states
{1} <== priority (allow/deny/prompt/audit/quiet)
{5} 0 (0x 4/0//0/0/0)
{1} perms: none
0x2 -> {5} 0 (0x 4/0//0/0/0)
0x4 -> {5} 0 (0x 4/0//0/0/0)
\a 0x7 -> {5} 0 (0x 4/0//0/0/0)
\t 0x9 -> {5} 0 (0x 4/0//0/0/0)
\n 0xa -> {5} 0 (0x 4/0//0/0/0)
\ 0x20 -> {5} 0 (0x 4/0//0/0/0)
4 0x34 -> {3}
{3} perms: none
0x0 -> {6}
{6} perms: none
1 0x31 -> {5} 0 (0x 4/0//0/0/0)
```
```
-D dfa-compressed-states
{1} <== priority (allow/deny/prompt/audit/quiet)
{2 == {5}} 0 (0x 4/0//0/0/0)
{1} perms: none
0x2 -> {2 == {5}} 0 (0x 4/0//0/0/0)
0x4 -> {2 == {5}} 0 (0x 4/0//0/0/0)
\a 0x7 -> {2 == {5}} 0 (0x 4/0//0/0/0)
\t 0x9 -> {2 == {5}} 0 (0x 4/0//0/0/0)
\n 0xa -> {2 == {5}} 0 (0x 4/0//0/0/0)
\ 0x20 -> {2 == {5}} 0 (0x 4/0//0/0/0)
4 0x34 -> {3}
{3} perms: none
0x0 -> {4 == {6}}
{4 == {6}} perms: none
1 0x31 -> {2 == {5}} 0 (0x 4/0//0/0/0)
```
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1474
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
I had this message in my log
```
Dez 30 08:14:46 kernel: audit: type=1400 audit(1735542886.787:307): apparmor="DENIED" operation="open" class="file" profile="/usr/sbin/cupsd" name="/etc/paperspecs" pid=317509 comm="cupsd" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
```
If the second commit is bad, I can drop it.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1472
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Construction of the chfa can reorder states from what the numbering
given during the hfa constuctions because of reordering for better
compression, dead state removal to ensure better packing etc.
This however means the dfa dump is difficult (it is possible using
multiple dumpes) to match up to the chfa that the kernel is
using. Make this easier by making the dfa dump be able to take the
emapping as input, and provide an option to dump the chfa equivalent
hfa.
Renumbered states will show up as {new <== {orig}} in the dump
Eg.
--D dfa-states
{1} <== priority (allow/deny/prompt/audit/quiet)
{5} 0 (0x 4/0//0/0/0)
{1} perms: none
0x2 -> {5} 0 (0x 4/0//0/0/0)
0x4 -> {5} 0 (0x 4/0//0/0/0)
\a 0x7 -> {5} 0 (0x 4/0//0/0/0)
\t 0x9 -> {5} 0 (0x 4/0//0/0/0)
\n 0xa -> {5} 0 (0x 4/0//0/0/0)
\ 0x20 -> {5} 0 (0x 4/0//0/0/0)
4 0x34 -> {3}
{3} perms: none
0x0 -> {6}
{6} perms: none
1 0x31 -> {5} 0 (0x 4/0//0/0/0)
-D dfa-compressed-states
{1} <== priority (allow/deny/prompt/audit/quiet)
{2 == {5}} 0 (0x 4/0//0/0/0)
{1} perms: none
0x2 -> {2 == {5}} 0 (0x 4/0//0/0/0)
0x4 -> {2 == {5}} 0 (0x 4/0//0/0/0)
\a 0x7 -> {2 == {5}} 0 (0x 4/0//0/0/0)
\t 0x9 -> {2 == {5}} 0 (0x 4/0//0/0/0)
\n 0xa -> {2 == {5}} 0 (0x 4/0//0/0/0)
\ 0x20 -> {2 == {5}} 0 (0x 4/0//0/0/0)
4 0x34 -> {3}
{3} perms: none
0x0 -> {4 == {6}}
{4 == {6}} perms: none
1 0x31 -> {2 == {5}} 0 (0x 4/0//0/0/0)
Signed-off-by: John Johansen <john.johansen@canonical.com>
Currently states are added to the reachable set when they are popped
from the workqueue. This however can result in states being
added to the work queue multiple times and reprocessed.
```
Eg. If state 2 has the transitions, and 9 is not in the reachable set
a -> 9
b -> 9
c -> 9
d -> 9
e -> 3
```
then 9 will get pushed onto the work 4 times. Even worse other states
on the workqueue may also add state 9 to the workqueue because it has
not been added to the reachable set.
Instead add states to the reachable set when they are added to the
workqueue. The first encounter with a state will result in it being
reachable and all other encounters will see that it already in the set
and not add it to the workqueue.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1473
Acked-by: seth.arnold@gmail.com
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Currently states are added to the reachable set when they are popped
from the workqueue. This however can result in states being
added to the work queue multiple times and reprocessed.
Eg. If state 2 has the transitions, and 9 is not in the reachable set
a -> 9
b -> 9
c -> 9
d -> 9
e -> 3
then 9 will get pushed onto the work 4 times. Even worse other states
on the workqueue may also add state 9 to the workqueue because it has
not been added to the reachable set.
Instead add states to the reachable set when they are added to the
workqueue. The first encounter with a state will result in it being
reachable and all other encounters will see that it already in the set
and not add it to the workqueue.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Gtk applications like Firefox request write access to the file
`/run/user/1000/dconf/user`. The code in `dconf_shm_open` opens the file
with `O_RDWR | O_CREAT`.
4057f8c84f/shm/dconf-shm.c (L68)
Fix priority for file rules, and the ability to dump the dfa at different stages, and update and fix the equality tests.
This in particular adds the ability to better debug the equality tests. Instead of just piping the parser output into the hash it creates a tmp dir and drops the binary files there so they can be manually examined. It adds new options particularly the -r option making so the tests will exit on first failure to make it easier to isolate and examine a failure.
Eg.
```
./equality.sh -r -d -v
Equality Tests:
................................................................................................................................................................................................................................
Binary inequality 'priority=-1'x'priority=-1' change_hat rules automatically inserted
FAIL: Hash values match
parser: ./../apparmor_parser -QKSq --features-file=./features_files/features.all
known-good (ee4f926922ecd341f1389a79dd155879) == profile-under-test (ee4f926922ecd341f1389a79dd155879) for the following profiles:
known-good /t { priority=-1 owner /proc/[0-9]*/attr/{apparmor/,}current a, ^test { priority=-1 owner /proc/[0-9]*/attr/{apparmor/,}current a, /f r, }}
profile-under-test /t { priority=-1 owner /proc/[0-9]*/attr/{apparmor/,}current w, ^test { priority=-1 owner /proc/[0-9]*/attr/{apparmor/,}current w, /f r, }}
files retained in "/tmp/eq.3240859-deHu10/"
```
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1455
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
There is a general industry wide effort to move off of md5 and even
sha1 (see recent kernel changes). While in this particular use case it
doesn't make a difference (besides slightly lowering the chance of a
collision) switch to sha256sum to make sure our code doesn't depend on
tools that are deprecated and there is an effort to remove.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Similar to the deny x permission tests, the tests that test carving
out r permissions need to be updated to be conditional on what
priority is being used on the rule.
Signed-off-by: John Johansen <john.johansen@canonical.com>
With priority rules, deny does not carve out permissions from the
higher priority rule. Technically it doesn't from lower priority either
as it completely overrides them, but that case already results in
an inequality so does not cause the tests to fail.
Signed-off-by: John Johansen <john.johansen@canonical.com>
cx rules using a specified profile transition, may be emulated by
using px and a hierarchical profile name. That is
cx -> b
may be transformed into
px -> profile//b
which will generate an xtable entry of
profile//b
which means the previous patch using
pivot_root -> b,
to reliably add b to the xtable will not cover this case.
transition to using two pivot_root rules to provide the xtable entries
pivot_root /a -> b,
pivot_root /c -> /t//b,
the paths /a and /c are irrelavent as long as they don't have an
overlap with the generic globbing expression in the test, Two table
entries will be generated. We guarantee no overlap by converting the
/** to /f**
Also the xtable reserving rules are moved to the end of the profile so
the table order can be reliably created. A follow on MR around xtable
improvements should add reliability to xtable order.
Signed-off-by: John Johansen <john.johansen@canonical.com>
exec rules that specify an specific target profile generate an entry
in the xtable. The test entries containing " -> b" are an example of
this.
Currently the parser allocates the xtable entry before priorities are
applied in the backend, or minimization is done. Further more the
parser does not ref count the xtable entry to know what it is no
longer referenced.
The equality tests generate rules that are designed to completely
override and remove a lower priority rule, and remove it. Eg.
/t { priority=1 /* ux, /f px -> b, }
and then compares the generated profile to the functionaly equivalent
profile eg.
/t { priority=1 /* ux, }
To verify the overridden rule has been completely removed.
Unfortunately the compilation is not removing the unused xtable entry
for the specified transition, causing the equality comparison to fail.
Ideally the parser should be fixed so unused xtable entries are removed,
but that should be done in a different MR, and have its own test.
To fix the current tests, and another rule that adds an xtable entry
to the same target that can not be overriden by the x rule using
pivot_root. The parser will dedup the xtable entry resulting in the
known and test profile both having the same xtable. So the test will
pass and meet the original goal of verifying the x rule being overriden
and eliminated.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Failed equality tests can be hard to debug. The profiles aren't always
enough to figure out what is going on. Add several options that will
help in debugging, and developing new tests.
Add switches and arg parsing.
Add the ability to run tests individually
Add a -r flag to allow retaining the test and output
similar to the regression tests, so the exact output from the
tests can be examined.
Add a -d flag to dump dfa build information.
Allow overriding the parser, features, and description for a given
test run.
Signed-off-by: John Johansen <john.johansen@canonical.com>
printf of failure/error info should be going to stderr. Unfortunately
the test has a mix of 2>&1 and 1>&2. Having a mix is just wrong, we
could standardize on either but since the info is error info 1>&2
seems to be the better choice.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The test was passing because the file priority was always zero bug
resulting in the priority rule always being correctly combined
with the specific match x rule, instead of overriding it.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The test was passing because the file priority always being zero bug,
the supplied rule always had the same priority as the implied
rule. Resulting in binary_equality always passing even though the
specified priority should have resulted in a failure.
Fix this by checking if the priorities are equal to the implied
rule other wise it should result in an inequality.
Signed-off-by: John Johansen <john.johansen@canonical.com>
When there is a failure output the exact call info used to invoke the
parser. To facilitate manually recreating the test.
Signed-off-by: John Johansen <john.johansen@canonical.com>
With the file priority fix the xequality (expected equal but known
failure) tests are now passing. So convert them to regular equality
tests.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The dfa goes through several stages during the build. Allow dumping it
at the various stages instead of only at the end.
Signed-off-by: John Johansen <john.johansen@canonical.com>
File rules could drop priority info when rule matched a rule
that was the same except for having different priority. For now
fix this by treating them as a different rule.
The priority was also be dropped when add_prefix was used to
add the priority during the parse resulting in file rules always
getting a default priority of 0.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Depending on the system, copying echo to the loop device fails because the echo binary is too large.
Especially on systems that have echo be just a symlink to coreutils (e.g. busybox) (as opposed to echo being its own binary) 16k is just not enough.
2M seems fine on my system, but this might need yet a higher value depending on what coreutils other people actually run.
The crash in question:
```
cp: error writing '/tmp/sdtest.3937422-31490-Bxvi6g/mount_target/echo': No space left on device
Fatal Error (file_unbindable_mount): Unexpected shell error. Run with -x to debug
rm: cannot remove '/tmp/sdtest.3937422-31490-Bxvi6g/mount_target': Device or resource busy
```
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1469
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Depending on the system, copying echo to the loop device fails because the echo binary is too large.
Especially on systems that have echo be just a symlink to coreutils (e.g. busybox) 16k is just not enough.
2M seems fine on my system, but this might need yet a higher value depending on what coreutils other people actually run.
The actual loop device needs to be larger to properly fit the allocated file size. Testing shows 4M is sufficient, but this is basically arbitrary.
- previously, aa-status --json --show profiles would return non-standard json
- adding the --pretty flag would crash completely
- closes#470
Things done:
- removed trailing ", " in json generation
- generate json seperator (", ") for each new json field
(profiles/processes) after the header if json is enabled
Tested on NixOS and apparmor 4.0.3 base, but should work on any version the patch applies on.
Closes#470
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1451
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
This MR is meant to resolve warnings such as "Warning: execname '/home/username/Documents/apparmor/tests/regression/apparmor/file_unbindable_mount': no such file or directory" when running tests like the one in the current version of !1448.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1450
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Starting with Python 3.8, you can use the PYTHONPYCACHEPREFIX environment
variable to define a cache directory for Python [1]. I think most people would set
this dir to @{HOME}/.cache/python/ , so the python abstraction should allow
writing to this location.
[1]: https://docs.python.org/3/using/cmdline.html#envvar-PYTHONPYCACHEPREFIX
TL;DR: Replace `aa[profile][hat]` with `active_profiles['profile//hat']` as a preparation to get rid of `aa`'s limits, especially to enable handling nested childs.
Since this is an extremely shortened summary, I recommend to check the individual commits for a readable and understandable diff and more details.
Note that this MR is "just" a preparation - nested childs are not supported yet. Also, `include` still uses the old structure. Both will be separate MRs - this one is already big enough ;-)
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1360
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
Note that the old code assigned dummy_prof to aa[profile][hat] and
active_profiles[profile] (= the main/parent profile) - which is
diffferent when testing a log for a child profile.
aa[profile][hat] was the wrong place - but since we used exactly that
again when checking for added exec rules, this error was hidden.
Now that the test is switched to using active_profiles, only check the
main profile for exec rules added by ask_exec(). (This will need to be
adjusted when we add a test for exec rules/events in nested childs, but
not earlier ;-)
This is mostly a search-and-replace patch.
In most cases, that means replacing `aa[profile][hat]` with
`active_profiles[full_profile]`.
In cases where the main/parent profile is meant, switch from
`aa[profile][profile]` to `active_profiles[profile]`.
Checks like `p in apparmor.aa` that check if a (main) profile exists
become `active_profiles.profile_exists(p)`.
write_profile() gets changed to loop over
`active_profiles.get_profile_and_childs()` which makes the code simpler.
`split_to_merged(aa)` becomes just `active_profiles`.
The only change that is not search-and-replace style is in
write_piece(). It expects a dict (not a ProfileList), therefore adjust
serialize_profile() so that it always hands over a dict.
This also changes the internal structure - instead of the nested dict
original_aa[profile][hat], we now have a ProfileList original_profiles[profile//hat].
Drop `comment.replace('\\n', '\n')` because that doesn't make sense and
doesn't change anything - not even a comment that contains the literal
string '\n' (backslash + letter n).
Besides that, get rid of the 'string' variable and store everything in
'data'.
... including just-created child profiles and hats.
Also ensure that serialize_profile() doesn't print them out as child
profiles AND external hats.
This commit includes a bugfix for a rare corner case:
Since create_new_profile() can return more than one profile if the
program has required_hats, add all of them to active_profiles.
(aa only got the expected profile added, but not the required_hats.)
... and make it non-optional
Note that read_profile() in aa.py skips child profiles and hats,
therefore active_profiles for now only contains the main profiles.
While the mount syscall documentation disallows this, the kernel silently
ignores make-* flags when doing a remount, and real applications were
passing this conflicting set of flags. Because changing the kernel to
reject this combination would break userspace, we should allow them
instead.
For an example: see https://bugs.launchpad.net/apparmor/+bug/2091424.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
The previous code would concatenate all of them together without spacing.
While dump_flags and the corresponding operator<< function aren't currently used,
this will help for when dump_flags is used to debug parser problems.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Swap on ZFS is *weird*. Getting it working needs some special casing, see e.g. https://askubuntu.com/questions/1198903/can-not-use-swap-file-on-zfs-files-with-holes
Currently, the swap regression test fails on my system (with /tmp in zfs):
```bash
tests/regression/apparmor ❯ ./swap.sh
Error: swap failed. Test 'SWAPON (unconfined)' was expected to 'pass'. Reason for failure 'FAIL: swapon /tmp/sdtest.872368-19048-kN4FN2/swapfile failed - Invalid argument'
Error: swap failed. Test 'SWAPOFF (unconfined)' was expected to 'pass'. Reason for failure 'FAIL: swapoff /tmp/sdtest.872368-19048-kN4FN2/swapfile failed - Invalid argument'
swapon: /tmp/sdtest.872368-19048-kN4FN2/swapfile: skipping - it appears to have holes.
Fatal Error (swap): Unexpected shell error. Run with -x to debug
```
However, just doing a file mount does make the test work on zfs, similar to how it is done with tmpfs. This means we don't need any special-casing for zfs beyond what is already there for working around (similar) tmpfs limitations.
Also, while researching this, it is possible a similar patch is needed for btrfs, but i currently don't have an easy way to test that.
This is non-breaking for anyone *not* using zfs, and it is currently broken with zfs anyways.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1462
Approved-by: Ryan Lee <rlee287@yahoo.com>
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
previously, this check would fail if the setuptools version would contain non-integers.
On my system, that is the case: `setuptools.__version__` is `'75.1.0.post0'`
I believe it is entirely fair to just check the relevant bits and refuse to continue if those can not be checked properly.
Having some extra slug on the version should not immediately cause issues (e.g. the `post0` here, or slugs like `beta`, `alpha` and the likes).
Probably only very few systems are running setuptools with weird version info, but supporting this is a simple one-line change i figured i might as well MR.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1460
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
previously, this check would fail if the setuptools version would contain non-integers.
On my system, that is the case: `setuptools.__version__` is `'75.1.0.post0'`
I believe it is entirely fair to just check the relevant bits and refuse to continue if those can not be checked properly.
But haviong something extra on the version should not immediately cause issues (e.g. the `post0` here, or slugs like `beta`, `alpha` and the likes).
Probably only very few systems are running setuptools with weird version info, but supporting this doesn't cost much, i believe.
This is neeed for "inherit owner = yes" in smb.conf.
From man smb.conf:
inherit owner (S)
The ownership of new files and directories is normally governed by
effective uid of the connected user. This option allows the Samba
administrator to specify that the ownership for new files and
directories should be controlled by the ownership of the parent
directory.
Fixes: https://bugzilla.suse.com/show_bug.cgi?id=1234327
I propose this fix for 3.x, 4.x and master.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1456
Approved-by: Ryan Lee <rlee287@yahoo.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
MS_SYNC is a flag for msync(2) while MS_SYNCHRONOUS is a flag for mount(2).
The header used to define MS_SYNC but IMO this is confusing since that's an
unrelated flag.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
By placing a bzImage into the top level of the AppArmor git repository one can
instruct spread and image-garden to use that image instead of booting
traditionally with an EFI / full disk image pair.
In addition, make error handling in qemu more robust, so failures are both
surfaced and do not cause endless attempts to allocate.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1452
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
By placing a bzImage into the top level of the AppArmor git repository one can
instruct spread and image-garden to use that image instead of booting
traditionally with an EFI / full disk image pair.
In addition, make error handling in qemu more robust, so failures are both
surfaced and do not cause endless attempts to allocate.
Please update image-garden to at least 5a00ead9964df6463e19432ae50e7760fc6da755
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This is neeed for "inherit owner = yes" in smb.conf.
From man smb.conf:
inherit owner (S)
The ownership of new files and directories is normally governed by
effective uid of the connected user. This option allows the Samba
administrator to specify that the ownership for new files and
directories should be controlled by the ownership of the parent
directory.
Fixes: https://bugzilla.suse.com/show_bug.cgi?id=1234327
When settest was called with two parameters, one for the test name and
the other for the test wrapper/binary, the profile created with
genprofile would show the test name, causing an error if the file
didn't exist.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Some of the tests using the --stdin option of mkprofile.pl are adding
more than one profile at a time. Whenever a profile is created in the
test, its name is added to the file profile.names so the test
infrastructure can tell if the profile is loaded or removed when
appropriately. The issue is that the name of the second profile
created by --stdin is not added, so these checks are not applied.
This patch adds the option of appending a second profile (not rules).
The option --append was used instead of a short -A because the short
options are arguments of mkprofile.pl, which --append is not.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The test adds a very small and simple smoke test that shows that a mount rule
with both fstype and options allows mounts to be performed on a real running
kernel.
The test is structured in a way that should make it easy to extend with new
variants (flags, fstype) in the future.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
- previously, aa-status --json --show profiles would return non-standard json
- adding the --pretty flag would crash completely
- closes#470
Things done:
- removed trailing ", " in json generation
- generate json seperator (", ") for each new json field
(profiles/processes) after the header if json is enabled
Tested on NixOS and apparmor 4.0.3 base, but should work on any version the patch applies on.
Due to how the tests were implemented in the past, permissions could
be passed along with the image name, and the permission part would be
discarded. The issue is that permissions are usually separated by ':',
but namespaces also contain ':', which would cause a conflict.
Since permissions are no longer passed as part of the image name,
remove that description so profile names in namespaces can be
supported.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Spread is a full-system, or integration test suite runner initially developed
to test snapd. Over time it has spread to other projects where it provides a
structured way to organize, run and debug complex full-system interactions.
Spread is documented on https://github.com/canonical/spread and is used in
production since late 2016.
Spread has a notion of backends which are responsible for allocating and
discarding test machines. For the purpose of running AppArmor regression tests,
I've combined spread with my own tool, image garden. The tool provides
off-the-shelf images, constructed on-the-fly from freely available images, and
makes them easily available to spread.
The reason for doing it this way is so that using non-free cloud systems is not
required and anyone can repeat the test process locally, on their own computer.
Vanilla spread is somewhat limited to x86-64 systems but the way I've used it
here makes it equally possible to test x86_64 *and* aarch64 systems. I've done
most of the development on an ARM single-board-computer running on my desk.
Spread requires a top-level spread.yaml file and a collection of task.yaml
files that describe individual tasks (for us, those are just tests). Tasks have
no implied dependency except that to reach a given task, spread will run all
the _prepare_ statements leading to that task, starting from the project, test
suite and then task. With proper care one can then run a specific individual
test with a one-line command, for example:
```
spread -v garden:ubuntu-cloud-24.04:tests/regression/apparmor:at_secure
```
This will prepare a fresh ubuntu-cloud-24.04 system (matching the CPU
architecture of the host), copy the project tree into the test machine, install
all the build dependencies, build all the parts of apparmor and then run one
specific variant of the regression test, namely the at_secure program.
Importantly the same test can also run on, say debian-cloud-13 (Debian Trixie),
but also, if you have a Google cloud account, on Google Compute Engine or in
one of the other backends either built into spread or available as a fork of
spread or as a helper for ad-hoc backend. Spread can also create more than one
worker per system and distribute the tests to all of the available instances.
In no way are we locking ourselves out of the ability to run our test suite on
our target of choice.
Spread has other useful switches, such as:
- `-reuse` for keeping machines around until discarded with -discard
- `-resend` for re-sending updated copy of the project (useful for -reuse)
- `-debug` for starting an interactive shell on any failure
- `-shell` for starting an interactive shell instead of the `execute` phase
This first patch contains just the spread elements, assuming that both spread
and image-garden are externally installed. A GitLab continuous integration
installing everything required and running a subset of tests will follow
shortly.
I've expanded the initial selection of systems to allow running all the tests
on several versions of Ubuntu, Debian and openSUSE, mainly as a sanity check
but also to showcase how practical spread is at covering real-world systems.
A number of tests are currently failing:
- garden:debian-cloud-12:tests/regression/apparmor:attach_disconnected
- garden:debian-cloud-12:tests/regression/apparmor:deleted
- garden:debian-cloud-12:tests/regression/apparmor:unix_fd_server
- garden:debian-cloud-12:tests/regression/apparmor:unix_socket_pathname
- garden:debian-cloud-13:tests/regression/apparmor:attach_disconnected
- garden:debian-cloud-13:tests/regression/apparmor:deleted
- garden:debian-cloud-13:tests/regression/apparmor:unix_fd_server
- garden:debian-cloud-13:tests/regression/apparmor:unix_socket_pathname
- garden:opensuse-cloud-15.6:tests/regression/apparmor:attach_disconnected
- garden:opensuse-cloud-15.6:tests/regression/apparmor:deleted
- garden:opensuse-cloud-15.6:tests/regression/apparmor:e2e
- garden:opensuse-cloud-15.6:tests/regression/apparmor:unix_fd_server
- garden:opensuse-cloud-15.6:tests/regression/apparmor:unix_socket_pathname
- garden:opensuse-cloud-15.6:tests/regression/apparmor:xattrs_profile
In addition, only on openSUSE, I've skipped the entire test suite of the utils
directory, as it requires python3 ttk themes, which I cannot find in packaged
form.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1432
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
It's no longer in python standard library starting
at version 3.13. Fixes:
root@qemuarm64:~# aa-complain /etc/apparmor.d/*
Traceback (most recent call last):
File "/usr/sbin/aa-complain", line 18, in <module>
from apparmor.fail import enable_aa_exception_handler
File "/usr/lib/python3.13/site-packages/apparmor/fail.py", line 12, in <module>
import cgitb
ModuleNotFoundError: No module named 'cgitb'
Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
Tests #466 but is marked as expected fail due to that bug not being resolved.
Depends on !1441 which adds the xfail infrastructure to the parser equality testing framework, and should be rebased on top of master once that MR is merged.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1443
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
- Tests defined in utils/test are now described by a task.yaml in the same
directory and can run concurrently across many machines.
- Tests for utils/ are now executed on openSUSE Tumbleweed since ttk themes is
no longer a hard dependency in master.
- Tests no longer run on openSUSE Leap 15.6 due to the age of default
Python (3.6) and gcc/g++. The tight integration with SWIG which does
not seem to support other Python versions very well. Perl hard-codes
old GCC for extension modules. The upcoming openSUSE Leap 16 should be
a viable target. In the meantime we can still test everything through
rolling-release Tumbleweed.
- Formatting of YAML files is now more uniform, at four spaces per tab.
- The run-spread.sh script is now in the root of the tree. The script allows
running all spread tests sequentially on one system, while collecting logs
and artifacts for convenient analysis after the fact.
- All systems are adjusted to run _four_ workers in parallel with _two_ virtual
cores each and equipped with 1.5GB of virtual memory. This aims to best
utilize the capacity of a typical CI worker with two to four cores and about
8GB of available memory.
- Failing tests are marked as such, so that as a whole the entire spread suite
can pass and be useful at catching regressions.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Compared to v1 the following improvements have been made:
- The cost of installing packages have been shifted from each startup to image
preparation phase, thanks to the integration of custom cloud-init profiles
into image-garden. This has dramatic impact on iteration time while also
entirely removing requirement to be online to run once a prepared image is
available.
- Support for running on Google Compute Engine has been removed since it would
not be able to use cloud-init the same way would currently only complicate
setup.
- The number of workers have been tuned for local iteration, aiming for
comfortable work with 16GB of memory on the host. Once CI/CD pipeline
support is introduced I will add a dedicated entry so that resources are
utilized well both locally and when running in CI.
- The set of regression tests listed in tests/regression/apparmor/task.yaml is
now cross-checked so introduction of a new test to the makefile there is
automatically flagged and causes spread to fail with a clear message.
- The task tests/unit/utils has been improved to generate profiles. Thanks to
Christian Boltz for explaining this relationship between tests.
- A number of comments have been improved and cleaned up for readability,
accuracy and sometimes better grammar.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Spread is a full-system, or integration test suite runner initially developed
to test snapd. Over time it has spread to other projects where it provides a
structured way to organize, run and debug complex full-system interactions.
Spread is documented on https://github.com/canonical/spread and is used in
production since late 2016.
Spread has a notion of backends which are responsible for allocating and
discarding test machines. For the purpose of running AppArmor regression tests,
I've combined spread with my own tool, image garden. The tool provides
off-the-shelf images, constructed on-the-fly from freely available images, and
makes them easily available to spread.
The reason for doing it this way is so that using non-free cloud systems is not
required and anyone can repeat the test process locally, on their own computer.
Vanilla spread is somewhat limited to x86-64 systems but the way I've used it
here makes it equally possible to test x86_64 *and* aarch64 systems. I've done
most of the development on an ARM single-board-computer running on my desk.
Spread requires a top-level spread.yaml file and a collection of task.yaml
files that describe individual tasks (for us, those are just tests). Tasks have
no implied dependency except that to reach a given task, spread will run all
the _prepare_ statements leading to that task, starting from the project, test
suite and then task. With proper care one can then run a specific individual
test with a one-line command, for example:
```
spread -v garden:ubuntu-cloud-24.04:tests/regression/apparmor:at_secure
```
This will prepare a fresh ubuntu-cloud-24.04 system (matching the CPU
architecture of the host), copy the project tree into the test machine, install
all the build dependencies, build all the parts of apparmor and then run one
specific variant of the regression test, namely the at_secure program.
Importantly the same test can also run on, say debian-cloud-13 (Debian Trixie),
but also, if you have a Google cloud account, on Google Compute Engine or in
one of the other backends either built into spread or available as a fork of
spread or as a helper for ad-hoc backend. Spread can also create more than one
worker per system and distribute the tests to all of the available instances.
In no way are we locking ourselves out of the ability to run our test suite on
our target of choice.
Spread has other useful switches, such as:
- `-reuse` for keeping machines around until discarded with -discard
- `-resend` for re-sending updated copy of the project (useful for -reuse)
- `-debug` for starting an interactive shell on any failure
- `-shell` for starting an interactive shell instead of the `execute` phase
This first patch contains just the spread elements, assuming that both spread
and image-garden are externally installed. A GitLab continuous integration
installing everything required and running a subset of tests will follow
shortly.
I've expanded the initial selection of systems to allow running all the tests
on several versions of Ubuntu, Debian and openSUSE, mainly as a sanity check
but also to showcase how practical spread is at covering real-world systems.
A number of systems and tests are currently failing:
- garden:debian-cloud-12:tests/regression/apparmor:attach_disconnected
- garden:debian-cloud-12:tests/regression/apparmor:deleted
- garden:debian-cloud-12:tests/regression/apparmor:unix_fd_server
- garden:debian-cloud-12:tests/regression/apparmor:unix_socket_pathname
- garden:debian-cloud-13:tests/regression/apparmor:attach_disconnected
- garden:debian-cloud-13:tests/regression/apparmor:deleted
- garden:debian-cloud-13:tests/regression/apparmor:unix_fd_server
- garden:debian-cloud-13:tests/regression/apparmor:unix_socket_pathname
- garden:opensuse-cloud-15.6:tests/regression/apparmor:attach_disconnected
- garden:opensuse-cloud-15.6:tests/regression/apparmor:deleted
- garden:opensuse-cloud-15.6:tests/regression/apparmor:e2e
- garden:opensuse-cloud-15.6:tests/regression/apparmor:unix_fd_server
- garden:opensuse-cloud-15.6:tests/regression/apparmor:unix_socket_pathname
- garden:opensuse-cloud-15.6:tests/regression/apparmor:xattrs_profile
- garden:opensuse-cloud-tumbleweed:tests/regression/apparmor:attach_disconnected
- garden:opensuse-cloud-tumbleweed:tests/regression/apparmor:deleted
- garden:opensuse-cloud-tumbleweed:tests/regression/apparmor:unix_fd_server
- garden:opensuse-cloud-tumbleweed:tests/regression/apparmor:unix_socket_pathname
- garden:ubuntu-cloud-22.04:tests/regression/apparmor:attach_disconnected
In addition, only on openSUSE, I've skipped the entire test suite of the utils
directory, as it requires python3 ttk themes, which I cannot find in packaged
form.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
The new check-one-test-% pattern rule allows running individual test scripts.
This allows them to be tested in parallel across many Make worker threads or
across many distinct machines with spread.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
When the test name and test binary differed and genprofile was used, there would be an execname warning about the original expected binary not existing. This fixes that warning.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
currently the equality tests require the tests to PASS as known equality
or inequality. Add the ability to add tests that are a known problem
and are expected to fail the equality, or inequality test.
This is done by using
verify_binary_xequality
verify_binary_xinequality
This allows new tests to be added to document a known issue, without
having to develop the fix for the issue. The use of this facility
is expected to be temporary, so any test marked as xequality or
xinequality will be noisy but not fail the other tests until they
are fixed, at which point they will cause the tests to fail to
force them to be updated to the correct equality or inequality
test.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1441
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
currently the equality tests require the tests to PASS as known equality
or inequality. Add the ability to add tests that are a known problem
and are expected to fail the equality, or inequality test.
This is done by using
verify_binary_xequality
verify_binary_xinequality
This allows new tests to be added to document a known issue, without
having to develop the fix for the issue. The use of this facility
is expected to be temporary, so any test marked as xequality or
xinequality will be noisy but not fail the other tests until they
are fixed, at which point they will cause the tests to fail to
force them to be updated to the correct equality or inequality
test.
Signed-off-by: John Johansen <john.johansen@canonical.com>
I am upstreaming this patch that is part of the nix package of apparmor for close to a year now.
This fixes the issue at https://github.com/NixOS/nixpkgs/issues/273164 for more distros than just NixOS.
The original merge Request on the nix side patching this was https://github.com/NixOS/nixpkgs/pull/285915.
However, people had issues with gitlab, so this never hit apparmor upstream until now. This does however also mean this patch has seen production and seems to work quite well.
## Original reasoning/message of the patch author:
This check is intended for ensuring that the profiles file can actually
be opened. The *actual* check is performed by the shell, not the read
utility, which won't even be executed if the input redirection (and
hence the test) fails.
If the test succeeds, though, using `read` here might actually
jeopardize the test result if there are no profiles loaded and the file
is empty.
This commit fixes that case by simply using `true` instead of `read`.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1438
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
This check is intended for ensuring that the profiles file can actually
be opened. The *actual* check is performed by the shell, not the read
utility, which won't even be executed if the input redirection (and
hence the test) fails.
If the test succeeds, though, using `read` here might actually
jeopardize the test result if there are no profiles loaded and the file
is empty.
This commit fixes that case by simply using `true` instead of `read`.
The regression test suite uses root with capabilities restricted in
several tests. This can cause the test suite to fail in weird and
confusing ways.
Add a test to check for DAC permissiosns from / to the testsuite
and abort running the tests with an error message if DAC permissions
are going to cause the test suite to fail.
Currently the test is pretty basic, but is better than nothing.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1411
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
Using this version of make:
```
GNU Make 4.2.1
Built for x86_64-suse-linux-gnu
```
I'm not entirely sure why but the alternative syntax I've used works correctly.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This fixes the test to pass on openSUSE Tumbleweed, where the small size
prevented alloction of an inode for the `lost+found` directory:
```
garden:opensuse-cloud-tumbleweed .../tests/regression/apparmor# mkfs.ext2 -F -m 0 -N 10 /tmp/sdtest.32929-21402-6x826m/image.ext3
mke2fs 1.47.0 (5-Feb-2023)
Discarding device blocks: done
Creating filesystem with 512 1k blocks and 8 inodes
Allocating group tables: done
Writing inode tables: done
ext2fs_mkdir: Could not allocate inode in ext2 filesystem while creating /lost+found
```
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
This fixes an error with Python 3.11:
```
test/test-parser-simple-tests.py:420:21: E502 the backslash is redundant between brackets
```
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Which is technically not POSIX and command -v works everywhere. This fixes
building and running the test suite on openSUSE Tumbleweed.
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
On a test system without bison installed, make setup fails with:
/bin/sh: 1: bison: not found
/bin/sh: 1: test: -ge: unexpected operator
Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Because this is used in parsing profiles, we keep backwards compatibility by including
both names and mapping them to the same underlying signal number.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Because sig_names is only used to dump parsed signals for debugging purposes,
renaming SIGTSTP "stp" to "tstp" is not a breaking change.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Match Flags convert output to hex but don't restore after outputting
the flag resulting in following numbers being hex encoded. This
results in dumps that can be confusing eg.
rule: \d2 -> \x2 priority=1001 (0x4/0)< 0x4>
rule: \d7 -> \a priority=3e9 (0x4/0)< 0x4>
rule: \d10 -> \n priority=3e9 (0x4/0)< 0x4>
rule: \d9 -> \t priority=3e9 (0x4/0)< 0x4>
rule: \d14 -> \xe priority=1001 (0x4/0)< 0x4>
where priority=3e9 is the hex encoded priority 1001.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1419
Approved-by: Maxime Bélair <maxime.belair@canonical.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
The old code implicitly initialized it to 0 by overwriting a
zero-initialized array terminator. Now that we construct the new entry
from scratch, we need to do this manually.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Most `tests/regression/apparmor/*.sh` scripts contain
. $bin/prologue.inc
This will explode if one of the parent directories contains a space.
Minimized reproducer:
```
# cat test.sh
pwd=`dirname $0`
pwd=`cd $pwd ; /bin/pwd`
bin=$pwd
echo "pwd: $bin"
. $bin/prologue.inc
# ./test.sh
pwd: /tmp/foo bar
./test.sh: line 9: /tmp/foo: No such file or directory
```
Notice that test.sh tries to source `/tmp/foo` instead of `/tmp/foo bar/prologue.inc`.
The fix is to quote the prologue.inc path:
. "$bin/prologue.inc"
While on it, also fix other uses of $bin - directly and indirectly - by quoting them.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1418
Approved-by: Ryan Lee <rlee287@yahoo.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Unfortunately, meaningfully parallelizing parser testing is a giant task:
- Parser equality testing is a shell script based framework where adding parallelism would be a major rework.
- Parser testing using Python’s unittest framework also needs a different test runner to enable parallelism.
- Parser testing using Perl’s prove framework already supports parallelism, but adding -j to Prove does not result in speedups. Thus, I suspect most of the overhead is in spawning the processes, and that speeding this part up will require making the parser a library and testing it that way.
The commit in this MR passes a `-j` parallelism flag to Perl's prove framework, but local testing has shown that this does not create speedups, and Gitlab CI has a very modest improvement of 11 minutes 16 seconds for the parser testing stage without `-j $(nproc)` vs 10 minutes 51 seconds with `-j $(nproc)`. Instead of passing `-j $(nproc)`, pass a fixed `-j 2` to gain some speedups, as the overhead of `-j $(nproc)` on a system with more than 2 cores eats up any time gains that parallelism would have brought.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1416
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
The regression test suite uses root with capabilities restricted in
several tests. This can cause the test suite to fail in weird and
confusing ways.
Add a test to check for DAC permissiosns from / to the testsuite
and abort running the tests with an error message if DAC permissions
are going to cause the test suite to fail.
Currently the test is pretty basic, but is better than nothing.
Signed-off-by: John Johansen <john.johansen@canonical.com>
This function was broken all this time: instead of duplicating each entry
in the list, it would duplicate the first entry n times. Since this
function is currently not used anywhere, delete it instead of fixing it.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This is simple enough to fix even if weld_file_to_policy isn't used in practice
with the compat layer that uses it being a target for deletion
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
There is a null check before storing invflags into inv, but not before initializing the value at inv to 0.
Assuming the null check is needed, it should be there in both places.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Match Flags convert output to hex but don't restore after outputting
the flag resulting in following numbers being hex encoded. This
results in dumps that can be confusing eg.
rule: \d2 -> \x2 priority=1001 (0x4/0)< 0x4>
rule: \d7 -> \a priority=3e9 (0x4/0)< 0x4>
rule: \d10 -> \n priority=3e9 (0x4/0)< 0x4>
rule: \d9 -> \t priority=3e9 (0x4/0)< 0x4>
rule: \d14 -> \xe priority=1001 (0x4/0)< 0x4>
where priority=3e9 is the hex encoded priority 1001.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The parser recently changed how/where deny information is applied.
commit 1fa45b7c1 ("parser: dfa minimization prepare for extended
permissions") removed the implicit filtering of explicit denies during
the minimization pass. The implicit clear allowed the explicit
information to be carried into the minimization pass and merged with
implicit denies. The end result being a minimized dfa with the explicit
deny information available to be applied post minimization, and
then dropped later at permission encoding in the accept entries.
Extended permission however enable carrying explicit deny information
into the kernel to fix certain bugs like complain mode not being
able to distinguish between implicit and explicit deny rules (ie.
deny rules get ignored in complain mode). However keeping explicit
deny information when unnecessary result in a larger state machine
than necessary and slower compiles.
commit 179c1c1ba ("parser: fix minimization check for filtering_deny")
Moved the explicit apply_and_clear_deny() pass to before minimization
to restore mnimization's ability to create a minimized dfa with
explicit and implicit deny information merged but this also cleared
the explicit deny information that used to be carried through
minimization. This meant that when the deny information was applied
post minimization it resulted in the audit and quiet information
being cleared.
This resulted in the query_label tests failing as they are checking
for the expected audit infomation in the permissions.
Fixes: 179c1c1ba ("parser: fix minimization check for filtering_deny")
Bug: https://gitlab.com/apparmor/apparmor/-/issues/461
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1408
Approved-by: Ryan Lee <rlee287@yahoo.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
... to avoid issues with spaces in a parent directory's name.
"Indirect uses" means usage of $bin via another variable, for example
`foo=$bin/whatever`
Most `tests/regression/apparmor/*.sh` scripts contain
. $bin/prologue.inc
This will explode if one of the parent directories contains a space.
Minimized reproducer:
```
pwd=`dirname $0`
pwd=`cd $pwd ; /bin/pwd`
bin=$pwd
echo "pwd: $bin"
. $bin/prologue.inc
pwd: /tmp/foo bar
./test.sh: line 9: /tmp/foo: No such file or directory
```
Notice that test.sh tries to source `/tmp/foo` instead of `/tmp/foo bar/prologue.inc`.
The fix - as done in this commit - is to quote the prologue.inc path:
. "$bin/prologue.inc"
Builds for risc64 are much slower than on other architectures (4-5
seconds with qemu-user or on Litchi Pi 4A).
Since the timeout is only meant as a safety net, increase it generously,
and hopefully for the last time.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/463
Besides of transitioning towards C++ this also eliminates the linear scan search that the functions using these arrays did.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Because the main overhead of the parser_sanity test suite is process
spawning, parallelizing too much could end up hurting performance instead
of helping it. Thus, use a fixed value of 2 instead of $(nproc).
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
The mapping of AA_CONT_MATCH was being dropped resulting in the
tcp tests failing because they would only match up to the first conditional
match check in the layout.
Bug: https://gitlab.com/apparmor/apparmor/-/issues/462
Fixes: e29f5ce5f ("parser: if extended perms are supported by the kernel build a permstable")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Instead of encoding permissions in the accept and accept2 tables
extended perms uses a permissions table and accept becomes an index
into the table.
Add the ability to dump the permissions table so that it can be
compared and debugged.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The chfa dump is missing information about the accept2 entry. The
accept2 information is necessary to help with debugging state machine
builds as accept2 is used to store quiet and audit information in the
old format or conditional information in the extended perms format.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The Makefile is missing some of its .h depenedncies causing compiles
to either fail or worse result in bad builds when rebuilding in an
already built tree.
Move the header dependencies into a variable and use it for each
target. While some targets don't need every include as a dependency
and this will result in unnecessary rebuilds in some cases, it makes
the Makefile cleaner, easier to maintain and makes sure a dependency
isn't accidentally missed.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The parser recently changed how/where deny information is applied.
commit 1fa45b7c1 ("parser: dfa minimization prepare for extended
permissions") removed the implicit filtering of explicit denies during
the minimization pass. The implicit clear allowed the explicit
information to be carried into the minimization pass and merged with
implicit denies. The end result being a minimized dfa with the explicit
deny information available to be applied post minimization, and
then dropped later at permission encoding in the accept entries.
Extended permission however enable carrying explicit deny information
into the kernel to fix certain bugs like complain mode not being
able to distinguish between implicit and explicit deny rules (ie.
deny rules get ignored in complain mode). However keeping explicit
deny information when unnecessary result in a larger state machine
than necessary and slower compiles.
commit 179c1c1ba ("parser: fix minimization check for filtering_deny")
Moved the explicit apply_and_clear_deny() pass to before minimization
to restore mnimization's ability to create a minimized dfa with
explicit and implicit deny information merged but this also cleared
the explicit deny information that used to be carried through
minimization. This meant that when the deny information was applied
post minimization it resulted in the audit and quiet information
being cleared.
This resulted in the query_label tests failing as they are checking
for the expected audit infomation in the permissions.
Fixes: 179c1c1ba ("parser: fix minimization check for filtering_deny")
Bug: https://gitlab.com/apparmor/apparmor/-/issues/461
Signed-off-by: John Johansen <john.johansen@canonical.com>
The prefix can be done in higher-level languages via slicing and having an explicit length exposes an out-of-bounds memory read footgun to those higher level languages
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Surprisingly, SWIG did not pick up the "typedef int pid_t" from the C headers.
As such, we need to provide our own wrapper. We don't just replicate the typdef
because we still support systems that have 16-bit PIDs.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
I don't know when (or even: if) this function was in use. A quick look
at the git history of aa.py shows that the function was (blindly?)
updated a few times. However, I didn't find a commit that uses or stops
using profile_exists(), so maybe it was never used at all.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1404
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
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.
This bug was introduced in 4e09f315c3, therefore I propose this patch for 3.0..master
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1403
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
In Python, return status is signalled by exceptions (or lack thereof)
instead of int. Keep the typemap portable for any other languages we may
add in the future.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This includes a custom typemap to handle (char **label, char **mode)
pairs and a cstring_output_allocate declaration for char **mnt.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
The only use of this _clone function passes in the same function that was
hardcoded, so this doesn't change any functionality.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Even if file descriptor values would not exercise the full range provided
by int, it doesn't hurt to allocate enough space for all ints.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
It is common for packaged PHP applications to ship a PHP-FPM
configuration using a scheme of "$app.sock" or or "$app.socket" instead
of using a generic FPM socket.
Signed-off-by: Georg Pfuetzenreuter <mail@georg-pfuetzenreuter.net>
I don't know when (or even: if) this function was in use. A quick look
at the git history of aa.py shows that the function was (blindly?)
updated a few times. However, I didn't find a commit that uses or stops
using profile_exists(), so maybe it was never used at all.
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.
ArchLinux ships a secondary PHP package called php-legacy with different
paths. As of now, the php-fpm profile will cover this binary but
inadequately restrict it.
Fixes: #454
Bash will try to read the passwd database to find the shell of a user if
$SHELL is not set. This causes zgrep to trigger
```
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/nsswitch.conf" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/passwd" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
```
if called in a sanitized environment. As the functionality of zgrep is
not impacted by a limited Bash environment, add deny rules to avoid the
potentially misleading AVC messages.
Signed-off-by: Georg Pfuetzenreuter <mail@georg-pfuetzenreuter.net>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1361
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Instead of always storing the name of the main profile, store the child
profile/hat name if we are in a child profile or hat.
As a result, we always get the correct "profile xy" header even for
child profiles when dumping the ProfileStorage object.
Also extend the tests to check that the name gets stored correctly.
.
Add aa-complain tests for profile with hats and subprofiles
So far, change_profile_flags() in aa.py is the only user of
ProfileStorage's 'name'.
Rewrite minitools test_cleanprof() so that most of its code can be
reused, and add a test that runs 'aa-complain
/usr/bin/a/simple/cleanprof/test/profile' on cleanprof.in to ensure
aa-complain still works as expected on subprofiles and hats.
Note: aa-complain $profilename will change the flags of hats, but not
child profiles. This is a known issue, and doesn't change with this MR.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1359
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Seen on various VMs, my guess is that bash wants to translate a uid to a
username.
Log events (slightly shortened)
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/nsswitch.conf" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/passwd" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
I propose this patch for 3.0..master
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1357
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
When the find fails but the insertion also fails, we leak the new node
that we generated. Delete the new node in this case to avoid leaking
memory.
The question remains, however, as to whether we should implement `operator==` in addition to `operator<` so that they are consistent with each other and `find` works correctly.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1399
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
When the find fails but the insertion also fails, we leak the new node
that we generated. Delete the new node in this case to avoid leaking
memory.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
commit 1fa45b7c1 ("parser: dfa minimization prepare for extended
permissions") removed implicit filtering of explicit denies in the
minimization pass (the information was ignored in building the set of
final accept states).
The filtering of explicit denies reduces the size of the produced
dfa. Since we need to be smarter about when explicit denies are
kept (eg. during complain mode), and most dfas are limited to 65k
states we currently need to filter explicit deny perms by default.
To compensate commit 2737cb2c2 ("parser: minimization - remove
unnecessary second minimization pass") moved the
apply_and_clear_deny() to before minimization. However its check to
apply removal denials before minimization is broken. Remove minimization
triggering apply_and_clear_deny() and just set the FILTER_DENY flag
by default, until we have better selection of rules/conditions where
explicit deny information should be carried through to the backend.
Fixes: 2737cb2c2 ("parser: minimization - remove unnecessary second minimization pass")
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1397
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
There is an integer overflow when comparing priorities when cmp is
used because it uses subtraction to find lessthan, equal, and greater
than in one operation.
But INT_MAX and INT_MIN are being used by priorities and this results
in INT_MAX - INT_MIN and INT_MIN - INT_MAX which are both overflows
causing an incorrect comparison result and selection of the wrong
rule permission.
Closes: https://gitlab.com/apparmor/apparmor/-/issues/452
Fixes: e3fca60d1 ("parser: add the ability to specify a priority prefix to rules")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Closes#452
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1396
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
There is an integer overflow when comparing priorities when cmp is
used because it uses subtraction to find lessthan, equal, and greater
than in one operation.
But INT_MAX and INT_MIN are being used by priorities and this results
in INT_MAX - INT_MIN and INT_MIN - INT_MAX which are both overflows
causing an incorrect comparison result and selection of the wrong
rule permission.
Closes: https://gitlab.com/apparmor/apparmor/-/issues/452
Fixes: e3fca60d1 ("parser: add the ability to specify a priority prefix to rules")
Signed-off-by: John Johansen <john.johansen@canonical.com>
commit 1fa45b7c1 ("parser: dfa minimization prepare for extended
permissions") removed implicit filtering of explicit denies in the
minimization pass (the information was ignored in building the set of
final accept states).
The filtering of explicit denies reduces the size of the produced
dfa. Since we need to be smarter about when explicit denies are
kept (eg. during complain mode), and most dfas are limited to 65k
states we currently need to filter explicit deny perms by default.
To compensate commit 2737cb2c2 ("parser: minimization - remove
unnecessary second minimization pass") moved the
apply_and_clear_deny() to before minimization. However its check to
apply removal denials before minimization is broken. Remove minimization
triggering apply_and_clear_deny() and just set the FILTER_DENY flag
by default, until we have better selection of rules/conditions where
explicit deny information should be carried through to the backend.
Fixes: 2737cb2c2 ("parser: minimization - remove unnecessary second minimization pass")
Signed-off-by: John Johansen <john.johansen@canonical.com>
'testcase01', 'testcase12' and 'testcase13' contain a strange mix of
exec and network events.
Nevertheless, there's enough information to parse them as good-enough
exec events. While this is not perfectly correct, it's better than
skipping these logs in this test.
Stop expecting that these profiles have a wrong content, and adjust them
so that they contain the (somewhat) expected exec rule.
So far, exec events were accidentally skipped in
test-libapparmor-test_multi.py because aa[profile][hat] was not
initialized, and ask_exec() exited early because of this.
Initialize aa[profile][hat] in the test to fix this.
To avoid that someone needs to select "inherit" each time the tests run,
add an optional default_ans parameter to ask_exec(), and let the test
call it with 'CMD_ix'.
(In case you wonder - defaulting to CMD_cx would ask to sanitize the
environment. CMD_ix avoids this.)
Also, we have to copy over aa[profile][hat] to log_dict in the test
because ask_exec() modifies aa[...], but the test only checks its local
log_dict.
Finally, add the expected exec rules to the *.profile files
peer name=... is invalid in dbus message rules.
Note that this testcase is currently disabled in the utils tests because
it's based on a multiline log.
It is handled correctly in the current codebase.
It would be even better if it would generate a link rule that includes
the source, but let's leave that for a later fix.
confirm_and_abort() is unused (note that a function with the same name
exists in ui.py and is used there)
Also delete the now-unused delete_profile() - luckily it was never used,
because it would also have deleted profiles that were "just" modified.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1388
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Christian Boltz <apparmor@cboltz.de>
confirm_and_abort() is unused (note that a function with the same name
exists in ui.py and is used there)
Also delete the now-unused delete_profile() - luckily it was never used,
because it would also have deleted profiles that were "just" modified.
Systemd's PrivateTmp= in transmission service is causing mount namespaces to be used leading to disconnected paths
[395201.414562] audit: type=1400 audit(1727277774.392:573): apparmor="ALLOWED" operation="sendmsg" class="file" info="Failed name lookup - disconnected path" error=-13 profile="transmission-daemon" name="run/systemd/notify" pid=193060 comm="transmission-da" requested_mask="w" denied_mask="w" fsuid=114 ouid=0
Fixes: https://bugs.launchpad.net/bugs/2083548
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1355
Approved-by: Ryan Lee <rlee287@yahoo.com>
Merged-by: Steve Beattie <steve+gitlab@nxnw.org>
The artifact restoration step does not preserve mtime, resulting in source files newer than built files, resulting in a needless rebuild of everything before actually running the tests.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
When a log like system.journal is passed on to aa-genprof, for
example, the user receives a TypeError exception: in method
'parse_record', argument 1 of type 'char *'
This patch catches that exception and displays a more meaningful
message.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/436
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
As for %s when the pointer is null: even if gcc prints (null) this is still undefined behavior, so we should do this explicitly
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
These are small changes to the man pages, with the most important one being updating some function signatures to be consistent with apparmor.h.
We should put together a man page for aalogparse functions too, but I'm submitting this MR first to get the smaller changes in faster.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1378
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Ryan Lee <rlee287@yahoo.com>
The timeout parameter for subprocess.Popen.communicate has been available since Python 3.3. Given the fragility of SIGALRM based mechanisms, there's no reason to reimplement our own timeout instead of using the built-in one.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1377
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
As I have read multiple MR mentioning the `nameservice-strict`. Therefore, I thought it would make sense to directly import it here.
To give some context, this abstraction is probably the most commonly included abstraction (after `base`). In `apparmor.d`, it is used by over 700 profiles (only counting direct import). Therefore, adding new rules can have an important impact over a lot of profiles.
Note: the abstraction is a direct import from https://gitlab.com/roddhjav/apparmor.d. The license is the same, I obviously kept Morfikov copyright line. However, I am not sure either or not the SPDX identifier can be used here.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1368
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Approved-by: Christian Boltz <apparmor@cboltz.de>
Approved-by: Ryan Lee <rlee287@yahoo.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
Bash will try to read the passwd database to find the shell of a user if
$SHELL is not set. This causes zgrep to trigger
```
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/nsswitch.conf" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/passwd" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
```
if called in a sanitized environment. As the functionality of zgrep is
not impacted by a limited Bash environment, add deny rules to avoid the
potentially misleading AVC messages.
Signed-off-by: Georg Pfuetzenreuter <mail@georg-pfuetzenreuter.net>
So far, change_profile_flags() in aa.py is the only user of
ProfileStorage's 'name'.
Rewrite minitools test_cleanprof() so that most of its code can be
reused, and add a test that runs 'aa-complain
/usr/bin/a/simple/cleanprof/test/profile' on cleanprof.in to ensure
aa-complain still works as expected on subprofiles and hats.
Note: aa-complain $profilename will change the flags of hats, but not
child profiles. This is a known issue, and doesn't change with this MR.
Instead of always storing the name of the main profile, store the child
profile/hat name if we are in a child profile or hat.
As a result, we always get the correct "profile xy" header even for
child profiles when dumping the ProfileStorage object.
Also extend the tests to check that the name gets stored correctly.
Seen on various VMs, my guess is that bash wants to translate a uid to a
username.
Log events (slightly shortened)
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/nsswitch.conf" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
apparmor="DENIED" operation="open" class="file" profile="zgrep" name="/etc/passwd" comm="zgrep" requested_mask="r" denied_mask="r" fsuid=0 ouid=0
Systemd's PrivateTmp= in transmission service is causing mount namespaces to be used leading to disconnected paths
[395201.414562] audit: type=1400 audit(1727277774.392:573): apparmor="ALLOWED" operation="sendmsg" class="file" info="Failed name lookup - disconnected path" error=-13 profile="transmission-daemon" name="run/systemd/notify" pid=193060 comm="transmission-da" requested_mask="w" denied_mask="w" fsuid=114 ouid=0
Fixes: https://bugs.launchpad.net/bugs/2083548
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
This is one of those functions that never worked anyways, because it
modified the passed-in label in place. Moreover, it is a low-level
interface that requires its callers to manually construct a binary query.
As such, it would be better not to expose it and to add wrappers like
aa_query_file_path for the other query classes if that functionality is
needed later.
The removal of this function from the bindings was dropped from !1337 because it exposed functionality that was not present in wrappers around aa_query_label. However, upon further discussion, we decided that it'd be better to remove it now and add other wrappers to libapparmor itself if the functionality provided by the existing wrappers became insufficient.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1352
Approved-by: John Johansen <john@jjmx.net>
Merged-by: Ryan Lee <rlee287@yahoo.com>
This is one of those functions that never worked anyways, because it
modified the passed-in label in place. Moreover, it is a low-level
interface that requires its callers to manually construct a binary query.
As such, it would be better not to expose it and to add wrappers like
aa_query_file_path for the other query classes if that functionality is
needed later.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This pipeline only makes sense to run in the upstream project where
the coverity variables are defined, so they currently fail in forks.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
It doesn't make sense to expose the *_raw functions or the varg version
of aa_change_hatv to higher-level languages. While technically a breaking
change, the generated bindings for these functions never actually worked
anyways:
- aa_change_hat_vargs uses C varargs, which SWIG passes in NULL for by
default. It does not attempt to process the passed-in arguments at all
(and in fact caused an unused-argument compiler warning when compiling
the generated bindings).
- aa_getprocattr_raw and aa_getpeercon_raw both place output into a ``char
**mode`` pointer. SWIG by default generates these as opaque pointer
object arguments, rendering them unusable for getting output. Future
patches would be needed to fix ``char**`` arguments for the other functions
that use them. Moreover, these functions expect their caller to handle
memory allocation, which is also not possible from a higher-level
language point of view.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1337
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Ryan Lee <rlee287@yahoo.com>
It doesn't make sense to expose the *_raw functions or the varg version
of aa_change_hatv to higher-level languages. While technically a breaking
change, the generated bindings for these functions never actually worked
anyways:
- aa_change_hat_vargs uses C varargs, which SWIG passes in NULL for by
default. It does not attempt to process the passed-in arguments at all
(and in fact caused an unused-argument compiler warning when compiling
the generated bindings).
- aa_getprocattr_raw and aa_getpeercon_raw both place output into a char
**mode pointer. SWIG by default generates these as opaque pointer
object arguments, rendering them unusable for getting output. Future
patches would be needed to fix char** arguments for the other functions
that use them. Moreover, these functions expect their caller to handle
memory allocation, which is also not possible from a higher-level
language point of view.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This patchset adds annotations so that SWIG can automatically manage the memory lifetimes of aa_log_record objects, and ensures proper cleanup is done in the %exception handler.
This is the first of a sequence of MRs to overhaul the SWIG bindings and fix pieces that never actually worked in the first place. As fixing those other pieces will require breaking changes, I am separating out the non-breaking changes into separate MRs.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1334
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Ryan Lee <rlee287@yahoo.com>
Swig generates a "thisown" attribute, which is an escape hatch in case
higher-level code does something weird and needs to tell SWIG whether to
free the C object when Python garbage collects it. Adding this attribute
is not a breaking change w.r.t access to the other attributes of the parsed
record.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Unfortunately SWIG_exception does not support throwing OSError, so this
still requires Python-specific code.
Unlike just returning NULL, this will clean up intermediate allocations.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This change matches the names in the .c source and the man page for aa_query_label,
and also simplifies the typemap annotations needed to make the SWIG versions usable.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
The autoconf infrastructure for building this doesn't even show up in the Git history, so there should be no issue with removing the ghosts of Java from the codebase
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
The parser and binutils pot file have not been recently refreshed. Update them to current code and add missing pot files for aa_load and aa_status. Also give aa_status base support for translations to populate its pot file.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1318
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Tested by using Valgrind's Helgrind and DRD against the reentrancy test that I wrote: they both report no errors with the changes while reporting many errors with the old versions.
Commits "Inline _parse_yacc in libaalogparse" and "Make parse_record take a const char pointer since it never modified str anyways" have a tiny potential to be backwards-incompatible changes: I have justified why they shouldn't be in the commit messages, but it's worth looking over in case I was mistaken and we need to back those out.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1322
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
... without all the profiles generated by the gen-*.py scripts.
This target is meant for local manual testing, especially when working
on additional simple_tests profiles.
It makes local testing much faster (15 seconds for ~2k profiles vs.
several minutes for the additional ~70k profiles generated by gen-*.py)
Needless to say that the CI should continue to use the parser_sanity
target that includes all the generated profiles.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1325
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
... without all the profiles generated by the gen-*.py scripts.
This target is meant for local manual testing, especially when working
on additional simple_tests profiles.
It makes local testing much faster (15 seconds for ~2k profiles vs.
several minutes for the additional ~70k profiles generated by gen-*.py)
Needless to say that the CI should continue to use the parser_sanity
target that includes all the generated profiles.
The entry AA_RECORD_SYNTAX_V1 is only there for API compatibility reasons.
If we wanted to remove it, we could just renumber the other two entries
to preserve ABI compatibility. However, it seems easier to just delete the
entry if we ever break backcompat with a libapparmor2.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This shouldn't be a breaking change because it's fine to pass a
non-const pointer to a function taking a const pointer, but not the other way round
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This function was only ever called once inside libaalogparse.c, and it looks
simple enough to not need to be split out into its own helper function.
As this function was never exposed publicly in installed header files, removing it
is not a breaking API change.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
The generated grammar.h already sets the correct YYDEBUG value regardless
of whether parse.trace is defined
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
To run the network port range equality tests, we need to check if the
kernel supports the network_v8/af_inet feature. Also, a new file
features.af_inet is needed containing the af_inet feature.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
When the profile already contains a "file" rule containing the owner
prefix and the tool is trying to handle a new file entry, it tries to
show it in the logprof header as "old mode".
The issue is that when the owner rule is an implicit all files
permission, then the object "FileRule" is used instead of the set of
permissions. When subtracting FileRule from set() a TypeError
exception is thrown.
Fix this by "translating" FileRule.ALL perms to "mrwlkix".
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/429
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Unfortunately aa_status did not support translations. Add a base support
and the initial pot file. There are no translations done at this time.
Signed-off-by: John Johansen <john.johansen@canonical.com>
aa_load was missing a pot file for translations. Add a pot file for
aa_load and sync it to the code.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The parser pot file should have been updated before beta. Make
sure it is up to date with the current code.
Signed-off-by: John Johansen <john.johansen@canonical.com>
utils: ignore peer when parsing logs for non-peer access modes
Some access modes (create, setopt, getopt, bind, shutdown, listen,
getattr, setattr) cannot be used with a peer in network rules.
Due to how auditing is implemented in the kernel, the peer information
might be available in the log (as faddr= but not daddr=), which causes
a failure in log parsing.
When parsing the log, check if that's the case and ignore the peer,
avoiding the exception on the NetworkRule constructor.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/427
Reported-by: Evan Caville <evan.caville@canonical.com>
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Closes#427
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1314
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: John Johansen <john@jjmx.net>
The wording of "scrub the environment" with respect to execution modes is misleading, because a quick read of it could imply that it removes all environment variables. However, it actually enables ld.so's secure-execution mode, which removes a very limited subset of them. This MR rewords the relevant documentation and prompts. If proper environment variable filtering is added later, the documentation can be updated again then.
Synchronizes-with:
- Wiki page update, which I can do after this MR is approved
- Kernel patch to update wording of debug logs (patch submitted to the Apparmor mailing list [here](https://lists.ubuntu.com/archives/apparmor/2024-August/013339.html))
Things that may need updating first:
- Translations: attempting to update `utils/po/apparmor-utils.pot` resulted in a bunch of unrelated changes, so I'd like to ask about translation statuses before making a commit that updates that file properly.
- Adding info on which libc's actually behave differently based on AT_SECURE: glibc and musl libc both do, but they may do subtly different things. I don't know about other libc's.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1315
Approved-by: John Johansen <john@jjmx.net>
Merged-by: Ryan Lee <rlee287@yahoo.com>
Some access modes (create, setopt, getopt, bind, shutdown, listen,
getattr, setattr) cannot be used with a peer in network rules.
Due to how auditing is implemented in the kernel, the peer information
might be available in the log (as faddr= but not daddr=), which causes
a failure in log parsing.
When parsing the log, check if that's the case and ignore the peer,
avoiding the exception on the NetworkRule constructor.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/427
Reported-by: Evan Caville <evan.caville@canonical.com>
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
io_uring and userns mediation are encoding permissions on the class
byte. This is a mistake that should never have been allowed.
With the addition of rule priorities the class byte mediates rule,
that ensure the kernel can determine a class is being mediated is
given the highest priority possible, to ensure class mediation can not
be removed by a deny rule. See
61b7568e1 ("parser: bug fix mediates_X stub rules.")
for details.
Unfortunately this breaks rule classes that encode permissions on the
class byte, because those rules will always have a lower priority and
the class mediates rule will always be selected over them resulting in
only the class mediates permission being on the rule class state.
Fix this by adding the mediaties class rules for these rule classes
with the lowest priority possible. This means that any rule mediating
the class will wipe out the mediates class rule. So add a new mediates
class rule at the same priority, as the rule being added.
This is a naive implementation and does result in more mediates rules
being added than necessary. The rule class could keep track of the
highest priority rule that had been added, and use that to reduce the
number of mediates rules it adds for the class.
Technically we could also get away with not adding the rules for allow
rules, as the kernel doesn't actually check the encoded permission but
whether the class state is not the trap state. But it is required with
deny rules to ensure the deny rule doesn't result in permissions being
removed from the class, resulting in the kernel thinking it is
unmediated. We also want to ensure that mediation is encoded for other
rule types like prompt, and in the future the kernel could check the
permission so we do want to guarantee that the class state has the
MAY_READ permission on it.
Note: there is another set of classes (file, mqueue, dbus, ...) which
encodes a default rule permission as
class .* <perm>
this encoding is unfortunate in that it will also add the permission
to the class byte, but also sets up following states with the permission.
thankfully, this accespt anything, including nothing generally isn't
valid in the nothing case (eg. a file without any absolute name). For
this set of classes, the high priority mediates rule just ensures
that the null match case does not have permission.
Fixes: 61b7568e1 parser: bug fix mediates_X stub rules.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1307
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
af_protos.h is a generated table of the protocols created by looking
for definitions of IPPROTO_* in netinet/in.h. Depending on the
architecture, the order of the table may change when using -dM in the
compiler during the extraction of the defines.
This causes an issue because there is more than one IPPROTO defined
by the value 0: IPPROTO_IP and IPPROTO_HOPOPTS which is a header
extension used by IPv6. So if IPPROTO_HOPOPTS was first in the table,
then protocol=0 in the audit logs would be translated to hopopts.
This caused a failure in arm 32bit:
Output doesn't match expected data:
--- ./test_multi/testcase_unix_01.out 2024-08-15 01:47:53.000000000 +0000
+++ ./test_multi/out/testcase_unix_01.out 2024-08-15 23:42:10.187416392 +0000
@@ -12,7 +12,7 @@
Peer Addr: @test_abstract_socket
Network family: unix
Socket type: stream
-Protocol: ip
+Protocol: hopopts
Class: net
Epoch: 1711454639
Audit subid: 322
By the time protocol is resolved in grammar.y, we don't have have
access to the net family to check if it's inet6. Instead of making
protocol dependent on the net family, make the order of the
af_protos.h table consistent between architectures using -dD.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1309
Approved-by: John Johansen <john@jjmx.net>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
af_protos.h is a generated table of the protocols created by looking
for definitions of IPPROTO_* in netinet/in.h. Depending on the
architecture, the order of the table may change when using -dM in the
compiler during the extraction of the defines.
This causes an issue because there is more than one IPPROTO defined
by the value 0: IPPROTO_IP and IPPROTO_HOPOPTS which is a header
extension used by IPv6. So if IPPROTO_HOPOPTS was first in the table,
then protocol=0 in the audit logs would be translated to hopopts.
This caused a failure in arm 32bit:
Output doesn't match expected data:
--- ./test_multi/testcase_unix_01.out 2024-08-15 01:47:53.000000000 +0000
+++ ./test_multi/out/testcase_unix_01.out 2024-08-15 23:42:10.187416392 +0000
@@ -12,7 +12,7 @@
Peer Addr: @test_abstract_socket
Network family: unix
Socket type: stream
-Protocol: ip
+Protocol: hopopts
Class: net
Epoch: 1711454639
Audit subid: 322
By the time protocol is resolved in grammar.y, we don't have have
access to the net family to check if it's inet6. Instead of making
protocol dependent on the net family, make the order of the
af_protos.h table consistent between architectures using -dD.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
io_uring and userns mediation are encoding permissions on the class
byte. This is a mistake that should never have been allowed.
With the addition of rule priorities the class byte mediates rule,
that ensure the kernel can determine a class is being mediated is
given the highest priority possible, to ensure class mediation can not
be removed by a deny rule. See
61b7568e1 ("parser: bug fix mediates_X stub rules.")
for details.
Unfortunately this breaks rule classes that encode permissions on the
class byte, because those rules will always have a lower priority and
the class mediates rule will always be selected over them resulting in
only the class mediates permission being on the rule class state.
Fix this by adding the mediaties class rules for these rule classes
with the lowest priority possible. This means that any rule mediating
the class will wipe out the mediates class rule. So add a new mediates
class rule at the same priority, as the rule being added.
This is a naive implementation and does result in more mediates rules
being added than necessary. The rule class could keep track of the
highest priority rule that had been added, and use that to reduce the
number of mediates rules it adds for the class.
Technically we could also get away with not adding the rules for allow
rules, as the kernel doesn't actually check the encoded permission but
whether the class state is not the trap state. But it is required with
deny rules to ensure the deny rule doesn't result in permissions being
removed from the class, resulting in the kernel thinking it is
unmediated. We also want to ensure that mediation is encoded for other
rule types like prompt, and in the future the kernel could check the
permission so we do want to guarantee that the class state has the
MAY_READ permission on it.
Note: there is another set of classes (file, mqueue, dbus, ...) which
encodes a default rule permission as
class .* <perm>
this encoding is unfortunate in that it will also add the permission
to the class byte, but also sets up following states with the permission.
thankfully, this accespt anything, including nothing generally isn't
valid in the nothing case (eg. a file without any absolute name). For
this set of classes, the high priority mediates rule just ensures
that the null match case does not have permission.
Fixes: 61b7568e1 parser: bug fix mediates_X stub rules.
Signed-off-by: John Johansen <john.johansen@canonical.com>
This enables adding a priority to a rules in policy.
Rules have a default priority of 0. The priority prefix can be added
before the other currently support rule prefixes, ie.
[priority prefix][audit qualifier][rule mode][owner]
If present a numerical priority can be assigned to the rule, where the
greater the number the higher the priority. Eg.
priority=1 audit file r /etc/passwd,
priority=-1 deny file w /etc/**,
Rule priority allows the rule with the highest priority to completely
override lower priority rules where they overlap. Within a given
priority level rules will accumulate in standard apparmor fashion.
Eg. given
priority=1 w /*c,
priority=0 r /a*,
priority=-1 k /*b*,
/abc, /bc, /ac .. will have permissions of w
/ab, /abb, /aaa, .. will have permissions of r
/b, /bcb, /bab, .. will have permissions of k
User specified rule priorities are currently capped at the arbitrary
values of 1000, and -1000.
Notes:
* not all rule types support the priority prefix. Rukes like
- network
- capability
- rlimits
need to be reworked to properly preserve the policy rule structure.
* this patch does not support priority on rule blocks
* this patch does not support using a variable in the priority value.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1261
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
the ix portion of file, causes x conflicts in regular priority. The
long term goal is to fix this by using dominance for x rules. But in
the mean time we can fix by giving the ix portion of the rule a
reduced priority.
Signed-off-by: John Johansen <john.johansen@canonical.com>
This enables adding a priority to a rules in policy, finishing out the
priority work done to plumb priority support through the internals in
the previous patch.
Rules have a default priority of 0. The priority prefix can be added
before the other currently support rule prefixes, ie.
[priority prefix][audit qualifier][rule mode][owner]
If present a numerical priority can be assigned to the rule, where the
greater the number the higher the priority. Eg.
priority=1 audit file r /etc/passwd,
priority=-1 deny file w /etc/**,
Rule priority allows the rule with the highest priority to completely
override lower priority rules where they overlap. Within a given
priority level rules will accumulate in standard apparmor fashion.
Eg. given
priority=1 w /*c,
priority=0 r /a*,
priority=-1 k /*b*,
/abc, /bc, /ac .. will have permissions of w
/ab, /abb, /aaa, .. will have permissions of r
/b, /bcb, /bab, .. will have permissions of k
User specified rule priorities are currently capped at the arbitrary
values of 1000, and -1000.
Notes:
* not all rule types support the priority prefix. Rukes like
- network
- capability
- rlimits need to be reworked
need to be reworked to properly preserve the policy rule structure.
* this patch does not support priority on rule blocks
* this patch does not support using a variable in the priority value.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Currently mediates_X stub rules are added to the dfa to ensure a valid
transition state will exist if X should be mediated. The kernel uses
this to test whether the dfa supports certain mediation classes.
Unfortunately the mediates stub rules can be removed by other rules,
combined with minimization. In the allow case this is not a problem,
as if the stub rule is removed it will be due to state merging and the
test will still be valid. Unfortunately the deny case can wipe out the
stub rule in a couple of cases, meaning the when the kernel tests that
its in a valid state for mediation it will fail and treat the dfa as
not mediating the rule type, which results in allowing instead of
denying.
Fix this by making sure mediated stub rules can't be overridden by a
deny rule by giving them maximum priority.
Note: there is another issue with stub rule elimination in the allow
case. It will can cause equality tests to fail when combined
with priority rules, because the stub rules where added at
priority 0 and an actual rule of higher priority could
completely override it removing the permission on the stub rule.
This issue will be caught by the equality.sh tests in the
following patch that exposes priority to rules in policy.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The prefix comparison doesn't need to do as many operations as it is
doing, and the operator< can be based on the cmp() fn further reducing
the chance that the code will get out of sync if prefixes are changed.
Signed-off-by: John Johansen <john.johansen@canonical.com>
the parser front end boolean is used for both boolean and integer
values. This is confusing when integer values different than 1 or 0
are being assigned to and from boolean.
Split its uses into the correct semantic boolean and integer cases.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Currently use of extended perms are dependent on prompt rules being present
in policy. Switch to using extended perms if they are supported.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Moving apply_and_clear_deny() before the first minimization pass, which
was necessary to propperly support building accept information for
older none extended permission dfas, allows us to also get rid of doing a
second minimization pass if we want to force clearing explicit deny
info from extended permission tables.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Instead of compressing the permission set into 128 bit and using that
as the index in the permission map, just use the permissions directly
as the index into the permission map.
Note: this will break equality and minimization tests. Because deny
is not being cleared it will result in more partitions in the initial
setup. This will be addressed and the tests will be fixed in a follow
on patch.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Hash minimization was removed in
f0b154528 Fix dfa minimization
however some remnants of minimization remained. A comment and the use
of the hash but only as a 0 value. Drop this dead code and comment.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The hfa stores next/check transitions in 16 bit fields to reduce memory
usage. However this means the state machine can on contain 2^16
states.
Allow the next/check tables to be 32 bit. This theoretically could allow
for 2^32 states however the base table uses the top 8 bits as flags
giving us only 2^24 bits to index into the next/check tables. With
most states having at least 1 transition this effectively caps the
number of states at 2^24.
To obtain 2^32 possible states a flags table needs to be added. Add
a skeleton around supporting a flags table, so we can note the remaining
work that needs to be done. This patch will only allow for 2^24 states.
Bug: https://gitlab.com/apparmor/apparmor/-/issues/419
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1303
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
The hfa stores next/check transitions in 16 bit fields to reduce memory
usage. However this means the state machine can on contain 2^16
states.
Allow the next/check tables to be 32 bit. This theoretically could allow
for 2^32 states however the base table uses the top 8 bits as flags
giving us only 2^24 bits to index into the next/check tables. With
most states having at least 1 transition this effectively caps the
number of states at 2^24.
To obtain 2^32 possible states a flags table needs to be added. Add
a skeleton around supporting a flags table, so we can note the remaining
work that needs to be done. This patch will only allow for 2^24 states.
Bug: https://gitlab.com/apparmor/apparmor/-/issues/419
Signed-off-by: John Johansen <john.johansen@canonical.com>
Older kernels do not support an xtable grouped with the policy dfa.
The presence of a policy.dfa does not indicate whether we should create
an xtable with the policy dfa.
Instead the check should be if the kernel supports the extended
permstable32 format.
Signed-off-by: John Johansen <john.johansen@canonical.com>
__uint128 is not supported by gcc on 32 bit architectures so rework
the 128 bit map key to be a pair of 64bit numbers.
Signed-off-by: John Johansen <john.johansen@canonical.com>
switch permission bits to use perm32_t type. This is just annotating
the code as it is no different than uint32_t at this time.
We do not convert the accept values as they may be mapped permission
bits or they may be and index value.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The use of xbits can not pass verification so we need to leave them
off this makes the profile a leaf profile.
Signed-off-by: John Johansen <john.johansen@canonical.com>
v1 of permstable32 has some broken verification checks. By using two
copies of a merged dfa and an xtable the same size of the permstable
we can work around the issue.
Signed-off-by: John Johansen <john.johansen@canonical.com>
There are two distinct declarations of perms_t.
rule.h: typedef uint32_t perms_t
hfa.h: class perms_t
these definitions clash when the front end and backend share more info.
To avoid this rename rule.h to perm32_t, and move the definition into
perms.h and use it in struct aa_perms.
Signed-off-by: John Johansen <john.johansen@canonical.com>
If extended permissions are supported use them. We need to build a
permission table and set the accept state of the chfa up as an index
into the table.
For now map the front end permission layout into the old format and
then convert that to the perms table just as the kernel does.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Add the ability to parse the prompt qualifier but do not provide
functionality because the backend does not currently support prompt
permissions.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Remove conditional logic from the parser and move it to its own class,
that way any improvements or conditional features will make cleaner
changes.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
I couldn't figure out why the show info window was using a different
font color than the theme default but this forces its use.
Also, add padding when "Show Current Profile" button is not shown.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
In this change, I'm also removing the messagebox window and reusing
the more info GUI already implemented
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
aa-notify: Enhanced Graphical User Interfaces
- Added support for --prompt-filter=userns: a popup GUI now appears when an unprivileged, unconfined process attempts to create a user namespace, enabling automatic generation of specific unconfined profiles.
- Added GUIs for easy rule addition.
- Upgraded notifications to two-button format, enabling extended information display and direct rule addition.
- Initial support for customized notification messages based on rule type.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1281
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
Code assumes full username would be printed, but this actually requires an extra command line option:
Please double check that this is the only place where `last` is called as a binary before merging this MR.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1293
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
POSIX states that d_name has up to NAME_MAX (255) characters, and glibc
stores d_name as an array of size NAME_MAX+1 (256). Thus, supplying
PATH_MAX (4096) as the max length could trigger a buffer overrun. This
could be an even bigger issue on other libcs, as POSIX states that d_name
can be unsized.
Fortunately, this does not seem to cause actual issues, as the length is
only used to compare d_name to a short fixed string. However, it'd be better
to pass the actual correct max length to strnlen.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1290
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
POSIX states that d_name has up to NAME_MAX (255) characters, and glibc
stores d_name as an array of size NAME_MAX+1 (256). Thus, supplying
PATH_MAX (4096) as the max length could trigger a buffer overrun. This
could be an even bigger issue on other libcs, as POSIX states that d_name
can be unsized.
Fortunately, this does not seem to cause actual issues, as the length is
only used to compare d_name to a short fixed string. However, it'd be better
to pass the actual correct max length to strnlen.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
The linkage of aa-load with the dynamic libapparmor fails with:
aa_load.c:273: undefined reference to `aa_split_overlay_str'
That is because when aa_split_overlay_str was added to libapparmor,
the function was not added to the library map.
Fixes: 50054ff0 ("add aa_split_overlay_str")
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1288
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
The linkage of aa-load with the dynamic libapparmor fails with:
aa_load.c:273: undefined reference to `aa_split_overlay_str'
That is because when aa_split_overlay_str was added to libapparmor,
the function was not added to the library map.
Fixes: 50054ff0 ("add aa_split_overlay_str")
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Commonly used by applications to determine if Linux is running in
FIPS mode. As we already allow access to FIPS specific library files
as part of base, allow this there as well.
Signed-off-by: Georg Pfuetzenreuter <mail@georg-pfuetzenreuter.net>
If the test ran under a fs mounted with nosuid option, then these bits
would be ignored and the test would fail. In that case, detect it and
run the test in a tmpfs mountpoint without nosuid.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
When /tmp is of type tmpfs, the test didn't run because you can't
mount a swapfile on it. This patch mounts an ext2 mountpoint on
$tmpdir so that the swapfile can be mounted on top of it instead of
tmpfs.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
When /tmp is mounted, remounting / as private for tests that don't
work when shared still fail because /tmp remains as shared. The option
-T in findmnt helps determine the mountpoint in a certain directory,
so use that with $tmpdir to determine the root.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The tests that use pivot_root or move mountpoints with mount have to
make sure that / is private for the tests to work. Refactor that logic
into a file to be sourced by the test scripts
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The abi is not being respected by mqueue rules in many cases. If policy
does ot specify an mqueue rule the abi is correctly applied but if
an mqueue rule is specified explicitly or implicitly (eg. allow all).
without setting the mqueue type OR setting the mqueue type to sysv.
The abi will be ignored and mqueue will be enforced for policy regadless.
Known good mqueue rule that respects abi
mqueue type=posix,
# and all variations that keep type=posix
Known bad mqueue rules that do not respect abi
mqueue,
# and all variants that do not specify the type= option
mqueue type=sysv,
# and all variants that specify the type=sysv option
Issue: https://gitlab.com/apparmor/apparmor/-/issues/412
Fixes: d98c5c4cf ("parser: add parser support for message queue mediation")
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1277
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
The kernel does not expect a name and it is not used even within the
parser so drop it. Correct the padding calculation.
sizeof(th_version)
includes the trailing \0 in the count so we should not be adding it
explicitly. Doing so made it seem like we were writing an extra byte
and messing things up, because the string write below did not include
the \0 which we had to add explicitly.
Switch to writing the th_version using size_of() bytes as is used in
the pad calculation, to avoid confusion around the header padding.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The abi is not being respected by mqueue rules in many cases. If policy
does ot specify an mqueue rule the abi is correctly applied but if
an mqueue rule is specified explicitly or implicitly (eg. allow all).
without setting the mqueue type OR setting the mqueue type to sysv.
The abi will be ignored and mqueue will be enforced for policy regadless.
Known good mqueue rule that respects abi
mqueue type=posix,
# and all variations that keep type=posix
Known bad mqueue rules that do not respect abi
mqueue,
# and all variants that do not specify the type= option
mqueue type=sysv,
# and all variants that specify the type=sysv option
Issue: https://gitlab.com/apparmor/apparmor/-/issues/412
Fixes: d98c5c4cf ("parser: add parser support for message queue mediation")
Signed-off-by: John Johansen <john.johansen@canonical.com>
`execpath` allows to reliably store the path of the binary that triggered a log.
This is useful because comm was not sufficient to reliably identify a binary
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
This reverts commit 78ae956087.
Commit 78ae956087 causes policy to not
to conform to protocol as determined by the kernel. Technically the
reverted patch is correct and the kernel is wrong but we can not
change 15 years of history.
The reason it breaks the policy in the kernel is because the kernel
does not use the name field, and does not expect it. It just expects
the size with a single trailing 0. This doesn't break because this
section is all padded to 64 bytes so writing the extra 0 doesn't
hurt as it is effectively just manually adding to the padding.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Some applications using the bwrap profile don't function properly due to "Failed name lookup - deleted entry". The following denials trying to start flatpak KeePassXC is an example showing that it happens for both bwrap and unpriv_bwrap profiles:
Jul 12 09:44:37 ubuntu2404 kernel: audit: type=1400 audit(1720741477.106:310): apparmor="DENIED" operation="link" class="file" info="Failed name lookup - deleted entry" error=-2 profile="bwrap" name="/home/\*\*\*\*/.var/app/org.keepassxc.KeePassXC/config/keepassxc/#317211" pid=4021 comm="keepassxc" requested_mask="l" denied_mask="l" fsuid=1000 ouid=1000
Jul 12 09:44:37 ubuntu2404 kernel: audit: type=1400 audit(1720741477.341:317): apparmor="DENIED" operation="link" class="file" profile="unpriv_bwrap" name="/home/**/.var/app/org.keepassxc.KeePassXC/config/keepassxc/keepassxc.ini" pid=4021 comm="keepassxc" requested_mask="l" denied_mask="l" fsuid=1000 ouid=1000 target="/home/**/.var/app/org.keepassxc.KeePassXC/config/keepassxc/#317214"
Fixes: https://launchpad.net/bugs/2072811
I propose this fix for master and apparmor-4.0
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1272
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Without AA_MAY_MOUNT, mount was not allowed by the allow all
rule. AA_DUMMY_REMOUNT does become AA_MAY_MOUNT, but it fixes the
flags to remount only, so other options are not included. Also, add
allow all rule testcases to the mount regression tests.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/410
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
By specifying 0 in the unix type, all rules were allowing only the
"none" type, when it wanted to allow all types, so replace it by
0xffffffff. Also, add this testcase to the unix regression tests.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/410
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Some applications using the bwrap profile don't function properly due
to "Failed name lookup - deleted entry". The following denials trying
to start flatpak KeePassXC is an example showing that it happens for
both bwrap and unpriv_bwrap profiles:
Jul 12 09:44:37 ubuntu2404 kernel: audit: type=1400 audit(1720741477.106:310): apparmor="DENIED" operation="link" class="file" info="Failed name lookup - deleted entry" error=-2 profile="bwrap" name="/home/****/.var/app/org.keepassxc.KeePassXC/config/keepassxc/#317211" pid=4021 comm="keepassxc" requested_mask="l" denied_mask="l" fsuid=1000 ouid=1000
Jul 12 09:44:37 ubuntu2404 kernel: audit: type=1400 audit(1720741477.341:317): apparmor="DENIED" operation="link" class="file" profile="unpriv_bwrap" name="/home/****/.var/app/org.keepassxc.KeePassXC/config/keepassxc/keepassxc.ini" pid=4021 comm="keepassxc" requested_mask="l" denied_mask="l" fsuid=1000 ouid=1000 target="/home/****/.var/app/org.keepassxc.KeePassXC/config/keepassxc/#317214"
Fixes: https://launchpad.net/bugs/2072811
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Saw these couple of accesses fail recently on my Ubuntu 22.04 system:
`Jun 3 15:29:24 darkstar kernel: [5401883.070129] audit: type=1107 audit(1717442964.884:9223): pid=729 uid=102 auid=4294967295 ses=4294967295 subj=unconfined msg='apparmor="DENIED" operation="dbus_method_call" bus="system" path="/org/freedesktop/UPower" interface="org.freedesktop.DBus.Properties" member="GetAll" mask="send" name=":1.28" pid=2164500 label="firefox" peer_pid=2502 peer_label="unconfined"`
`Jun 3 15:29:24 darkstar kernel: [5401883.070588] audit: type=1107 audit(1717442964.884:9224): pid=729 uid=102 auid=4294967295 ses=4294967295 subj=unconfined msg='apparmor="DENIED" operation="dbus_method_call" bus="system" path="/org/freedesktop/UPower" interface="org.freedesktop.UPower" member="EnumerateDevices" mask="send" name=":1.28" pid=2164500 label="firefox" peer_pid=2502 peer_label="unconfined"`
Also, I noticed that the `firefox` profile in the Ubuntu 24.04 package has a rule for `/etc/writable/timezone` that is not present in Git. Figured that should be in here.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/409Closes#409
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1253
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
This is needed to avoid a "Conflicting profiles" error if there are two
profiles for an application, with one of them disabled.
This is not a theoretical usecase - for example, apparmor.d ships some
profiles that replace our "userns+unconfined" profiles. These profiles
use a different filename, and apparmor.d also creates a disable symlink
for the "userns+unconfined" profile it replaces.
I propose this patch for 4.0 and master.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1264
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
... and not only for events in missing hats.
This fixes a crash if the log contains exec events for a hat where not
even the parent profile exists.
I propose this patch for master, 4.0 and 3.1.
(In 3.0, `aa` is still a `hasher` which avoids the crash, therefore it doesn't really need this patch.)
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1265
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Christian Boltz <apparmor@cboltz.de>
Contrary to what the name would imply aa-unconfined displays info for
both confined and unconfined processes. Add a --short option that only
output processes that are not confined. Eg.
$ ./utils/aa-unconfined
17192 /snap/chromium/2890/usr/lib/chromium-browser/chrome (/snap/chromium/2890/usr/lib/chromium-browser/chrome --password-store=basic --disable-features=TFLiteLanguageDetectionEnabled) confined by 'snap.chromium.chromium (enforce)'
17395 /snap/chromium/2890/usr/lib/chromium-browser/chrome (/snap/chromium/2890/usr/lib/chromium-browser/chrome --type=utility --utility-sub-type=network.mojom.NetworkService --lang=en-US --service-sandbox-type=none --crashpad-handler-pid=17337 --enable-crash-reporter=,snap --change-stack-guard-on-fork=enable --shared-files=v8_context_snapshot_data:100 --field-trial-handle=3,i,16674663885832976354,18417931519279121981,262144 --disable-features=TFLiteLanguageDetectionEnabled --variations-seed-version) confined by 'snap.chromium.chromium (enforce)'
17981 /snap/firefox/4451/usr/lib/firefox/firefox confined by 'snap.firefox.firefox (enforce)'
1353664 /tmp/.mount_OrcaSl7G1va5/bin/orca-slicer not confined
is trimmed to
$ ./utils/aa-unconfined --short
1353664 /tmp/.mount_OrcaSl7G1va5/bin/orca-slicer not confined
Signed-off-by: John Johansen <john.johansen@canonical.com>
The prompt/user upcall mode shows up as a mode of (user). And for
stacked policy with different modes (mixed) is used. Add these to the
list of modes to screen.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Add the ability to list applications that are unconfined and have
any open network socket open, both listening and non-listening.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Add the abiity to list applications that are unconfined and have
open connection ports that are not listening.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The documentation of --paranoid is wrong. It lists all processes and
does not exclude based on whether it has a network port open.
Signed-off-by: John Johansen <john.johansen@canonical.com>
This is needed to avoid a "Conflicting profiles" error if there are two
profiles for an application, with one of them disabled.
This is not a theoretical usecase - for example, apparmor.d ships some
profiles that replace our "userns+unconfined" profiles. These profiles
use a different filename, and apparmor.d also creates a disable symlink
for the "userns+unconfined" profile it replaces.
* UnixRule: Fix handling of peers with a ? and peers that are/need to be quoted
`?` is a valid AARE char, add it to the regexes that match the AARE.
Also add some tests to ensure this is really fixed, and make the error
output of the tests more useful/verbose.
* Fix handling of quoted peers in UnixRule (and others)
In UnixRule (and probably also in other rules that use
print_dict_values()` and `initialize_cond_dict()`), the handling of
peers with a value that is quoted and/or needs to be quoted was broken
because
- quotes didn't get stripped in `initialize_cond_dict()`
- `print_dict_values()` didn't use `quote_if_needed()`
Note: print_dict_values also handles integers (like network ports).
Convert them to a string so that `if ' ' in data` in `quote_if_needed()`
doesn't explode.
Also enable the test that uncovered this bug.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/404Closes#404
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1262
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
In UnixRule (and probably also in other rules that use
print_dict_values()` and `initialize_cond_dict()`), the handling of
peers with a value that is quoted and/or needs to be quoted was broken
because
- quotes didn't get stripped in `initialize_cond_dict()`
- `print_dict_values()` didn't use `quote_if_needed()`
Note: print_dict_values also handles integers (like network ports).
Convert them to a string so that `if ' ' in data` in `quote_if_needed()`
doesn't explode.
Also enable the test that uncovered this bug.
`?` is a valid AARE char, add it to the regexes that match the AARE.
Also add some tests to ensure this is really fixed, and make the error
output of the tests more useful/verbose.
Note: One of the added tests (with a space in the peer name) uncovered a
bug in quote handling. This will be fixed in the next commit.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/404
Update the state machine readme to better reflect how the chfa is
encoded and works. It still needs a lot more but fixes several errors
in the doc and adds some info about state differential encoding, oobs,
and comb compression.
In addition fix an off by own error during chfa encoding. This has
likely never triggered as it gets hidden by being in a section that
is being in a section that is padded to an 8 byte boundary.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1244
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Expression simplification can get into an infinite loop due to eps
pairs hiding behind and alternation that can't be caught by
normalize_eps() (which exists in the first place to stop a similar
loop).
The loop in question happens in AltNode::normalize when a subtree has
the following structure.
1. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
2. if (normalize_eps(dir)) results in
alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
3. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
alt alt
/\ /\
/ \ / \
/ \ / \
eps eps eps eps
4. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps eps
5. if (normalize_eps(dir)) results in
alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps eps
6. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
back to beginning of cycle
Fix this by detecting the creation of an eps_pair in rotate_node(),
that pair can be immediately eliminated by simplifying the tree in that
step.
In the above cycle the pair creation is caught at step 3 resulting
in
3. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
4. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps eps
which gets reduced to
alt
/\
/ \
/ \
eps eps
breaking the normalization loop. The degenerate alt node will be caught
in turn when its parent is dealt with.
This needs to be backported to all releases
Closes: https://gitlab.com/apparmor/apparmor/-/issues/398
Fixes: 846cee506 ("Split out parsing and expression trees from regexp.y")
Reported-by: Christian Boltz <apparmor@cboltz.de>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Closes#398
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1252
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
Expression simplification can get into an infinite loop due to eps
pairs hiding behind and alternation that can't be caught by
normalize_eps() (which exists in the first place to stop a similar
loop).
The loop in question happens in AltNode::normalize when a subtree has
the following structure.
1. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
2. if (normalize_eps(dir)) results in
alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
3. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
alt alt
/\ /\
/ \ / \
/ \ / \
eps eps eps eps
4. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps eps
5. if (normalize_eps(dir)) results in
alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps eps
6. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
back to beginning of cycle
Fix this by detecting the creation of an eps_pair in rotate_node(),
that pair can be immediately eliminated by simplifying the tree in that
step.
In the above cycle the pair creation is caught at step 3 resulting
in
3. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
alt eps
/\
/ \
/ \
eps eps
4. elseif (child[dir]->is_type(ALT_NODE)) rotate_node too
alt
/\
/ \
/ \
eps alt
/\
/ \
/ \
eps eps
whch gets reduces to
alt
/\
/ \
/ \
eps eps
breaking the normalization loop. The degenerate alt node will be caught
in turn when its parent is dealt with.
This needs to be backported to all releases
Closes: https://gitlab.com/apparmor/apparmor/-/issues/398
Fixes: 846cee506 ("Split out parsing and expression trees from regexp.y")
Reported-by: Christian Boltz <apparmor@cboltz.de>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Adding mediation classes in unconfined profiles caused nested profiles
to be mediated, inside a container for example.
As a first step, skip the addition of mediation classes into the dfa.
The creation of unprivileged user namespaces is an exception, where we
always want to mediate it.
Fixes: https://bugs.launchpad.net/apparmor/+bug/2067900
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
technically a # leading a value in an assignment expression is allowed,
however people are also using it to a comment at the end of a line.
ie.
```
@{var1}=value1 # comment about this value or for a given system
```
this unsurprisingly leads to odd/unexpected behavior when the variable
is used.
```
allow rw /@{var1},
```
expands into
```
allow rw /{value1,#,comment,about,this,value,or,for,a,given,system},
```
change a leading # of a value in an assignment expression to a comment.
If the # is really supposed to lead the value, require it to be escaped
or in quotes.
ie.
```
@{var1}=value1 \#not_a_comment
```
Note: this could potentially break som policy if the # was used as the
leading character for a value in an assignment expression, but
is worth it to avoid the confusion.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1255
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
The parser writes
sizeof(th)) + th_version + (char)0 + name + (char)0;
but the padding currently is computed as
sizeof(th)) + th_version + name + (char)0;
missing the internal (char)0, add 1 to the pad and fill to ensure
this is correct.
Reported-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Update the state machine readme to better reflect how the chfa is
encoded and works. It still needs a lot more but fixes several errors
in the doc and adds some info about state differential encoding, oobs,
and comb compression.
Signed-off-by: John Johansen <john.johansen@canonical.com>
technically a # leading a value in an assignment expression is allowed,
however people are also using it to a comment at the end of a line.
ie.
@{var1}=value1 # comment about this value or for a given system
this unsurprisingly leads to odd/unexpected behavior when the variable
is used.
allow rw /@{var1},
expands into
allow rw /{value1,#,comment,about,this,value,or,for,a,given,system},
change a leading # as value in an assignment expression to a comment.
If the # is really supposed to lead the value, require it to be escaped
or in quotes.
ie.
@{var1}=value1 \#not_a_comment
Note: this could potentially break som policy if the # was used as the
leading character for a value in an assignment expression, but
is worth it to avoid the confusion.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Similarly to https://gitlab.com/apparmor/apparmor/-/merge_requests/689, use the
global CFLAGS when building Python library, so we honor extra flags set by
distributions, such
as -fstack-protector-strong -fstack-clash-protection -Werror=format-security -fcf-protection.
Spotted by blhc on Debian.
Gbp-Pq: Name Honor-global-CFLAGS-when-building-Python-library.patch
This file contains the same kind of information as @{PROC}/@{pid}/net/route
and both files are world readable:
```
$ ls -l /proc/self/net/*route
-r--r--r-- 1 root root 0 Jun 3 15:33 /proc/self/net/ipv6_route
-r--r--r-- 1 root root 0 Jun 3 15:33 /proc/self/net/route
```
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1246
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
This file contains the same kind of information as @{PROC}/@{pid}/net/route
and both files are world readable:
```
$ ls -l /proc/self/net/*route
-r--r--r-- 1 root root 0 Jun 3 15:33 /proc/self/net/ipv6_route
-r--r--r-- 1 root root 0 Jun 3 15:33 /proc/self/net/route
```
Signed-off-by: Simon Deziel <simon.deziel@canonical.com>
aa-remove-unknown doesn't deal properly with profiles that contain
spaces in their names.
Using profile "MongoDB Compass" as an example, awk's sub returns the
number of matches - either 1 or 0 and replaces the actual string ($0)
with the substitution. By accessing the return of sub with $, awk
would be accessing $1 which would return only "MongoDB".
Fix this by using $0 instead of $str.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/395
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Closes#395
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1243
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
In the course of preparing !1207, I found that the validation rules in `profiles/Makefile` did not take kindly to the new `profiles/apparmor/profiles/extras/abstractions/` directory. I tried a couple rounds of quick fixes, but it became clear that the rules as currently written were just not amenable to the new addition, and needed more attention than I could give it by-the-by.
So I separated out that commit, and revised the makefile more thoroughly. The updated rules now rely more on `find(1)` than `$(wildcard)`, and have a number of [what I believe to be] small quality-of-life improvements. Taken together, `make check` passes cleanly with the new files from my other MR present.
One thing I noticed was that the profiles under `apparmor.d/` were not previously being checked for the `include if exists <local/*>` bit---only the ones under `extras/`. I've thus included a fix to the `sbuild-shell` profile, which fortunately was the only one that failed the check.
Note that at present, you'll get a couple of harmless `find: ‘./apparmor/profiles/extras/abstractions’: No such file or directory` errors when running the checks, since that directory won't appear until the other MR is merged. I figure, better to bear that for now, and not have to touch the makefile again later.
NOTE: The CI pipeline here will need to be updated to invoke the `check-local` target instead of `check-extras`. This target was renamed as it is no longer limited in scope to the profiles under `extras/`.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1214
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
aa-remove-unknown doesn't deal properly with profiles that contain
spaces in their names.
Using profile "MongoDB Compass" as an example, awk's sub returns the
number of matches - either 1 or 0 and replaces the actual string ($0)
with the substitution. By accessing the return of sub with $, awk
would be accessing $1 which would return only "MongoDB".
Fix this by using $0 instead of $str.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/395
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Without this patch, aa-remove-unknown uses 'profile_name (unconfined)'
when trying to unload unconfined profiles, which fails for obvious
reasons with (picking a random example)
Removing 'busybox (unconfined)'
/sbin/aa-remove-unknown: line 112: echo: write error: No such file or directory
I propose this patch for 4.0 and master.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1240
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
Without this patch, aa-remove-unknown uses 'profile_name (unconfined)'
when trying to unload unconfined profiles, which fails for obvious
reasons with (picking a random example)
Removing 'busybox (unconfined)'
/sbin/aa-remove-unknown: line 112: echo: write error: No such file or directory
... and tests for it.
This replaces the old code that just stores the full rule as text.
We also get rid of the old ['allow'] and ['deny'] items in
ProfileStorage, the handling of old write functions, and the last usage
of _Raw_Rule (and therefore _Raw_Rule itsself).
Also delete the old test-pivot_root_parse.py which relied on the ancient
code, and even used a wrong syntax in its test rules.
Oh, and aa-logprof can now ask about pivot_root events.
See the individual commits for details.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1232
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Christian Boltz <apparmor@cboltz.de>
Mount Rules with options in { remount, [make-] { [r]unbindable, [r]shared, [r]private, and [r]slave }} do not support specifying a source. This commit aligns utils implementation to apparmor_parser's, which prohibits having a both source and a destination simultaneously, instad of just prohibiting source.
Therefore, both `mount options=(unbindable) /a,` and `mount options=(unbindable) -> /a,` are now supported (and equivalent for apparmor_parser). However, `mount options=(unbindable) /a -> /b,` is invalid.
For the same reason, specifying a fstype in these cases is also prohibited.
Similarly, we prohibit to specify a fstype for bind mount rules.
Fixes: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2065685
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1236
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
Mount Rules with options in { remount, [make-] { [r]unbindable, [r]shared, [r]private, and [r]slave }} do not support specifying a source. This commit aligns utils implementation to apparmor_parser's, which prohibits having a both source and a destination simultaneously, instad of just prohibiting source.
Therefore, both `mount options=(unbindable) /a,` and `mount options=(unbindable) -> /a,` are now supported (and equivalent for apparmor_parser). However, `mount options=(unbindable) /a -> /b,` is invalid.
For the same reason, specifying a fstype in these cases is also prohibited.
Similarly, we prohibit to specify a fstype for bind mount rules.
Fixes: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2065685
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
... for handling pivot_root rules.
This replaces the old code that just stores the full rule as text.
We also get rid of the old ['allow'] and ['deny'] items in
ProfileStorage, the handling of old write functions, and the last usage
of _Raw_Rule (and therefore _Raw_Rule itsself).
Also delete the old test-pivot_root_parse.py which relied on the ancient
code, and even used a wrong syntax in its test rules.
The following exceptions were added to flake8 since they have several
expected uses in the tools and their tests:
E501: Line lengths are recommended to be no greater than 79 characters.
E241: Multiple spaces after ','
W503: Line break occurred before a binary operator
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
ProfileStorage knows a whole profile, therefore it should also include the profile header in `__repr__()`.
Also add a test for this.
While on it, add a test for an invalid type change for a type that doesn't have special handling in `__setitem__()` to increase test coverage.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1233
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Christian Boltz <apparmor@cboltz.de>
Since musl 1.2.5, basename(3) prototype is only provided in libgen.h
(as mandated by POSIX) and not in strings.h. Also there is a major
difference between the gnu basename and the one defined in libgen.h,
the latter modify the argument string making them incompatible.
Fix this by defining a portable version of basename using strchr.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1234
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
Since musl 1.2.5, basename(3) prototype is only provided in libgen.h
(as mandated by POSIX) and not in strings.h. Also there is a major
difference between the gnu basename and the one defined in libgen.h,
the latter modify the argument string making them incompatible.
Fix this by defining a portable version of basename using strchr.
audit.log entries for mount events don't always include `class=mount`,
but can still be the base for mount rules.
Change logparser.py to also consider `operation=mount` as a mount event.
Actually we already had such a log and profile in our collection
(testcase_mount_01), but since it existed years before MountRule was
implemented, it was excluded in test-libapparmor-test_multi.py.
Therefore we didn't notice that it failed to produce a profile rule when
MountRule was introduced.
Remove testcase_mount_01 from the list of known failures so that it gets
tested - and fix the syntax error in the hand-written
testcase_mount_01.profile.
Also add testcase_mount_02 which is a mount event without fstype,
srcname and class.
I propose this fix for 4.0 and master.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1229
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
audit.log entries for mount events don't always include `class=mount`,
but can still be the base for mount rules.
Change logparser.py to also consider `operation=mount` as a mount event.
Actually we already had such a log and profile in our collection
(testcase_mount_01), but since it existed years before MountRule was
implemented, it was excluded in test-libapparmor-test_multi.py.
Therefore we didn't notice that it failed to produce a profile rule when
MountRule was introduced.
Remove testcase_mount_01 from the list of known failures so that it gets
tested - and fix the syntax error in the hand-written
testcase_mount_01.profile.
Also add testcase_mount_02 which is a mount event without fstype,
srcname and class.
Did some testing on a fresh post-release image of noble, and uncovered some new denials:
#### Xorg
`2024-05-06T19:55:36.782484-04:00 image-ubuntu64 kernel: audit: type=1400 audit(1715039736.765:174): apparmor="DENIED" operation="link" class="file" profile="Xorg" name="/tmp/.X0-lock" pid=1366 comm="Xorg" requested_mask="r" denied_mask="r" fsuid=0 ouid=0 target="/tmp/.tX0-lock"`
#### chromium_browser
`2024-05-06T21:17:09.674963-04:00 image-ubuntu64 kernel: audit: type=1400 audit(1715040834.256:168): apparmor="DENIED" operation="userns_create" class="namespace" profile="chromium_browser" pid=2133 comm="chromium" requested="userns_create" denied="userns_create"`
#### firefox
`2024-05-06T21:33:09.387356-04:00 image-ubuntu64 kernel: audit: type=1400 audit(1715045589.369:505): apparmor="DENIED" operation="userns_create" class="namespace" profile="firefox" pid=3610 comm="firefox" requested="userns_create" denied="userns_create"`
`2024-05-06T21:36:48.911280-04:00 image-ubuntu64 kernel: audit: type=1400 audit(1715045808.884:682): apparmor="DENIED" operation="open" class="file" profile="firefox" name="/sys/fs/cgroup/user.slice/user-1000.slice/session-c2.scope/cpu.max" pid=4348 comm="firefox" requested_mask="r" denied_mask="r" fsuid=1000 ouid=0`
`2024-05-06T21:42:34.152955-04:00 image-ubuntu64 dbus-daemon[1628]: apparmor="DENIED" operation="dbus_bind" bus="session" name="org.mpris.MediaPlayer2.firefox.instance_1_82" mask="bind" pid=4348 label="firefox"`
#### Xorg (second commit)
I neglected to set `abi/4.0` when this went in originally. (I was using the profile on jammy, hence the `3.0`.)
Also, testing on an older laptop that *doesn't* support KMS revealed that Xorg needs some pretty serious permissions then. I've added them in commented-out form, with an explanatory comment. (The `#nokms#` bit is meant to simplify uncommenting those two lines mechanically, e.g. `sed -i 's/#nokms#//'`)
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1227
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
Balena Etcher runs in a degraded sandbox mode when unprivileged userns
is not available. Add an unconfined profile so it's properly
sandboxed.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Shellcheck is complaining that some of the functions are never called,
but they are called from rc.apparmor.functions, causing a false
positive.
This issue only appears in shellcheck version 0.9.0, which is the one
used in ubuntu 24.04, that's why it only failed in the pipeline now.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/388
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Since we are using ubuntu:latest, and noble was released, some tests
are failing.
shellcheck needs python3 to run, which was possibly installed by
default in previous ubuntu images and is no longer the case.
Ignore dist-packages python files during our coverage tests.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/388
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Rename the "check-extras" target to "check-local" as it is no longer
limited to the extra profiles, and also fix a local include in the
sbuild-shell profile so that it passes the newly-applied CI check.
Add a flag that allows setting the error code AppArmor will send when
an operation is denied. This should not be used normally.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
This is a profile to contain the Xorg X11 server, which still runs as root in many scenarios (not least under [LightDM](https://github.com/canonical/lightdm/issues/18)).
I've tested this under every X display manager available in Debian/Ubuntu, as well as plain `startx(1)`. Both rootful and rootless modes are covered. The hardware I've tried this on predominantly uses Intel integrated graphics, with one Nouveau system represented. If someone has an Nvidia GPU running the proprietary driver, that would be a good data point to double-check, owing to the different driver architecture.
As you can see, I avoided going too far into the weeds enumerating everything the X server needs to run. The general pattern I found was that it needs read access to a lot of things, but write access to relatively few.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1075
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
This updates the man page for the recent inet mediation patch.
This is an extension of MR 1202, it adds a patch that changes the anonymous ip address anon to be ip address none which is a better fit.
This patch adds documentation of the recent network changes which extended all network rules to support access permissions, and added address and port matching for inet and inet6 families.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1213
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
inet mediation allows specifying rules for sockets that don't have
a known address, whether because it is unbound or because the
kernel doesn't make the address available.
The current code uses the word anon for anonymous, but that has
proven to be unclear. Switch from using anon to none, to emphasize
that this is a case where there just isn't an address to use as
part of mediation.
Signed-off-by: John Johansen <john.johansen@canonical.com>
When a family is specified in the network rules, we have to make sure
the conditionals match the family. A netlink rule should not be able
to specify ip and port for local and remote (peer) sockets, for example.
When type or protocol is specified in network rules along with inet
conditionals, we should only generate rules for the families that
support those conditionals.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The bwrap and unshare profiles are special profiles in the same
vein as the unconfined profiles but they actual enforce restrictions
on the applications that are launched.
As such they have come to late in the 4.0 dev cycle to consider enabling
by default. Disable them but ship them so users or distros can easily
enable them.
Signed-off-by: John Johansen <john.johansen@canonical.com>
This adds a bwrap profile to allow it to function on a system with
user namespace restrictions enabled.
The child task of bwrap will enter into a profile without capabilities
thus preventing bwrap from being able to be used to arbitrarily
by-pass user namespace restrictions.
This profile does prevent applications launch with privilege (eg.
sudo bwrap ...) from functioning so it may break some use cases.
Note: The unpriv_bwrap profile is deliberately stacked against the
bwrap profile due to bwraps uses of no-new-privileges.
Fixes: https://bugs.launchpad.net/ubuntu/+source/pageedit/+bug/2046844
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1205
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
This adds a bwrap profile to allow it to function on a system with
user namespace restrictions enabled.
The child task of bwrap will enter into a profile without capabilities
thus preventing bwrap from being able to be used to arbitrarily
by-pass user namespace restrictions.
This profile does prevent applications launch with privilege (eg.
sudo bwrap ...) from functioning so it may break some use cases.
Note: The unpriv_bwrap profile is deliberately stacked against the
bwrap profile due to bwraps uses of no-new-privileges.
Fixes: https://bugs.launchpad.net/ubuntu/+source/pageedit/+bug/2046844
Signed-off-by: John Johansen <john.johansen@canonical.com>
The version of tarball version of firefox downloaded from mozilla.org
installs to /opt/firefox/firefox. Support this location so that the
firefox from the tarball works.
Note this does not support running firefox from the users home directory
in this case the user must update the profile accordingly.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1203
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
This adds an unshare profile to allow it to function on a system
with user namespace restrictions enabled.
The child task of unshare will enter into a profile without capabilities
thus preventing unshare from being able to arbitrarily being used to
by-pass the user namespace restriction.
This profile does prevent applications launch with privilege (eg.
sudo unshare ...) from functioning so it may break some use cases.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The version of tarball version of firefox downloaded from mozilla.org
installs to /opt/firefox/firefox. Support this location so that the
firefox from the tarball works.
Note this does not support running firefox from the users home directory
in this case the user must update the profile accordingly.
Signed-off-by: John Johansen <john.johansen@canonical.com>
In some cases, ldd might obtain information by executing the given
binary (see ldd(1)) - which is not something we should do on potentially
unknown binaries, especially because aa-genprof and aa-autodep (and
therefore also ldd) are often started as root.
Additionally, the ldd result typically listed libraries already covered
by abstractions/base, which makes the ldd call superfluous.
While on it,
- remove all references to ldd
- remove code only used for calling ldd and handling its results
- remove tests checking ldd results, and the fake_ldd script
- adjust a test where fake_ldd had added some libraries
- remove ldd path from logprof.conf [settings]
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1201
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Christian Boltz <apparmor@cboltz.de>
- Before this commit, fstype had to match a known fs. However, having and maintaining the exhaustive list of fstypes proved challenging (see !1195 and !1176). Therefore, we add support for any filesystem name.
- Completing AARE support for fstype (brace expressions like ext{3,4} are now supported).
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1198
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
- Before this commit, fstype had to match a known fs. However, having and maintaining the exhaustive list of fstypes proved challenging (see !1195 and !1176). Therefore, we add support for any filesystem name.
- Completing AARE support for fstype (brace expressions like ext{3,4} are now supported).
In some cases, ldd might obtain information by executing the given
binary (see ldd(1)) - which is not something we should do on potentially
unknown binaries, especially because aa-genprof and aa-autodep (and
therefore also ldd) are often started as root.
Additionally, the ldd result typically listed libraries already covered
by abstractions/base, which makes the ldd call superfluous.
While on it,
- remove all references to ldd
- remove code only used for calling ldd and handling its results
- remove tests checking ldd results, and the fake_ldd script
- adjust a test where fake_ldd had added some libraries
- remove ldd path from logprof.conf [settings]
Give access to @{HOMEDIRS}, just like in usr.sbin.smbd, so that
usershares in /home/ can be accessed.
apparmor="DENIED" operation="open" class="file" profile="samba-rpcd-classic" name="/home/user/path/to/usershare/" pid=4781 comm="rpcd_classic" requested_mask="r" denied_mask="r" fsuid=0 ouid=1000
- Adding support for --output-dir in aa-logprof and aa-genprof, allowing to work on profiles without applying the modified version
- Adding support for --allow-all in aa-logprof that creates non-interactively 'allow' rules for all logs
- Adding support for --no-abstraction in aa-logprof and aa-genprof
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1177
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
When the setup of the notify options failed, they were exiting the
program without cleaning up the mqueue. Fix this by returning instead
of exiting, since the main function does the cleanup in case of any
failures. If the test succeeds, then it exits successfully.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
mq_notify only notifies if the queue is empty, so if the sender wins
the race and sends a message before mq_notify is set up, mq_notify
will timeout.
Adding synchronization using pipes the same way it was used in the
setns tests should fix it. The pipe now needs rw permissions, so add
that to the tests.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The child which sends the message was winning the race and causing a
timeout when the receiver was waiting for the message.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
net_inet makes more sense since other finegrained network types can be
added in the future.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The abstraction lxc/start-container shipped by the liblxc-common
package uses the following mount rule which was not allowed by our
regexes:
mount options=(rw, make-slave) -> **,
mount options=(rw, make-rslave) -> **,
Since in AppArmor regex ** includes '/' but * by itself doesn't, I'm
adding explicit support for **.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1195
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
The abstraction lxc/start-container shipped by the liblxc-common
package uses the following mount rule which was not allowed by our
regexes:
mount options=(rw, make-slave) -> **,
mount options=(rw, make-rslave) -> **,
Since in AppArmor regex ** includes '/' but * by itself doesn't, I'm
adding explicit support for **.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
The network cmp function was missing the new attributes added, causing
rules to be dropped.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
When the ip is not specified, then we should generate rules for ip
types: anonymous, ipv4 and ipv6. And that's the case for both local
and peer when considering recv and send permissions.
std::ostringstream does not have a copy constructor, that's why in
several places one can see streaming the string of one stream into
another.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
We want to be able to determine label in the future and build the
policy dfa based on its presence or not.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
According to the protocol expected by the kernel, the field
representing the ip size should be an enum instead of the actual ip
size. This is more future-proof.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Before the inet patches, protocol was not handled, so the information
was ignored. This patch introduces the ability to start mediating
protocol.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
We're not trying to execute a command in EXTRA_WARNINGS, so don't try to spawn
a subshell for it which gives:
```
./configure: 14770: EXTRA_WARNINGS: not found
checking whether C compiler accepts -flto-partition=none... yes
```
We can either use ${} or just $ (style). Use $ to be consistent with other
uses in the file.
Signed-off-by: Sam James <sam@gentoo.org>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1184
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
We're not trying to execute a command in EXTRA_WARNINGS, so don't try to spawn
a subshell for it which gives:
```
./configure: 14770: EXTRA_WARNINGS: not found
checking whether C compiler accepts -flto-partition=none... yes
```
We can either use ${} or just $ (style). Use $ to be consistent with other
uses in the file.
Signed-off-by: Sam James <sam@gentoo.org>
Latest pam_unix always runs /usr/sbin/unix_chkpwd instead of reading
/etc/shadow itsself. Add exec permissions to abstraction/authentication.
It also needs to read /proc/@{pid}/loginuid
Also cleanup the now-superfluous rules from the smbd profile.
Fixes: https://bugzilla.opensuse.org/show_bug.cgi?id=1219139
With abstractions/openssl now being included from abstraction/base
(via the indirection of abstractions/crypto) anything already
including abstraction/base can stop including abstractions/openssl
directly.
This is a follow up to 3d1dedfa as suggested by @cboltz
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1179
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
With abstractions/openssl now being included from abstraction/base
(via the indirection of abstractions/crypto) anything already
including abstraction/base can stop including abstractions/openssl
directly.
Administrators might want to define global limits (e.g. disabling
a particular feature) via configuration files, but to make that work
all confined software needs to be allowed to read those files or
otherwise the risk is to silently fall back to internal defaults.
This adds the abstraction already defined for openssl to
abstraction/crypto as it is about cryptography, but also because
abstraction/base includes abstraction/crypto and therefore it will
be allowed in general.
Administrators might want to define global limits (e.g. disabling
a particular feature) via configuration files, but to make that work
all confined software needs to be allowed to read those files or
otherwise the risk is to silently fall back to internal defaults.
This adds the paths usually used by gnutls to abstraction/crypto
as it is about cryptography, but also because abstraction/base
includes abstraction/crypto and therefore it will be allowed
in general.
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
