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

[Simon Déziel]

[Seth Arnold (seth-arnold)]

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

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

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

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

[Seth Arnold (seth-arnold)]

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

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

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

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

@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

[lp:~sdeziel/apparmor-profiles/unbound-refresh]

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

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

=== modified file 'ubuntu/16.04/usr.sbin.unbound'
--- ubuntu/16.04/usr.sbin.unbound	2015-10-27 01:49:53 +0000
+++ ubuntu/16.04/usr.sbin.unbound	2016-01-12 21:30:58 +0000
@@ -6,6 +6,13 @@
   #include <abstractions/base>
   #include <abstractions/nameservice>
 
+  # needlessly chown'ing the PID, for details see:
+  # https://www.nlnetlabs.nl/bugs-script/show_bug.cgi?id=734
+  deny capability chown,
+  deny capability dac_override,
+
+  capability fowner,
+  capability fsetid,
   capability net_bind_service,
   capability setgid,
   capability setuid,
@@ -15,6 +22,9 @@
   # root trust anchor
   owner /var/lib/unbound/root.key* rw,
 
+  # root hints from dns-data-root
+  /usr/share/dns/root.* r,
+
   # non-chrooted paths
   /etc/unbound/** r,
   owner /etc/unbound/*.key* rw,
@@ -32,4 +42,7 @@
   /usr/sbin/unbound mr,
 
   /{,var/}run/unbound.pid rw,
+
+  # Unix control socket
+  /{,var/}run/unbound.ctl rw,
 }