Their is no reason for the parse to stitch 2 dfas together this way.
In the future there will be better ways to do this using unconpressed
dfas.
Dropping this also allows for some simplification, in other parts of
the code.
Drop the dead/unused code
Signed-off-by: John Johansen <john.johansen@canonical.com>
prompt_compat_permsv1 and prompt_compat_dev were used to support
prompt during early dev. We do not support any kernel using these
so drop them.
This also allows us to drop the propogation of prompt as a parameter
through several functions.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The xtable on perms32 capable systems is being padded to the size of
the accept state tables. This was a hack to get around issue in a buggy
perms32 v1. We do not support any system using perms 32 v1 so we can
drop the hack.
Signed-off-by: John Johansen <john.johansen@canonical.com>
When variable expansion occurs, the expansion attempts to replace the
memory location of the string containing the variable, and frees the
string it is replacing. However, this occurs before the variable lookup
occurs to determine if there is an appropriate declaration for the
variable. When the failing expansion occurs in a profile name, this
causes a read-after-free (followed by a double free) because the error
handling path attempts to report the profile name in the error message.
This can be reproduced like so, using the
tst/simple_tests/vars/vars_profile_name_23.sd testcase:
```
$ ../apparmor_parser --config-file=./parser.conf -M features_files/features.all -S -I /home/sbeattie/git/apparmor/parser/tst/./simple_tests/ ./simple_tests/vars/vars_profile_name_23.sd
Failed to find declaration for: @{FOO}
ERROR expanding variables for profile #xQV, failed to load
free(): double free detected in tcache 2
```
Fix this by waiting to free the profile name field until after the
variable declaration has successfully been looked up. This results in
the test case reporting the following error:
```
$ ../apparmor_parser --config-file=./parser.conf -M features_files/features.all -S -I /home/sbeattie/git/apparmor/parser/tst/./simple_tests/ ./simple_tests/vars/vars_profile_name_23.sd
Failed to find declaration for: @{FOO}
ERROR expanding variables for profile /does/not/exist@{FOO}, failed to load
```
Fixes: dfbd2dc4b ("parser: refactor variables and symbols table into their own class")
Signed-off-by: Steve Beattie <steve@nxnw.org>
Ref: https://gitlab.com/apparmor/apparmor/-/merge_requests/1747
Fix issues introduced in coverity's snapshot 89167
- CID 532797: (#1 of 1): Use of auto that causes a copy (AUTO_CAUSES_COPY)
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
While symtab for now has only static members, it will allow for a
change in the future for each profile to have their own symbols like
profile_name, etc.
According to commit cce5bd6e95ae9a9f01caceea0d5d75b612dd3fbc, the
apparmor_parser does not collapse consecutive / characters in the
beginning of paths, since it indicates posix namespaces. Add a
equality test to make sure we maintain this behavior.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Otherwise tst_regex would log as being from parser_common.c instead of
being from the actual source of parser_regex.c
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
test_profile.sh contained some bash-specific code and a bug in a regex
that failed to flag some profiles where read access to their attachment
path was not allowed.
Replace it with a Python script, more robust and maintenable.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
Have the parser extract the attachment path from the profile declaration
and make it available as a local variable within the profile. This allows
profile rules to use the executable attachment path in rules.
eg.
```
profile ex /bin/** {
@{attach_path} r,
# ...
}
profile /path/to/bin {
@{attach_path} r,
# ...
}
```
if a profile does not define an attachment like
```
profile noattach {
@{attach_path} r,
}
```
the apparmor_parser will fail the compile with the error.
```
Found reference to variable attach_path, but is never declared
```
While not recommended for rules directly in a profile the above
the undeclared variable error can be avoided in in abstractions
by wrapping the variable in a conditional.
```
if defined @{attach_path} {
@{attach_path r,
}
```
The attachment xattr/label conditionals are not made available at
this time as regular file path rules can not use them.
Similarly a @{exec_path} variable is made available. It is different
than @{attach_path} in that it is intended to be a kernel variable
that represents the specific executable that was matched at run
time. However to support policy on kernels that don't define the
kernel variable it has a fallback value that is the same as
@{attach_path}.
This patch is a follow on to MR:1637 (https://gitlab.com/apparmor/apparmor/-/me\
rge_requests/1637)
and is similar to how the apparmor.d project uses the manually setup
@{exec_path} variable.
We can bike shed over the variable name. @{attach_path} was chosen
here because this is the attachment conditional path for the
executable, not the executable's actual path. While @{exec_path} is
intended to be the applications actual executable path.
support the @{exec_path} kernel variable (all of them atm).
Notes:
The minimize.sh tests are changed because this patch causes path based
profile names to create an attachment. This could be done by doing the
attach_variable expansion in the alternate location marked by the
patch, but since the kernel is going to start doing this for all
profiles that don't have an attachment it is better for the parser to
do it, as it can optimize better.
This patch series may cause breakage if policy declares either
@{attach_path} or @{exec_path} by shadowing those previously declared
variables in the profile block. The previously declared variable
is available in the attachment specification so uses like the
apparmor.d project won't break as it with transfer its variable
value to the attachment which will the transfer that value into
the automatic local var.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1643
Approved-by: Maxime Bélair <maxime.belair@canonical.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Due to how labeling is implemented, during the creation it is not yet
defined, so we need to grant create permissions without attaching the
label yet. Also, adjust tests to pass when label support is
implemented in the kernel.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1623
Approved-by: Maxime Bélair <maxime.belair@canonical.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Make it so the @{exec_path} and @{attach_path} variables behavior
completely as local variables, overriding global variables of the
same name, instead of conflicting with them.
The exec var is only validate for the profile block after the attachment
is defined so the pattern
@{exec_path}=/path
profile test @{exec_path} {
@{exec_path} rw,
}
is valid with the global var defining the attachent which then sets
the local auto @{exec_path} and @{attach_path} variables.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Have the parser extract the attachment path from the profile declaration
and make it available as a variable within the profile. This allows
profile rules to use the executable attachment path in rules.
eg.
```
profile ex /bin/** {
@{attach_path} r,
# ...
}
profile /path/to/bin {
@{attach_path} r,
# ...
}
```
if a profile does not define an attachment like
```
profile noattach {
@{attach_path} r,
}
```
the apparmor_parser will fail the compile with the error.
```
Found reference to variable attach_path, but is never declared
```
The attachment xattr/label conditionals are not made available at
this time as regular file path rules can not use them.
Similarly a @{exec_path} variable is made available. It is different
than @{attach_path} in that it is intended to be a kernel variable
that represents the specific executable that was matched at run
time. However to support policy on kernels that don't define the
kernel variable it has a fallback value that is the same as
@{attach_path}.
This patch is a follow on to MR:1637 (https://gitlab.com/apparmor/apparmor/-/merge_requests/1637)
and is similar to how the apparmor.d project uses the manually setup
@{exec_path} variable.
We can bike shed over the variable name. @{attach_path} was chosen
here because this is the attachment conditional path for the
executable, not the executable's actual path. While @{exec_path} is
intended to be the applications actual executable path.
support the @{exec_path} kernel variable (all of them atm).
Notes:
The minimize.sh tests are changed because this patch causes path based
profile names to create an attachment. This could be done by doing the
attach_variable expansion in the alternate location marked by the
patch, but since the kernel is going to start doing this for all
profiles that don't have an attachment it is better for the parser to
do it, as it can optimize better.
This patch may cause breakage if policy declares either @{attach_path}
or @{exec_path} this will not be dealt with here, but in a subsequent
patch that allows variables to have a local scope so that the compiler
defined vars will just get declared locally.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Specifying norelatime should set the corresponding MS_RELATIME flag clear
bit. Instead, it ORed in MS_NORELATIME, which expands to 0. Properly set
the clear bit by using MS_RELATIME.
Fixes: c9e31b7f "Add mount rules"
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1679
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
The internal permission accumulation is currently broken in that
the ordering of rules matter to whether deny is clearing accumulated
perms.
If a deny node comes before an allow node the deny bits will get set
but the following allow bits won't get cleared by the deny node.
This isn't currently an actual issue for mediation as the deny
bit will be applied at one of
1. apply_and_clear_deny
2. permission remapping
3. run time mediation
but it does result in the internal state having sometimes having both
allow and deny bits set, dependent on order of computation, resulting
in state machines with different sizes because minimization
partitioning is based on the internal permissions.
This means that dfa minimization may not result in a truly minimal
state machine, and even worse can cause inconsistenty and failure in
tests that rely on internal state like the equality and minimization
test, as seen in https://gitlab.com/apparmor/apparmor/-/issues/513
The failure was due to musl stl sets implementation producing a
different ordering of the nodes than glibc. So when the permissions
where accumulated the internal set of permissions were different.
Fix this by giving the different node classes their own internal priority.
This will ensure the bits are properly cleared for that priority before
accumulating.
Note: other ways of fixing.
1. Fixup internal accumulation to use accumulating perms of "higher"
priority as part of the mask (deny and allow mask prompt).
2. Do a hard masking apply at the end after all bits have been accumulated
(ie, in accept_perms after the for loop).
the priority route was chosen because it is a little smaller and
scales better if we get new Node types we have to deal with
(eg. planned complain node).
BugLink: https://gitlab.com/apparmor/apparmor/-/issues/513
Fixes: 1ebd99115 ("parser: change priority so that it accumulates based on permissions")
Signed-off-by: John Johansen <john.johansen@canonical.com>
Specifying norelatime should set the corresponding MS_RELATIME flag clear
bit. Instead, it ORed in MS_NORELATIME, which expands to 0. Properly set
the clear bit by using MS_RELATIME.
Fixes: c9e31b7f "Add mount rules"
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
This helps to limit the amount of rules skipped in the utils tests
(because the utils don't support the `unsafe` keyword)
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1671
Approved-by: Maxime Bélair <maxime.belair@canonical.com>
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Approved-by: Ryan Lee <rlee287@yahoo.com>
Merged-by: Ryan Lee <rlee287@yahoo.com>
The lack of a space after $testtype is a syntax error and was causing the
equality tests on Ubuntu Xenial to be silently skipped and marked PASS.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
- set filetype, instead of syntax, in vim modelines
- replace filetype of subdomain with apparmor
- move modelines in the first or last five lines of each file so that
vim can recognize them
We need to be able to dump the initial partition assignments, and then
the partitions after minimization but before remapping to be able to
check on what is being done by minimization.
Add these as part of -D dfa-minimize-partitions
Ideally we would rework the code so that the existing mininimization
dump could share the dump routine but, its interwined with computation
state and information is thrown away before reaching the end.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Unconfined delegates access to open file descriptors. Therefore when running a confined binary from unconfined, it will work even when the attachment path is not read-allowed.
However, as soon as these confined binaries are run from another confined process, this delegation is not permitted anymore and the program breaks.
This has been the cause of several bugs such as https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107455 or https://github.com/canonical/snapd/pull/15181 .
This MR makes sure every confining AppArmor profiles explicitly allow (at least) read access to their attachment path.
This Merge request:
- Introduce `test_profile.sh`, a helper script that ensures confining AppArmor profiles explicitly allow (at least) read access to their attachment path.
- Modifies a lot of profiles so that all profiles have r/mr access to their attachment path
- Extends `make check` to automatically ensure all AppArmor profile grant explicit read access to their attachment path, preventing future omissions.
- Modifies apparmor_parser to show attachment in --debug output
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1637
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
glibc defines bsd's rlimit ofile as nofile, however musl.
Instead of just dropping ofile which would be bad for policy portability
make sure it is defined to be nofile.
This is a partial fie for
https://gitlab.com/apparmor/apparmor/-/issues/513
Signed-off-by: John Johansen <john.johansen@canonical.com>
Unconfined delegates access to open file descriptors. Therefore when
running a confined binary from unconfined, it will work even when the
attachment path is not read-allowed.
However, as soon as these confined binaries are run from another
confined process, this delegation is not permitted anymore and the
program breaks.
This has been the cause of several bugs such as
https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/2107455 or
https://github.com/canonical/snapd/pull/15181 .
Introduce `test_profile.sh`, a helper script that ensures confining
AppArmor profiles explicitly allow (at least) read access to their
attachment path.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
When showing the content of profiles with `apparmor_parser --debug`, the
attachment path is now displayed within the 'Debugging built structures'
section.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
Due to how labeling is implemented, during the creation it is not yet
defined, so we need to grant create permissions without attaching the
label yet.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
I am unsure how to test this equivalency due to how abi declarations interact with feature file command line arguments, so advice on that would be welcome.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1585
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Currently abi=<kernel> always grabs the kernels actual features but
it should respect --kernel-features=. This is causing the simple
tests to fail when abi=<kernel> is specified.
Fix it so abi=<kernel> respects the kernel abi specified in the configs
or on the command line.
Signed-off-by: John Johansen <john.johansen@canonical.com>
This enables us to exercise the front perms parse logic in the utils rule parsing through the simple tests as well
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
... and adjust the tools to raise an exception if such a rule is found.
While this is not nice, it's better than the previous behaviour where
only the last 'options' was kept, and the others were lost when writing
the rule.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1617
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
MR: 1561 Added the ability to specify special a keyword to allow
detached mounts. Unfortunately it updated remount to use the device
and devbuffer when remounts current encoding doesn't support it.
This caused the mount.sh regression test to fail in the following
way.
```
$ sudo bash mount.sh
[sudo] password for jj:
using mount rules ...
Error: mount failed. Test 'MOUNT (confined cap bind mount remount rprivate conflict)' was expected to 'pass'. Reason for failure 'FAIL: mount /tmp/sdtest.358520-12403-ASaOnn/mountpoint2 on /tmp/sdtest.358520-12403-ASaOnn/mountpoint failed - Permission denied'
not supported by parser - skipping mount options=(nodirsync),
Error: mount failed. Test 'MOUNT (confined cap mount remount option)' was expected to 'pass'. Reason for failure 'FAIL: mount /dev/loop40 on /tmp/sdtest.358520-12403-ASaOnn/mountpoint failed - Permission denied'
Error: mount failed. Test 'MOUNT (confined cap mount remount)' was expected to 'pass'. Reason for failure 'FAIL: mount /dev/loop40 on /tmp/sdtest.358520-12403-ASaOnn/mountpoint failed - Permission denied'
Error: mount passed. Test 'MOUNT (confined cap mount remount deny option)' was expected to 'fail'
```
Revert the change to remount. This fixes the regression failure.
fa0746f2e parser: add special casing for detached move mounts
Signed-off-by: John Johansen <john.johansen@canonical.com>
... and adjust the tools to raise an exception if such a rule is found.
While this is not nice, it's better than the previous behaviour where
only the last 'fstype' was kept, and the others were lost when writing
the rule.
... and adjust the tools to raise an exception if such a rule is found.
While this is not nice, it's better than the previous behaviour where
only the last 'options' was kept, and the others were lost when writing
the rule.
upsteam move_mount mediation now allows for a detached (disconnected)
mount to be move mounted into a namespace.
Add support for this by detecting 'detached' as a keyword for the
source/device and using it to create a null match. Because existing
mount encoding using a null separator between the mount terms null
match followed by the null seperator will separate detached mounts
within the existing encoding.
Eg.
mount detached -> /destination,
mount options=(ro) fstype=ext4 detached -> /destination,
This is functionally equivalent to using
mount "" -> /destination,
However using "" does not provide any context that about what the rule is allowing or why so the 'detached' form is preferred.
This is not a perfect solution, but is what can be currently supported
by the kernel without more LSM hooks.
On kernels that don't support detached mount detection, rules using
the detached souce conditional will be ignored (never matched).
This encoding also allows the existing
mount,
mount options=(move),
mount options=(move) -> /destination,
to continue to work with both detached and regular mounts on kernels
that support the move_mount() syscall.
Signed-off-by: John Johansen <john.johansen@canonical.com>
