Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

interfaces/home: add 'read' attribute to allow non-owner read to @{HOME} #5016

Merged
merged 8 commits into from May 23, 2018
Merged

interfaces/home: add 'read' attribute to allow non-owner read to @{HOME} #5016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d361726
interfaces/home: add 'read' attribute to allow non-owner read to @{HOME}
Apr 6, 2018
f828ad9
Merge remote-tracking branch 'upstream/master' into home-nonowner-read
May 17, 2018
ca0c514
fix typo in error message
May 17, 2018
6f6022a
adjust testsuite for last commit
May 17, 2018
bf65b10
Merge remote-tracking branch 'upstream/master' into home-nonowner-read
May 23, 2018
5237bea
only support 'read: all' and not 'read: owner'
May 23, 2018
a958603
simplify attribute validation check. Thanks to niemeyer
May 23, 2018
bf86a34
also adjust basedeclaration_test.go fr 'owner' removal
May 23, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 57 additions & 5 deletions interfaces/builtin/home.go
View file
Open in desktop
@@ -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
Expand All @@ -19,18 +19,31 @@

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 = `
home:
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.
Expand Down Expand Up @@ -63,14 +76,53 @@ 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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this documented anywhere public? The home interface is probably the most used interface we have.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not yet-- this is new :) I will update the documentation for it, yes.

@{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
// 'all'
if r, ok := plug.Attrs["read"]; ok && r != "all" {
return fmt.Errorf(`home plug requires "read" be 'all'`)
}

return nil
}

func (iface *homeInterface) AppArmorConnectedPlug(spec *apparmor.Specification, plug *interfaces.ConnectedPlug, slot *interfaces.ConnectedSlot) error {
var read string
_ = plug.Attr("read", &read)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we use plug.Attr in the BeforePreparePlug above?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm doing it this way in BeforePreparePlug because I want to see if it exists at all (since it is ok if it doesn't) and if it does make sure it is a string. I could just ignore if it isn't a string, but this isn't what we do elsewhere (eg browser-support, content and docker-support). The reason I can use plug.Attr() in AppArmorConnectedPlug is because by the time it is run, I'm sure things are ok and if read isn't an attribute, then the 'read' variable is simply empty and won't match read != "all". As such, I prefer to leave the logic as is. If people feel strongly about changing it, we should change it in all of the interfaces that do similar checks.

// '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{
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must say I'm really happy we're doing this. It makes to-and-from common vs custom interface transitions so much easier than before.

name: "home",
summary: homeSummary,
implicitOnCore: true,
implicitOnClassic: true,
baseDeclarationSlots: homeBaseDeclarationSlots,
connectedPlugAppArmor: homeConnectedPlugAppArmor,
reservedForOS: true,
})
}})
}
95 changes: 91 additions & 4 deletions interfaces/builtin/home_test.go
View file
Open in desktop
@@ -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
Expand Down Expand Up @@ -76,17 +76,104 @@ 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 "read" be 'all'`)
}

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 "read" be 'all'`)
}

func (s *HomeInterfaceSuite) TestSanitizePlugWithBadAttribOwner(c *C) {
const mockSnapYaml = `name: home-plug-snap
version: 1.0
plugs:
home:
read: owner
`
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) 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)
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) 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) {
Expand Down
51 changes: 50 additions & 1 deletion interfaces/policy/basedeclaration_test.go
View file
Open in desktop
@@ -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
Expand Down Expand Up @@ -222,6 +222,55 @@ 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) TestHomeReadDefault(c *C) {
const plugYaml = `name: plug-snap
version: 0
plugs:
home: null
`
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()
Expand Down