interfaces/apparmor: use the cache in mtime-resilient way

This patch changes how we invoke apparmor_parser (along with the set of
options we pass for cache control). In the past we would just ask
apparmor to parse, compile, load into the kernel and write the cache,
for any profiles (changed or unchanged) we know about, for a given snap.

This was a safe default, we delegate the task of making this fast to
apparmor_parser and just ask it to load _all_ of the profiles, period.

On devices like the Raspberry Pi, that don't have a battery backed
real-time clock, we ran into an issue where on early boot, before NTP
had a chance to correct it, the time was essentially stuck in some form
of 2016. Here all the source profiles were correct (after being
re-written by snapd on system key change in the early boot), the cache
was however from the future (since the device wrote the cache on prior
boot when it was NTP-synced into 2018).

When the cache is from the future it is used, regardless of the contents
of the source files. This resulted in apparmor profiles from the
previous boot (and old system key) to apply to the freshly booted
system, with catastrophic effects.

While we wait for apparmor to improve its caching in apparmor 2.13 and
beyond we can do a simple workaround. Whenever we detect that an
apparmor profile has _really_ changed on disk (and this is simple thanks
to the ensure-dir-state approach that we use) we call apparmor_parser
with an extra command line argument, --skip-cache-read, that totally
ignores the cache (and its perhaps-futuristic mtime), parsers, compiles,
load the profile and _writes a new cache_

This way, while our booting device may think it is 2016, it will at
least generate and _load_ the updated security profiles correctly.

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>

interfaces/apparmor/apparmor.go

@@ -42,7 +42,7 @@ import (
 // If no such profile was previously loaded then it is simply added to the kernel.
 // If there was a profile with the same name before, that profile is replaced.
 func LoadProfile(fname string) error {
-	return loadProfile(fname, dirs.AppArmorCacheDir)
+	return loadProfile(fname, dirs.AppArmorCacheDir, 0)
 }
 
 // UnloadProfile removes the named profile from the running kernel.
@@ -53,9 +53,23 @@ func UnloadProfile(name string) error {
 	return unloadProfile(name, dirs.AppArmorCacheDir)
 }
 
-func loadProfile(fname, cacheDir string) error {
+type aaParserFlags int
+
+const (
+	// skipReadCache causes apparmor_parser to be invoked with --skip-read-cache.
+	// This allows us to essentially overwrite a cache that we know is stale regardless
+	// of the time and date settings (apparmor_parser caching is based on mtime).
+	// Note that writing of the cache relies on --write-cache but we pass that
+	// command-line option unconditionally.
+	skipReadCache aaParserFlags = 1 << iota
+)
+
+func loadProfile(fname, cacheDir string, flags aaParserFlags) error {
 	// Use no-expr-simplify since expr-simplify is actually slower on armhf (LP: #1383858)
 	args := []string{"--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s", cacheDir)}
+	if flags&skipReadCache != 0 {
+		args = append(args, "--skip-read-cache")
+	}
 	if !osutil.GetenvBool("SNAPD_DEBUG") {
 		args = append(args, "--quiet")
 	}

interfaces/apparmor/backend.go

@@ -166,7 +166,7 @@ func (b *Backend) Initialize() error {
 	}
 
 	// We are not using apparmor.LoadProfile() because it uses other cache.
-	if err := loadProfile(profilePath, dirs.SystemApparmorCacheDir); err != nil {
+	if err := loadProfile(profilePath, dirs.SystemApparmorCacheDir, skipReadCache); err != nil {
 		// When we cannot reload the profile then let's remove the generated
 		// policy. Maybe we have caused the problem so it's better to let other
 		// things work.
@@ -342,22 +342,36 @@ func (b *Backend) Setup(snapInfo *snap.Info, opts interfaces.ConfinementOptions,
 	if err := os.MkdirAll(dir, 0755); err != nil {
 		return fmt.Errorf("cannot create directory for apparmor profiles %q: %s", dir, err)
 	}
-	_, removed, errEnsure := osutil.EnsureDirStateGlobs(dir, globs, content)
-	// NOTE: load all profiles instead of just the changed profiles.  We're
-	// relying on apparmor cache to make this efficient. This gives us
-	// certainty that each call to Setup ends up with working profiles.
-	all := make([]string, 0, len(content))
+	changed, removed, errEnsure := osutil.EnsureDirStateGlobs(dir, globs, content)
+	// Find the set of unchanged profiles.
+	unchanged := make([]string, 0, len(content)-len(changed))
 	for name := range content {
-		all = append(all, name)
-	}
-	sort.Strings(all)
-	errReload := reloadProfiles(all, dir, cache)
+		// changed is pre-sorted by EnsureDirStateGlobs
+		x := sort.SearchStrings(changed, name)
+		if x < len(changed) && changed[x] == name {
+			continue
+		}
+		unchanged = append(unchanged, name)
+	}
+	sort.Strings(unchanged)
+	// Load all changed profiles with a flag that asks apparmor to skip reading
+	// the cache (since we know those changed for sure).  This allows us to
+	// work despite time being wrong (e.g. in the past). For more details see
+	// https://forum.snapcraft.io/t/apparmor-profile-caching/1268/18
+	errReloadChanged := reloadChangedProfiles(changed, dir, cache)
+	// Load all unchanged profiles anyway. This ensures those are correct in
+	// the kernel even if the files on disk were not changed. We rely on
+	// apparmor cache to make this performant.
+	errReloadOther := reloadProfiles(unchanged, dir, cache)
 	errUnload := unloadProfiles(removed, cache)
 	if errEnsure != nil {
 		return fmt.Errorf("cannot synchronize security files for snap %q: %s", snapName, errEnsure)
 	}
-	if errReload != nil {
-		return errReload
+	if errReloadChanged != nil {
+		return errReloadChanged
+	}
+	if errReloadOther != nil {
+		return errReloadOther
 	}
 	return errUnload
 }
@@ -512,7 +526,17 @@ func addContent(securityTag string, snapInfo *snap.Info, opts interfaces.Confine
 
 func reloadProfiles(profiles []string, profileDir, cacheDir string) error {
 	for _, profile := range profiles {
-		err := loadProfile(filepath.Join(profileDir, profile), cacheDir)
+		err := loadProfile(filepath.Join(profileDir, profile), cacheDir, 0)
+		if err != nil {
+			return fmt.Errorf("cannot load apparmor profile %q: %s", profile, err)
+		}
+	}
+	return nil
+}
+
+func reloadChangedProfiles(profiles []string, profileDir, cacheDir string) error {
+	for _, profile := range profiles {
+		err := loadProfile(filepath.Join(profileDir, profile), cacheDir, skipReadCache)
 		if err != nil {
 			return fmt.Errorf("cannot load apparmor profile %q: %s", profile, err)
 		}

interfaces/apparmor/backend_test.go

@@ -122,8 +122,8 @@ func (s *backendSuite) TestInstallingSnapWritesAndLoadsProfiles(c *C) {
 	c.Check(err, IsNil)
 	// apparmor_parser was used to load that file
 	c.Check(s.parserCmd.Calls(), DeepEquals, [][]string{
-		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", updateNSProfile},
-		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", profile},
+		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", updateNSProfile},
+		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", profile},
 	})
 }
 
@@ -137,8 +137,8 @@ func (s *backendSuite) TestInstallingSnapWithHookWritesAndLoadsProfiles(c *C) {
 	c.Check(err, IsNil)
 	// apparmor_parser was used to load that file
 	c.Check(s.parserCmd.Calls(), DeepEquals, [][]string{
-		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", updateNSProfile},
-		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", profile},
+		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", updateNSProfile},
+		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", profile},
 	})
 }
 
@@ -164,8 +164,8 @@ func (s *backendSuite) TestInstallingSnapWithLayoutWritesAndLoadsProfiles(c *C)
 	// TODO: check for layout snippets inside the generated file once we have some snippets to check for.
 	// apparmor_parser was used to load them
 	c.Check(s.parserCmd.Calls(), DeepEquals, [][]string{
-		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", updateNSProfile},
-		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", appProfile},
+		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", updateNSProfile},
+		{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", appProfile},
 	})
 }
 
@@ -250,8 +250,8 @@ func (s *backendSuite) TestUpdatingSnapMakesNeccesaryChanges(c *C) {
 		// apparmor_parser was used to reload the profile because snap revision
 		// is inside the generated policy.
 		c.Check(s.parserCmd.Calls(), DeepEquals, [][]string{
+			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", profile},
 			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", updateNSProfile},
-			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", profile},
 		})
 		s.RemoveSnap(c, snapInfo)
 	}
@@ -269,10 +269,10 @@ func (s *backendSuite) TestUpdatingSnapToOneWithMoreApps(c *C) {
 		// file called "snap.sambda.nmbd" was created
 		_, err := os.Stat(nmbdProfile)
 		c.Check(err, IsNil)
-		// apparmor_parser was used to load the both profiles
+		// apparmor_parser was used to load all the profiles, the nmbd profile is new so we force invalidate its cache (if any).
 		c.Check(s.parserCmd.Calls(), DeepEquals, [][]string{
+			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", nmbdProfile},
 			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", updateNSProfile},
-			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", nmbdProfile},
 			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", smbdProfile},
 		})
 		s.RemoveSnap(c, snapInfo)
@@ -293,10 +293,10 @@ func (s *backendSuite) TestUpdatingSnapToOneWithMoreHooks(c *C) {
 		// Verify that profile "snap.samba.hook.configure" was created
 		_, err := os.Stat(hookProfile)
 		c.Check(err, IsNil)
-		// apparmor_parser was used to load all the profiles
+		// apparmor_parser was used to load all the profiles, the hook profile has changed so we force invalidate its cache.
 		c.Check(s.parserCmd.Calls(), DeepEquals, [][]string{
+			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--skip-read-cache", "--quiet", hookProfile},
 			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", updateNSProfile},
-			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", hookProfile},
 			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", nmbdProfile},
 			{"apparmor_parser", "--replace", "--write-cache", "-O", "no-expr-simplify", fmt.Sprintf("--cache-loc=%s/var/cache/apparmor", s.RootDir), "--quiet", smbdProfile},
 		})
@@ -867,6 +867,7 @@ func (s *backendSuite) testSetupSnapConfineGeneratedPolicyWithNFS(c *C, profileF
 		"--write-cache",
 		"-O", "no-expr-simplify",
 		"--cache-loc=" + dirs.SystemApparmorCacheDir,
+		"--skip-read-cache",
 		"--quiet",
 		profilePath,
 	}})
@@ -1162,6 +1163,7 @@ func (s *backendSuite) testSetupSnapConfineGeneratedPolicyWithOverlay(c *C, prof
 		"--write-cache",
 		"-O", "no-expr-simplify",
 		"--cache-loc=" + dirs.SystemApparmorCacheDir,
+		"--skip-read-cache",
 		"--quiet",
 		profilePath,
 	}})