[apparmor] Nested profile problem upgrading from apparmor-2.9.0 to apparmor-2.11.0

[Christian Boltz]

Hello,

Am Dienstag, 19. Juni 2018, 07:53:32 CEST schrieb John Johansen:
> On 06/18/2018 09:21 PM, apparmor at raf.org wrote:
> > I'm upgrading from debian8/apparmor-2.9.0 to
> > debian9/apparmor-2.11.0 and am seeing an error message
> > when using aa-complain and aa-enforce (but not when using
> > apparmor_parser).
> > 
> >  # aa-enforce usr.sbin.apache2
> >  ERROR: Profile /usr/sbin/apache2^indexcgi defined twice in
> >  
> >    /etc/apparmor.d/usr.sbin.apache2, last found in line 89
> >  
> >  # aa-complain usr.sbin.apache2
> >  ERROR: Profile /usr/sbin/apache2^indexcgi defined twice in
> >  
> >    /etc/apparmor.d/usr.sbin.apache2, last found in line 89
> 
> The tools have never properly supported nesting beyond a single
> child. I assume some of the refactoring to clean them up broke
> this for you

Maybe not a recent change, but if you take 2.9 as starting point... ;-)

I'd guess this happens in aa.py parse_profile_start() which does

    else:  # stand-alone profile
        profile = matches['profile']
        if len(profile.split('//')) >= 2:
            profile, hat = profile.split('//')[:2]  # <---------
            pps_set_hat_external = True
        else:
            hat = profile
            pps_set_hat_external = False

I didn't do any testing, but I'm quite sure that the marked line is what 
breaks for you.

OK, I'll test it:

--- a/utils/test/test-aa.py
+++ b/utils/test/test-aa.py
@@ -544,6 +544,11 @@ class AaTest_parse_profile_start(AATest):
         expected = ('/foo', '/foo', None, 'complain', False, False, False)
         self.assertEqual(result, expected)
 
+    def test_parse_profile_start_07(self):
+        result = self._parse('/foo///bar///baz {', None, None) # external hat
+        expected = ('/foo', '/bar', None, None, False, False, True)  # XXX missing 'baz'
+        self.assertEqual(result, expected)
+
 
     def test_parse_profile_start_invalid_01(self):
         with self.assertRaises(AppArmorException):

... and it gives the wrong result I expected - profile '/foo', hat 
'/bar', and '/baz' gets lost.

Obviously the error you see happens later, but parse_profile_start() is
the root cause.

I already promised to enhance the tools to support deeper nested 
profiles, but this will need quite some work, and I don't know yet when 
I'll have time to do it.

Until then - what's your prefered way the tools should behave?
a) current status
b) error out if a deep nesting level is found (which would basically
   mean a better error message)
c) (any better idea?)

> > But removing it with "apparmor_parser -R usr.sbin.apache2",
> > 
> > produces:
> >  apparmor_parser: Unable to remove
> >  "/usr/sbin/apache2//indexcgi//enscript".>  
> >    Profile doesn't exist
> :
> :( , that should work. Looks like a bug

Not necessarily. AFAIK the parser unloads child profiles when unloading
the main profile:

# echo 'profile foo {
 profile bar {
 }
 }' |apparmor_parser
# aa-status |grep foo
   foo
   foo//bar
# echo 'profile foo {}' | apparmor_parser -R
# aa-status |grep foo
#

Your external child profiles are defined after the main profile, therefore
apparmor_parser tries to unload them after unloading the main profile
(including all child profiles).

> > Is there something wrong with the above?
> > Has the syntax changed for nested profiles?
> 
> no it hasn't, the code has been slowly being cleaned up and
> you have found a regression

I wouldn't be surprised if 2.9 has a similar bug, but with different 
results - in worst case writing only the first level of child profiles.
(Note: I didn't look at the 2.9 code for a long time, so this is only
a guess.)

> > I originally tried to put the last three profiles inside
> > the parent profile but that syntax wasn't supported at the time
> > and I was advised to do it this way.
> 
> correct more than a single level of nesting is not supported yet

If you want to have the tools working *now*, a possible solution would
be to reduce the nesting level. For example, you could change
  profile /usr/sbin/apache2//indexcgi//mutt//exim4 {
to
  profile /usr/sbin/apache2//indexcgi--mutt--exim4 {
and adjust your Px targets accordingly.


Regards,

Christian Boltz
-- 
The only valid reason to change passwords is when you suspect them to be
compromised. So, every single time an admin is fired. [Vinzent Höfler on
https://plus.google.com/+KristianKöhntopp/posts/cpEDJCF6tUN]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.ubuntu.com/archives/apparmor/attachments/20180619/5e0e9650/attachment.sig>