[pkg-aa-profiles-team] a bunch of improvements i don't know where to throw
Antoine Beaupré
anarcat at debian.org
Thu Jun 19 18:35:05 UTC 2014
On 2014-06-19 13:49:03, intrigeri wrote:
> Hi anarcat,
>
> first, thanks a lot for trying out this stuff!
my pleasure. :)
> anarcat wrote (19 Jun 2014 14:28:09 GMT) :
>> I have installed apparmor-profiles and dumped all the "extras" into
>> /etc/apparmor.d (yes, i'm like that).
>
> Wow, yeah, you're like that. Never dared doing that given what the
> README in that directory says. For the profiles taken from
> /usr/share/doc/apparmor-profiles/extras/, pkg-aa-profiles-team hasn't
> volunteered to maintain these in Debian, and AFAIK probably nobody
> really has; that's why they are in /usr/share/doc :)
>
> So, I think the best place to report issues is the upstream
> mailing-list: apparmor at lists.ubuntu.com. They're very nice and all,
> and they love eating pull requests and patches for breakfast.
> That's why there'll be a lot of "Want to take this upstream?" below.
I see. Well, I was hoping for a little more of a "hit and run" scenario
here. ;)
I would love it if you guys took all my torches and ran with them, but I
would understand if you wouldn't have the energy for this... I know I
don't. :/
>> First off, one thing that is totally broken is the "chromium"
>> abstraction from lightdm: that just needs to go, otherwise we get silly
>> warnings that make aa-logprof basically unusable. I lost the error
>> message, but removing
>
>> apparmor.d/abstractions/ubuntu-browsers.d/chromium-browser
>
>> and fixing apparmor.d/lightdm-guest-session so it doesn't include it
>> fixed it for me.
>
> I believe I've had this fixed in the latest lightdm package (by simply
> dropping the lightdm-guest-session profile, since it attempts to
> confine functionality that is not shipped in the Debian package
> anyway). I'd love to see this confirmed.
Well, that could very well be. Hard for me to tell because i patched the
file locally... but i did upgrade to the latest lightdm in testing last
night and it didn't complain about my changes, so I guess it's all good.
>> The second thing is that aa-logprof really makes it difficult to review
>> changes to apparmor.d with versionning tools.
>
> I have to shamelessly admit I've never used aa-logprof.
It's great! :) It should probably be added to the wiki as a recommended
tool, IMHO. That and installing auditd, without which it's pretty useless.
>> Part of this is because the apparmor.d files use arbitrary order for
>> the file flags, so we get stuff like:
>> [...]
>> That's probably not all of them, and it's really silly: we should
>> probably just sanitise those files to the same format as outputted by
>> aa-logprof using some tool before they are committed to the package.
>
> I see what you mean, and I don't want you to suffer because of such
> inconsistencies. I think your proposed solution makes sense. Shall we
> (pkg-aa-profiles) take this matter upstream, or do you want to give it
> a try?
Please do.
>> Finally, we use dash. Everywhere. Almost every package out there I could
>> find, I had to do:
>
>> /bin/bash mixr,
>> + /bin/dash mixr,
>
> Indeed:
>
> $ rgrep --files-with-match bin/bash /usr/share/doc/apparmor-profiles/extras/ | wc -l
> 21
> $ rgrep --files-with-match /bin/bash /usr/share/doc/apparmor-profiles/extras/ \
> | xargs grep --files-with-match /bin/dash
> 1
>
> ... this tells a *lot* about the current state of maintenance of these
> "extra" profiles, especially considering Ubuntu has switched to
> dash-as-default-/bin/sh way before Debian.
Right. My feeling is that throwing those things in /usr/share/doc
doesn't help in improving them... Since we can easily run things in
"complain" mode, I don't see why we wouldn't enable all this stuff by
default. It will make some noise, but that's what sid is for. :)
Then things will start getting fixed. If it stays in "extras" as
"untested and unfinished", it becomes a self-fulfilling prophecy.
>> (it's also terrifying to see the number of things that call bash...)
>
> Yeah, that's one of the "interesting" side-effects of using a MAC tool
> such as AppArmor: on gets to discover what external tools our programs
> actually call...
Fun times.
>> So here's my gigantic patch of messed up improvements. I'm sorry that I
>> can't submit this in a more piecemeal fashion, but I hope that can be
>> picked up and factored in at some point. I did try to do a significant
>> amount of work to avoid noise in the diff, so it should still be pretty
>> readable.
>
> Thanks a lot. I'm commenting inline below.
>
>> --- a/apparmor.d/abstractions/p11-kit
>> +++ b/apparmor.d/abstractions/p11-kit
>> @@ -12,6 +12,7 @@
>> /etc/pkcs11/pkcs11.conf r,
>> /etc/pkcs11/modules/ r,
>> /etc/pkcs11/modules/* r,
>> + /usr/share/p11-kit/modules/ r,
>
> Fixed upstream already.
Great!
>> --- a/apparmor.d/etc.cron.daily.logrotate
>> +++ b/apparmor.d/etc.cron.daily.logrotate
>> [...]
>
> Looks alright at first glance. Want to submit it upstream?
Please do.
>> --- a/apparmor.d/program-chunks/postfix-common
>> +++ b/apparmor.d/program-chunks/postfix-common
>> @@ -15,7 +15,9 @@
>> capability sys_chroot,
>
>> /etc/postfix/*.cf r,
>> + /etc/postfix/my* r,
>
> No idea what's this one about. Upstream would want a rationale,
> I guess, unless they know Postfix better than me.
In my case, I have a configuration like this:
mydestination = $myhostname, $mydomain, localhost.$mydomain, localhost, pcre:/etc/postfix/mydestination
it tells various postfix daemons to look into a table made of a bunch of
PCRE regex in the /etc/postfix/mydestionation file.
It's a fairly common configuration, most Postfix daemons with configs a
little more complicated than the default will have something similar.
I am not sure how standard the my* naming scheme is however, but I don't
see why we shouldn't allow read access to /etc/postfix to postfix
daemons...
>> + /{,var/spool/postfix/}etc/localtime r,
>
> Looks good, worth upstreaming!
Please do! :)
>> --- a/apparmor.d/usr.bin.man
>> +++ b/apparmor.d/usr.bin.man
>> [...]
>> + #include <abstractions/lightdm>
>
> Uh?
I ... don't remember. :) This was probably aa-logprof playing tricks on
me.
>> [...]
>> +
>> +
>> + /home/*/.lesshst r,
>> + /root/.lesshst r,
>
> These ones want to be merged using @{HOME}, and prefixed by "owner".
Right.
>> /usr/lib/man-db/man Px,
>> + /var/cache/man/** mrwkix,
>
> "ix", really? I have no executable file there on my system.
> "m" surprises me a bit too, but why not.
Frankly, I have no idea what those flags mean, i'm just obeying
aa-logprof blindly. :)
Okay, the man one was really quirky, here's another attempt:
diff --git a/apparmor.d/usr.bin.man b/apparmor.d/usr.bin.man
index e69d44c..68ae114 100644
--- a/apparmor.d/usr.bin.man
+++ b/apparmor.d/usr.bin.man
@@ -14,11 +14,21 @@
/usr/bin/man flags=(complain) {
#include <abstractions/base>
+ #include <abstractions/consoles>
#include <abstractions/nameservice>
+ #include <abstractions/user-manpages>
capability setgid,
capability setuid,
+ /bin/dash r,
+ /bin/less rix,
+ /etc/manpath.config r,
+ /usr/bin/nroff rix,
+ /usr/bin/preconv rix,
+ /usr/bin/tbl px,
/usr/lib/man-db/man Px,
+ /var/cache/man/** mrwkix,
+ owner @{HOME}/.lesshst rw,
}
still - i wonder why and how this overlaps with usr.lib.man-db.man...
>> --- a/apparmor.d/usr.bin.procmail
>> +++ b/apparmor.d/usr.bin.procmail
>> @@ -24,6 +24,10 @@
>
>> # users want procmail to do many things:
>> @{HOME}/** rwl,
>> + /var/mail/* w,
>
> The included user-mail abstraction grants 'rwl' access to
> /var/spool/mail/*, which is (at least on my system) a symlink to
> /var/mail. So, I think this belongs to that abstraction instead.
Okay.
>> + # necessary on my postfix setup
>> + /var/spool/postfix/* w,
>> + /var/spool/postfix/*/ w,
>
> The great news is that profiles managed with dh_apparmor have this
> fancy "#include local/..." facility. Of course, it's not the case with
> profiles shipped in /usr/share/doc.
One more reason to ship those builtin i guess. :P
> Now, for the rest of the Postfix profiles, everything looks good to me
> (as in: I trust you that it's needed to make things work, and doesn't
> look scary) => should be upstreamed. I'm only commenting on things I'm
> less sure about.
>
>> --- a/apparmor.d/usr.lib.postfix.bounce
>> +++ b/apparmor.d/usr.lib.postfix.bounce
>> @@ -21,9 +21,11 @@
>
>> /usr/lib/postfix/bounce rmix,
>
>> + /{var/spool/postfix/,}active/* rwk,
>> /{var/spool/postfix/,}active/[0-9A-F]/[0-9A-F]/* rwl,
>> /{var/spool/postfix/,}active/[0-9A-F]/[0-9A-F]/ rwl,
>> /{var/spool/postfix/,}active/[0-9A-F]/ rwl,
>> + /{var/spool/postfix/,}bounce/* rwk,
>
> Can't these two ones be restrained a bit more, e.g. similarly to the
> existing lines?
Well, I don't understand why they're restrained in the first
place. Sure, queue files are usually hexadecimal, but why should we
enforce this policy on Postfix? As soon as it changes its policy, this
would break catastrophically. You don't want mail servers to fail
catastrophically.
I would even shorten all those rules to:
/{var/spool/postfix/,}{active,bounce,corrupt,defer,deferred,flush,hold,incoming,maildrop}/**
to save some trouble, but it would probably be best to specify the right
queue each daemon should have access to explicitely.
At the very least, it should be /**, because the hashing structure will
change arbitrarily between installs.
[...]
>> diff --git a/apparmor.d/usr.sbin.sshd b/apparmor.d/usr.sbin.sshd
>> index 67da06d..a547262 100644
>> --- a/apparmor.d/usr.sbin.sshd
>> +++ b/apparmor.d/usr.sbin.sshd
>> @@ -21,6 +21,7 @@
>> #include <abstractions/consoles>
>> #include <abstractions/nameservice>
>> #include <abstractions/wutmp>
>> + #include <abstractions/openssl>
>
> I'm surprised. Any idea why?
It seems to read openssl.cnf, no idea why.
>> @@ -31,6 +32,7 @@
>> capability setgid,
>> capability setuid,
>> capability audit_control,
>> + capability audit_write,
>
> If you say so :)
Again, I have no idea what those things mean. :p It's what logprof told
me...
>> @@ -43,7 +45,7 @@
>> /proc/*/oom_adj rw,
>> /proc/*/oom_score_adj rw,
>> /usr/sbin/sshd mrix,
>> - /var/log/btmp r,
>> + /var/log/btmp rw,
>> /{,var/}run w,
>> /{,var/}run/sshd{,.init}.pid wl,
>
>> I'll keep on trying to improve on this as I go along, but really, it's
>> annoyingly difficult to do so when there's so much noise in the diff.
>> Fixing the order of the flags in apparmor-profiles seems like a must if
>> we want this maintainable in the long term.
>
> Yes. It seems that the "extras" upstream profiles desperately need
> users (if there were, these bugs would have been spotted earlier), and
> likely interested people to send patches. Maybe we've just
> found someone?
I'd be happy to keep throwing patches at this mailing list, but I don't
have much time to go much further than that.
> TBH, I personally won't spend much time on these profiles until the
> Jessie freeze: for Jessie, I'd rather instead focus on integrating
> into Debian existing well-tested and mature profiles, such as some of
> the ones Ubuntu turns on by default. But I'm happy to act as
> a first-level filter, like I just did, if it may help. I'd be
> delighted to see these profiles reach a state when I can just drop it
> and see it work, to start with :)
I think we underestimate those profiles. They provide a good base to
start with, and provided they are cleaned up for better merging, would
be very easy to improve with aa-logprof.
Thanks for your work and feedback,
A.
--
La nature n'a créé ni maîtres ni esclaves
Je ne veux ni donner ni recevoir de lois.
- Denis Diderot
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 818 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/pkg-aa-profiles-team/attachments/20140619/20192847/attachment-0001.sig>
More information about the Pkg-aa-profiles-team
mailing list