sandbox/apparmor: don't let vendored apparmor conflict with system (#12909)

* sandbox/apparmor: don't let vendored apparmor conflict with system Don't enable the vendored apparmor if the system installed apparmor will try and load policy that would be generated by the vendored apparmor and hence may conflict with that by using newer features not supported by the system installed apparmor (LP: 2024637) Signed-off-by: Alex Murray <alex.murray@canonical.com> * apparmor: add unit testing for SystemAppArmorLoadsSnapPolicy() * tests: add test that checks regression in lp-2024637 * apparmor: only log non ENOENT errors in systemAppArmorLoadsSnapPolicy * tests: fix snapd-snap test on 14.04-18.04 This commit will skip apparmor vendor testing if /lib/apparmor/functions still references /var/lib/snapd/apparmor/. See LP:2024637 * tests: fix typo in snapd-snap test Signed-off-by: Alex Murray <alex.murray@canonical.com> * i/apparmor: allow read of /lib/apparmor/functions in snap-update-ns Snapd at startup will inspect this file now to ensure that the vendored apparmor can be used. So the snap-update-ns profile also needs to get updated as this happens during an early init(). --------- Signed-off-by: Alex Murray <alex.murray@canonical.com> Co-authored-by: Michael Vogt <mvo@ubuntu.com>

7 files changed, 115 insertions, 1 deletions

diff --git a/interfaces/apparmor/template.go b/interfaces/apparmor/template.go
index e9f3eafaa4..11e218acf3 100644
--- a/interfaces/apparmor/template.go
+++ b/interfaces/apparmor/template.go
@@ -1072,6 +1072,7 @@ profile snap-update-ns.###SNAP_INSTANCE_NAME### (attach_disconnected) {
 
 # snap checks if vendored apparmor parser should be used at startup
 /usr/lib/snapd/info r,
+ /lib/apparmor/functions r,
 
 ###SNIPPETS###
 }
diff --git a/sandbox/apparmor/apparmor.go b/sandbox/apparmor/apparmor.go
index d0932ee1d1..24d82f04ca 100644
--- a/sandbox/apparmor/apparmor.go
+++ b/sandbox/apparmor/apparmor.go
@@ -20,6 +20,7 @@
 package apparmor
 
 import (
+	"bufio"
	"bytes"
	"fmt"
	"io/ioutil"
@@ -31,6 +32,7 @@ import (
	"sync"
 
	"github.com/snapcore/snapd/dirs"
+	"github.com/snapcore/snapd/logger"
	"github.com/snapcore/snapd/osutil"
	"github.com/snapcore/snapd/snapdtool"
	"github.com/snapcore/snapd/strutil"
@@ -405,6 +407,32 @@ func probeParserFeatures() ([]string, error) {
	return features, nil
 }
 
+func systemAppArmorLoadsSnapPolicy() bool {
+	// on older Ubuntu systems the system installed apparmor may try and
+	// load snapd generated apparmor policy (LP: #2024637)
+	f, err := os.Open(filepath.Join(dirs.GlobalRootDir, "/lib/apparmor/functions"))
+	if err != nil {
+	if !os.IsNotExist(err) {
+	logger.Debugf("cannot open apparmor functions file: %v", err)
+	}
+	return false
+	}
+	defer f.Close()
+
+	scanner := bufio.NewScanner(f)
+	for scanner.Scan() {
+	line := scanner.Text()
+	if strings.Contains(line, dirs.SnapAppArmorDir) {
+	return true
+	}
+	}
+	if scanner.Err() != nil {
+	logger.Debugf("cannot scan apparmor functions file: %v", scanner.Err())
+	}
+
+	return false
+}
+
 func snapdAppArmorSupportsReexecImpl() bool {
	hostInfoDir := filepath.Join(dirs.GlobalRootDir, dirs.CoreLibExecDir)
	_, flags, err := snapdtool.SnapdVersionFromInfoFile(hostInfoDir)
@@ -422,7 +450,7 @@ func AppArmorParser() (cmd *exec.Cmd, internal bool, err error) {
	// - but only use the internal one when we know that the system
	// installed snapd-apparmor support re-exec
	if path, err := snapdtool.InternalToolPath("apparmor_parser"); err == nil {
-	if osutil.IsExecutable(path) && snapdAppArmorSupportsReexec() {
+	if osutil.IsExecutable(path) && snapdAppArmorSupportsReexec() && !systemAppArmorLoadsSnapPolicy() {
	prefix := strings.TrimSuffix(path, "apparmor_parser")
	// when using the internal apparmor_parser also use
	// its own configuration and includes etc plus
diff --git a/sandbox/apparmor/apparmor_test.go b/sandbox/apparmor/apparmor_test.go
index 038865aea3..863e966c10 100644
--- a/sandbox/apparmor/apparmor_test.go
+++ b/sandbox/apparmor/apparmor_test.go
@@ -33,6 +33,7 @@ import (
	. "gopkg.in/check.v1"
 
	"github.com/snapcore/snapd/dirs"
+	"github.com/snapcore/snapd/logger"
	"github.com/snapcore/snapd/osutil"
	"github.com/snapcore/snapd/sandbox/apparmor"
	"github.com/snapcore/snapd/snapdtool"
@@ -539,3 +540,62 @@ func (s *apparmorSuite) TestSetupConfCacheDirsWithInternalApparmor(c *C) {
	apparmor.SetupConfCacheDirs("/newdir")
	c.Check(apparmor.SnapConfineAppArmorDir, Equals, "/newdir/var/lib/snapd/apparmor/snap-confine.internal")
 }
+
+func (s *apparmorSuite) TestSystemAppArmorLoadsSnapPolicyErr(c *C) {
+	fakeroot := c.MkDir()
+	dirs.SetRootDir(fakeroot)
+	fakeApparmorFunctionsPath := filepath.Join(fakeroot, "/lib/apparmor/functions")
+	err := os.MkdirAll(filepath.Dir(fakeApparmorFunctionsPath), 0750)
+	c.Assert(err, IsNil)
+
+	os.Setenv("SNAPD_DEBUG", "1")
+	defer os.Unsetenv("SNAPD_DEBUG")
+
+	log, restore := logger.MockLogger()
+	defer restore()
+
+	// no log output on missing file
+	c.Check(apparmor.SystemAppArmorLoadsSnapPolicy(), Equals, false)
+	c.Check(log.String(), Equals, "")
+
+	// permissions are ignored as root
+	if os.Getuid() == 0 {
+	return
+	}
+	// log generated for errors
+	err = ioutil.WriteFile(fakeApparmorFunctionsPath, nil, 0100)
+	c.Assert(err, IsNil)
+	c.Check(apparmor.SystemAppArmorLoadsSnapPolicy(), Equals, false)
+	c.Check(log.String(), Matches, `(?ms).* DEBUG: cannot open apparmor functions file: open .*/lib/apparmor/functions: permission denied`)
+}
+
+func (s *apparmorSuite) TestSystemAppArmorLoadsSnapPolicy(c *C) {
+	fakeroot := c.MkDir()
+	dirs.SetRootDir(fakeroot)
+
+	// systemAppArmorLoadsSnapPolicy() will look at this path so it
+	// needs to be the real path, not a faked one
+	dirs.SnapAppArmorDir = dirs.SnapAppArmorDir[len(fakeroot):]
+
+	fakeApparmorFunctionsPath := filepath.Join(fakeroot, "/lib/apparmor/functions")
+	err := os.MkdirAll(filepath.Dir(fakeApparmorFunctionsPath), 0755)
+	c.Assert(err, IsNil)
+
+	for _, tc := range []struct {
+	apparmorFunctionsContent string
+	expectedResult bool
+	}{
+	{"", false},
+	{"unrelated content", false},
+	// 16.04
+	{`PROFILES_SNAPPY="/var/lib/snapd/apparmor/profiles"`, true},
+	// 18.04
+	{`PROFILES_VAR="/var/lib/snapd/apparmor/profiles"`, true},
+	} {
+	err := ioutil.WriteFile(fakeApparmorFunctionsPath, []byte(tc.apparmorFunctionsContent), 0644)
+	c.Assert(err, IsNil)
+
+	loadsPolicy := apparmor.SystemAppArmorLoadsSnapPolicy()
+	c.Check(loadsPolicy, Equals, tc.expectedResult, Commentf("%v", tc))
+	}
+}
diff --git a/sandbox/apparmor/export_test.go b/sandbox/apparmor/export_test.go
index 567b36218d..0027859da5 100644
--- a/sandbox/apparmor/export_test.go
+++ b/sandbox/apparmor/export_test.go
@@ -102,6 +102,7 @@ var (
	PreferredParserFeatures = preferredParserFeatures
 
	SnapdAppArmorSupportsRexecImpl = snapdAppArmorSupportsReexecImpl
+	SystemAppArmorLoadsSnapPolicy = systemAppArmorLoadsSnapPolicy
 )
 
 func FreshAppArmorAssessment() {
diff --git a/tests/core/basic18/task.yaml b/tests/core/basic18/task.yaml
index 2ba03a3e48..b898689e25 100644
--- a/tests/core/basic18/task.yaml
+++ b/tests/core/basic18/task.yaml
@@ -25,3 +25,12 @@ execute: |
 
 echo "Ensure passwd/group is available for snaps"
 test-snapd-sh-core18.sh -c 'cat /var/lib/extrausers/passwd' | MATCH test
+
+ # ensure apparmor works, see LP: 2024637
+ systemctl status apparmor.service
+ 
+ # reboot to double check that apparmor still works after the reboot
+ # (LP: 2024637)
+ if [ "$SPREAD_REBOOT" = 0 ]; then
+ REBOOT
+ fi
diff --git a/tests/core/basic20plus/task.yaml b/tests/core/basic20plus/task.yaml
index 40a297ca02..63647c39be 100644
--- a/tests/core/basic20plus/task.yaml
+++ b/tests/core/basic20plus/task.yaml
@@ -111,3 +111,12 @@ execute: |
 echo "${loop}" | MATCH "/dev/loop[0-9]+"
 losetup -O ro -n --raw "${loop}" | MATCH "1"
 done
+
+ # ensure apparmor works, see LP: 2024637
+ systemctl status apparmor.service
+
+ # reboot to double check that apparmor still works after the reboot
+ # (LP: 2024637)
+ if [ "$SPREAD_REBOOT" = 0 ]; then
+ REBOOT
+ fi
diff --git a/tests/main/snapd-snap/task.yaml b/tests/main/snapd-snap/task.yaml
index 64e2bef335..d6c88542fb 100644
--- a/tests/main/snapd-snap/task.yaml
+++ b/tests/main/snapd-snap/task.yaml
@@ -235,6 +235,12 @@ execute: |
 echo "Ensure we restarted into the snapd snap"
 "$TESTSTOOLS"/journal-state match-log 'restarting into "/snap/snapd/'
 
+ # see LP:2024637
+ if grep -q /var/lib/snapd/apparmor/ /lib/apparmor/functions; then
+ echo "SKIP: cannot test builtin apparmor parser until /lib/apparmor/functions stops loading the snapd profiles"
+ exit 0
+ fi
+
 echo "Ensure sandbox-features shows the internal apparmor_parser"
 snap debug sandbox-features --required apparmor:parser:snapd-internal