This is not a complete fix for the spaces issue, but it is the next simple step that can be taken before the more difficult work of finding the remaining bugs in each shell script.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1424
Approved-by: Christian Boltz <apparmor@cboltz.de>
Merged-by: Christian Boltz <apparmor@cboltz.de>
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>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1423
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>
Fix libapparmor_re/Makefile so it works correctly with rebuilds and improve state machine dump information, to aid with debugging of permission handling during the compile.
Signed-off-by: John Johansen <john.johansen@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1410
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 test "Complain mode profile (file exec cx permission entry)" currently will only pass on a Ubuntu Oracular system due to a kernel bugfix patch that has not yet been upstreamed or backported.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1415
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
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>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1421
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
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>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1420
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
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 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>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1409
Approved-by: Ryan Lee <rlee287@yahoo.com>
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
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
I propose this patch for 4.0 and master.
Closes#463
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1417
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
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>
Even if we can't run the regression tests in our GitLab CI environment, we can at least ensure the binaries in the regression test suite compile successfully.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1414
Approved-by: Steve Beattie <steve+gitlab@nxnw.org>
Merged-by: Steve Beattie <steve+gitlab@nxnw.org>
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>
We use ProfileStorage everywhere, which makes checking if a specific
rule_type exists obsolete.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1405
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.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.
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>
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>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1406
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
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>
