... but don't get a new value assigned.
Found by pyflakes 3.3.2 / python 3.13.3
While on it, remove some obsolete, commented out debugging code.
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1708
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Parsing mount options (also) accepted partial matches as long as the
option started with the right characters. For example, 'options=syncfoo'
was parsed as 'sync'. This is also the reason why the list of mount
options was re-ordered so that 'r' and 'w' came last to give longer
options a chance to match (otherwise, 'rw' would be interpreted as 'r').
Fix parsing by adding a lookahead match so that the regex enforces that
the mount option is followed by whitespace, or is at the end of
rule_details.
Note that this issue only affected the options=foo syntax.
options=(foo) worked correctly even without this fix.
Now that this is fixed, move 'r' and 'w' back to their original position
in the list of mount options.
Also add a test where a mount rule ends with 'options=rw,' to ensure
that the '$' lookahead works.
Parsing `mount options=x` results in "Passed unknown options keyword to
MountRule: x", while parsing `mount options=xy` results in "Can't parse mount rule".
This difference happens because the code checks (besides the list of
known options) for a regex `([A-Za-z0-9])` which only matched a
single-character unknown option.
Change that regex to also match multiple characters, and also allow to
match `-` (used in some known mount options, so it's likely that it also
gets used in so far unknown mount options)
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1710
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: Georgia Garcia <georgia.garcia@canonical.com>
Parsing `mount options=x` results in "Passed unknown options keyword to
MountRule: x", while parsing `mount options=xy` results in "Can't parse mount rule".
This difference happens because the code checks (besides the list of
known options) for a regex `([A-Za-z0-9])` which only matched a
single-character unknown option.
Change that regex to also match multiple characters, and also allow to
match `-` (used in some known mount options, so it's likely that it also
gets used in so far unknown mount options)
The whole pattern already has `(...)*`, therefore there's no need to
make option_pattern optional.
Before this change, mount_condition_pattern could have matched
- on empty strings (it still can, thanks to the trailing `*` which can
also mean "zero matches") or
- whitespace-only strings (which is covered by the two regexes using
mount_condition_pattern - they both have `\s*` and/or `\s+` around it)
Improve #LOGPROF-SUGGEST metadata to support a set of space-separated
regexes. If this tag is present, the abstraction is only proposed to
aa-logprof if one of the regexes is matched.
If this abstraction should not be proposed to any profile, it is
possible to tell #LOGPROF-SUGGEST: no
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
The new option --show-matching-path shows a path that matches in the host
filesystem, to prove that the profile is indeed used.
Also, profiles' xattrs are now parsed into a dict and are taken in
consideration when looking for matching profiles.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
Many test provide their own implementation of cmd(). This commit makes
all of them rely on common.py implementation of cmd()
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
Introduce aa-show-usage, a new helper allowing to determine which
profiles on the system are used and which are not. A profile is marked as
used when at least one file installed in the machine matches the attach point
specified in the profile.
This tool supports filtering options, allowing users to, for example,
display only unconfined profiles that are currently in use. This can
notably help sysadmins to evaluate the security of their systems.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
The tools don't support having multiple options specified in mount
rules as it is allowed in the parser.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
This is a set of small tweaks to the merged rule interface window
- don't specify, font or size when setting bold
- improve message around unknown profiles
- add a custom message for snap profile
- output tktheme missing message, to help users identify they can improve the interface when started manually
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1529
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
Snap policy is a special case of the unknown profile. Give the user
a slightly better message for these messages.
Signed-off-by: John Johansen <john.johansen@canonical.com>
The current notification can be confusing, in that it can present a
profile followed by a list of rules that can't be selected.
Explictly state that the Unknown profile can't be modified so the user
has some indication that not being able to select the shown rules is
expected.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Instead of specifying the font type and size, which will not work for
all display configuration, use the the default BOLD font that tkinter
supplies.
Signed-off-by: John Johansen <john.johansen@canonical.com>
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>
- the autovars not being defined because the profile doesn't have an
attachment
- the autovar conflicting with a user defined var of the same name
Signed-off-by: John Johansen <john.johansen@canonical.com>
Add basic support for the priority rules prefix. This patch does not
allow the utils to set or suggest priorities. It allows parsing and
retaining of the priority prefix if it already exists on rules and
checking if it's in the supported range.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Some classes don't support modifiers like audit and deny. Only rlimit
has been checking for the allow keyword, but the others shouldn't
support it as well. Since they all do the same check, refactor them
into a method from BaseRule in case more modifiers are added.
Signed-off-by: Georgia Garcia <georgia.garcia@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
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>
The wrong clean rule is generated when unix rules contain qualifiers,
with the order inverted with the rule name.
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/511
Signed-off-by: Georgia Garcia <georgia.garcia@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 '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.
Note: If multiple fstype= or options= are given, this is not detected as
an error (to keep the regex simpler). When writing back such a rule,
only one fstype and options will "survive".
Adjust the exclude list in test-parser-simple-tests.py accordingly:
- several valid mount rules no longer fail
- two invalid mount rules which so far accidentally raised an exception
because of the fstype/options order no longer raise this exception
(conflicting mount options, which are the real reason why these rules
are invalid, are not detected in the tools)
Fixes: https://gitlab.com/apparmor/apparmor/-/issues/501
The tools are wrong in parsing the detached mount test.
Until that can be fixed, mark the tools as wrong.
Signed-off-by: John Johansen <john.johansen@canonical.com>
--configdir is meant for testing and should override all other configs,
instead of being combined with them. Config combination causes aa-notify
test failures if e.g. the user-local config sets filtering options.
Also supply a notify.conf file for exclusive use during testing.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1610
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
--configdir is meant for testing and should override all other configs,
instead of being combined with them. Config combination causes aa-notify
test failures if e.g. the user-local config sets filtering options.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Both the unconfined profile and unprivileged_userns are part of the
default notify.conf, so the default fallback when no configurations are
present should also match this default.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
Fixes https://bugs.launchpad.net/apparmor/+bug/2106033
Improve the validation of AARE file paths by introducing a new regex
that supports paths starting with '{' (e.g. '{/,/org/freedesktop/DBus}').
These paths are notably used in snap.lxd.* profiles.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1607
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
These are the default directory mounts used by Flatpak's system cache for mounting revokefs-fuse. Unfortunately, the new rules are quite broad, but we might not be able to do much better than that.
Signed-off-by: Ryan Lee <ryan.lee@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1562
Approved-by: Georgia Garcia <georgia.garcia@canonical.com>
Merged-by: John Johansen <john@jjmx.net>
Fixes https://bugs.launchpad.net/apparmor/+bug/2106033
Improve the validation of AARE file paths by introducing a new regex
that supports paths starting with '{' (e.g. '{/,/org/freedesktop/DBus}').
These paths are notably used in snap.lxd.* profiles.
Signed-off-by: Maxime Bélair <maxime.belair@canonical.com>
If the following scenario was present in a profile, cleanprof
would fail with a RecursionError exception (maximum recursion
depth exceeded)
/parent { }
/parent///child { }
This occured because in aa.py, in the write_piece function, the
wrong depth was being passed, along with a wrong hat. The
formatting of the spaces was also incorrect.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
MR: https://gitlab.com/apparmor/apparmor/-/merge_requests/1605
Approved-by: John Johansen <john@jjmx.net>
Merged-by: John Johansen <john@jjmx.net>
If the following scenario was present in a profile, cleanprof
would fail with a RecursionError exception (maximum recursion
depth exceeded)
/parent { }
/parent///child { }
This occured because in aa.py, in the write_piece function, the
wrong depth was being passed, along with a wrong hat. The
formatting of the spaces was also incorrect.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Profiles that are defined like below did not have the parent attribute
set in profile storage:
/parent///child {}
The condition on which child profiles were written was also changed so
they are not removed from the profile if /parent does not exist.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
Commit c9d41a3ebb introduced a regression on profile header
generation.
This commit removes the name parameter from the get_header function
since the ProfileStorage should already contain all the information
required to generate the header for profiles and hats. The tests
needed to be updated as well to make sure the ProfileStorage object
contained the information needed by the get_header method.
Fixes: c9d41a3ebb ("utils: fix profile and hat header generation")
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com>
