Seth pointed out that dirat_for_each() didn't correctly handle the
return value from readdir_r(). On error, it directly returns a positive
errno value. This would have resulted in that positive errno value being
returned, with an undefined errno value set, from dirat_for_each().
However, the dirat_for_each() documentation states that -1 is returned,
with errno set, on error.
This patch results in readdir_r()'s return value being handled
appropriately. In addition, it ensures that 0 is always returned on
success.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Reported-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Like net_find_af_name, this assumed that AF_* values were consecutive.
[smcv: split out from a larger patch, added commit message,
removed dead declaration]
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
The network_families array is automatically built from AF_NAMES, which is
extracted from the defines in <bits/socket.h>. The code assumes that
network_families is indexed by the AF defines. However, since the
defines are sparse, and the gaps in the array are not packed with
zeroes, the array is shorter than expected, and the indexing is wrong.
When this function was written, the network families that were
covered might well have been consecutive, but this is no longer true:
there's a gap between AF_LLC (26) and AF_CAN (29). In addition,
the code that parses <sys/socket.h> does not recognise AF_DECnet (12)
due to the lower-case letters, leading to a gap betwen AF_ROSE (11)
and AF_NETBEUI (13).
This assumption caused a crash in our testing while parsing the rule
"network raw".
[smcv: split out from a larger patch, added commit message]
Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>
Don't pass an ostream reference into another ostream via <<.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Abstract af_unix socket names can contain a null character, however the
aare to pcre conversion explicitly disallows null characters because they
are not valid characters for pathnames. Fix this so that they type of
globbing is selectable.
this is a partial fix for
Bug: http://bugs.launchpad.net/bugs/1413410
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Since the Makefile cleanup, the _clean target is only used to delete
manpages etc. generated from *.pod files.
This patch renames the _clean target to pod_clean to make it obvious
what it does.
Acked-by: John Johansen <john.johansen@canonical.com>
- drop the symlink magic of the common/ directory, and just include
files directly from there.
- update comments indicating required steps to take when including
common/Make.rules
- drop make clean steps that refer to no longer generated tarballs,
specfiles, and symlinks to the common directory/Make.rules.
- don't silence clean steps if VERBOSE is set
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Christian "Ghostbuster" Boltz <apparmor@cboltz.de>
This patch creates expected pass tests for all known mount options as
well as expected fail tests for some known bad mount options.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Bug: https://launchpad.net/bugs/1399027
This patch restricts MS_REC to only be used while defining the MS_RBIND,
MS_RUNBINDABLE, MS_RPRIVATE, MS_RSLAVE, and MS_RSHARED macros.
The MS_R* macros are simply an OR of the corresponding non-recursive
macro and MS_REC:
#define MS_RBIND (MS_BIND | MS_REC)
Previously, a shortcut was taken when needing to specify the
non-recursive and recursive macros:
(MS_BIND | MS_UNBINDABLE | MS_PRIVATE | MS_SLAVE | MS_SHARED | MS_REC)
By using MS_REC above, it is not immediately clear that
MS_R{BIND,UNBINDABLE,PRIVATE,SLAVE,SHARED} are also included.
By restricting the use of MS_REC, this patch improves readability by
forcing the use of the MS_R{BIND,UNBINDABLE,PRIVATE,SLAVE,SHARED} macros
instead of relying on the MS_REC shortcut.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
The parser correctly rejects mount make-* options (make-shared,
make-slave, make-private, make-unbindable) when a device is specified
(the source argument of mount(2)). However, it was not rejecting the
recursive make-* options (make-rshared, make-rslave, make-rprivate,
make-runbindable) when a device was specified.
This patch adds the MS_REC bit, which is used to indicate a recursive
option, to the MS_CMDS macro. Without this change, the recursive options
are treated as normal mount options.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
The parser should not indicate success when mount rules contain
unknown mount options:
$ echo "/t { mount options=(XXX) -> **, }" | apparmor_parser -qQ
$ echo $?
0
This patch modifies the parser so that it prints an error message and
exits with 1:
$ echo "/t { mount options=(XXX) -> **, }" | apparmor_parser -qQ
unsupported mount options
$ echo $?
1
Bug: https://bugs.launchpad.net/bugs/1401621
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
There are a number of differences between what the apparmor.d(5) man
page lists as valid AppArmor mount rule options and what apparmor_parser
looks for when parsing mount rules. There are also typos in the man page
and parser around mount options. Here's the breakdown of problems and
fixes made in this patch:
* The apparmor.d(5) man page improperly documented a "nodirsync"
option.
- That mount option does not exist and the parser did not honor it.
Remove the mention from the apparmor.d(5) man page.
* The loud option was typoed as "load" in both the man page and parser
- There's no sense in preserving backwards compatibility. "load" is
simply wrong and should not be honored. The man page and parser are
updated to only use "loud".
* The rbind option wasn't listed in the man page.
- Add rbind to the man page. No change needed for the parser.
* The documented unbindable, private, slave, and shared options were
not correctly parsed. The parser expected
make-{unbindable,private,slave,shared}.
- The parser is updated to accept both the documented
{unbindable,private,slave,shared} options and their variants
prefixed with "make-". The man page will not document the "make-"
variants.
* The recursive {runbindable,rprivate,rslave,rshared} options were not
documented and were only recognized by the parser if they were
prefixed with "make-".
- The man page is updated to document the option strings that are not
prefixed with "make-". The parser still accepts the "make-"
variants.
* The man page documented a "rec" option but the parser didn't honor
it. The MS_REC macro is used by the mount utility to be bitwise OR'ed
with MS_{UNBINDABLE,PRIVATE,SLAVE,SHARED} to indicate the
corresponding recursive mount options.
- This is not an option that should be exposed in the AppArmor policy
since we already allow have the
{runbindable,rprivate,rslave,rshared} options.
* The man page typoed the {no,}relatime options as {no,}relative.
- The man page is updated to document the correct option strings. The
parser requires no change.
Bug: https://bugs.launchpad.net/bugs/1401619
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Those *.spec{,.in} files were not updated for years (last change
2006/2007) and don't fit the current "one tarball for everything" model.
Acked-by: Steve Beattie <steve@nxnw.org>
Both valgrind and strace report the parser doing
close(-1) = -1 EBADF (Bad file descriptor)
This happens the skip kernel load argument is specified in combination
with any of --add, --replace, or --remove arguments (the default
is --add if no other option is specified).
This happens when the parser is not processing profiles but not
writing them out (eg. no kernel load, dump to stdout, file ...)
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Update the apparmor_parser documentation for the new ability to load
profiles from a specified directory.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Tyler Hicks <tyhicks@canonical.com>
Currently the apparmor parser warns about rules that are not enforced or
downgraded. This is a problem for distros that are not carrying the out of
tree kernel patches, as most profile loads result in warnings.
Change the behavior to not output a message unless a warn flag is passed.
This patch adds 2 different warn flags
--warn rule-downgraded # warn if a rule is downgraded
--warn rule-not-enforced # warn if a rule is not enforced at all
If the warnings are desired by default the flags can be set in the
parser.conf file.
v2 of patch
- update man page
- add --warn to usage statement
- make --quiet clear warn flags
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
BugLink: http://bugs.launchpad.net/bugs/1378091
The audit flags are not being set correctly by the parser so that
audit capability XXX,
will not result in an audit message being logged when the capability
is used.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Bug: https://bugzilla.novell.com/show_bug.cgi?id=895495
We define the __unused macro as a shortcut for __attribute__((unused))
to quiet compiler warnings for functions where an argument is unused,
for whatever reason. However, on 64 bit architectures, older glibc's
bits/stat.h header defines an array variable with the name __unused
that collides with our macro and causes the parser to fail to build,
because the resulting macro expansion generates invalid C code.
This commit fixes the issue by removing the __unused macro where it's
not needed (mod_apparmor) and renaming it to 'unused' elsewhere. It also
in some instances reorders the arguments so that the unused macro
appears last consistently.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
Acked-by: Tyler Hicks <tyhicks@canonical.com>
BugLink: http://bugs.launchpad.net/bugs/1373085
The parser fails to accept certain characters, even when escaped
or quoted as part of the profile or label name in ipc rules. This
is due to the lexer not accepting those characters as part of the
input pattern.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
Currently the parser is bailing when it fails to load a profile,
not processing any potential subsequent profiles in the dir or passed
in list. This results in all policy after the first error failing
to load, instead of just the profile(s) with the error.
This is a different behavior than what has been done by initscripts
that have driven it with xargs -n1, passing it a single profile
at a time.
Fix this so that the parser only exits on first error if specifically
told to do so.
Note: this does not fix the various failure points in the parser
that call exit, instead of returning an error.
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>, thanks.
With the move to C++-ification of the parser, the parser's makefile was
not updated to take into account .cc files when deriving object files.
This would result in the final linking compilation of the parser binary
including all of the .cc files in its command line, rather than the ,o
files. This patch fixes the issue as well as an additional typo in the
dependency list for af_unix.o that was not triggered because af_unix.o
was not being built independently.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
This patch updates the parser code to reject rules that contain local
socket permissions and peer conditional elements. The error message for
that condition is also corrected to resolve a copy and paste mistake
from the D-Bus rule parsing code.
The patch also updates the man page to correctly describe the two sets
of socket permissions and fixes an example rule that resulted in a
parser error after the change described above.
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: Steve Beattie <steve@nxnw.org>
while it is not possible to specify a rule with local conditionals with
peer conditionals
eg.
unix listen peer=(addr=@foo),
a rule such as
unix peer=(addr=@foo),
is possible, and was setting all permissions for local as well as the peer
condition permissions.
Currently this means the create permission must be specified in a separate
rule from a rule with a peer= condition, if create is to be allowed. This
isn't too much of an issue but it does mean rule such as
unix connect peer=(addr=@foo),
Can not imply the ability to create a socket. Which may indeed be the
behavior if we wish to enforce that the socket was created in another
process and passed in. Is this what we want to do?
Signed-off-by: John Johansen <john.johansen@canonical.com>
Acked-by: Tyler Hicks <tyhicks@canonical.com>
This patch adds a 'check_pod_files' make target to the common make
rules, and then fixes the errors it highlighted as well as most of
the warnings. It will cause 'make check' in most of the directories to
fail if there are errors in a pod file (but not if there are warnings).
Common issues were:
- using an '=over/=back' pair for code-like snippets that did not
contain any =items therein; the =over keyword is intended for
indenting lists of =item entries, and generates a warning if
there isn't any.
- not escaping '<' or '>'
- blank lines that contained spaces or tabs
The second -warnings flag passed to podchecker is to add additional
warnings, un-escaped '<' and '>' being of them.
I did not fix all of the warnings in apparmor.d.pod, as I have not come
up with a good warning-free way to express the BNF of the language
similar in format to what is currently generated. The existing
libapparmor warnings (complaints about duplicate =item definition
names) are actually a result of passing the second -warnings flag.
The integration into libapparmor is suboptimal due to automake's
expectation that there will be a test driver program(s) for make check
targets; that's why I added the podchecker call to the manpage
generation point.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
---
changehat/mod_apparmor/Makefile | 3
changehat/mod_apparmor/mod_apparmor.pod | 28 ++-
common/Make.rules | 4
libraries/libapparmor/doc/Makefile.am | 7
parser/Makefile | 2
parser/apparmor.d.pod | 275
+++++++++++++-------------------
utils/Makefile | 3
utils/aa-cleanprof.pod | 2
utils/aa-complain.pod | 2
utils/aa-decode.pod | 2
utils/aa-easyprof.pod | 69 +++-----
utils/aa-enforce.pod | 2
utils/aa-genprof.pod | 2
utils/aa-logprof.pod | 6
utils/aa-sandbox.pod | 64 ++-----
utils/logprof.conf.pod | 2
utils/vim/Makefile | 2
17 files changed, 212 insertions(+), 263 deletions(-)
In profile.h, flagvals is declared to be class, but then in the
Profile class, the flags field declares it as a struct. This patch
makes the field declaration type consistent.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: John Johansen <john.johansen@canonical.com>
In trunk commit 2615, make targets for af_rule.o and af_unix.o were
added. Unfortunately, the af_rule.o target's dependency on rule.h was
missing the .h suffix. This patch fixes the issue and adds some other
headers that the source file are dependent on.
Signed-off-by: Steve Beattie <steve@nxnw.org>
Acked-by: Seth Arnold <seth.arnold@canonical.com>
Acked-by: John Johansen <john.johansen@canonical.com>
