Fix leaked memory if calloc() fails. Found by cppcheck.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
Seth Arnold noticed an ugly string.clear(); convert_entry(string,
NULL) pattern occurred frequently following the conversion to using
std::string. This patch replaces that by using a static pointer to
a constant string matching pattern, and also converts other uses of
that pattern. It also adds a function wrapper that will clear the
passed buffer before calling convert_entry().
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
As noted by Seth Arnold, in expand_by_alternations() if our set
variable has at least one value, then we're going to rewrite the entry,
so rather than sprinkle the free()s near where the reallocation occurs,
use one free() once we're guaranteed to need to do so.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
As suggested by Seth Arnold, we can use string::find_last_not_of()
instead of using C++'s hideous reverse iterators.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
As noted by Seth Arnold, e_buffer_overflow is no longer set in
convert_aaregex_to_pcre(), so remove it and the sole check for it.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch converts a stack allocated buffer into an std::ostringstream
object. The stringstream interface for specifying the equivalent of
a printf %02x conversion is a bit of an awkward construction, however.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
Based on feedback from Seth Arnold, the convert_aaregex_to_pcre()'s
first argument is const char *, and thus the unit test macros don't need
to pass a copy of the input string to it, as it's guaranteed to be
unmodified by the function.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch eliminates the bison warning about "%name-prefix =" being
deprecated.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch includes the errno in the log messages generated by two
different failed aa_change_hat() calls and the failure to open
/dev/urandom to get the random token, to further ease failure
diagnosis.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch removes unnecessary back out aa_change_hat() calls that occur
if the prior call to aa_change_hat() call failed. It used to be case
that an aa_change_hat() call that failed would result in the task being
placed in a profile with no permissions except the ability to
aa_change_hat() back out, but this behavior has been removed from
apparmor for many, many years now.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch adds code that checks the resulting hat that apache gets
placed into, and verifies that if the apache configuration specified
that an AAHatName or AADefaultHatName should have been the resulting
hat. If it wasn't, emit a warning message to the apache log, as this
likely indicates a mismatch between the apache configuration and its
apparmor policy (i.e. why define AAHatName if you aren't going to
create the corresponding hat in the apparmor policy?)
Note for AADefaultHatName, a message is not logged if a defined
AAHatName would also apply or if there is a hat defined for the uri,
as each of those come first in the order of attempted hats.
Also note that the way the hat name is manually calculated will break
for nested profiles and stacking. It should be fine for all current
deployments as we don't allow nesting beyond the first subprofile level
in policy yet. And stacking will likely only be used between namespaces
where aa_getcon() will not report parent namespace info. However, when
libapparmor adds functionality to query the hatname, the code that
computes it here should be replaced by a call to that library function.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch converts the request entry point from using multiple (if
necessary) aa_change_hat() calls into a single aa_change_hatv() call,
simplifying the code a bit, requiring fewer round trips between
mod_apparmor and the kernel for each request, as well as providing more
information when the apache profile is in complain mode.
Patch history:
v1: initial version
v2: - the server config (scfg) code accidentally re-added the
directory config (dcfg) hat to the vector of hats, fix that
- actually add the DEFAULT_URI hat to the vector of hats, instead
of only logging that that is happening.
- pass errno to ap_log_rerror() if aa_change_hatv() call fails.
- don't call aa_change_hat again if aa_change_hatv() call fails,
as this is no longer necessary.
v3: - Based on feedback from jjohansen, convert exit point
aa_change_hat() call to aa_change_hatv(), in order to work
around aa_change_hat() bug addressed in trunk rev 2329,
which causes the exiting aa_change_hat() call to fail and
results in the apache process being killed by the kernel.
When it's no longer likely that mod_apparmor could run into
a system libapparmor that still contains this bug, this can
be converted back.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
Bug: https://bugs.launchpad.net/ubuntu/+source/apparmor/+bug/1207424
This patch makes the default value for AADefaultHatName be the
server/vhost name, which can be specified in apache via the ServerName
configuration declaration. It can be overridden by setting
AADefaultHatName directly. Thus, with this patch applied, the order of
attempted hats will be:
1. try to aa_change_hat(2) into a matching AAHatName hat if it exists
and applies, otherwise
2. try to aa_change_hat(2) into the URI itself, otherwise
3. try to aa_change_hat(2) into the value of ServerName, unless
AADefaultHatName has been explicitly set for this server/vhost, in
which case that value will be used, otherwise
4. try to aa_change_hat(2) into the DEFAULT_URI hat, if it exists,
otherwise
5. fall back to the global Apache policy
This should eliminate the need for most admins to define both
ServerName and AADefaultHatName, unless there's a specific need for
the values to deviate.
Man page documentation is updated as well, though probably more
wordsmithing is needed there for clarity.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
When defining an AADefaultHatName entry, it was being stored in the
passed mconfig location, which is not the module specific server
config, but instead the top level (i.e. no path defined) default
directory/location config. This would be superceded by a more specific
directory config if it applied to the request. Thus, if an AAHatName was
defined that applied, but the named hat was not defined in the apparmor
policy, mod_apparmor would not attempt to fall back to the defined
AADefaultHatName, but instead jump directly to trying the DEFAULT_URI
hat.
This patch fixes it by storing the defined AADefaultHatName correctly in
the module specific storage in the related server data structure. It
also adds a bit of developer debugging statements.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
Bug: https://launchpad.net/bugs/1207424
This patch adds the name of the hat to the log message about the
initial aa_change_hat call, just to be explicit about what's happening
when debugging and changes the formatting slightly of the exiting
change_hat log message.
Patch history:
v1: initial version
v2: tweak output of exit trace message
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
mod_apparmor never got converted to use the renamed aa_change_hat()
call (there's a compatibility macro in sys/apparmor.h); this patch does
that as well as converting the type of the magic_token to long from int.
(This patch is somewhat mooted by a later patch in the series to
convert to using aa_change_hatv(), but would be a safer candidate
for e.g. the 2.8 branch.)
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch converts the debug_dump_uri() function to use the trace
loglevels and enable it all the time, rather than just when DEBUG is
defined at compile time.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
Apache 2.4 added addition logging levels. This patch converts some of
the log messages that are more intended for mod_apparmor development
and debugging than for sysadmins configuring mod_apparmor to use trace1
(APLOG_TRACE1) level instead. Since apache 2.2. does not contain this
level (or define), we define it back to APLOG_DEBUG.
Patch history:
v1: initial version
v2: mark a couple of additional log messages as trace1 level
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
The apache2 mod_apparmor module was failing to log debugging messages
when the apache loglevel was set to debug or lower (i.e. traceN). This
patch fixes it by using ap_log_rerror() (for request specific messages,
with the request passed for context) and ap_log_error() (more general
messages outside of a request context).
Also, the APLOG_USE_MODULE macro is called, to mark the log messages as
belonging to the apparmor module, so that the apache 2.4 feature of
enabling debug logging for just the apparmor module will work, with an
apache configuration entry like:
LogLevel apparmor:debug
See
http://ci.apache.org/projects/httpd/trunk/doxygen/group__APACHE__CORE__LOG.html
for specific about the ap_log_*error() and APLOG_USE_MODULE functions
and macros, and
http://httpd.apache.org/docs/2.4/mod/core.html.en#loglevel
for the bits about module specific logging.
Patch history:
v1: initial version
v2: - revert to using ap_log_error with (the 2.4 specific)
ap_server_conf outside of a request specific context, as the
pool specific ap_log_perror messages weren't being reported.
- add compatibility workaround for apache 2.2
v3: keep commented out merge function's log call consistent with the
others
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
This patch fixes the format string for the magic token in aa_change_hat
to match the type of the magic token (long). Without this, on 64
bit platforms, only the bottom 32 bits of the token would be used.
aa_change_hatv() has the correct format string, so an aa_change_hatv()
call followed by an exiting aa_change_hat() call would result in the
latter having a different token, which would cause the process to be
killed by apparmor.
(Hat tip to John Johansen for spotting the actual bug.)
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
(collected in the openSUSE package over the last months)
- add abstractions/samba to usr.sbin.winbindd profile
(and cleanup things that are included in the abstraction - the cleanup
part is not in the openSUSE package)
- add capabilities ipc_lock and setuid to usr.sbin.winbindd profile
(bnc#851131)
- updates for samba 4.x and kerberos (bnc#846586#c12 and #c15,
bnc#845867, bnc#846054)
- drop always-outdated "Last Modified" comment
References: see the bnc# above (they are bug numbers at
bugzilla.novell.com)
Acked-by: John Johansen <john.johansen@canonical.com>
This patch eliminates the complaints from running:
pep8 --ignore=E501 aa-easyprof vim/
(E501 is 'line too long', which I'm not too chuffed about.)
Mostly, it's a lot of whitespace touchups, with a few conversions from
'==' to 'is'.
Commit includes applied feedback from cboltz.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
Current builds include many warnings when building translations message
files like so:
msgfmt -c -o ja.mo ja.po
ja.po:5: warning: header field 'Language' missing in header
According to what I read in the entry for Language in
http://www.gnu.org/software/gettext/manual/gettext.html#Header-Entry
the language entry should be (in our case) the same as the file name
minus the .po suffix. This patch adds the language field for those
po files that were missing it.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
This patch adds several assorted language tests, to exercise various
parts of the parser that were not being covered by the language tests
previously. Areas lacking were found using the coverage compilation
option; coverage from the language tests is still incomplete.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
This patch updates the Report-Msgid-Bugs-To: to point to the apparmor
list instead of the old Novell forge address. It also makes the
Project-Id-Version: field consistent.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
The rlimits syntax checking support in apparmor.vim was broken in
various unhelpful ways:
- lacked support for the 'infinity' keyword (aka RLIM_INFINITY)
- lacked support for the 'ofile' rlimit, an alias for the nofile
rlimit
- lacked support for the 'cpu' rlimit (aka RLIMIT_CPU)
- incorrect syntax for nofile|nproc|rtprio rlimits (didn't include
required '<=' between the limit name and value)
- incorrect syntax for specifying optional SI units for size based
rlimits (e.g. 'MB' is required, but syntax only allowed incorrect
'M'; that said, one could argue the parser is overly strict here,
and the pattern should be '[KMG]B?')
(See the setrelimit(2) man page for more details on the specifics of the
rlimit definitions.)
This patch fixes the above issues.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
The parser was lacking language tests for rlimits. This test adds
several, one for each rlimit type.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
As noted by Seth Arnold, there's now only one failure case in the
function and thus does not warrant a goto target (especially since
there's no cleanup to occur).
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian Boltz <apparmor@cboltz.de>
Change uservars.inc subdomain variable to use the in-tree parser by
default.
Also, clean up some commented out subdomain values that don't look to be
in use any longer and add one commented out value pointing to the system
parser.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Subtle change to remove the "..." between the test description and
result and also to single-space the output. This brings the output in
line with what minimize.sh outputs, which is the test that runs just
before equality.sh.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
The regression test README examples use sh when showing how to run
individual tests but bash is needed, instead.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
This test ensures that the proper DFA minimization occurs when a
permissive D-Bus abstraction #include's the corresponding strict
abstraction.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-By: Christian Boltz <apparmor@cboltz.de>
Create a new strict accessibility bus abstraction.
The strict abstraction only allows for calling the Hello, AddMatch,
RemoveMatch, GetNameOwner, NameHasOwner, and StartServiceByName methods
that are exported by the D-Bus daemon.
The permissive abstraction reuses the strict abstraction and then allows
all communications on the accessibility bus.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Move some of the file rules from the existing permissive session bus
abstraction into a new strict session bus abstraction. Leave the
dbus-launch rule in the permissive profile since not all applications
will need it.
The strict abstraction only allows for calling the Hello, AddMatch,
RemoveMatch, GetNameOwner, NameHasOwner, and StartServiceByName methods
that are exported by the D-Bus daemon.
The permissive abstraction reuses the strict abstraction and then allows
all communications on the session bus.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-By: Jamie Strandboge <jamie@canonical.com>
Move the file rule from the existing permissive system bus abstraction
into a new strict system bus abstraction.
The strict abstraction only allows for calling the Hello, AddMatch,
RemoveMatch, GetNameOwner, NameHasOwner, and StartServiceByName methods
that are exported by the D-Bus daemon.
The permissive abstraction reuses the strict abstraction and then allows
all communications on the system bus.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
std::max in C++ requires that both arguments be the same type. The
previous fix added std::max comparisons between unsigned long numeric
constants and size_t, this fix casts the numeric constants to size_t.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
Tests should be added for other rule types but this is a good start at
testing DFA minimization.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-By: Christian Boltz <apparmor@cboltz.de>
Make the accept information dump output be in hexidecimal like the
other dumps so its easier to reference between them.
Signed-off-by: John Johansen <john.johansen@canonical.com>
This was part of the original minimization patch, but got dropped when
applying to bzr. Again bzr status didn't show any files out of place
nor did the patching fail :(
Signed-off-by: John Johansen <john.johansen@canonical.com>
This diff is part of the diffencode patch but was dropped when it was
applied to bzr. I have no idea why and status showed a clean tree.
Signed-off-by: John Johansen <john.johansen@canonical.com>
So DFA minimization has a bug and feature that keeps it from minimizing
some dfas completely. This feature/bug did not result in incorrect dfas,
it just fails to result in full minimization.
The same mappings comparison is wrong. Or more correctly it is right when
transitions are not remapped to minimization partitions, but it may be
wrong when states are remapped. This means it will cause excess
partitioning (not removing all the states it should).
The trans hashing does a "guess" at partition splitting as a performance
enhancement. Basically it leverages the information that states that have
different transitions or transitions on different characters are not the
same. However this isn't always the case, because minimization can cause
some of those transitions to be altered. In previous testing this was
always a win, with only a few extra states being added some times. However
this changes with when the same mappings are fixed, as the hashing that was
done was based on the same flawed mapping as the broken same mappings.
If the same mappings are fixed and the hashing is not removed then there
is little to no change. However with both changes applied some dfas see
significant improvements. These improvements often result in performance
improvements despite minimization doing more work, because it means less
work to be done in the chfa comb compression
eg. test case that raised the issue (thanks tyler)
/t { mount fstype=ext2, mount, }
used to be minimized to
{1} <== (allow/deny/audit/quiet)
{6} (0x 2/0/0/0)
{1} -> {2}: 0x7
{2} -> {3}: 0x0
{2} -> {2}: []
{3} -> {4}: 0x0
{3} -> {3}: []
{4} -> {6}: 0x0
{4} -> {7}: 0x65 e
{4} -> {5}: []
{5} -> {6}: 0x0
{5} -> {5}: []
{6} (0x 2/0/0/0) -> {6}: [^\0x0]
{7} -> {6}: 0x0
{7} -> {8}: 0x78 x
{7} -> {5}: []
{8} -> {6}: 0x0
{8} -> {5}: 0x74 t
{8} -> {5}: []
with the patch it is now properly minimized to
{1} <== (allow/deny/audit/quiet)
{6} (0x 2/0/0/0)
{1} -> {2}: 0x7
{2} -> {3}: 0x0
{2} -> {2}: []
{3} -> {4}: 0x0
{3} -> {3}: []
{4} -> {6}: 0x0
{4} -> {4}: []
{6} (0x 2/0/0/0) -> {6}: [^\0x0]
The evince profile set sees some significant improvements picking a couple
example from its "minimized" dfas (it has 12) we see a reduction from 9720
states to 6232 states, and 6537 states to 3653 states. All told seeing the
performance/profile size going from
2.8 parser: 4.607s 1007267 bytes
dev head: 3.48s 1007267 bytes
min fix: 2.68s 549603 bytes
of course evince is an extreme example so a few more
firefox
2.066s 404549 bytes
to
1.336s 250585 bytes
cupsd
0.365s 90834 bytes
to
0.293s 58855 bytes
dnsmasq
0.118s 35689 bytes
to
0.112s 27992 bytes
smbd
0.187s 40897 bytes
to
0.162s 33665 bytes
weather applet profile from ubuntu touch
0.618s 105673 bytes
to
0.432s 89300 bytes
I have not seen a case where the parser regresses on performance but it is
possible. This patch will not cause a regression on generated policy size,
at worst it will result in policy that is the same size
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Differential state compression encodes a state's transitions as the
difference between the state and its default state (the state it is
relative too).
This reduces the number of transitions that need to be stored in the
transition table, hence reducing the size of the dfa. There is a
trade off in that a single input character may have to traverse more
than one state. This is somewhat offset by reduced table sizes providing
better locality and caching properties.
With carefully encoding we can still make constant match time guarentees.
This patch guarentees that a state that is differentially encoded will do at
most 3m state traversal to match an input of length m (as opposed to a
non-differentially compressed dfa doing exactly m state traversals).
In practice the actually number of extra traversals is less than this becaus
we selectively choose which states are differentially encoded.
In addition to reducing the size of the dfa by reducing the number of
transitions that have to be stored. Differential encoding reduces the
number of transitions that need to be considered by comb compression,
which can result in tighter packing, due to a reduction in sparseness, and
also reduces the time spent in comb compression which currently uses an
O(n^2) algorithm.
Differential encoding will always result in a DFA that is smaller or equal
in size to the encoded DFA, and will usually improve compilation times,
with the performance improvements increasing as the DFA gets larger.
Eg. Given a example DFA that created 8991 states after minimization.
* If only comb compression (current default) is used
52057 transitions are packed into a table of 69591 entries. Achieving an
efficiency of about 75% (an average of about 7.74 table entries per state).
With a resulting compressed dfa16 size of 404238 bytes and a run time for
the dfa compilation of
real 0m9.037s
user 0m8.893s
sys 0m0.036s
* If differential encoding + comb compression is used, 8292 of the 8991
states are differentially encoded, with 31557 trans removed. Resulting in
20500 transitions are packed into a table of 20675 entries. Acheiving an
efficiency of about 99.2% (an average of about 2.3 table entries per state
With a resulting compressed dfa16 size of 207874 bytes (about 48.6%
reduction) and a run time for the dfa compilation of
real 0m5.416s (about 40% faster)
user 0m5.280s
sys 0m0.040s
Repeating with a larger DFA that has 17033 states after minimization.
* If only comb compression (current default) is used
102992 transitions are packed into a table of 137987 entries. Achieving
an efficiency of about 75% (an average of about 8.10 entries per state).
With a resultant compressed dfa16 size of 790410 bytes and a run time for d
compilation of
real 0m28.153s
user 0m27.634s
sys 0m0.120s
* with differential encoding
39374 transition are packed into a table of 39594 entries. Achieving an
efficiency of about 99.4% (an average of about 2.32 entries per state).
With a resultant compressed dfa16 size of 396838 bytes (about 50% reduction
and a run time for dfa compilation of
real 0m11.804s (about 58% faster)
user 0m11.657s
sys 0m0.084s
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
By raising an error for being unable to find libapparmor any time
a make command is run, we break things like make clean and other
targets that don't strictly depend on libapparmor existing (note that
Tyler's implementation for the parser did not do this). This patch
fixes this for the regression tests, mod_apparmor and pam_apparmor
by making a separate libapparmor_check target that looks to see if
an error message should be generated.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
