Thanks to a bug in the apparmor.d manpage, NetworkRule rejected rules
that contained only TYPE (for example "network stream,"). A bugreport on
IRC and some testing with the parser showed that this is actually
allowed, so NetworkRule should of course allow it.
Note: not strip()ing rule_details is the easiest way to ensure we have
whitespace in front of the TYPE in TYPE-only rules, which is needed by
the RE_NETWORK_DETAILS regex.
Also adjust the tests to the correct behaviour.
Acked-by: Steve Beattie <steve@nxnw.org>
Instead of always showing a backtrace,
- for AppArmorException (used for profile syntax errors etc.), print only
the exceptions value because a backtrace is superfluous and would
confuse users.
- for other (unexpected) exceptions, print backtrace and save detailed
information in a file in /tmp/ (including variable content etc.) to
make debugging easier.
This is done by adding the apparmor.fail module which contains a custom
exception handler (using cgitb, except for AppArmorException).
Also change all python aa-* tools to use the new exception handler.
Note: aa-audit did show backtraces only if the --trace option was given.
This is superfluous with the improved exception handling, therefore this
patch removes the --trace option. (The other aa-* tools never had this
option.)
If you want to test the behaviour of the new exception handler, you can
use this script:
#!/usr/bin/python
from apparmor.common import AppArmorException, AppArmorBug
from apparmor.fail import enable_aa_exception_handler
enable_aa_exception_handler()
# choose one ;-)
raise AppArmorException('Harmless example failure')
#raise AppArmorBug('b\xe4d bug!')
#raise Exception('something is broken!')
Acked-by: Seth Arnold <seth.arnold@canonical.com>
As shown in parser/tst/simple_tests/profile/flags/flags_ok_whitespace.sd,
the parser is quite tolerant to additional or missing whitespace around
flags=, while the tools are more strict.
This patch updates the RE_PROFILE_START regex to follow this tolerance.
Acked-by: Steve Beattie <steve@nxnw.org>.
The only difference between PROFILE_MODE_RE and PROFILE_MODE_NT_RE
was that the latter one additionally allowed 'x', which looks wrong.
(Standalone 'x' is ok for deny rules, but those are handled by
PROFILE_MODE_DENY_RE.)
This patch completely drops PROFILE_MODE_NT_RE and the related code in
validate_profile_mode().
Also wrap the two remaining regexes in '^(...)+$' instead of doing it
inside validate_profile_mode(). This makes the code more readable and
also results in a 2% performance improvement when parsing profiles.
Acked-by: Steve Beattie <steve@nxnw.org> for trunk and 2.9.
Add the missing "pux" to PROFILE_MODE_RE and PROFILE_MODE_NT_RE.
Also move those regexes and PROFILE_MODE_DENY_RE directly above
validate_profile_mode() which is the only user.
Acked-by: Steve Beattie <steve@nxnw.org> for trunk and 2.9
Parsing of boolean assignments failed with
TypeError: '_sre.SRE_Match' object is not subscriptable
because of a missing ".groups()"
Acked-by: Steve Beattie <steve@nxnw.org> for trunk and 2.9
Errors include typos ("DESCRIPT__ON"), missing value after #=EXRESULT
and #=EXRESULT=PASS (= instead of space).
Acked-by: Steve Beattie <steve@nxnw.org> for trunk and 2.9
Bug: https://bugs.launchpad.net/apparmor/+bug/1470985
The ptrace regression test fails to compile on the arm64 platform,
because it uses PTRACE_GETREGS and not the newer PTRACE_GETREGSET
interface for getting access to arch-specific register information[0].
However, fixing it is complicated by the fact that the struct name
for for the general purpose registers is not named consistently
across architectures. This patch attempts to address those issues,
and compiles at least on i386, amd64, arm64, arm (armhf), ppc64,
and ppc64el. The test is verified to continue to function correctly
on i386 and amd64.
[0] https://sourceware.org/ml/archer/2010-q3/msg00193.html
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
RlimitRule accidently used 'ms' (milliseconds) as default unit for
rttime rules, but rttime without unit means 'us' (microseconds). This
patch fixes this.
Also add some tests with 'us' as unit, and two more to cover terribly
invalid corner cases (and to improve test coverage by 2 lines ;-)
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Change minitools tests to use AATest and work inside a tmpdir.
This results in lots of changes ('./profiles' -> self.profile_dir,
local_profilename -> self.local_profilename etc.) and also moves some
code from the global area to AASetup().
Also drop the no longer needed clean_profile_dir() and add linebreaks
in assert* calls with a long error message specified.
Acked-by: Steve Beattie <steve@nxnw.org>
It's allowed to only specify a TYPE without specifying a DOMAIN.
Also add a missing "]" for QUALIFIERS.
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The current rule simplification algorithm has issues that need to be
addressed in a rewrite, but it is still often a win, especially for
larger profiles.
However doing rule simplification as a single pass limits what it can
do. We default to right simplification first because this has historically
shown the most benefits. For two reasons
1. It allowed better grouping of the split out accept nodes that we
used to do (changed in previous patches)
2. because trailing regexes like
/foo/**,
/foo/**.txt,
can be combined and they are the largest source of node set
explosion.
However the move to unique node sets, eliminates 1, and forces 2 to
work within only the single unique permission set on the right side
factoring pass, but it still incures the penalty of walking the whole
tree looking for potential nodes to factor.
Moving tree simplification into the construction phases gets rid of
the need for the right side factoring pass to walk other node sets
that will never combine, and since we are doing simplification we can
do it before the cat and permission nodes are added reducing the
set of nodes to look at by another two.
We do loose the ability to combine nodes from different sets during
the left factoring pass, but experimentation shows that doing
simplification only within the unique permission sets achieve most of
the factoring that a single global pass would achieve.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Currently rules are added to the expression tree in order, and then
tree simplification and factoring is done. This forces simplification
to "search" through the tree to find rules with the same permissions
during right factoring, which dependent on ordering of factoring may
not be able to group all rules of the same permissions.
Instead of having tree factoring do the work to regroup rules with the
same permissions, pregroup them as part of the expr tree construction.
And only build the full tree when the dfa is constructed.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
accept nodes per perm bit where done from the very begining in a
false belief that they would help produce minimized dfas because
a nfa states could share partial overlapping permissions.
In reality they make tree factoring harder, reduce in longer nfa
state sets during dfa construction and do not result in a minimized
dfa.
Moving to unique permission sets, allows us to minimize the number
of nodes sets, and helps reduce recreating each set type multiple
times during the dfa construction.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
parser_regex.c includes libapparmor_re/aare_rules.h and thus it should
depend on it in the Makefile.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
LSMs, such as AppArmor, aren't consulted when a program calls access(2).
This can result in access(2) returning 0 but a subsequent open(2)
failing.
The aa-status utility was doing the access() -> open() sequence and we
became aware of a large number of tracebacks due to open() failing for
lack of permissions. This patch catches any IOError exceptions thrown by
open(). It continues to print the same error message as before when
access() failed but also prints that error message when AppArmor blocks
the open of the apparmorfs profiles file.
https://launchpad.net/bugs/1466768
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
The function is basically a wrapper around a regex, so regex.py is a
much better home.
While on it, rename the regex to RE_INCLUDE, change it to named matches,
use RE_EOL to handle comments and compile it outside the function, which
should result in a (small) performance improvement.
Also rewrite re_match_include(), let it check for empty include
filenames ("#include <>") and let it raise AppArmorException in that
case.
Finally, adjust code calling it to the new location, and add some tests
for re_match_include()
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
profile_storage() returns an empty, properly initialized profile.
It doesn't explicitly init all keys (yet) and will be extended over
time, with the final goal to get rid of hasher().
Also change various places in aa.py to use it (instead of an empty
hasher or sub-hasher), and remove various "init rule class (if not done
yet)" cases.
This also avoids a crash in aa-cleanprof remove_duplicate_rules().
Hats weren't properly initialized in aa.py parse_profile_data()
(especially rule classes were missing), which caused a crash because
hasher doesn't support the delete_duplicates() method.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
Change hat declarations ("^hat,") are no longer supported (see previous
patch for details). Therefore remove support for writing them.
This also means to completely remove the 'declared' flag, which was only
needed for hat declarations, and was (after the previous patch) always
set to False.
Also add a hat to the cleanprof_test.{in,out} test profile to make sure
aa-cleanprof doesn't break hats, and a hat declaration with the same
name to make sure it gets removed and doesn't break the "real" hat.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
Hat declarations ("^hat,") were added in 2.3 for declaring external
hats, but in the meantime aren't supported by the parser anymore (tested
with 2.9.2 parser).
Additionally, if a profile contains both a hat declaration and the hat
("^hat { ...}"), the hat declaration can overwrite the content of the
hat on a "last one wins" base.
This is caused by setting 'declared' to True, which means write_piece()
will only write the "^hat," line, but not the "^hat { ... }" block.
Therefore no longer set 'declared' to True, print a warning that hat
declarations are no longer supported, and ignore the rule. This also
means that running aa-cleanprof can make the profile valid again :-)
Also no longer change 'hat' when hitting a profile declaration, which
also looks wrong.
Note: This change removes the only usage of 'declared'. A follow-up
patch (trunk only) will completely remove the 'declared' handling.
Reproducer profile (run aa-cleanprof on it):
(will crash in remove_duplicate_rules() 80% of the time - if so, try
multiple times. One of the next patches will fix that. Or just try 2.9,
which doesn't have the crash in remove_duplicate_rules().)
/usr/bin/true {
^FOO {
capability setgid,
}
# deletes the content of ^FOO when saving the profile! (last one wins)
# additionally, the parser says this is invalid syntax
^FOO,
}
See also the "Hat declarations" thread on the ML,
https://lists.ubuntu.com/archives/apparmor/2015-June/008107.html
Acked-by: Kshitij Gupta <kgupta8592@gmail.com> for both 2.9 and trunk.
Change aa.py to use RlimitRule and RlimitRuleset instead of a sub-hasher
to store and write rlimit rules. In detail:
- drop all rlimit rule parsing from parse_profile_data() and
serialize_profile_from_old_profile() - instead, just call
RlimitRule.parse()
- change write_rlimits() to use RlimitRuleset
- add removal of superfluous/duplicate change_profile rules (the old
code didn't do this)
- update the comment about aa[profile][hat] usage - rlimit and
change_profile are no longer dicts.
Also cleanup RE_PROFILE_RLIMIT in regex.py - the parenthesis around
'<=' are no longer needed.
Note: This patch is quite small because aa-logprof doesn't ask for
rlimit rules.
I tested all changes manually with aa-cleanprof and aa-logprof (adding
some file rules, rlimit rules kept unchanged)
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
check-logprof in profiles/Makefile needs the local/* files.
Add a dependency to make sure they are generated.
Acked-by: Kshitij Gupta <kgupta8592@gmail.com>
Only use the special %exception directive for functions that return a
negative int and set errno upon error.
This prevents, for example, _aa_is_blacklisted() from raising an
exception when it returns -1. This is important because it doesn't set
errno so an exception based on the value of errno would be
unpredictable.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
When is_blacklisted() was internal to the parser, it would print an
error message when encountering some file names. If the path parameter
was non-null, the error message would include the file path instead of
the file name.
Now that the function has been moved to libapparmor, callers are
expected to print the appropriate error message if _aa_is_blacklisted()
returns -1. Since the error message printing no longer occurs inside of
_aa_is_blacklisted(), the path parameter can be removed.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Ignore README files when performing an operation on a list of files.
This matches the behavior of the is_skipped_file() function in aa.py.
The hope is that is_skippable_file() can reuse _aa_is_blacklisted().
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
It looks odd to access the first character of a string before checking
to see if the string's length is zero. This is actually fine, in
practice, since strlen() looks at the first character of the string for
the presence of '\0' which means this is entirely a cosmetic change.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Prepend the function prototypes with extern to match the style of the
existing prototypes.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
The errno values libapparmor's aa_policy_cache_new() uses to indicate
when the cache directory does not exist and when an existing, invalid
cache already exists needed to be separated out. They were both ENOENT
but now the latter situation uses EEXIST.
libapparmor also needed to be updated to not print an error message to
the syslog from aa_policy_cache_new() when the max_caches parameter is
0, indicating that a new cache should not be created, and the cache
directory does not exist. This is an error situation but a debug message
is more appropriate.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Create a section 3 man page for the aa_policy_cache family of functions.
Additionally, update the in-code descriptions to match the descriptions
in the man page.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Create a section 3 man page for the aa_kernel_interface family of
functions. Additionally, update the in-code descriptions to match the
descriptions in the man page.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Create a section 3 man page for the aa_features family of functions.
Additionally, update the in-code descriptions to match the descriptions
in the man page.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johanse@canonical.com>
The aa_policy_cache_new() and aa_policy_cache_remove() functions are
changed to accept a dirfd parameter.
The cache dirfd (by default, /etc/apparmor.d/cache) is opened earlier in
aa_policy_cache_new(). Previously, the directory wasn't accessed until
later in the following call chain:
aa_policy_cache_new() -> init_cache_features() -> create_cache()
Because of this change, the logic to create the cache dir must be moved
from create_cache() to aa_policy_cache_new().
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Instead of only accepting a path in the aa_features API, accept a
directory file descriptor and a path like then openat() family of
syscalls. This type of interface is better since it can operate exactly
like a path-only interface, by passing AT_FDCWD or -1 as the dirfd.
However, using the dirfd/path combination, it can eliminate string
allocations needed to open files in subdirectories along with the
even more important benefits mentioned in the open(2) man page.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Make the function prototype for reading a features directory the same
as the function prototype for reading a features file.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Two different implementations were in use for reading features files.
One for reading a single file and another for reading a single file
after walking a directory. This patch creates a single function that is
used in both cases.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The _aa_dirat_for_each() function used the DIR * type for its first
parameter. It then switched back and forth between the directory file
descriptors, retrieved with dirfd(), and directory streams, retrieved
with fdopendir(), when making syscalls and calling the call back
function.
This patch greatly simplifies the function by simply using directory
file descriptors. No functionality is lost since callers can still
easily use the function after calling dirfd() to retrieve the underlying
file descriptor.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
The most common case when creating an aa_kernel_interface object will be
to do so while using the current kernel's feature set for the
kernel_features parameter. Rather than have callers instantiate their
own aa_features object in this situation, aa_kernel_interface_new()
should do it for them if they specify NULL for the kernel_features
parameter.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
The most common case when creating an aa_policy_cache object will be to
do so while using the current kernel's feature set for the
kernel_features parameter. Rather than have callers instantiate their
own aa_features object in this situation, aa_policy_cache_new() should
do it for them if they specify NULL for the kernel_features parameter.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
The aa_features object that is passed to aa_policy_cache_new() does not
have to represent the currently running kernel. It may represent a
different kernel, such as a kernel that was just installed, that is not
currently running.
This patch adjusts the function comments to remove mentions of
"... the currently running kernel".
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
