Merge lp:~sdeziel/apparmor-profiles/unbound-refresh into lp:apparmor-profiles

Proposed by Simon Déziel on 2016-01-11
Status: Merged
Merged at revision: 156
Proposed branch: lp:~sdeziel/apparmor-profiles/unbound-refresh
Merge into: lp:apparmor-profiles
Diff against target: 35 lines (+13/-0)
1 file modified
ubuntu/16.04/usr.sbin.unbound (+13/-0)
To merge this branch: bzr merge lp:~sdeziel/apparmor-profiles/unbound-refresh
Reviewer Review Type Date Requested Status
Seth Arnold 2016-01-11 Approve on 2016-01-28
Review via email: mp+282230@code.launchpad.net
To post a comment you must log in.
Seth Arnold (seth-arnold) wrote :

Are unbound developers aware of the new requirements for their pid handling? It's usually possible to handle pid files without requiring chown and dac_override privileges.

THanks

Simon Déziel (sdeziel) wrote :

Upstream wants the PID file to be tentatively deleted when the daemon stops. Since this is not always possible failing to do so isn't fatal:

        /* this unlink may not work if the pidfile is located outside
         * of the chroot/workdir or we no longer have permissions */
        if(daemon->pidfile) {
                int fd;
                /* truncate pidfile */
                fd = open(daemon->pidfile, O_WRONLY | O_TRUNC, 0644);
                if(fd != -1)
                        close(fd);
                /* delete pidfile */
                unlink(daemon->pidfile);
        }

In fact, on Debian/Ubuntu, even with the needed caps to make the PID writeable by the low priv user, it cannot delete it because it's outside of the chroot. I just tested with/without the caps and the PID is never deleted.

One drawback of denying chown/dac_overide is that unbound complains:

  unbound: [21915:0] error: cannot chown 112.121 /run/unbound.pid: Operation not permitted

The error could be turned into a debug message and then we could do without the additional caps. I'll check with upstream if they would accept such change. Thanks for the feedback Seth!

Simon Déziel (sdeziel) wrote :

FYI, I proposed a patch to turn the error into a debug message: https://www.nlnetlabs.nl/bugs-script/show_bug.cgi?id=734

Simon Déziel (sdeziel) wrote :

@Seth, upstream accepted the patch. What do you suggest to do until this reaches Debian then Ubuntu?

Seth Arnold (seth-arnold) wrote :

Simon, that's great. Nice job :) Since the error message is essentially harmless, and granting the permissions wouldn't actually allow the unlink anyway (since we're doing the chroot too), I think we can also ignore giving these permissions to unbound. We could add "deny" lines to silence the AppArmor denials but that might mask actual problems if unbound is modified in the future to require these privileges.

So, I propose we skip the new capabilities if we can; the new file rules look sane, adding those sounds like a good idea.

Does that sound fair?

Thanks

Simon Déziel (sdeziel) wrote :

Hmm, I might have been too quick to add the caps denial. I understand your concern about masking future problems but on the other hand, Apparmor denials would have people wondering about what's going on.

I was tempted to ask upstream to skip the chown'ing if the PID is outside of the chroot but then one setting the PID to be in the chroot would be missing the caps... Would that be the least bad option?

Simon Déziel (sdeziel) wrote :

@Seth, upstream accepted another patch that will only do the PID chown'ing if the PID is inside the chroot. Since the PID lives outside the chroot on Debian/Ubuntu, the chown/dac_override caps won't be needed when Unbound 1.5.8 will be released.

Until that release happens and reaches Ubuntu, what do you recommend doing with this profile refresh?

Seth Arnold (seth-arnold) wrote :

Well, we could merge this as-is and remember to come back and remove the caps when the new unbound is synced, or just check in this with the caps removed and if the new unbound isn't synced, merge a fix for that later...

I think I prefer merging this with the capabilities removed, but either way is fine with me.

Thanks

Simon Déziel (sdeziel) wrote :

@Seth, if you could merge this as-is (useless caps denied) that would be great. I'll follow-up when the fixed unbound reaches Ubuntu.

Thanks

156. By Seth Arnold on 2016-01-28

  Simon Deziel 2016-01-12 usr.sbin.unbound: deny chown/dac_override caps,
  add fowner fsetid caps, root hints, unbound.ctl socket

Seth Arnold (seth-arnold) wrote :

Thanks for working with us and unbound upstream on this. It was more back-and-forth than I expected but the end result looks great. Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntu/16.04/usr.sbin.unbound'
2--- ubuntu/16.04/usr.sbin.unbound 2015-10-27 01:49:53 +0000
3+++ ubuntu/16.04/usr.sbin.unbound 2016-01-12 21:30:58 +0000
4@@ -6,6 +6,13 @@
5 #include <abstractions/base>
6 #include <abstractions/nameservice>
7
8+ # needlessly chown'ing the PID, for details see:
9+ # https://www.nlnetlabs.nl/bugs-script/show_bug.cgi?id=734
10+ deny capability chown,
11+ deny capability dac_override,
12+
13+ capability fowner,
14+ capability fsetid,
15 capability net_bind_service,
16 capability setgid,
17 capability setuid,
18@@ -15,6 +22,9 @@
19 # root trust anchor
20 owner /var/lib/unbound/root.key* rw,
21
22+ # root hints from dns-data-root
23+ /usr/share/dns/root.* r,
24+
25 # non-chrooted paths
26 /etc/unbound/** r,
27 owner /etc/unbound/*.key* rw,
28@@ -32,4 +42,7 @@
29 /usr/sbin/unbound mr,
30
31 /{,var/}run/unbound.pid rw,
32+
33+ # Unix control socket
34+ /{,var/}run/unbound.ctl rw,
35 }

Subscribers

People subscribed via source and target branches

to status/vote changes: