interfaces/builtin/account_control: use gid owning /etc/shadow to setup seccomp rules

[bboozzoo]

The /etc/shadow file is owned by user root across all supported distributions.
However, the group owning that file is either 'shadow' or 'root' (Arch). Drop
the group filter to avoid the need for detecting the right group at runtime.

This is the first part of changes related to weeding out cgo from snapd proper. @niemeyer @zyga @mvo5 want to take a look?

[zyga]

This will fail, you need to put - in place of the removed filter.

[mvo5]

👍 Especially since it means we can drop the last bits of cgo code when we don't do group lookups!

[codecov-io]

Codecov Report

Merging #4185 into master will decrease coverage by <.01%.
The diff coverage is 75%.

@@            Coverage Diff             @@
##           master    #4185      +/-   ##
==========================================
- Coverage   75.87%   75.87%   -0.01%     
==========================================
  Files         440      440              
  Lines       38273    38293      +20     
==========================================
+ Hits        29039    29053      +14     
- Misses       7220     7224       +4     
- Partials     2014     2016       +2

Impacted Files	Coverage Δ
interfaces/builtin/account_control.go	80.64% <75%> (-19.36%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 264f349...2398537. Read the comment docs.

[jdstrand]

NAK. While this patch is super clean, it also means that the account-control interface is now allowed to change any file it has write access to an arbitrary group. This is wrong and we shouldn't weaken policy because we don't want to perform a gid lookup.

I don't understand why the facility needs to be removed-- it is useful and we may in the future want to be able to perform other uid and gid lookups from within snapd (that's why I added it in the first place). I always envisioned this section of code would either have a mapping that snapd would maintain (if Debian/Ubuntu use g:shadow, if Arch use g:root, if Solus use...) or we would look up the file's group like you did in the first attempt at fixing this. The fact that Go doesn't offer what we need by default and the apparent strong desire to move away from cgo further supports my historical reservations for moving to bpf caching within Go in the first place and Go's suitability for lowlevel OS operations. :\

If cgo is so terrible that it must be removed, instead of weakening the policy, how about shelling out to getent group <gid> or writing a small C helper that snap-seccomp can shell out to for the operations not yet supported in Go. Specifically, write cmd/snap-lookup-helper that can perform getgrnam() and adjust snapd to detect if user.LookupGroup is available, and use snap-lookup-helper if it isn't. You could then drop all the cgo code around LookupGroup. Then to properly fix this PR, lookup the file's group and add getgrgid() to snap-lookup-helper (heck, you don't even need getgrgid() to do this: simply lookup the group of /etc/shadow (which gives you the gid) and make the rule simple by fchown - u:root <gid> (as opposed to fchown - u:root g:<group> like you had before)).

Shelling out is an extra exec of course, but it is only going to happen when the bpf cache is regenerated (ie, on security profile changes). If we make it conditional on Go's support of LookupGroup, it will also go away in the fullness of time.

[jdstrand]

To be clear, since g:<group> is only used by account-control and the default template, you could get rid of all the cgo code by changing the default template to use chown - u:root 0 and account-control to do a stat() on /etc/shadow to get the gid, and then using fchown - u:root <gid>. I would prefer that we continue to support g:<group> for when we need it (by using the shell out method I mentioned when user.LookupGroup is not available).

[bboozzoo]

I've updated the account control interface setup to poke /etc/shadow, and use owning gid to setup seccomp.

[jdstrand]

Thank you for making this change. With this, any cgo changes/removals can happen in a subsequent PR.

[jdstrand]

Can you update this to be:

# useradd requires chowning to 0:'{{group}}'

[bboozzoo]

Updated, thanks.

[jdstrand]

I think you don't need to change init in this manner. Can't you simply:

-		connectedPlugSecComp:  accountControlConnectedPlugSecComp,
+		connectedPlugSecComp:  getSecCompSnippet(),

(or similar)

[bboozzoo]

In theory stat() may fail and there's no other way to return errors in init() other than panicking

[zyga]

Not a strong opinion but I think we could just use init and handle the error internally.

[bboozzoo]

There's a silly mistake right here: https://github.com/snapcore/snapd/pull/4185/files#diff-3bc8ebdf9693ccc7718f010fe1d991a6R91

The relevant unit test failed as expected:

account_control_test.go:108:
    c.Check(seccompSpec.SnippetForTag("snap.other.app2"), testutil.Contains,
        fmt.Sprintf("\nfchown - u:root %v\n", group))
... container string = "" +
...     "\n" +
...     "# useradd requires chowning to 0:'2a'\n" +
...     "fchown - u:root 2a\n" +
...     "fchown32 - u:root 2a\n" +
...     "\n" +
...     "# from libaudit1\n" +
...     "bind\n" +
...     "socket AF_NETLINK - NETLINK_AUDIT\n" +
...     "\n"
... elem string = "" +
...     "\n" +
...     "fchown - u:root 42\n"

Interestingly /etc/shadow is owned by 0:42 in Ubuntu.

[bboozzoo]

Travis build failed with:

2017-11-10 13:43:40 Failed project prepare: 1
    - linode:fedora-25-64:project

++ dnf -q -y --refresh install 'pkgconfig(udev)'
Error: Failed to synchronize cache for repo 'updates'

502 on fedora mirrors again?

[zyga]

The fedora mirror situation is not changed. We believe that it fails to sync while the mirror itself is being updated.

[zyga]

Some comments for your consideration, otherwise +1

[zyga]

I'd just put this into SecCompConnectedPlug

[zyga]

You can omit the else { } as the if part returns.

[bboozzoo]

Another cherry pick, did it once already in #4135 :)

[jdstrand]

I think you don't need to change init in this manner. Can't you simply:

-		connectedPlugSecComp:  accountControlConnectedPlugSecComp,
+		connectedPlugSecComp:  getSecCompSnippet(),

(or similar)

[bboozzoo]

In theory stat() may fail and there's no other way to return errors in init() other than panicking

[zyga]

Not a strong opinion but I think we could just use init and handle the error internally.

[bboozzoo]

Not a strong opinion but I think we could just use init and handle the error internally.

Don't know why, but I cannot respond under the comment. In order to avoid a stat() on each call to SecCompConnectedPlug() I could cache the result on first successful call to makeAccountControlSecCompSnippet().

Something like this:

diff --git a/interfaces/builtin/account_control.go b/interfaces/builtin/account_control.go
index 0516acbe0..affdbb128 100644
--- a/interfaces/builtin/account_control.go
+++ b/interfaces/builtin/account_control.go
@@ -79,6 +79,7 @@ socket AF_NETLINK - NETLINK_AUDIT

 type accountControlInterface struct {
        commonInterface
+       secCompSnippet string
 }

 func makeAccountControlSecCompSnippet() (string, error) {
@@ -94,11 +95,14 @@ func makeAccountControlSecCompSnippet() (string, error) {
 }

 func (iface *accountControlInterface) SecCompConnectedPlug(spec *seccomp.Specification, plug *interfaces.Plug, Attrs map[string]interface{}, slot *interfaces.Slot, slotAttrs map[string]interface{}) error {
-       snippet, err := makeAccountControlSecCompSnippet()
-       if err != nil {
-               return err
+       if iface.secCompSnippet == "" {
+               snippet, err := makeAccountControlSecCompSnippet()
+               if err != nil {
+                       return err
+               }
+               iface.secCompSnippet = snippet
        }
-       spec.AddSnippet(snippet)
+       spec.AddSnippet(iface.secCompSnippet)
        return nil
 }

Edit: I pushed this as a commit here: bboozzoo/snapd@f94bfe9 in case anyone wants to take a look.

[zyga]

LGTM with two remarks.

[zyga]

Can you please add a comment like this:
// Cache the snippet after it is computed once.

[zyga]

If you put this on the preceding line does go fmt let it through? If so this could be a nice way to address common/custom interface transitions without big deltas.

[bboozzoo]

Is this one good to go?