[apparmor] [patch] Allow aa-complain etc. to change profiles for non-existing binaries

[Christian Boltz]

Hello,

Am Mittwoch, 3. Juni 2015 schrieb Steve Beattie:
> On Mon, May 25, 2015 at 05:44:20PM +0200, Christian Boltz wrote:
> > aa-complain, aa-enforce, aa-disable and aa-audit refused to change
> > profiles for non-existing binaries. This patch also allows paths
> > starting with /. This also makes it possible to use
> > 
> >     aa-complain '/{usr/,}bin/ping'
> > 
> > and
> > 
> >     aa-complain /etc/apparmor.d/bin.ping
> > 
> > This patch fixes https://bugs.launchpad.net/apparmor/+bug/1416346
> > 
> > [ 36-allow-aa-complain-for-non-existing-binary.diff ]
> 
> Acked-by: Steve Beattie <steve at nxnw.org>

Thanks.

I forgot to ask - should I also commit this patch to the 2.9 branch?

> > Well, mostly - we still need to decide how we handle wildcards in
> > 
> > profile names:
> >     aa-complain ping
> >     aa-complain /usr/bin/ping
> > 
> > will still error out with "Profile not found" because it isn't an
> > exact match (and matching the wildcard would change more than the
> > user wants).
> > 
> > Any opinions how to handle this?
> 
> At some point we need to write a general function that takes an
> apparmor path regex and converts it into a python re object, such
> that we can then test a given string against it and get the answer to
> whether or not it matches -- it would be useful in many locations
> (this issue, aa-status detecting whether a policy should apply,
> etc.).

We have that (convert_regexp() in common.py), and it does some 
interesting[tm] replace and re.sub magic to convert a path to an even 
more intereting regex ;-)

That said - having a class that does the path handling, matching, 
whatever would really be nice ;-) - it would "hide" the magic and even 
allow some performance optimization (if a path doesn't contain any 
special chars, we could skip the whole re stuff and do plain text 
comparisons.)

> Once we had that, a possible solution would be to apply the change
> above to a regex applied profile if that profile is the only one to
> apply to that binary; 

I agree with the intention, but you ignored a side effect.

Let's say I have a profile /usr/lib/dovecot/* and run aa-complain 
/usr/lib/dovecot/anvil. With your proposal, the /usr/lib/dovecot/* 
profile would be changed, which means the change affects all the 
binaries in /usr/lib/dovecot.

That will at least surprise the user, and sounds like unexpected 
behaviour to me ("hey, I only wanted to change the profile for anvil!").

Somehow I'm afraid we should just error out with an useful error message 
like:

    /usr/lib/dovecot/anvil is using a profile that is shared with 
    multiple binaries. If you really want to change it, please use
        aa-complain '/usr/lib/dovecot/*'

> if a non-regex match and a regex match would
> both apply, update the non-regex one (since I believe that's the
> one the kernel will prefer); 

Agreed (and that's also what we currently do).

> if multiple regex matches apply then
> perhaps an error is in order, but I've forgotten the semantics of
> kernel behavior there.

Yes, and see above.

> > Oh, and this patch also fixes the last failure in minitools_test.py.
> > (Should we rename it to test-minitools.py to include it in "make
> > check"?)
> I get seven failures with minitools_test.py unless I have pre-run make
> in the profiles directory.

Indeed. I always have apparmor.d/local/ populated, so I didn't notice 
this. So we can choose between a) calling "make -C ../../profiles" from 
utils/test/Makefile and b) copying over the whole ../../profiles 
directory and run make inside each test tempdir.

> It should be using a temp dir per-test. to populate profiles into,
> rather than CWD (you added infrastructure to common_test.py to make
> this relatively painless), in no small part to ensure that the tests
> independent of each other (ensuring that a failure in one test case to
> change the state of a profile doesn't cause another test case to
> fail).
> 
> The calls to subprocess.check_output() should not need to use a shell.
> Although, taking the cmd() function from test-aa-decode.py might be
> preferred, as the output from the cmd can be reported in the test
> failure, rather than just going to stderr (would need to catch the
> subprocess.CalledProcessError exception to do the same with
> subprocess.check_output()).

Both added to my TODO list.

> It has the same issue as test-aa.py, in that the import of aa.py
> fails if /etc/apparmor is not populated. This prevents make check from
> being run as part of a bootstrapping build process where portions of
> apparmor have not already been installed.

Yes, that's https://bugs.launchpad.net/apparmor/+bug/1393979 and 
probably affects lots of (all?) tests.


Regards,

Christian Boltz
-- 
Hmm.. Good point Adrian. I should get used to that thing
close to my keyboard... how did you call it? Mouse? :-)
[Dominique Leuenberger in opensuse-factory]