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

[Christian Boltz]

Hello,

Am Donnerstag, 4. Juni 2015 schrieb Steve Beattie:
> On Thu, Jun 04, 2015 at 01:45:33PM +0200, Christian Boltz wrote:

> > > 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.
> 
> Sigh, sorry, I didn't complete what I'd been thinking, which was that
> it would need to satisfy to conditions:
> 
>   1) only one profile applies

So for the   /{usr/,}bin/ping   profile, we have:

# ls -l /bin/ping /usr/bin/ping
lrwxrwxrwx 1 root root    13 27. Apr 22:23 /bin/ping -> /usr/bin/ping
-rwxr-xr-x 1 root root 39384 27. Apr 22:23 /usr/bin/ping

Well, things aren't that easy... ;-)
(and no, I don't want to readlink() etc. to check if all names point to
the same binary - that would mean to cover one of 100 corner cases, and
leave the other 99 uncovered)

>   2) the regex match pattern only applies to one existing binary. I
>      grant this latter one is harder to check, but should be possible
>      to do (we could start by supporting a subset of aa-regex pattern
>      types).

It's probably even less likely that a * or ** applies to only one binary ;-)

> > That will at least surprise the user, and sounds like unexpected
> > behaviour to me ("hey, I only wanted to change the profile for
> > anvil!").
> An alternative solution would be to create a separate profile copy
> for just the passed path.

I hope you are joking ;-) - that would end up with a big mess.

> > 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/*'

I still think this is the only reliable solution.

> > > 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.
> 
> Well, the root cause is really that our minitools tests depend on
> the state of external files outside of the tests control.

I know, but even if it's annoying, it has an advantage: we automatically 
make sure it works with real-world profiles, not only outdated testcases.

> > > 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.
> 
> Ugh, we've regressed a lot there, these are the tests according to
> runtests-py3.sh that fail due to this issue:

That's not a regression. We have this problem since we have aa.py -but
nobody noticed it (until I opened the bugreport) ;-)

> *** The following tests failed:
> ***    aa_test.py minitools_test.py test-aa.py test-dbus_parse.py
>        test-mount_parse.py test-pivot_root_parse.py
> test-ptrace_parse.py test-regex_matches.py test-signal_parse.py
> test-unix_parse.py

I'm not surprised.

Minimal testcase:

python3 -c 'from apparmor.aa import fetch_profiles_by_name'   # that function just does "return None, None"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "/home/cb/apparmor/HEAD-patches-applied/utils/apparmor/aa.py", line 4354, in <module>
    if cfg['settings'].get('default_owner_prompt', False):
  File "/usr/lib64/python3.4/configparser.py", line 937, in __getitem__
    raise KeyError(key)
KeyError: 'settings'

The problem is that aa.py reads logparser.conf in the global area 
(for details what it does exactly, scroll down to the end of the file), 
so you'll trigger reading /etc/apparmor/logprof.conf by just importing
a function from apparmor.aa


An interesting detail is that it errors out quite late - so configparser
doesn't error out on non-existing files:

# python3
Python 3.4.1 (default, May 23 2014, 17:48:28) [GCC] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import configparser
>>> config = configparser.ConfigParser()
>>> config.read('/foo/bar')
[]
>>> 

Oh, and the documentation says that's intended:
https://docs.python.org/3.4/library/configparser.html#configparser.ConfigParser.read

Hmm, should we add a "file exists?" check in apparmor.config so that we 
get a clear error message instead of an empty result? Or should we just
make sure to have good defaults for everything so that the tools (and
the tests) work even without /etc/apparmor/logprof.conf?
(That would still leave the issue that an existing logprof.conf can 
influence test results.)


Regards,

Christian Boltz
-- 
It's never too late for a new feature
(C) Novell
:-)
[Marcel Hilzinger in opensuse-factory]