From d3617268cf5344e5ae713fb79b4cfacb278e330c Mon Sep 17 00:00:00 2001 From: Jamie Strandboge Date: Fri, 6 Apr 2018 21:26:54 +0000 Subject: [PATCH 1/6] interfaces/home: add 'read' attribute to allow non-owner read to @{HOME} If unspecified, 'read' defaults to 'read: owner'. When 'read: all' is specified, deny connection and auto-connection (thus requiring a manual review). References: https://forum.snapcraft.io/t/home-access-as-root-from-confined-snaps/3802 --- interfaces/builtin/home.go | 65 +++++++++++++++-- interfaces/builtin/home_test.go | 88 +++++++++++++++++++++-- interfaces/policy/basedeclaration_test.go | 52 +++++++++++++- 3 files changed, 195 insertions(+), 10 deletions(-) diff --git a/interfaces/builtin/home.go b/interfaces/builtin/home.go index dca03ea3073..066bf22f614 100644 --- a/interfaces/builtin/home.go +++ b/interfaces/builtin/home.go @@ -1,7 +1,7 @@ // -*- Mode: Go; indent-tabs-mode: t -*- /* - * Copyright (C) 2016 Canonical Ltd + * Copyright (C) 2016-2018 Canonical Ltd * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 3 as @@ -19,6 +19,13 @@ package builtin +import ( + "fmt" + "github.com/snapcore/snapd/interfaces" + "github.com/snapcore/snapd/interfaces/apparmor" + "github.com/snapcore/snapd/snap" +) + const homeSummary = `allows access to non-hidden files in the home directory` const homeBaseDeclarationSlots = ` @@ -26,11 +33,17 @@ const homeBaseDeclarationSlots = ` allow-installation: slot-snap-type: - core + deny-connection: + plug-attributes: + read: all deny-auto-connection: - on-classic: false + - + on-classic: false + - + plug-attributes: + read: all ` -// http://bazaar.launchpad.net/~ubuntu-security/ubuntu-core-security/trunk/view/head:/data/apparmor/policygroups/ubuntu-core/16.04/home const homeConnectedPlugAppArmor = ` # Description: Can access non-hidden files in user's $HOME. This is restricted # because it gives file access to all of the user's $HOME. @@ -63,8 +76,50 @@ owner /run/user/[0-9]*/gvfs/{,**} r, owner /run/user/[0-9]*/gvfs/*/** w, ` +const homeConnectedPlugAppArmorWithAllRead = ` +# Allow non-owner read to non-hidden and non-snap files and directories +@{HOME}/ r, +@{HOME}/[^s.]** r, +@{HOME}/s[^n]** r, +@{HOME}/sn[^a]** r, +@{HOME}/sna[^p]** r, +@{HOME}/snap[^/]** r, +@{HOME}/{s,sn,sna}{,/} r, +` + +type homeInterface struct { + commonInterface +} + +func (iface *homeInterface) BeforePreparePlug(plug *snap.PlugInfo) error { + // It's fine if 'read' isn't specified, but if it is, it needs to be + // either 'all' or 'owner' + if r, ok := plug.Attrs["read"]; ok { + _, ok = r.(string) + if !ok || (r != "all" && r != "owner") { + return fmt.Errorf(`home plug requires the "read" be either 'all' or 'owner'`) + } + } + + return nil +} + +func (iface *homeInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error { + var read string + _ = plug.Attr("read", &read) + // 'owner' is the standard policy + spec.AddSnippet(homeConnectedPlugAppArmor) + + // 'all' grants standard policy plus read access to home without owner + // match + if read == "all" { + spec.AddSnippet(homeConnectedPlugAppArmorWithAllRead) + } + return nil +} + func init() { - registerIface(&commonInterface{ + registerIface(&homeInterface{commonInterface{ name: "home", summary: homeSummary, implicitOnCore: true, @@ -72,5 +127,5 @@ func init() { baseDeclarationSlots: homeBaseDeclarationSlots, connectedPlugAppArmor: homeConnectedPlugAppArmor, reservedForOS: true, - }) + }}) } diff --git a/interfaces/builtin/home_test.go b/interfaces/builtin/home_test.go index a1bb476c4c6..a90802c646f 100644 --- a/interfaces/builtin/home_test.go +++ b/interfaces/builtin/home_test.go @@ -1,7 +1,7 @@ // -*- Mode: Go; indent-tabs-mode: t -*- /* - * Copyright (C) 2016 Canonical Ltd + * Copyright (C) 2016-2018 Canonical Ltd * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 3 as @@ -76,17 +76,97 @@ func (s *HomeInterfaceSuite) TestSanitizeSlot(c *C) { "home slots are reserved for the core snap") } -func (s *HomeInterfaceSuite) TestSanitizePlug(c *C) { +func (s *HomeInterfaceSuite) TestSanitizePlugNoAttrib(c *C) { c.Assert(interfaces.BeforePreparePlug(s.iface, s.plugInfo), IsNil) } -func (s *HomeInterfaceSuite) TestUsedSecuritySystems(c *C) { - // connected plugs have a non-nil security snippet for apparmor +func (s *HomeInterfaceSuite) TestSanitizePlugWithAttrib(c *C) { + const mockSnapYaml = `name: home-plug-snap +version: 1.0 +plugs: + home: + read: all +` + info := snaptest.MockInfo(c, mockSnapYaml, nil) + plug := info.Plugs["home"] + c.Assert(interfaces.BeforePreparePlug(s.iface, plug), IsNil) +} + +func (s *HomeInterfaceSuite) TestSanitizePlugWithBadAttrib(c *C) { + const mockSnapYaml = `name: home-plug-snap +version: 1.0 +plugs: + home: + read: bad +` + info := snaptest.MockInfo(c, mockSnapYaml, nil) + plug := info.Plugs["home"] + c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, + `home plug requires the "read" be either 'all' or 'owner'`) +} + +func (s *HomeInterfaceSuite) TestSanitizePlugWithEmptyAttrib(c *C) { + const mockSnapYaml = `name: home-plug-snap +version: 1.0 +plugs: + home: + read: "" +` + info := snaptest.MockInfo(c, mockSnapYaml, nil) + plug := info.Plugs["home"] + c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, + `home plug requires the "read" be either 'all' or 'owner'`) +} + +func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithoutAttrib(c *C) { apparmorSpec := &apparmor.Specification{} err := apparmorSpec.AddConnectedPlug(s.iface, s.plug, s.slot) c.Assert(err, IsNil) c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.other.app"}) c.Check(apparmorSpec.SnippetForTag("snap.other.app"), testutil.Contains, `owner @{HOME}/ r,`) + c.Check(apparmorSpec.SnippetForTag("snap.other.app"), Not(testutil.Contains), `# Allow non-owner read`) +} + +func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithAttribOwner(c *C) { + const mockSnapYaml = `name: home-plug-snap +version: 1.0 +plugs: + home: + read: owner +apps: + app2: + command: foo +` + info := snaptest.MockInfo(c, mockSnapYaml, nil) + plug := interfaces.NewConnectedPlug(info.Plugs["home"], nil) + + apparmorSpec := &apparmor.Specification{} + err := apparmorSpec.AddConnectedPlug(s.iface, plug, s.slot) + c.Assert(err, IsNil) + c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.home-plug-snap.app2"}) + c.Check(apparmorSpec.SnippetForTag("snap.home-plug-snap.app2"), testutil.Contains, `owner @{HOME}/ r,`) + c.Check(apparmorSpec.SnippetForTag("snap.home-plug-snap.app2"), Not(testutil.Contains), `# Allow non-owner read`) +} + +func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithAttribAll(c *C) { + const mockSnapYaml = `name: home-plug-snap +version: 1.0 +plugs: + home: + read: all +apps: + app2: + command: foo +` + info := snaptest.MockInfo(c, mockSnapYaml, nil) + plug := interfaces.NewConnectedPlug(info.Plugs["home"], nil) + + apparmorSpec := &apparmor.Specification{} + err := apparmorSpec.AddConnectedPlug(s.iface, plug, s.slot) + c.Assert(err, IsNil) + c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.home-plug-snap.app2"}) + c.Check(apparmorSpec.SnippetForTag("snap.home-plug-snap.app2"), testutil.Contains, `owner @{HOME}/ r,`) + c.Check(apparmorSpec.SnippetForTag("snap.home-plug-snap.app2"), testutil.Contains, `# Allow non-owner read`) } func (s *HomeInterfaceSuite) TestInterfaces(c *C) { diff --git a/interfaces/policy/basedeclaration_test.go b/interfaces/policy/basedeclaration_test.go index 9bb4b5c4281..4d817443288 100644 --- a/interfaces/policy/basedeclaration_test.go +++ b/interfaces/policy/basedeclaration_test.go @@ -1,7 +1,7 @@ // -*- Mode: Go; indent-tabs-mode: t -*- /* - * Copyright (C) 2016 Canonical Ltd + * Copyright (C) 2016-2018 Canonical Ltd * * This program is free software: you can redistribute it and/or modify * it under the terms of the GNU General Public License version 3 as @@ -221,6 +221,56 @@ func (s *baseDeclSuite) TestInterimAutoConnectionHome(c *C) { c.Check(err, ErrorMatches, `auto-connection denied by slot rule of interface \"home\"`) } +func (s *baseDeclSuite) TestHomeReadAll(c *C) { + const plugYaml = `name: plug-snap +version: 0 +plugs: + home: + read: all +` + restore := release.MockOnClassic(true) + defer restore() + cand := s.connectCand(c, "home", "", plugYaml) + err := cand.Check() + c.Check(err, NotNil) + + err = cand.CheckAutoConnect() + c.Check(err, NotNil) + + release.OnClassic = false + err = cand.Check() + c.Check(err, NotNil) + + err = cand.CheckAutoConnect() + c.Check(err, NotNil) +} + +func (s *baseDeclSuite) TestHomeReadOwner(c *C) { + const plugYaml = `name: plug-snap +version: 0 +plugs: + home: + read: owner +` + restore := release.MockOnClassic(true) + defer restore() + cand := s.connectCand(c, "home", "", plugYaml) + err := cand.Check() + c.Check(err, IsNil) + + // Same as TestInterimAutoConnectionHome() + err = cand.CheckAutoConnect() + c.Check(err, IsNil) + + release.OnClassic = false + err = cand.Check() + c.Check(err, IsNil) + + // Same as TestInterimAutoConnectionHome() + err = cand.CheckAutoConnect() + c.Check(err, NotNil) +} + func (s *baseDeclSuite) TestAutoConnectionSnapdControl(c *C) { cand := s.connectCand(c, "snapd-control", "", "") err := cand.CheckAutoConnect() From ca0c514b39d3da654b80873c328502045765229a Mon Sep 17 00:00:00 2001 From: Jamie Strandboge Date: Thu, 17 May 2018 19:51:53 +0000 Subject: [PATCH 2/6] fix typo in error message --- interfaces/builtin/home.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/interfaces/builtin/home.go b/interfaces/builtin/home.go index 066bf22f614..6f2300232dd 100644 --- a/interfaces/builtin/home.go +++ b/interfaces/builtin/home.go @@ -97,7 +97,7 @@ func (iface *homeInterface) BeforePreparePlug(plug *snap.PlugInfo) error { if r, ok := plug.Attrs["read"]; ok { _, ok = r.(string) if !ok || (r != "all" && r != "owner") { - return fmt.Errorf(`home plug requires the "read" be either 'all' or 'owner'`) + return fmt.Errorf(`home plug requires "read" be either 'all' or 'owner'`) } } From 6f6022aef8b9dbe5f90c1f88e4ce8b9fe410bb4c Mon Sep 17 00:00:00 2001 From: Jamie Strandboge Date: Thu, 17 May 2018 21:19:08 +0000 Subject: [PATCH 3/6] adjust testsuite for last commit --- interfaces/builtin/home_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/interfaces/builtin/home_test.go b/interfaces/builtin/home_test.go index a90802c646f..62e6c99002a 100644 --- a/interfaces/builtin/home_test.go +++ b/interfaces/builtin/home_test.go @@ -102,7 +102,7 @@ plugs: info := snaptest.MockInfo(c, mockSnapYaml, nil) plug := info.Plugs["home"] c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, - `home plug requires the "read" be either 'all' or 'owner'`) + `home plug requires "read" be either 'all' or 'owner'`) } func (s *HomeInterfaceSuite) TestSanitizePlugWithEmptyAttrib(c *C) { @@ -115,7 +115,7 @@ plugs: info := snaptest.MockInfo(c, mockSnapYaml, nil) plug := info.Plugs["home"] c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, - `home plug requires the "read" be either 'all' or 'owner'`) + `home plug requires "read" be either 'all' or 'owner'`) } func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithoutAttrib(c *C) { From 5237bea828d5e82bd5085fd14424d7a70c874b31 Mon Sep 17 00:00:00 2001 From: Jamie Strandboge Date: Wed, 23 May 2018 13:31:42 +0000 Subject: [PATCH 4/6] only support 'read: all' and not 'read: owner' --- interfaces/builtin/home.go | 6 +++--- interfaces/builtin/home_test.go | 32 ++++++++++++-------------------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/interfaces/builtin/home.go b/interfaces/builtin/home.go index 6f2300232dd..a286d58e7ee 100644 --- a/interfaces/builtin/home.go +++ b/interfaces/builtin/home.go @@ -93,11 +93,11 @@ type homeInterface struct { func (iface *homeInterface) BeforePreparePlug(plug *snap.PlugInfo) error { // It's fine if 'read' isn't specified, but if it is, it needs to be - // either 'all' or 'owner' + // 'all' if r, ok := plug.Attrs["read"]; ok { _, ok = r.(string) - if !ok || (r != "all" && r != "owner") { - return fmt.Errorf(`home plug requires "read" be either 'all' or 'owner'`) + if !ok || r != "all" { + return fmt.Errorf(`home plug requires "read" be 'all'`) } } diff --git a/interfaces/builtin/home_test.go b/interfaces/builtin/home_test.go index 62e6c99002a..03fe08c5a5f 100644 --- a/interfaces/builtin/home_test.go +++ b/interfaces/builtin/home_test.go @@ -102,7 +102,7 @@ plugs: info := snaptest.MockInfo(c, mockSnapYaml, nil) plug := info.Plugs["home"] c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, - `home plug requires "read" be either 'all' or 'owner'`) + `home plug requires "read" be 'all'`) } func (s *HomeInterfaceSuite) TestSanitizePlugWithEmptyAttrib(c *C) { @@ -115,37 +115,29 @@ plugs: info := snaptest.MockInfo(c, mockSnapYaml, nil) plug := info.Plugs["home"] c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, - `home plug requires "read" be either 'all' or 'owner'`) + `home plug requires "read" be 'all'`) } -func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithoutAttrib(c *C) { - apparmorSpec := &apparmor.Specification{} - err := apparmorSpec.AddConnectedPlug(s.iface, s.plug, s.slot) - c.Assert(err, IsNil) - c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.other.app"}) - c.Check(apparmorSpec.SnippetForTag("snap.other.app"), testutil.Contains, `owner @{HOME}/ r,`) - c.Check(apparmorSpec.SnippetForTag("snap.other.app"), Not(testutil.Contains), `# Allow non-owner read`) -} - -func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithAttribOwner(c *C) { +func (s *HomeInterfaceSuite) TestSanitizePlugWithBadAttribOwner(c *C) { const mockSnapYaml = `name: home-plug-snap version: 1.0 plugs: home: read: owner -apps: - app2: - command: foo ` info := snaptest.MockInfo(c, mockSnapYaml, nil) - plug := interfaces.NewConnectedPlug(info.Plugs["home"], nil) + plug := info.Plugs["home"] + c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, + `home plug requires "read" be 'all'`) +} +func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithoutAttrib(c *C) { apparmorSpec := &apparmor.Specification{} - err := apparmorSpec.AddConnectedPlug(s.iface, plug, s.slot) + err := apparmorSpec.AddConnectedPlug(s.iface, s.plug, s.slot) c.Assert(err, IsNil) - c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.home-plug-snap.app2"}) - c.Check(apparmorSpec.SnippetForTag("snap.home-plug-snap.app2"), testutil.Contains, `owner @{HOME}/ r,`) - c.Check(apparmorSpec.SnippetForTag("snap.home-plug-snap.app2"), Not(testutil.Contains), `# Allow non-owner read`) + c.Assert(apparmorSpec.SecurityTags(), DeepEquals, []string{"snap.other.app"}) + c.Check(apparmorSpec.SnippetForTag("snap.other.app"), testutil.Contains, `owner @{HOME}/ r,`) + c.Check(apparmorSpec.SnippetForTag("snap.other.app"), Not(testutil.Contains), `# Allow non-owner read`) } func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithAttribAll(c *C) { From a958603637f3f8291ecbad1fdeb8a1c904b9630f Mon Sep 17 00:00:00 2001 From: Jamie Strandboge Date: Wed, 23 May 2018 13:37:27 +0000 Subject: [PATCH 5/6] simplify attribute validation check. Thanks to niemeyer --- interfaces/builtin/home.go | 7 ++----- interfaces/builtin/home_test.go | 15 +++++++++++++++ 2 files changed, 17 insertions(+), 5 deletions(-) diff --git a/interfaces/builtin/home.go b/interfaces/builtin/home.go index a286d58e7ee..2f5742a4092 100644 --- a/interfaces/builtin/home.go +++ b/interfaces/builtin/home.go @@ -94,11 +94,8 @@ type homeInterface struct { func (iface *homeInterface) BeforePreparePlug(plug *snap.PlugInfo) error { // It's fine if 'read' isn't specified, but if it is, it needs to be // 'all' - if r, ok := plug.Attrs["read"]; ok { - _, ok = r.(string) - if !ok || r != "all" { - return fmt.Errorf(`home plug requires "read" be 'all'`) - } + if r, ok := plug.Attrs["read"]; ok && r != "all" { + return fmt.Errorf(`home plug requires "read" be 'all'`) } return nil diff --git a/interfaces/builtin/home_test.go b/interfaces/builtin/home_test.go index 03fe08c5a5f..b3bfd4bf063 100644 --- a/interfaces/builtin/home_test.go +++ b/interfaces/builtin/home_test.go @@ -131,6 +131,21 @@ plugs: `home plug requires "read" be 'all'`) } +func (s *HomeInterfaceSuite) TestSanitizePlugWithBadAttribDict(c *C) { + const mockSnapYaml = `name: home-plug-snap +version: 1.0 +plugs: + home: + read: + all: bad + bad: all +` + info := snaptest.MockInfo(c, mockSnapYaml, nil) + plug := info.Plugs["home"] + c.Assert(interfaces.BeforePreparePlug(s.iface, plug), ErrorMatches, + `home plug requires "read" be 'all'`) +} + func (s *HomeInterfaceSuite) TestConnectedPlugAppArmorWithoutAttrib(c *C) { apparmorSpec := &apparmor.Specification{} err := apparmorSpec.AddConnectedPlug(s.iface, s.plug, s.slot) From bf86a342df9dc8fc6a3c9c2edd14bb65d0d8dbf1 Mon Sep 17 00:00:00 2001 From: Jamie Strandboge Date: Wed, 23 May 2018 13:49:28 +0000 Subject: [PATCH 6/6] also adjust basedeclaration_test.go fr 'owner' removal --- interfaces/policy/basedeclaration_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/interfaces/policy/basedeclaration_test.go b/interfaces/policy/basedeclaration_test.go index 813e627228e..11d91342fd7 100644 --- a/interfaces/policy/basedeclaration_test.go +++ b/interfaces/policy/basedeclaration_test.go @@ -246,12 +246,11 @@ plugs: c.Check(err, NotNil) } -func (s *baseDeclSuite) TestHomeReadOwner(c *C) { +func (s *baseDeclSuite) TestHomeReadDefault(c *C) { const plugYaml = `name: plug-snap version: 0 plugs: - home: - read: owner + home: null ` restore := release.MockOnClassic(true) defer restore()