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

[apparmor at raf.org]

Christian Boltz wrote:

> 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):

Um, should those be triple forward slashes? or only double?

> ... 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?)

Thanks for identifying the problem. Ideally, any amount of nesting
should work. For some reason I assumed that when a process called
another, the profiles had to be nested similarly. I don't know
why I assumed that.

However, it used to work (even if the syntax was clumsy) so it probably
wasn't too crazy an assumption.

And apparmor_parser still loads it without error (but unloading it
does give an error but works anyway). It's strange that aa-enforce
and aa-complain fail but apparmor_parser works.

Given that it's a lot of work to support full nesting and, if the
profiles don't need to be deeply nested to reflect process parentage,
then I think that better error messages is probably good enough
for now.

> > > 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).

Perhaps it could remember which profiles it has unloaded and
suppress the error if it encounters it again. It's a harmless
error message so silencing it would be nice.

> > > 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.)

With 2.9, aa-status does show all of the nested profiles so it
looks like they were all loaded:

   /usr/sbin/apache2
   /usr/sbin/apache2//indexcgi
   /usr/sbin/apache2//indexcgi//enscript
   /usr/sbin/apache2//indexcgi//mutt
   /usr/sbin/apache2//indexcgi//mutt//exim4
   /usr/sbin/apache2//officecgi

> > > 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.

Thanks. I've done that (but with underscores) and now aa-enforce
and aa-complain and apparmor_parser all load without problem
and unloading is fine too.

   /usr/sbin/apache2
   /usr/sbin/apache2//indexcgi
   /usr/sbin/apache2//indexcgi__enscript
   /usr/sbin/apache2//indexcgi__mutt
   /usr/sbin/apache2//indexcgi__mutt__exim4
   /usr/sbin/apache2//officecgi

> 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]

Thanks again for your time.
And thanks for apparmor!

cheers,
raf