For every item in "cases", a new state is created, but if the creation
of one of them fails, the rest of the items in that list would not be
deleted and would leak. Fix it by continuing to iterate over the items
in the list and delete them, and then re-throw the exception.
$ /usr/bin/valgrind --leak-check=full --error-exitcode=151 ../apparmor_parser -Q -I simple_tests/ -M ./features_files/features.all simple_tests/xtrans/x-conflict.sd
==564911== 208 (48 direct, 160 indirect) bytes in 1 blocks are definitely lost in loss record 15 of 19
==564911== at 0x4846FA3: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==564911== by 0x179E74: CharNode::follow(Cases&) (expr-tree.h:447)
==564911== by 0x189F8B: DFA::update_state_transitions(optflags const&, State*) (hfa.cc:376)
==564911== by 0x18A25B: DFA::process_work_queue(char const*, optflags const&) (hfa.cc:442)
==564911== by 0x18CB65: DFA::DFA(Node*, optflags const&, bool) (hfa.cc:486)
==564911== by 0x178263: aare_rules::create_chfa(int*, std::vector<aa_perms, std::allocator<aa_perms> >&, optflags const&, bool, bool) (aare_rules.cc:258)
==564911== by 0x178A4F: aare_rules::create_dfablob(unsigned long*, int*, std::vector<aa_perms, std::allocator<aa_perms> >&, optflags const&, bool, bool) (aare_rules.cc:359)
==564911== by 0x14E4E1: process_profile_regex(Profile*) (parser_regex.c:791)
==564911== by 0x154CDF: process_profile_rules(Profile*) (parser_policy.c:194)
==564911== by 0x154E0F: post_process_profile(Profile*, int) (parser_policy.c:240)
==564911== by 0x154F7A: post_process_policy_list (parser_policy.c:257)
==564911== by 0x154F7A: post_process_policy(int) (parser_policy.c:267)
==564911== by 0x141B17: process_profile(int, aa_kernel_interface*, char const*, aa_policy_cache*) (parser_main.c:1227)
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/534
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
When the "conflicting x modifiers" exception was thrown, the DFA
object creation would fail, therefore the destructor would not be
called and the states previously allocated would leak.
Unfortunately there's no way to call the destructor if the object was
not created, so I moved the contents of the destructor into a cleanup
helper function to be called in both instances.
$ /usr/bin/valgrind --leak-check=full --error-exitcode=151 ../apparmor_parser -Q -I simple_tests/ -M ./features_files/features.all simple_tests/xtrans/x-conflict.sd
==564911== 592 (112 direct, 480 indirect) bytes in 1 blocks are definitely lost in loss record 16 of 19
==564911== at 0x4846FA3: operator new(unsigned long) (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==564911== by 0x189C9A: DFA::add_new_state(optflags const&, std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, std::set<ImportantNode*, std::less<ImportantNode*>, std::allocator<ImportantNode*> >*, State*) (hfa.cc:337)
==564911== by 0x18CB22: add_new_state (hfa.cc:357)
==564911== by 0x18CB22: DFA::DFA(Node*, optflags const&, bool) (hfa.cc:473)
==564911== by 0x178263: aare_rules::create_chfa(int*, std::vector<aa_perms, std::allocator<aa_perms> >&, optflags const&, bool, bool) (aare_rules.cc:258)
==564911== by 0x178A4F: aare_rules::create_dfablob(unsigned long*, int*, std::vector<aa_perms, std::allocator<aa_perms> >&, optflags const&, bool, bool) (aare_rules.cc:359)
==564911== by 0x14E4E1: process_profile_regex(Profile*) (parser_regex.c:791)
==564911== by 0x154CDF: process_profile_rules(Profile*) (parser_policy.c:194)
==564911== by 0x154E0F: post_process_profile(Profile*, int) (parser_policy.c:240)
==564911== by 0x154F7A: post_process_policy_list (parser_policy.c:257)
==564911== by 0x154F7A: post_process_policy(int) (parser_policy.c:267)
==564911== by 0x141B17: process_profile(int, aa_kernel_interface*, char const*, aa_policy_cache*) (parser_main.c:1227)
==564911== by 0x135421: main (parser_main.c:1771)
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/534
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Valgrind showed that the disconnected paths variables were leaking
during the merge. That happened because flagvals did not implement a
destructor freeing the variables, so they leaked. flagvals cannot
implement a destructor, because that would make it a non-trivial union
member and parser_yacc.y would not compile. This patch implements a
"clear" function that is supposed to act as the destructor.
$ /usr/bin/valgrind --leak-check=full --error-exitcode=151 ../apparmor_parser -Q -I simple_tests/ -M ./features_files/features.all flags_ok_disconnected_ipc15.sd
...
==3708747== 5 bytes in 1 blocks are definitely lost in loss record 1 of 11
==3708747== at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==3708747== by 0x492E35E: strdup (strdup.c:42)
==3708747== by 0x14C74E: set_disconnected_path (profile.h:188)
==3708747== by 0x14C74E: flagvals::init(char const*) (profile.h:223)
==3708747== by 0x14859B: yyparse() (parser_yacc.y:592)
==3708747== by 0x141A99: process_profile(int, aa_kernel_interface*, char const*, aa_policy_cache*) (parser_main.c:1187)
==3708747== by 0x135421: main (parser_main.c:1771)
...
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
perms = 0, therefore perms & something is always false.
Fixes: coverity#320937 and coverity#320937
Also remove nop code from mnt_rule::post_parse_profile(Profile &prof) as discussed in this MR.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1759
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
When the variable was being expanded, it needed to be reevaluated to
check if there was still unresolved variables. That allowed for a
weird bug to happen: If the string contained a variable preceded by @,
like in "user@@{uid}" and the variable was resolved to a case where {
is used, like in @{uid}={[0-9],[1-9][0-9]}, then on the second pass,
the parser would try to resolve the following variable
@{[0-9],[1-9][0-9]}, which is incorrect behavior. Fix it by not
including part of the string that was already resolved on the
subsequent passes.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
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>
