[apparmor] [PATCH 1/2] Fix compilation failure of deny link rules

[John Johansen]

On 03/19/2015 10:20 AM, Tyler Hicks wrote:
> On 2015-03-19 03:52:13, John Johansen wrote:
>> BugLink: http://bugs.launchpad.net/bugs/1433829
>>
>> The apparmor_parser fails to compile deny rules with only link permissions.
>>
>>   Eg.
>>        deny /f l,
>>        deny l /f,
>>        deny link /f -> /d,
>>
>> Will all fail to compile with the following assert
>>
>>   apparmor_parser: aare_rules.cc:99: Node* convert_file_perms(int, uint32_t, uint32_t, bool): Assertion `perms != 0' failed.
>>
>> Signed-off-by: John Johansen <john.johansen at canonical.com>
> 
> I don't understand the purpose link pair rules and can't find a
> description of them in the source and/or commit messages. However, if
> the intended purpose of this patch is to prevent the first pair rule
> from being added when there is no second pair rule, then this patch gets
> my ack (pending the changes that cboltz requested).
> 
They are in the core policy reference manual which we desperately need to
updated, and they really need to be added to the man page as well.

All link rules are actually link pair rules including
  /foo l,
which can also be expressed as
  l /foo,
or
  link /foo,
with the last two forms providing the option of specifying the additional
parameters for the link rule.

The link pair takes in account both the link path to create and the target
file as well as an optional subset flag.

'link' ['subset'] <link file path> ' ' ('to'|'->') ' ' <target file path> ','

where the subset flag indicates the link path must have a subset of the
permissions that the target file has. This way globbing can be used without
allowing linking to bypass the permissions specified for a given file.

The defaults for the additional parameters are subset=true and target file
=/**,

so
  /foo rl,

is actually broken into two rules
  /foo r,
  /foo l,
and /foo l is mapped to
  link subset /foo -> /**,

> As a side note, when adding the first pair rule, the conditionals for
> checking the LINK_BITS and CHANGE_PROFILE bits are set up in an if-else
> style. When adding the second pair rule, the conditionals are set up in
> an if-if style. That seems wrong but I can't say if that's an issue in
> practice (would a mode ever have both of those sets of bits set?).
> 
correct.

The whole encoding scheme here is somewhat confusing because currently
file, link, change_profile all share the same entry type and code path
(I do plan to split them out eventually).

So the first bit of code layout the shared path encoding, then we
get the first if block which handle special permission handling
if link is set, else if NOT change_profile (which uses the same
entry type as file but can't be specified with other permissions
unlike link).

The second if block handles the pair encoding of the link and
change_profile rule. Which adds onto the exist part of the
encoding already laid down. Basically use the embedded null trick
to separate entries very similar to how dbus and unix rules do.

And thanks for forcing me to look at this as it made me take a
second look and find an error with my patch.