From 32a63a0326f365d0b008893c76a3371a740a79cf Mon Sep 17 00:00:00 2001 From: Zygmunt Krynicki Date: Thu, 26 Jul 2018 22:06:31 +0200 Subject: [PATCH] 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 --- interfaces/apparmor/apparmor.go | 18 +++++++++-- interfaces/apparmor/backend.go | 50 +++++++++++++++++++++-------- interfaces/apparmor/backend_test.go | 24 +++++++------- 3 files changed, 66 insertions(+), 26 deletions(-) diff --git a/interfaces/apparmor/apparmor.go b/interfaces/apparmor/apparmor.go index c4d72e03de4..b5221f9838c 100644 --- a/interfaces/apparmor/apparmor.go +++ b/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") } diff --git a/interfaces/apparmor/backend.go b/interfaces/apparmor/backend.go index 4f20500b2e6..e0bf3f3bb77 100644 --- a/interfaces/apparmor/backend.go +++ b/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) } diff --git a/interfaces/apparmor/backend_test.go b/interfaces/apparmor/backend_test.go index 0254d4e54ca..a0446d06fb9 100644 --- a/interfaces/apparmor/backend_test.go +++ b/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, }})