Hi Christian,
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, 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
> I am attaching an updated patch. It contains changes for bin.netstat (as
> discussed in the previous mail) and usr.bin.freshclam (more strict
> rules), the other profiles are unchanged compared to my first patch.
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.
> The following question is still open:
> > The first freshclam and logprof run showed me why I changed to ** - I
> > now have 30 rules for files in /var/lib/clamav ;-)
> > I could merge most of those rules to
> > owner /var/lib/clamav/clamav-*/clamav-*/daily.* rw,
> > Would that be OK for you?
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.
> === modified file 'profiles/apparmor/profiles/extras/bin.netstat'
ACK
> === modified file 'profiles/apparmor/profiles/extras/usr.bin.freshclam'
>...
> + /etc/gai.conf r,
> + owner /proc/*/status r,
These seems like they should live in abstractions.
> === 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.
> === 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.
> + @{HOME}/*/.lesshst rw,
Why does this need "w"?
> /usr/bin/cmp rmix,
> + /usr/bin/getopt mrix,
> /usr/bin/groff rmix,
> /usr/bin/grops rmix,
> /usr/bin/grotty rmix,
> /usr/bin/nroff rmix,
> /usr/bin/tbl rmix,
> /usr/bin/troff rmix,
> + /usr/bin/whatis rix,
> /usr/bin/zsoelim rmix,
> + owner /usr/lib/man-db/manconv m, # merge with the rule below to rmPx?
> + /usr/lib/man-db/manconv rPx,
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.
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. But then, why does imapd
need RPC names?
> - /usr/sbin/imapd r,
> + /usr/sbin/imapd mr,
This is another "m" case I don't think is right.
> === modified file 'profiles/apparmor/profiles/extras/usr.sbin.postmap'
ACK
> === modified file 'profiles/apparmor/profiles/extras/usr.sbin.postqueue'
ACK
> === modified file 'profiles/apparmor/profiles/extras/usr.sbin.useradd'
ACK
Thanks!
-Kees
--
Kees Cook
Ubuntu Security Team