Fixes two resource leaks. https://scan.coverity.com/projects/apparmor
I don't actually know how to link to the individual reports but the
first one comes from an early return. The second comes from an iterator
potentially being empty.
The current encoding makes every xattr optional and uses this to
propogate the permission from the tail to the individual rule match
points.
This however is wrong. Instead change the encoding so that an xattr
(unless optional) is required to be matched before allowing moving
onto the next xattr match.
The permission is carried on the end on each rule portion file match,
xattr 1, xattr 2, ...
Signed-off-by: John Johansen <john.johansen@canonical.com>
Make sure we can support optional xattrs distinct from optional xattr
values in the encoding.
Currently all xattrs specified are required to be present even
if there value is not specified. However under the old encoding there
was no way to distinguish if the presence of the xattr vs. the
xattr having a null length value.
Fix this so that if we decide to support optional xattrs it is possible
without having to change the abi.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Support profiles that choose to match the presence of an extended
attribute without validating its value. This lets AppArmor target xattrs
with binary data, such as security.ima and security.evm values. For
example, it's now possible to write a profile such as:
profile signed_binaries /** xattrs=(security.ima) {
# ...
}
Both presence and value matches can be used in the same profile. To
match a signed xattr, target both the xattr and the security.ima value:
profile python_script /** xattrs=(
security.evm
security.apparmor="python"
) {
# ...
}
Updated to work using out of band matching instead of separate data
array.
Signed-off-by: Eric Chiang <ericchiang@google.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
xattrs are a byte string that can contain all input characters including
the null character. Allow * ** and ? glob patterns to match the null
character while retaining their apparmor characteristics for '/'.
That is * and ? won't traverse a '/' treating it as a path element.
While ** will match anything.
Signed-off-by: John Johansen <john.johansen@canonical.com>
xattrs can contain NULL characters in their values which means we can
not user regular NULL transitions to separate values. To fix this
use out of band transition instead.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Currently the NULL character is used as an out of band transition
for string/path elements. This works for them as the NULL character
is not valid for this data. However this does not work for binary
data that can contain a NULL character.
So far we have only dealt with fixed length fields of binary data
making the NULL separator either unnecessary.
However binary data like in the xattr match and mount data field are
variable length and can contain NULL characters. To deal with this
add the ability to specify out of band transitions, that can only
be triggered by code not input data.
The out of band transition can be used to separate variable length
data fields just as the NULL transition has been used to separate
variable length strings.
In the compressed hfa out of band transitions are expressed as a
negative offset from the states base. This leaves us room to expand
the character match range in the future if desired and on average
makes the range between the out of band transition and the input
transitions smaller than would be had if the out of band transition
had been stored after the valid input transitions.
Out of band transitions in the dfa will not break old kernels
that don't know about them, but they won't be able to trigger
the out of band transition match. So they should not be used unless
the kernel indicates that it supports them.
It should be noted that this patch only adds support for a single
out of band transition. If multiple out of band transitions are
required. It is trivial to extend.
- Add a tag indicating support in the kernel
- add a oob max range field to the dfa header so the kernel knows
what the max range that needs verifying is.
- extend oob generation fns to generate oob based on value instead
of a fixed -1.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Currently the parser is not correctly setting the dfa flag value
and it hasn't been caught because base policy uses a flag value
of 0.
Signed-off-by: John Johansen <john.johansen@canonical.com>
As a step in preparing for out of band transitions and double walk
transitions rework the backend from using a char index to a class
with an larger range than char.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Update the language description to provide some over arching
principles, such as the policy is declarative.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Christian Boltz <apparmor@cboltz.de>
Acked-by: Steve Beattie <steve.beattie@canonical.com>
Correct the long option used to print the cache directory.
Fixes: e9d9395f91 ("parser: Add option to print the cache directory")
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
When cross compiling apparmor-parser, Makefile will use ar for
creating the static library. However, ar produces libraries on
the build platform. The right ar could be prefixed with the target
platform triples.
Signed-off-by: Xiang Fei Ding <dingxiangfei2009@gmail.com>
Signed-off-by: Steve Beattie <steve.beattie@canonical.com>
Ref: https://github.com/NixOS/nixpkgs/pull/63999
Bug: https://gitlab.com/apparmor/apparmor/issues/41
Instead of setting SFS_MOUNTPOINT in is_apparmor_loaded() (which is
called in most cases) and in is_container_with_internal_policy() (which
covers/fixes the remaining cases), set it globally.
This also fixes a bug in is_container_with_internal_policy() (introduced
in f10e72a14f) where the variable
definition tried to use the no longer existing $MODULE variable and
therefore got a wrong path for $SFS_MOUNTPOINT.
Besides this bug, there's a minor behaviour change / improvement if
securityfs isn't mounted - "file not found" error messages will now
contain the full/correct path ;-)
This change/cleanup is a follow-up of
https://gitlab.com/apparmor/apparmor/merge_requests/363 and some IRC
discussions 2019-04-16.
is_container_with_internal_policy() is called independently of
apparmor_*() in the systemd unit and potentially other consumers of
rc.apparmor.functions. When the unit and rc.apparmor.functions functions
were rewritten, they were written so that SFS_MOUNTPOINT was only set in
is_apparmor_loaded(), but this is only called in apparmor_start(),
remove_profiles(), apparmor_kill(), apparmor_restart(), apparmor_try_restart()
and apparmor_status() and not is_container_with_internal_policy().
While it is clear that is_container_with_internal_policy() is meant to
be called before apparmor_start(), is is unclear why SFS_MOUNTPOINT is
only defined in is_apparmor_loaded(). There are several ways to fix
this:
1. update is_container_with_internal_policy() to call is_apparmor_loaded()
2. identify the callers of is_container_with_internal_policy() and have
them call is_apparmor_loaded()
3. reorganize the code to remove duplicate calls and assignments
4. define SFS_MOUNTPOINT along with SECURITYFS and MODULE, at the top
level
5. also define SFS_MOUNTPOINT in is_container_with_internal_policy()
'1' would result in redundant calls in many common cases since the
systemd unit would call is_apparmor_loaded() both in
is_container_with_internal_policy() and prior to other calls.
'2' would like break consumers of rc.apparmor.funcions, like
Debian/Ubuntu's profile-load.
'3' is perhaps ok, but requires more effort and is regression-prone.
'4' seems the simplest, most correct fix
'5' is what this patch implements, which is as simple as '4' but tries
to maintain the original author's intent of when to set SFS_MOUNTPOINT.
PR: https://gitlab.com/apparmor/apparmor/merge_requests/363
Signed-off-by: Jamie Strandboge <jamie@strandboge.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
The parser currently skips the cache if optimizations are specified
because it can not determine if the cached policy was compiled
with the specified optimization. However this causes cache misses
even if policy is cached with those options, and distros are setting
some optimizations by default.
Instead of skipping reading the cache if optimizations are set, users
can force overwriting the cache if needed, until the parser can
store aditional meta info in the cache.
BugLink: http://bugs.launchpad.net/bugs/1820068
Signed-off-by: John Johansen <john.johansen@canonical.com>
The apparmor.d manpage listed 'to' as an alternative for '->' in link
rules.
However, the parser doesn't accept 'to', none of our examples and tests
include it, and nobody ever complained about it. Therefore I'll call
this a documentation bug ;-) and simply adjust the manpage to only list
'->' as valid syntax.
Add userland support for matching based on extended file attributes. This
leverages DFA based matching already in the kernel:
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=8e51f908https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=73f488cd
Matching is exposed via flags on the profile:
/usr/bin/* xattrs=(user.foo=bar user.bar=foo) {
# ...
}
xattr values are appended to the existing xmatch via a null transition.
$ echo '/usr/bin/* xattrs=(user.foo=foo user.bar=bar) {}' | \
./parser/apparmor_parser -QT -D expr-tree
DFA: Expression Tree
/usr/bin/[^\0000/]([^\0000/])*(\0000bar)?(\0000foo)?< 0x1>
DFA: Expression Tree
(\a|(\n|(\0002|\t)))< 0x4>
Tested manually on a 4.19 kernel via QEMU+KVM.
TODO:
* ~~Add regression tests~~ (EDIT: done)
* ~~EDIT: add support in the tools~~ (EDIT: done)
Questions for reviewers:
* ~~parser/libapparmor: regex construction probably needs cleaning up~~ (EDIT: done)
* ~~parser/parser_regex.c: confused what xmatch length is for~~ (EDIT: done)
/cc @mjg59
PR: https://gitlab.com/apparmor/apparmor/merge_requests/270
Signed-off-by: John Johansen <john.johansen@canonical.com>
I slightly ;-) doubt we'll change the module name.
PR: https://gitlab.com/apparmor/apparmor/merge_requests/354
Signed-off-by: Christian Boltz <apparmor@cboltz.de>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Compiling the parser currently prints a deprecation warning. Remove
throw(int) annotations from function signatures. These aren't required
to catch exceptions. This gets us closer to possibly enabling '-Werror'
in the future.
For example, the following program catches the exception without a
throw(int) annotation:
#include <iostream>
void throw_an_error()
{
throw 3;
return;
}
int main ()
{
try
{
throw_an_error();
}
catch (int e)
{
std::cout << "caught exception " << e << '\n';
}
return 0;
}
This program prints:
$ g++ -o error error.cc
$ ./error
caught exception 3
PR: https://gitlab.com/apparmor/apparmor/merge_requests/356
Signed-off-by: Eric Chiang <ericchiang@google.com>
Signed-off-by: John Johansen <john.johansen@canonical.com>
Note that $PARSER_OPTS has to stay without quotes because it can
sometimes be empty, and would (if quoted) be interpreted as empty
filename by apparmor_parser
Compiling the parser currently prints a deprecation warning. Remove
throw(int) annotations from function signatures. These aren't required
to catch exceptions.
For example, the following program catches the exception without a
throw(int) annotation:
#include <iostream>
void throw_an_error()
{
throw 3;
return;
}
int main ()
{
try
{
throw_an_error();
}
catch (int e)
{
std::cout << "caught exception " << e << '\n';
}
return 0;
}
This program prints:
$ g++ -o error error.cc
$ ./error
caught exception 3
Signed-off-by: Eric Chiang <ericchiang@google.com>
Extend the subshell so that the actual (possibly non-zero) value of
$retval gets returned. Before, the changed value was lost at "done"
(= leaving the subshell), and the initial $retval=0 was returned.
(found with shellcheck)
