[apparmor] [patch] several updates for profiles/extras

[Christian Boltz]

Hello,

Am Montag, 11. April 2011 schrieb Kees Cook:
> On Mon, Apr 11, 2011 at 12:01:33PM +0200, Christian Boltz wrote:
> > It looks they didn't jump too much - given the lack of feedback...
> > ;-)
> > 
> > I'd really like to avoid the "1 week timeout" rule when commiting
> > profile modifications - therefore: please review them!
> 
> Well, the problem is that I don't run these profiles on those tools,
> so beyond commenting on obviously weird changes, I can't really claim
> an "ACK" for many of them, 

I fully understand the problem. Nevertheless it is still better to check 
the profiles with common sense instead of waiting for the timeout.

> and given that it sounds like you are
> also not sure why some of the changes were needed, that doesn't make
> me think those changes should just blindly go in. :P

As I said: all changes were caused by audit.log entries via logprof.
The only thing I can't tell is if those permissions are still needed on 
current versions of $program, or if they were only needed in the past.

> I don't feel strongly about the extras/ tree, but I'm not sure what
> our general approach should be for profile review. It seems that we
> should really only commit things that have known changes.

This probably means I'll have to send a patch each time I run logprof so 
that I still know why a profile was changed ;-)

[freshclam]
> I'm okay with either. Mostly I just wanted it to be actually tested
> instead of cargo-culted from somewhere else. I'm okay with ** in
> /var/lib/clamav in the face of that many new rules.

I assume you'll also be OK with the merged
   owner /var/lib/clamav/clamav-*/clamav-*/daily.* rw,
rule that will replace most of the new rules ;-)

> > === modified file
> > 'profiles/apparmor/profiles/extras/usr.bin.freshclam'

> > +  /etc/gai.conf r,

Already in abstractions/nameservice. Removed from the profile.

> > +  owner /proc/*/status r,
> 
> These seems like they should live in abstractions.

This one is only in
abstractions/ubuntu-konsole:  @{PROC}/[0-9]*/status r,

That's not what I want to use in the freshclam profile ;-)

> > === modified file 'profiles/apparmor/profiles/extras/usr.bin.man'

> > +  /usr/bin/man r,
> 
> I still don't understand the need for this, but I won't NACK it.

I'll try without it...

> > === modified file
> > 'profiles/apparmor/profiles/extras/usr.lib.man-db.man'

> > -  /usr/man/** r,
> > +  /usr/man/** r, # obsolete?
> 
> There are still things living in /usr/man on some systems, but it
> should be extremely rare. I'm not sure how to handle this one.

I'd say: keep it (together with the comment ;-)  It can't do much harm - 
at least on openSUSE /usr/man does not even exist.

> > +  @{HOME}/*/.lesshst rw,
> 
> Why does this need "w"?

I'd guess because man ix'es less to display the man pages. 
~/.lesshst stores the search history.

The reproducer is probably something like
    man bash
    /variable

> >    /usr/bin/cmp rmix,
> > 
> > +  /usr/bin/getopt mrix,
> > 
> >    /usr/bin/groff rmix,
> >    /usr/bin/grops rmix,
[...]
> All these "m" uses make no sense to me. They're executables, not
> libraries. What is doing mmap(..., PROT_EXEC,...) on these files? I
> suspect all of these "m"s are wrong, and were accidentally added at
> a time when a system was profiled with READ_IMPLIES_EXEC attached to
> the process personality.

Might be (see "historically grown") - I'll test without the m.
That said: what about the r? Shouldn't "ix" be enough?

Oh, and several of the "m" are already in bzr - therefore you'll 
probably have to blame someone else for most of them *g*

> The rest looks okay.
> 
> > === modified file
> > 'profiles/apparmor/profiles/extras/usr.sbin.imapd'

> > +  /etc/rpc                                  r,
> 
> Seems like this should be in an abstraction again. 

None of the abstractions in bzr contain /etc/rpc...

> But then, why does imapd need RPC names?

Good question ;-)
(and hard to answer for me, I don't know what RPC is/does)

> > -  /usr/sbin/imapd                           r,
> > +  /usr/sbin/imapd                           mr,
> 
> This is another "m" case I don't think is right.

OK, I'll remove it and test again.

BTW: This profile also has

  /home/mailbox/**         lrw, 
   # TODO: mailbox location - make configurable via tunable?

What about a tunables/mailstore with a @{MAILSTORE} variable?
(That's something that would be useful for some more profiles I'm 
using.)


I'll send a new patch in the next days after testing the changes listed 
above. Until then, feel free to pick up some profiles in my "various new 
profiles" mail ;-)

> > === modified file 'profiles/apparmor/profiles/extras/bin.netstat'
> > === modified file
> > 'profiles/apparmor/profiles/extras/usr.sbin.postmap'
> > === modified file
> > 'profiles/apparmor/profiles/extras/usr.sbin.postqueue'
> > === modified file
> > 'profiles/apparmor/profiles/extras/usr.sbin.useradd'

(all ACKed)

I commited those to make the next diff smaller ;-)


Regards,

Christian Boltz
-- 
Benutzerfreundlichkeit
Der Benutzer hat zum Admin freundlich zu sein. [Thorsten Fenk]