cux and CUx are valid exec permissions, so they should be accepted
by validate_profile_mode() ;-)
Acked-by: John Johansen <john.johansen@canonical.com> for trunk and 2.9
Some of the include files added to simple_tests recently don't live in
one of the main include directories (includes/, includes-preamble/ or
include_tests/) which lets test-parser-simple-tests.py fail because
those files don't contain EXRESULT.
Instead of adding more exceptions to test-parser-simple-tests.py, this
patch adds DESCRIPTION and EXRESULT to those include files.
Acked-by: John Johansen <john.johansen@canonical.com>
- allow only a specific set of time units
- optionally allow whitespace between rlimit value and unit
- move check for invalid time units to time_to_int()
Also update the tests:
- add several tests with whitespace between value and unit
- change a test that used the (now invalid) "1m" to "1min"
- change the time_to_int() tests to use 'us' as default unit, and add
a test with 'seconds' as default unit
Acked-by: Steve Beattie <steve@nxnw.org>
currently the parser supports ambiguous units like m for time,
which could mean minutes or milliseconds. Fix this and refactor the
time parsing into a single routine.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Signed-off-by: Steve Beattie <steve@nxnw.org>
When @{profile_name} is used within a rule matching expression any
aare expressions should be matched literally and not be interpreted as
aare.
That is
profile /foo/** { }
needs /foo/** to expand into a regular expression for its attachment
but, /foo/** is also the profiles literal name. And when trying to
match @{profile_name} in a rule, eg.
ptrace @{profile_name},
the variable needs to be expaned to
ptrace /foo/\*\*,
not
ptrace /foo/**,
that is currently happening.
BugLink: http://bugs.launchpad.net/bugs/1317555
equality tests by
Tyler Hicks <tyhicks@canonical.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
The @{profile_name} is incorrectly expanded as a fully qualified path
including its namespace if one was specified in the profile declaration.
ie.
profile :ns://a {
ptrace @{profile_name},
# expands to
# ptrace :ns://a,
}
This is wrong however because within a profile if a rule refers
to a namespace it will be wrt a sub-namespace. That is in the above
example the ptrace rule is refering to a profile in a subnamespace
"ns".
Or from the current profile declaration scope
:ns//ns://a
Instead @{profile_name} should expand into the hname (hierarchical name),
which is the profile hierarchy specification within the namespace the
profile is part of.
In this case
a
or for a child profile case
profile :ns://a {
profile b {
ptrace @{profile_name},
}
}
the hname expansion would be
a//b
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
allow
@{FOO}=bar
/foo@{FOO} { }
to be expanded into
/foobar { }
and
@{FOO}=bar baz
/foo@{FOO} { }
to be expanded into
/foo{bar,baz} { }
which is used as a regular expression for attachment purposes
Further allow variable expansion in attachment specifications
profile foo /foo@{FOO} { }
profile name (if begun with profile keyword) and attachments to begin
with a variable
profile @{FOO} { }
profile /foo @{FOO} { }
profile @{FOO} @{BAR} {}
hats
^@{FOO}
hat @{FOO}
and for subprofiles as well
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
aa-logprof raises an exception if
- an include file contains a hat
- that file is included in a profile and
- aa-logprof hits an audit log entry for this profile
Reproducer ("works" on 2.9 and trunk):
python3 aa-logprof -f <(echo 'Jun 19 11:50:36 piorun kernel: [4474496.458789] audit: type=1400 audit(1434707436.696:153): apparmor="DENIED" operation="open" profile="/usr/sbin/apache2" name="/etc/gai.conf" pid=2910 comm="apache2" requested_mask="r" denied_mask="r" fsuid=0 ouid=0') -d ../profiles/apparmor.d/
This happens because profiles/apparmor.d/apache2.d/phpsysinfo was
already read when pre-loading the include files.
This patch changes aa.py parse_profile_data() to only raise the
exception if it is not handling includes currently.
Acked-by: Steve Beattie <steve@nxnw.org> for both trunk and 2.9.
Fix the regression that caused using 'include' instead of '#include' for
includes to stop working.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
is_known_rule() ignored directory includes, which resulted in asking for
and adding superfluous rules that are already covered by a file in the
included directory.
This patch looks bigger than it is because it moves quite some lines
into the "else:" branch. Everything inside the "else:" just got an
additional whitespace level.
References: https://bugs.launchpad.net/apparmor/+bug/1471425
(however, trunk didn't crash, it "just" ignored directory includes)
Acked-by: Steve Beattie <steve@nxnw.org>
is_known_rule() in aa.py checked only direct includes, but not includes
in the included files. As a result, aa-logprof asked about things that
are already covered by an indirect include.
For example, the dovecot/auth profile includes abstractions/nameservice,
and abstractions/nameservice includes abstractions/nis, which contains
"capability net_bind_service,".
Nevertheless, aa-logprof asked to add capability net_bind_service.
Reproducer: (asks for net_bind_service without this patch, should not
ask for anything after applying the patch):
python3 aa-logprof -d ../profiles/apparmor.d/ -f <(echo 'type=AVC msg=audit(1415403814.628:662): apparmor="ALLOWED" operation="capable" profile="/usr/lib/dovecot/auth" pid=15454 comm="auth" capability=13 capname="net_bind_service"')
The patch adds code to check include files included by other include
files. Note that python doesn't allow to change a list while looping
over it, therefore we have to use "while includelist" as workaround.
This fixes a regression for network rules (this patch is based on the
old match_net_include() code). Funnily it "only" fixes capability rule
handling (without the "regression" part) because the old
match_cap_include() didn't do the recursive include handling.
Acked-by: Steve Beattie <steve@nxnw.org>
For some (not yet known) reason, we get file_perm events without
request_mask set, which causes an aa-logprof crash.
Reproducer log entry:
Jun 19 12:00:55 piorun kernel: [4475115.459952] audit: type=1400 audit(1434708055.676:19629): apparmor="ALLOWED" operation="file_perm" profile="/usr/sbin/apache2" pid=3512 comm="apache2" laddr=::ffff:193.0.236.159 lport=80 faddr=::ffff:192.168.103.80 fport=61985 family="inet6" sock_type="stream" protocol=6
This patch changes logparser.py to ignore those events.
References: https://bugs.launchpad.net/apparmor/+bug/1466812/
Acked-by: Steve Beattie <steve@nxnw.org> for trunk and 2.9
According to the parser test profiles (which are the only
"documentation" I found about this), definition of boolean variables
is only allowed outside profiles, not inside them.
parse_profile_data() got it the wrong way round, therefore this patch
fixes the condition and updates the error message.
Acked-by: Steve Beattie <steve@nxnw.org> for both trunk and 2.9.
We need directory listings for #include <directory> in more than one
place, therefore split it off to its own function.
This is a preparation to fix https://bugs.launchpad.net/apparmor/+bug/1471425
Acked-by: Steve Beattie <steve@nxnw.org>
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>
