From c8ddb530156a15472c17208166ee4b9a7d602267 Mon Sep 17 00:00:00 2001 From: Katie May <63071677+maykathm@users.noreply.github.com> Date: Wed, 8 Jan 2025 07:55:56 +0100 Subject: [PATCH 1/9] interfaces/seccomp: added new at-variant syscalls to base template (#14902) --- interfaces/seccomp/template.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/interfaces/seccomp/template.go b/interfaces/seccomp/template.go index 8a1c288dfae..a26576de361 100644 --- a/interfaces/seccomp/template.go +++ b/interfaces/seccomp/template.go @@ -188,6 +188,7 @@ getuid32 getxattr fgetxattr lgetxattr +getxattrat inotify_add_watch inotify_init @@ -234,6 +235,7 @@ linkat listxattr llistxattr flistxattr +listxattrat lseek llseek @@ -336,6 +338,7 @@ remap_file_pages removexattr fremovexattr lremovexattr +removexattrat rename renameat @@ -428,6 +431,7 @@ set_tid_address setxattr fsetxattr lsetxattr +setxattrat shmat shmctl From 498b2a1afa278207016191b22686203498188b51 Mon Sep 17 00:00:00 2001 From: alfonsosanchezbeato Date: Mon, 13 Jan 2025 16:55:56 -0500 Subject: [PATCH 2/9] interfaces,overlord: allow access to kernel snap in profiles (#14899) * interfaces: allow access to kernel snap in profiles In UC24+ the kernel modules and firmware is accessed from symlinks in /lib/{modules,firmware} that can point to different locations. Make sure that the files can be read by any snap, as there are actions that can trigger the automatic loading of modules or firmware from the kernel. This is also needed so the kernel-modules-control can actually work as expected. * overlord: add kernel snap confinement option This option is needed to generate apparmor profiles that allow accessing kernel modules/firmware. * o/ifacestate: pass task to buildConfinementOptions so remodelling scenarios are handled properly. * interfaces/apparmor: some test fixes --- interfaces/apparmor/backend.go | 9 +++++ interfaces/apparmor/backend_test.go | 44 +++++++++++++++++++++- interfaces/apparmor/template.go | 1 + interfaces/backend.go | 3 ++ overlord/ifacestate/export_test.go | 4 +- overlord/ifacestate/handlers.go | 31 ++++++++++------ overlord/ifacestate/handlers_test.go | 51 +++++++++++++++++++++++++- overlord/ifacestate/helpers.go | 2 +- overlord/ifacestate/ifacestate_test.go | 44 +++++++++++----------- 9 files changed, 148 insertions(+), 41 deletions(-) diff --git a/interfaces/apparmor/backend.go b/interfaces/apparmor/backend.go index 4d3838d8ecf..aa900c7058d 100644 --- a/interfaces/apparmor/backend.go +++ b/interfaces/apparmor/backend.go @@ -700,6 +700,15 @@ func (b *Backend) addContent(securityTag string, snapInfo *snap.Info, cmdName st } policy = templatePattern.ReplaceAllStringFunc(policy, func(placeholder string) string { switch placeholder { + case "###KERNEL_MODULES_AND_FIRMWARE###": + if opts.KernelSnap != "" { + return fmt.Sprintf(` + # Allow access to kernel modules and firmware from the kernel snap + /snap/%[1]s/*/{modules,firmware}/{,**} r, + /snap/%[1]s/components/mnt/*/*/{modules,firmware}/{,**} r, + /var/snap/%[1]s/*/{modules,firmware}/{,**} r,`, opts.KernelSnap) + } + return "" case "###DEVMODE_SNAP_CONFINE###": if !opts.DevMode { // nothing to add if we are not in devmode diff --git a/interfaces/apparmor/backend_test.go b/interfaces/apparmor/backend_test.go index 2d1f92befa7..9dfa34c7091 100644 --- a/interfaces/apparmor/backend_test.go +++ b/interfaces/apparmor/backend_test.go @@ -996,7 +996,8 @@ func (s *backendSuite) TestDefaultCoreRuntimesTemplateOnlyUsed(c *C) { appSet, err := interfaces.NewSnapAppSet(snapInfo, nil) c.Assert(err, IsNil) // NOTE: we don't call apparmor.MockTemplate() - err = s.Backend.Setup(appSet, interfaces.ConfinementOptions{}, s.Repo, s.meas) + err = s.Backend.Setup(appSet, + interfaces.ConfinementOptions{KernelSnap: "mykernel"}, s.Repo, s.meas) c.Assert(err, IsNil) profile := filepath.Join(dirs.SnapAppArmorDir, "snap.samba.smbd") data, err := os.ReadFile(profile) @@ -1014,6 +1015,8 @@ func (s *backendSuite) TestDefaultCoreRuntimesTemplateOnlyUsed(c *C) { // defaultCoreRuntimeTemplateRules "# Default rules for core base runtimes\n", "/usr/share/terminfo/** k,\n", + // ###KERNEL_MODULES_AND_FIRMWARE### is present + "/snap/mykernel/*/{modules,firmware}/{,**} r,\n", } { c.Assert(string(data), testutil.Contains, line) } @@ -1037,7 +1040,8 @@ func (s *backendSuite) TestBaseDefaultTemplateOnlyUsed(c *C) { appSet, err := interfaces.NewSnapAppSet(snapInfo, nil) c.Assert(err, IsNil) // NOTE: we don't call apparmor.MockTemplate() - err = s.Backend.Setup(appSet, interfaces.ConfinementOptions{}, s.Repo, s.meas) + err = s.Backend.Setup(appSet, + interfaces.ConfinementOptions{KernelSnap: "mykernel"}, s.Repo, s.meas) c.Assert(err, IsNil) profile := filepath.Join(dirs.SnapAppArmorDir, "snap.samba.smbd") data, err := os.ReadFile(profile) @@ -1055,6 +1059,8 @@ func (s *backendSuite) TestBaseDefaultTemplateOnlyUsed(c *C) { // defaultOtherBaseTemplateRules "# Default rules for non-core base runtimes\n", "/{,s}bin/** mrklix,\n", + // ###KERNEL_MODULES_AND_FIRMWARE### is present + "/snap/mykernel/*/{modules,firmware}/{,**} r,\n", } { c.Assert(string(data), testutil.Contains, line) } @@ -2986,3 +2992,37 @@ func (s *backendSuite) TestRemoveAllSnapAppArmorProfiles(c *C) { c.Check(os.IsNotExist(err), Equals, true) } } + +func (s *backendSuite) TestKernelModulesAndFwRule(c *C) { + restoreTemplate := apparmor.MockTemplate("template\n###KERNEL_MODULES_AND_FIRMWARE###\n") + defer restoreTemplate() + restore := apparmor_sandbox.MockLevel(apparmor_sandbox.Full) + defer restore() + + for _, tc := range []struct { + opts interfaces.ConfinementOptions + suppress bool + expected Checker + }{ + { + opts: interfaces.ConfinementOptions{}, + expected: Not(testutil.Contains), + }, + { + opts: interfaces.ConfinementOptions{KernelSnap: "mykernel"}, + expected: testutil.Contains, + }, + } { + snapInfo := s.InstallSnap(c, tc.opts, "", ifacetest.SambaYamlV1, 1) + profile := filepath.Join(dirs.SnapAppArmorDir, "snap.samba.smbd") + data, err := os.ReadFile(profile) + c.Assert(err, IsNil) + + c.Assert(string(data), tc.expected, ` + # Allow access to kernel modules and firmware from the kernel snap + /snap/mykernel/*/{modules,firmware}/{,**} r, + /snap/mykernel/components/mnt/*/*/{modules,firmware}/{,**} r, + /var/snap/mykernel/*/{modules,firmware}/{,**} r,`) + s.RemoveSnap(c, snapInfo) + } +} diff --git a/interfaces/apparmor/template.go b/interfaces/apparmor/template.go index ffe45394bdc..08dd7be7a22 100644 --- a/interfaces/apparmor/template.go +++ b/interfaces/apparmor/template.go @@ -85,6 +85,7 @@ var templateCommon = ` #include #include #include + ###KERNEL_MODULES_AND_FIRMWARE### # While in later versions of the base abstraction, include this explicitly # for series 16 and cross-distro diff --git a/interfaces/backend.go b/interfaces/backend.go index 4f6f27c6ad1..aa237ce8264 100644 --- a/interfaces/backend.go +++ b/interfaces/backend.go @@ -72,6 +72,9 @@ type ConfinementOptions struct { // AppArmorPrompting indicates whether the prompt prefix should be used in // relevant rules when generating AppArmor security profiles. AppArmorPrompting bool + // KernelSnap is the name of the kernel snap in the system + // (empty for classic systems). + KernelSnap string } // SecurityBackendOptions carries extra flags that affect initialization of the diff --git a/overlord/ifacestate/export_test.go b/overlord/ifacestate/export_test.go index 838800234a2..323466f61f9 100644 --- a/overlord/ifacestate/export_test.go +++ b/overlord/ifacestate/export_test.go @@ -74,8 +74,8 @@ func NewInterfaceManagerWithAppArmorPrompting(useAppArmorPrompting bool) *Interf return m } -func (m *InterfaceManager) BuildConfinementOptions(st *state.State, snapInfo *snap.Info, flags snapstate.Flags) (interfaces.ConfinementOptions, error) { - return m.buildConfinementOptions(st, snapInfo, flags) +func (m *InterfaceManager) BuildConfinementOptions(st *state.State, task *state.Task, snapInfo *snap.Info, flags snapstate.Flags) (interfaces.ConfinementOptions, error) { + return m.buildConfinementOptions(st, task, snapInfo, flags) } type ConnectOpts = connectOpts diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index f34331bb4f5..7a3704bd160 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -82,18 +82,25 @@ func getExtraLayouts(st *state.State, snapInfo *snap.Info) ([]snap.Layout, error return extraLayouts, nil } -func (m *InterfaceManager) buildConfinementOptions(st *state.State, snapInfo *snap.Info, flags snapstate.Flags) (interfaces.ConfinementOptions, error) { +func (m *InterfaceManager) buildConfinementOptions(st *state.State, task *state.Task, snapInfo *snap.Info, flags snapstate.Flags) (interfaces.ConfinementOptions, error) { extraLayouts, err := getExtraLayouts(st, snapInfo) if err != nil { return interfaces.ConfinementOptions{}, fmt.Errorf("cannot get extra mount layouts of snap %q: %s", snapInfo.InstanceName(), err) } + kernelSnap := "" + deviceCtx, err := snapstate.DeviceCtx(st, task, nil) + if err == nil { + kernelSnap = deviceCtx.Kernel() + } + return interfaces.ConfinementOptions{ DevMode: flags.DevMode, JailMode: flags.JailMode, Classic: flags.Classic, ExtraLayouts: extraLayouts, AppArmorPrompting: m.useAppArmorPrompting, + KernelSnap: kernelSnap, }, nil } @@ -124,7 +131,7 @@ func (m *InterfaceManager) setupAffectedSnaps(task *state.Task, affectingSnap st return fmt.Errorf("building app set for snap %q: %v", affectingSnap, err) } - opts, err := m.buildConfinementOptions(st, affectedSnapInfo, snapst.Flags) + opts, err := m.buildConfinementOptions(st, task, affectedSnapInfo, snapst.Flags) if err != nil { return err } @@ -170,7 +177,7 @@ func (m *InterfaceManager) doSetupProfiles(task *state.Task, tomb *tomb.Tomb) er return nil } - opts, err := m.buildConfinementOptions(task.State(), snapInfo, snapsup.Flags) + opts, err := m.buildConfinementOptions(task.State(), task, snapInfo, snapsup.Flags) if err != nil { return err } @@ -340,7 +347,7 @@ func (m *InterfaceManager) setupProfilesForAppSet(task *state.Task, appSet *inte return fmt.Errorf("building app set for snap %q: %v", name, err) } - opts, err := m.buildConfinementOptions(st, snapInfo, snapst.Flags) + opts, err := m.buildConfinementOptions(st, task, snapInfo, snapst.Flags) if err != nil { return err } @@ -442,7 +449,7 @@ func (m *InterfaceManager) undoSetupProfiles(task *state.Task, tomb *tomb.Tomb) if err != nil { return err } - opts, err := m.buildConfinementOptions(task.State(), snapInfo, snapst.Flags) + opts, err := m.buildConfinementOptions(task.State(), task, snapInfo, snapst.Flags) if err != nil { return err } @@ -677,7 +684,7 @@ func (m *InterfaceManager) doConnect(task *state.Task, _ *tomb.Tomb) (err error) if err != nil { return err } - slotOpts, err := m.buildConfinementOptions(st, slotSnapInfo, slotSnapst.Flags) + slotOpts, err := m.buildConfinementOptions(st, task, slotSnapInfo, slotSnapst.Flags) if err != nil { return err } @@ -689,7 +696,7 @@ func (m *InterfaceManager) doConnect(task *state.Task, _ *tomb.Tomb) (err error) if err != nil { return err } - plugOpts, err := m.buildConfinementOptions(st, plugSnapInfo, plugSnapst.Flags) + plugOpts, err := m.buildConfinementOptions(st, task, plugSnapInfo, plugSnapst.Flags) if err != nil { return err } @@ -799,7 +806,7 @@ func (m *InterfaceManager) doDisconnect(task *state.Task, _ *tomb.Tomb) error { return fmt.Errorf("building app set for snap %q: %v", snapInfo.InstanceName(), err) } - opts, err := m.buildConfinementOptions(st, snapInfo, snapst.Flags) + opts, err := m.buildConfinementOptions(st, task, snapInfo, snapst.Flags) if err != nil { return err } @@ -923,7 +930,7 @@ func (m *InterfaceManager) undoDisconnect(task *state.Task, _ *tomb.Tomb) error if err != nil { return err } - slotOpts, err := m.buildConfinementOptions(st, slotSnapInfo, slotSnapst.Flags) + slotOpts, err := m.buildConfinementOptions(st, task, slotSnapInfo, slotSnapst.Flags) if err != nil { return err } @@ -935,7 +942,7 @@ func (m *InterfaceManager) undoDisconnect(task *state.Task, _ *tomb.Tomb) error if err != nil { return err } - plugOpts, err := m.buildConfinementOptions(st, plugSnapInfo, plugSnapst.Flags) + plugOpts, err := m.buildConfinementOptions(st, task, plugSnapInfo, plugSnapst.Flags) if err != nil { return err } @@ -1033,7 +1040,7 @@ func (m *InterfaceManager) undoConnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - slotOpts, err := m.buildConfinementOptions(st, slotSnapInfo, slotSnapst.Flags) + slotOpts, err := m.buildConfinementOptions(st, task, slotSnapInfo, slotSnapst.Flags) if err != nil { return err } @@ -1045,7 +1052,7 @@ func (m *InterfaceManager) undoConnect(task *state.Task, _ *tomb.Tomb) error { if err != nil { return err } - plugOpts, err := m.buildConfinementOptions(st, plugSnapInfo, plugSnapst.Flags) + plugOpts, err := m.buildConfinementOptions(st, task, plugSnapInfo, plugSnapst.Flags) if err != nil { return err } diff --git a/overlord/ifacestate/handlers_test.go b/overlord/ifacestate/handlers_test.go index 338749ebd15..40828a209e2 100644 --- a/overlord/ifacestate/handlers_test.go +++ b/overlord/ifacestate/handlers_test.go @@ -20,12 +20,14 @@ package ifacestate_test import ( + "errors" "path" . "gopkg.in/check.v1" "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/overlord/configstate/config" + "github.com/snapcore/snapd/overlord/devicestate" "github.com/snapcore/snapd/overlord/ifacestate" "github.com/snapcore/snapd/overlord/servicestate/servicestatetest" "github.com/snapcore/snapd/overlord/snapstate" @@ -34,6 +36,7 @@ import ( "github.com/snapcore/snapd/snap" "github.com/snapcore/snapd/snap/quota" "github.com/snapcore/snapd/snap/snaptest" + "github.com/snapcore/snapd/testutil" ) const snapAyaml = `name: snap-a @@ -42,14 +45,22 @@ base: base-snap-a ` type handlersSuite struct { + testutil.BaseTest st *state.State } var _ = Suite(&handlersSuite{}) +func (s *handlersSuite) mockModel() func() { + old := snapstate.DeviceCtx + snapstate.DeviceCtx = devicestate.DeviceCtx + return func() { snapstate.DeviceCtx = old } +} + func (s *handlersSuite) SetUpTest(c *C) { s.st = state.New(nil) dirs.SetRootDir(c.MkDir()) + s.AddCleanup(s.mockModel()) } func (s *handlersSuite) TearDownTest(c *C) { @@ -136,7 +147,42 @@ func (s *handlersSuite) TestBuildConfinementOptions(c *C) { snapInfo := mockInstalledSnap(c, s.st, snapAyaml) flags := snapstate.Flags{} - opts, err := m.BuildConfinementOptions(s.st, snapInfo, snapstate.Flags{}) + opts, err := m.BuildConfinementOptions(s.st, nil, snapInfo, snapstate.Flags{}) + + c.Check(err, IsNil) + c.Check(len(opts.ExtraLayouts), Equals, 0) + c.Check(opts.Classic, Equals, flags.Classic) + c.Check(opts.DevMode, Equals, flags.DevMode) + c.Check(opts.JailMode, Equals, flags.JailMode) + c.Check(opts.AppArmorPrompting, Equals, testAppArmorPrompting) + c.Check(opts.KernelSnap, Equals, "") + } +} + +func (s *handlersSuite) TestBuildConfinementOptionsWithTask(c *C) { + s.st.Lock() + defer s.st.Unlock() + + // This test is to check that the task is actually passed down to snapstate.DeviceCtx(), + // and that errors there are handled fine. + t := s.st.NewTask("foo", "description") + s.AddCleanup(func() func() { + old := snapstate.DeviceCtx + snapstate.DeviceCtx = func(st *state.State, task *state.Task, + providedDeviceCtx snapstate.DeviceContext) (snapstate.DeviceContext, error) { + c.Check(task, DeepEquals, t) + return nil, errors.New("classic, no context") + } + return func() { snapstate.DeviceCtx = old } + }()) + + for _, testAppArmorPrompting := range []bool{true, false} { + // Create fake InterfaceManager to hold fake AppArmor Prompting value + m := ifacestate.NewInterfaceManagerWithAppArmorPrompting(testAppArmorPrompting) + + snapInfo := mockInstalledSnap(c, s.st, snapAyaml) + flags := snapstate.Flags{} + opts, err := m.BuildConfinementOptions(s.st, t, snapInfo, snapstate.Flags{}) c.Check(err, IsNil) c.Check(len(opts.ExtraLayouts), Equals, 0) @@ -144,6 +190,7 @@ func (s *handlersSuite) TestBuildConfinementOptions(c *C) { c.Check(opts.DevMode, Equals, flags.DevMode) c.Check(opts.JailMode, Equals, flags.JailMode) c.Check(opts.AppArmorPrompting, Equals, testAppArmorPrompting) + c.Check(opts.KernelSnap, Equals, "") } } @@ -166,7 +213,7 @@ func (s *handlersSuite) TestBuildConfinementOptionsWithLogNamespace(c *C) { c.Assert(err, IsNil) flags := snapstate.Flags{} - opts, err := m.BuildConfinementOptions(s.st, snapInfo, snapstate.Flags{}) + opts, err := m.BuildConfinementOptions(s.st, nil, snapInfo, snapstate.Flags{}) c.Check(err, IsNil) c.Assert(len(opts.ExtraLayouts), Equals, 1) diff --git a/overlord/ifacestate/helpers.go b/overlord/ifacestate/helpers.go index 98a78bf7297..375e6bf7d79 100644 --- a/overlord/ifacestate/helpers.go +++ b/overlord/ifacestate/helpers.go @@ -219,7 +219,7 @@ func (m *InterfaceManager) regenerateAllSecurityProfiles(tm timings.Measurer) er logger.Noticef("cannot get current info for snap %q: %s", snapName, err) return interfaces.ConfinementOptions{} } - opts, err := m.buildConfinementOptions(m.state, snapInfo, snapst.Flags) + opts, err := m.buildConfinementOptions(m.state, nil, snapInfo, snapst.Flags) if err != nil { logger.Noticef("cannot get confinement options for snap %q: %s", snapName, err) } diff --git a/overlord/ifacestate/ifacestate_test.go b/overlord/ifacestate/ifacestate_test.go index c4abb9e66aa..03fca49da81 100644 --- a/overlord/ifacestate/ifacestate_test.go +++ b/overlord/ifacestate/ifacestate_test.go @@ -1796,8 +1796,8 @@ func (s *interfaceManagerSuite) testDisconnect(c *C, plugSnap, plugName, slotSna c.Assert(s.secBackend.SetupCalls, HasLen, 2) c.Assert(s.secBackend.RemoveCalls, HasLen, 0) - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) consumerAppSet := s.secBackend.SetupCalls[0].AppSet c.Check(consumerAppSet.InstanceName(), Equals, "consumer") @@ -4174,8 +4174,8 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecurityReloadsConnectionsWhenInv c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, "consumer") c.Check(s.secBackend.SetupCalls[1].AppSet.InstanceName(), Equals, "producer") - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) } func (s *interfaceManagerSuite) TestDoSetupSnapSecurityReloadsConnectionsWhenInvokedOnSlotSide(c *C) { @@ -4192,8 +4192,8 @@ func (s *interfaceManagerSuite) TestDoSetupSnapSecurityReloadsConnectionsWhenInv c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, "producer") c.Check(s.secBackend.SetupCalls[1].AppSet.InstanceName(), Equals, "consumer") - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) } func (s *interfaceManagerSuite) testDoSetupSnapSecurityReloadsConnectionsWhenInvokedOn(c *C, snapName string, revision snap.Revision) { @@ -4263,7 +4263,7 @@ func (s *interfaceManagerSuite) TestSetupProfilesHonorsDevMode(c *C) { c.Assert(s.secBackend.SetupCalls, HasLen, 1) c.Assert(s.secBackend.RemoveCalls, HasLen, 0) c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, "snap") - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{DevMode: true}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{DevMode: true, KernelSnap: "krnl"}) } func (s *interfaceManagerSuite) TestSetupProfilesSetupManyError(c *C) { @@ -5305,8 +5305,8 @@ func (s *interfaceManagerSuite) TestConnectSetsUpSecurity(c *C) { c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, "producer") c.Check(s.secBackend.SetupCalls[1].AppSet.InstanceName(), Equals, "consumer") - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) } func (s *interfaceManagerSuite) TestConnectWithComponentsSetsUpSecurity(c *C) { @@ -5345,8 +5345,8 @@ func (s *interfaceManagerSuite) TestConnectWithComponentsSetsUpSecurity(c *C) { c.Assert(s.secBackend.SetupCalls, HasLen, 2) c.Assert(s.secBackend.RemoveCalls, HasLen, 0) - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) producerAppSet := s.secBackend.SetupCalls[0].AppSet consumerAppSet := s.secBackend.SetupCalls[1].AppSet @@ -5444,8 +5444,8 @@ func (s *interfaceManagerSuite) TestDisconnectSetsUpSecurity(c *C) { c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, "consumer") c.Check(s.secBackend.SetupCalls[1].AppSet.InstanceName(), Equals, "producer") - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) } func (s *interfaceManagerSuite) TestDisconnectTracksConnectionsInState(c *C) { @@ -5875,13 +5875,13 @@ func (s *interfaceManagerSuite) TestSetupProfilesDevModeMultiple(c *C) { c.Assert(s.secBackend.SetupCalls, HasLen, 4) c.Assert(s.secBackend.RemoveCalls, HasLen, 0) c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, siC.InstanceName()) - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{DevMode: true}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{DevMode: true, KernelSnap: "krnl"}) c.Check(s.secBackend.SetupCalls[1].AppSet.InstanceName(), Equals, siP.InstanceName()) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) c.Check(s.secBackend.SetupCalls[2].AppSet.InstanceName(), Equals, siC.InstanceName()) - c.Check(s.secBackend.SetupCalls[2].Options, DeepEquals, interfaces.ConfinementOptions{DevMode: true}) + c.Check(s.secBackend.SetupCalls[2].Options, DeepEquals, interfaces.ConfinementOptions{DevMode: true, KernelSnap: "krnl"}) c.Check(s.secBackend.SetupCalls[3].AppSet.InstanceName(), Equals, siP.InstanceName()) - c.Check(s.secBackend.SetupCalls[3].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[3].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) } func (s *interfaceManagerSuite) TestCheckInterfacesDeny(c *C) { @@ -6334,7 +6334,7 @@ func (s *interfaceManagerSuite) TestUndoSetupProfilesOnRefresh(c *C) { c.Assert(s.secBackend.RemoveCalls, HasLen, 0) c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, snapInfo.InstanceName()) c.Check(s.secBackend.SetupCalls[0].AppSet.Info().Revision, Equals, snapInfo.Revision) - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) var snapst snapstate.SnapState err := snapstate.Get(s.state, "snap", &snapst) @@ -6723,16 +6723,16 @@ func (s *interfaceManagerSuite) TestUndoConnect(c *C) { c.Assert(s.secBackend.SetupCalls, HasLen, 4) c.Check(s.secBackend.SetupCalls[0].AppSet.InstanceName(), Equals, "producer") c.Check(s.secBackend.SetupCalls[1].AppSet.InstanceName(), Equals, "consumer") - c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[0].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[1].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) c.Check(s.secBackend.SetupCalls[0].AppSet.Runnables(), testutil.DeepUnsortedMatches, producerRunnablesFullSet) c.Check(s.secBackend.SetupCalls[1].AppSet.Runnables(), testutil.DeepUnsortedMatches, consumerRunnablesFullSet) // by undo c.Check(s.secBackend.SetupCalls[2].AppSet.InstanceName(), Equals, "producer") c.Check(s.secBackend.SetupCalls[3].AppSet.InstanceName(), Equals, "consumer") - c.Check(s.secBackend.SetupCalls[2].Options, DeepEquals, interfaces.ConfinementOptions{}) - c.Check(s.secBackend.SetupCalls[3].Options, DeepEquals, interfaces.ConfinementOptions{}) + c.Check(s.secBackend.SetupCalls[2].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) + c.Check(s.secBackend.SetupCalls[3].Options, DeepEquals, interfaces.ConfinementOptions{KernelSnap: "krnl"}) c.Check(s.secBackend.SetupCalls[2].AppSet.Runnables(), testutil.DeepUnsortedMatches, producerRunnablesFullSet) c.Check(s.secBackend.SetupCalls[3].AppSet.Runnables(), testutil.DeepUnsortedMatches, consumerRunnablesFullSet) } From 5c883f2ebf63f60a67a82dac4453a5f74b014332 Mon Sep 17 00:00:00 2001 From: Katie May <63071677+maykathm@users.noreply.github.com> Date: Tue, 7 Jan 2025 08:19:02 +0100 Subject: [PATCH 3/9] cmd/snap-seccomp/syscalls: added new syscalls (#14886) * cmd/snap-seccomp/syscalls: added new syscalls * cmd/snap-seccomp/syscalls: update git revision --- cmd/snap-seccomp/syscalls/syscalls.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/snap-seccomp/syscalls/syscalls.go b/cmd/snap-seccomp/syscalls/syscalls.go index a5e16167534..678b99b45f9 100644 --- a/cmd/snap-seccomp/syscalls/syscalls.go +++ b/cmd/snap-seccomp/syscalls/syscalls.go @@ -20,7 +20,7 @@ package syscalls // Generated using arch-syscall-dump test tool from libseccomp tree, git -// revision aa168d49243b95f63b9825a87351a1eb323dc792. +// revision 42b596818635bf9d1cae54fa87e8c7b2e16869d3. var SeccompSyscalls = []string{ "_llseek", "_newselect", @@ -175,6 +175,7 @@ var SeccompSyscalls = []string{ "getuid", "getuid32", "getxattr", + "getxattrat", "gtty", "idle", "init_module", @@ -214,6 +215,7 @@ var SeccompSyscalls = []string{ "listen", "listmount", "listxattr", + "listxattrat", "llistxattr", "lock", "lookup_dcookie", @@ -336,6 +338,7 @@ var SeccompSyscalls = []string{ "recvmsg", "remap_file_pages", "removexattr", + "removexattrat", "rename", "renameat", "renameat2", @@ -422,6 +425,7 @@ var SeccompSyscalls = []string{ "setuid", "setuid32", "setxattr", + "setxattrat", "sgetmask", "shmat", "shmctl", From 0a2e785287ab05a382b8d77709679291de86b9b8 Mon Sep 17 00:00:00 2001 From: Maciej Borzecki Date: Thu, 19 Dec 2024 12:25:26 +0100 Subject: [PATCH 4/9] interfaces/seccomp/template: allow epoll_pwait2 in the base template Add epoll_pwait2 in the base template. The syscall has been available since 5.11 (early 2021). Signed-off-by: Maciej Borzecki --- interfaces/seccomp/template.go | 1 + 1 file changed, 1 insertion(+) diff --git a/interfaces/seccomp/template.go b/interfaces/seccomp/template.go index a26576de361..7fc2b111cd3 100644 --- a/interfaces/seccomp/template.go +++ b/interfaces/seccomp/template.go @@ -125,6 +125,7 @@ epoll_create1 epoll_ctl epoll_ctl_old epoll_pwait +epoll_pwait2 epoll_wait epoll_wait_old eventfd From da151265babaeecb8d5cdcb2506cdfd077d4b9a5 Mon Sep 17 00:00:00 2001 From: Katie May <63071677+maykathm@users.noreply.github.com> Date: Wed, 18 Dec 2024 15:26:38 +0100 Subject: [PATCH 5/9] interfaces: update template with new syscalls (#14861) --- interfaces/builtin/hardware_observe.go | 2 ++ interfaces/builtin/mount_observe.go | 3 +++ interfaces/seccomp/template.go | 13 +++++++++++++ 3 files changed, 18 insertions(+) diff --git a/interfaces/builtin/hardware_observe.go b/interfaces/builtin/hardware_observe.go index 96391b1b14e..ede9379159f 100644 --- a/interfaces/builtin/hardware_observe.go +++ b/interfaces/builtin/hardware_observe.go @@ -145,6 +145,8 @@ const hardwareObserveConnectedPlugSecComp = ` # used by 'lspci -A intel-conf1/intel-conf2' iopl +riscv_hwprobe + # multicast statistics socket AF_NETLINK - NETLINK_GENERIC diff --git a/interfaces/builtin/mount_observe.go b/interfaces/builtin/mount_observe.go index 141b2873368..ceffb735147 100644 --- a/interfaces/builtin/mount_observe.go +++ b/interfaces/builtin/mount_observe.go @@ -76,6 +76,9 @@ quotactl Q_GETINFO - - - quotactl Q_GETFMT - - - quotactl Q_XGETQUOTA - - - quotactl Q_XGETQSTAT - - - + +listmount +statmount ` func init() { diff --git a/interfaces/seccomp/template.go b/interfaces/seccomp/template.go index 7fc2b111cd3..ecbbe31b729 100644 --- a/interfaces/seccomp/template.go +++ b/interfaces/seccomp/template.go @@ -55,6 +55,15 @@ set_tls usr26 usr32 +# Requires input fd and so should not pose more security +# issues than access to the file in the first place +# Flags are currently unused and should be 0 +cachestat - - - 0 + +# Flags are currently unused and should be 0 +mseal - - 0 +map_shadow_stack + capget # AppArmor mediates capabilities, so allow capset (useful for apps that for # example want to drop capabilities) @@ -68,6 +77,7 @@ fchdir chmod fchmod fchmodat +fchmodat2 # Daemons typically run as 'root' so allow chown to 'root'. DAC will prevent # non-root from chowning to root. @@ -147,8 +157,11 @@ flock fork ftime futex +futex_requeue futex_time64 +futex_wait futex_waitv +futex_wake get_mempolicy get_robust_list get_thread_area From b144fe04bd7e2b53c5a9fa40d78396d8c65b23b9 Mon Sep 17 00:00:00 2001 From: Philip Meulengracht Date: Tue, 17 Dec 2024 11:24:45 +0100 Subject: [PATCH 6/9] overlord: wait for snapd restart after requesting by undo of 'link-snap' (#14850) wait for snapd restart after requesting by undo of 'link-snap'. otherwise we can end up using tools from the snapd we are reverting away from, causing issues or generation of wrong apparmor profiles * overlord/ifacestate: correct the comment * tests/core/snapd-refresh-undo: couple of linting issues * tests,overlord: rectify comments, add a TODO for the restart request in undoUnlinkCurrentSnap, fix two unit tests * overlord/devicestate: fix double locking in unit test * overlord/snapstate: add sequence file check * overlord/snapstate: add TODO for undo path relating to snap-failure * tests/core/snapd-refresh-undo: revert snapd in end of test to fix restore * overlord/devicestate: make unit test more robust * overlord/snapstate,tests: use correct terminology for snapd restarts, and fix the description of the spread test * wrappers: more logging Signed-off-by: Maciej Borzecki * wrappers: snapd version specific quirk for < 2.62 versions Introduce a version specific quirk which works around a problem present in snapd in versions < 2.62. Prior to 2.622 the snapd.apparmor.service did not carry a do-not-start tag. This could lead to a problem where when executing the undo path of snapd, the snapd.apparmor.service is restarted after unlinking the current version of snapd. This could cause the snapd.apparmor to use either a potentially much older parser from the host one from the previous version of snapd, where in either case, the parser may not support features present in the profiles generated by current version of snapd. Signed-off-by: Maciej Borzecki --------- Signed-off-by: Maciej Borzecki Co-authored-by: Maciej Borzecki --- overlord/devicestate/firstboot20_test.go | 23 ++- overlord/devicestate/firstboot_test.go | 17 +- overlord/ifacestate/handlers.go | 14 ++ overlord/snapstate/handlers.go | 12 +- overlord/snapstate/handlers_link_test.go | 16 +- overlord/snapstate/snapstate.go | 3 + tests/core/snapd-refresh-undo/task.yaml | 46 +++++ wrappers/core18.go | 28 +++ wrappers/core18_test.go | 233 +++++++++++++++++++++-- wrappers/services_test.go | 2 +- 10 files changed, 364 insertions(+), 30 deletions(-) create mode 100644 tests/core/snapd-refresh-undo/task.yaml diff --git a/overlord/devicestate/firstboot20_test.go b/overlord/devicestate/firstboot20_test.go index d933807f319..52d04fbcb46 100644 --- a/overlord/devicestate/firstboot20_test.go +++ b/overlord/devicestate/firstboot20_test.go @@ -1359,14 +1359,16 @@ base: core20 st.Lock() c.Assert(err, IsNil) - // at this point the system is "restarting", pretend the restart has - // happened + // at this point snapd needs to restart along the 'Do' path before + // "auto-connect" c.Assert(chg.Status(), Equals, state.DoingStatus) restart.MockPending(st, restart.RestartUnset) + st.Unlock() err = s.overlord.Settle(settleTimeout) st.Lock() c.Assert(err, IsNil) + return chg } @@ -1493,8 +1495,21 @@ func (s *firstBoot20Suite) TestPopulateFromSeedCore20ValidationSetTrackingFailsU chg := s.testPopulateFromSeedCore20ValidationSetTracking(c, "run", []string{"canonical/base-set/2"}) - s.overlord.State().Lock() - defer s.overlord.State().Unlock() + st := s.overlord.State() + + func() { + st.Lock() + defer st.Unlock() + // at this point another restart is required, but it's snapd restarting + // because it's undoing + c.Assert(chg.Status(), Equals, state.UndoingStatus) + restart.MockPending(st, restart.RestartUnset) + }() + + err = s.overlord.Settle(settleTimeout) + st.Lock() + defer st.Unlock() + c.Assert(err, IsNil) c.Assert(chg.Status(), Equals, state.ErrorStatus) c.Check(chg.Err().Error(), testutil.Contains, "my-snap (required at revision 1 by sets canonical/base-set))") } diff --git a/overlord/devicestate/firstboot_test.go b/overlord/devicestate/firstboot_test.go index 27dabd3ba75..4c308c93799 100644 --- a/overlord/devicestate/firstboot_test.go +++ b/overlord/devicestate/firstboot_test.go @@ -2503,8 +2503,21 @@ func (s *firstBoot16Suite) TestPopulateFromSeedCore18ValidationSetTrackingUnmetC } chg := s.testPopulateFromSeedCore18ValidationSetTracking(c, []asserts.Assertion{a}, []interface{}{headers}) - s.overlord.State().Lock() - defer s.overlord.State().Unlock() + st := s.overlord.State() + + func() { + st.Lock() + defer st.Unlock() + // at this point another restart is required, but it's snapd restarting + // because it's undoing + c.Assert(chg.Status(), Equals, state.UndoingStatus) + restart.MockPending(st, restart.RestartUnset) + }() + + err = s.overlord.Settle(settleTimeout) + st.Lock() + defer st.Unlock() + c.Assert(err, IsNil) c.Assert(chg.Status(), Equals, state.ErrorStatus) c.Check(chg.Err().Error(), testutil.Contains, "pc-kernel (required at revision 7 by sets canonical/base-set))") } diff --git a/overlord/ifacestate/handlers.go b/overlord/ifacestate/handlers.go index 7a3704bd160..a497050b155 100644 --- a/overlord/ifacestate/handlers.go +++ b/overlord/ifacestate/handlers.go @@ -432,6 +432,20 @@ func (m *InterfaceManager) undoSetupProfiles(task *state.Task, tomb *tomb.Tomb) } snapName := snapsup.InstanceName() + // The previous task's undo (link-snap) may have triggered a restart, if this + // is the case we can only proceed once the restart has happened or we + // may be invoking tools (like apparmor) from the wrong snapd. We set + // the default to true as we cannot set it otherwise since the change will + // always have been created by the old snapd (that may not have "finish-restart") + logger.Debugf("finish restart from undoLinkSnap") + finishOpts := snapstate.FinishRestartOptions{ + // Only default to true for snapd snap + FinishRestartDefault: snapsup.Type == snap.TypeSnapd, + } + if err := snapstateFinishRestart(task, snapsup, finishOpts); err != nil { + return err + } + // Get the name from SnapSetup and use it to find the current SideInfo // about the snap, if there is one. var snapst snapstate.SnapState diff --git a/overlord/snapstate/handlers.go b/overlord/snapstate/handlers.go index 10819a91402..f92dd4db638 100644 --- a/overlord/snapstate/handlers.go +++ b/overlord/snapstate/handlers.go @@ -1628,6 +1628,9 @@ func (m *SnapManager) undoUnlinkCurrentSnap(t *state.Task, _ *tomb.Tomb) error { // if we just put back a previous a core snap, request a restart // so that we switch executing its snapd + // TODO: This needs changing, right now we restart snapd after undoing 'link-snap', + // inside 'undoSetupProfiles() and even though we request a restart here we are + // not actually waiting for the restart to occur in the task that comes before this. return m.finishTaskWithMaybeRestart(t, state.UndoneStatus, restartPossibility{info: oldInfo, RebootInfo: reboot}) } @@ -2902,11 +2905,10 @@ func (m *SnapManager) undoLinkSnap(t *state.Task, _ *tomb.Tomb) error { return err } - // restart only when snapd was installed for the first time and the rest of - // the cleanup is performed by snapd from core; - // when reverting a subsequent snapd revision, the restart happens in - // undoLinkCurrentSnap() instead - if firstInstall && newInfo.Type() == snap.TypeSnapd { + // When undoing the snapd snap refresh/install we must ensure that + // we are restarting into the previous snapd, undoSetupProfiles() handles + // the actual waiting for restart. + if newInfo.Type() == snap.TypeSnapd { restartPoss = &restartPossibility{info: newInfo, RebootInfo: boot.RebootInfo{RebootRequired: false}} } diff --git a/overlord/snapstate/handlers_link_test.go b/overlord/snapstate/handlers_link_test.go index c78ff1f5668..a31d8c1c6aa 100644 --- a/overlord/snapstate/handlers_link_test.go +++ b/overlord/snapstate/handlers_link_test.go @@ -2447,11 +2447,19 @@ func (s *linkSnapSuite) TestUndoLinkSnapdNthInstall(c *C) { c.Check(s.fakeBackend.ops.Ops(), DeepEquals, expected.Ops()) c.Check(s.fakeBackend.ops, DeepEquals, expected) - // 1 restart from link snap, the other restart happens - // in undoUnlinkCurrentSnap (not tested here) - c.Check(s.restartRequested, DeepEquals, []restart.RestartType{restart.RestartDaemon}) - c.Assert(t.Log(), HasLen, 1) + // 1 restart from link snap + // 1 restart from undo of 'setup-profiles' + // 1 in undoUnlinkCurrentSnap (not tested here) + c.Check(s.restartRequested, DeepEquals, []restart.RestartType{restart.RestartDaemon, restart.RestartDaemon}) + c.Assert(t.Log(), HasLen, 2) c.Check(t.Log()[0], Matches, `.*INFO Requested daemon restart \(snapd snap\)\.`) + c.Check(t.Log()[1], Matches, `.*INFO Requested daemon restart \(snapd snap\)\.`) + + // verify sequence file + // and check that the sequence file got updated + seqContent, err := os.ReadFile(filepath.Join(dirs.SnapSeqDir, "snapd.json")) + c.Assert(err, IsNil) + c.Check(string(seqContent), Equals, `{"sequence":[{"name":"snapd","snap-id":"snapd-snap-id","revision":"20"}],"current":"20","migrated-hidden":false,"migrated-exposed-home":false}`) } func (s *linkSnapSuite) TestDoUnlinkSnapRefreshAwarenessHardCheckOn(c *C) { diff --git a/overlord/snapstate/snapstate.go b/overlord/snapstate/snapstate.go index 31ce86e075b..d3b40d50033 100644 --- a/overlord/snapstate/snapstate.go +++ b/overlord/snapstate/snapstate.go @@ -1194,6 +1194,9 @@ func isChangeRequestingSnapdRestart(chg *state.Change) bool { // have been set up in link-snap, daemon restart is requested, link-snap // is marked as Done, and the auto-connect task is held off (in Do or // Doing states) until the restart completes + // TODO: This may need additional handling for snapd restart along the + // Undo path. For instance, 'link-snap' can request a restart in the undo + // direction, making 'setup-profiles' wait for restart. var haveSnapd, linkDone, autoConnectWaiting bool for _, tsk := range chg.Tasks() { kind := tsk.Kind() diff --git a/tests/core/snapd-refresh-undo/task.yaml b/tests/core/snapd-refresh-undo/task.yaml new file mode 100644 index 00000000000..e72cc4804f8 --- /dev/null +++ b/tests/core/snapd-refresh-undo/task.yaml @@ -0,0 +1,46 @@ +summary: Verify that snapd can undo the upgrade from non-snapd-snap to snapd-snap + +details: | + Verify that the snapd-snap can successfully undo after upgrading from a non snapd-snap + situation, and something causes a rollback even in spite of a successful snapd update. + +# UC16 still uses the core snap rather than the snapd snap, so disable this test +# for UC16 +systems: [-ubuntu-core-16-*] + +environment: + SNAP_NAME_BAD: test-snapd-service-v2-bad + +restore: | + # Remove all inactive revisions of snapd. + current=$(readlink /snap/snapd/current) + for revno_path in /snap/snapd/*; do + revno=$(basename "$revno_path") + if [ "$revno" == current ] || [ "$revno" == "$current" ]; then + continue + fi + snap remove snapd --revision="$revno" + done + +execute: | + snap pack "$TESTSLIB/snaps/$SNAP_NAME_BAD" + + # store a copy of the built snapd + cp /var/lib/snapd/snaps/snapd_x1.snap ./snapd.snap + + # refresh to a snapd prior to snapd snap, this one is chosen from + # a customer case where they were seeing issues with snapd reverting + snap refresh snapd --amend --revision=18357 + snap list | MATCH "snapd.*18357" + + # perform a refresh to the current snapd, this will fail, do it in a way + # that will make snapd revert + CHG_ID=$(snap install --no-wait --dangerous --transaction=all-snaps ./snapd.snap ./test-snapd-service_2.0_all.snap) + snap watch "$CHG_ID" || true + + # verify this has no undo failures for snapd related to security profiles + snap change "$CHG_ID" | MATCH "(Undone).*Setup snap \"snapd\" \(unset\) security profiles" + snap change "$CHG_ID" | grep -c "Error" | MATCH "1" + + # restore original snapd + snap revert snapd diff --git a/wrappers/core18.go b/wrappers/core18.go index 2f53efce476..c652deb7c2e 100644 --- a/wrappers/core18.go +++ b/wrappers/core18.go @@ -32,6 +32,7 @@ import ( "github.com/snapcore/snapd/osutil" "github.com/snapcore/snapd/release" "github.com/snapcore/snapd/snap" + "github.com/snapcore/snapd/strutil" "github.com/snapcore/snapd/systemd" ) @@ -41,6 +42,23 @@ var execStartRe = regexp.MustCompile(`(?m)^ExecStart=(/usr/bin/snap\s+.*|/usr/li // snapdToolingMountUnit is the name of the mount unit that provides the snapd tooling const SnapdToolingMountUnit = "usr-lib-snapd.mount" +func skipStartDueToQuirks(unit string, targetVersion string) bool { + switch unit { + case "snapd.apparmor.service": + // versions earlier than 2.62 did not have the do-not-start tag in + // snapd.apparmor.service, which was introduced in + // https://github.com/canonical/snapd/commit/d1cf336e7c584078dff3883c93f0581ae455811e + // due to which snapd may attempt to restart snapd.apparmor.service and + // in specific cases use apparmor_parser from the base + compare, err := strutil.VersionCompare(targetVersion, "2.62") + + // we're downgrading to version earlier than 2.62 + return err == nil && compare < 0 + default: + return false + } +} + func snapdSkipStart(content []byte) bool { return bytes.Contains(content, []byte("X-Snapd-Snap: do-not-start")) } @@ -266,6 +284,8 @@ func AddSnapdSnapServices(s *snap.Info, opts *AddSnapdSnapServicesOptions, inter return nil, nil } + logger.Debugf("removed units: %v", removed) + logger.Debugf("changed units: %v", changed) // stop all removed units first for _, unit := range removed { serviceUnits := []string{unit} @@ -315,8 +335,16 @@ func AddSnapdSnapServices(s *snap.Info, opts *AddSnapdSnapServicesOptions, inter // be started. Others like "snapd.seeded.service" are started // as dependencies of snapd.service. if snapdSkipStart(snapdUnits[unit].(*osutil.MemoryFileState).Content) { + logger.Debugf("skipping unit %v, has do-not-start tag", unit) continue } + // check for any version specific quirks + if skipStartDueToQuirks(unit, s.Version) { + logger.Debugf("skipping unit %v, due to version specific quirks for %v", unit, s.Version) + continue + } + + logger.Debugf("(re)starting snapd unit %v", unit) // Ensure to only restart if the unit was previously // active. This ensures we DTRT on firstboot and do // not stop e.g. snapd.socket because doing that diff --git a/wrappers/core18_test.go b/wrappers/core18_test.go index 25ec38d00d9..3749058b094 100644 --- a/wrappers/core18_test.go +++ b/wrappers/core18_test.go @@ -40,6 +40,10 @@ import ( ) func makeMockSnapdSnap(c *C) *snap.Info { + return makeMockSnapdSnapWithOverrides(c, snapdYaml, nil) +} + +func makeMockSnapdSnapWithOverrides(c *C, metaSnapYaml string, extra [][]string) *snap.Info { err := os.MkdirAll(dirs.SnapServicesDir, 0755) c.Assert(err, IsNil) err = os.MkdirAll(dirs.SnapUserServicesDir, 0755) @@ -49,29 +53,82 @@ func makeMockSnapdSnap(c *C) *snap.Info { err = os.MkdirAll(dirs.SnapDBusSessionPolicyDir, 0755) c.Assert(err, IsNil) - info := snaptest.MockSnapWithFiles(c, snapdYaml, &snap.SideInfo{Revision: snap.R(1)}, [][]string{ + defaultContent := [][]string{ // system services - {"lib/systemd/system/snapd.service", "[Unit]\n[Service]\nExecStart=/usr/lib/snapd/snapd\n# X-Snapd-Snap: do-not-start"}, - {"lib/systemd/system/snapd.system-shutdown.service", "[Unit]\n[Service]\nExecStart=/bin/umount --everything\n# X-Snapd-Snap: do-not-start"}, - {"lib/systemd/system/snapd.autoimport.service", "[Unit]\n[Service]\nExecStart=/usr/bin/snap auto-import\n# X-Snapd-Snap: do-not-start"}, - {"lib/systemd/system/snapd.socket", "[Unit]\n[Socket]\nListenStream=/run/snapd.socket"}, - {"lib/systemd/system/snapd.snap-repair.timer", "[Unit]\n[Timer]\nOnCalendar=*-*-* 5,11,17,23:00"}, + {"lib/systemd/system/snapd.service", "" + + "[Unit]\n[Service]\n" + + "ExecStart=/usr/lib/snapd/snapd\n" + + "# X-Snapd-Snap: do-not-start", + }, + {"lib/systemd/system/snapd.system-shutdown.service", "" + + "[Unit]\n" + + "[Service]\n" + + "ExecStart=/bin/umount --everything\n" + + "# X-Snapd-Snap: do-not-start", + }, + {"lib/systemd/system/snapd.autoimport.service", "" + + "[Unit]\n" + + "[Service]\n" + + "ExecStart=/usr/bin/snap auto-import\n" + + "# X-Snapd-Snap: do-not-start", + }, + {"lib/systemd/system/snapd.socket", "" + + "[Unit]\n" + + "[Socket]\n" + + "ListenStream=/run/snapd.socket", + }, + {"lib/systemd/system/snapd.snap-repair.timer", "" + + "[Unit]\n" + + "[Timer]\n" + + "OnCalendar=*-*-* 5,11,17,23:00", + }, + {"lib/systemd/system/snapd.apparmor.service", "" + + "[Unit]\n[Service]\n" + + "ExecStart=/usr/lib/snapd/snapd-apparmor start\n" + + "# X-Snapd-Snap: do-not-start\n", + }, // user services - {"usr/lib/systemd/user/snapd.session-agent.service", "[Unit]\n[Service]\nExecStart=/usr/bin/snap session-agent"}, - {"usr/lib/systemd/user/snapd.session-agent.socket", "[Unit]\n[Socket]\nListenStream=%t/snap-session.socket"}, + {"usr/lib/systemd/user/snapd.session-agent.service", "" + + "[Unit]\n" + + "[Service]\n" + + "ExecStart=/usr/bin/snap session-agent", + }, + {"usr/lib/systemd/user/snapd.session-agent.socket", "" + + "[Unit]\n" + + "[Socket]\n" + + "ListenStream=%t/snap-session.socket", + }, // D-Bus configuration {"usr/share/dbus-1/session.d/snapd.session-services.conf", ""}, {"usr/share/dbus-1/system.d/snapd.system-services.conf", ""}, // Extra non-snapd D-Bus config that shouldn't be copied {"usr/share/dbus-1/system.d/io.netplan.Netplan.conf", ""}, // D-Bus activation files - {"usr/share/dbus-1/services/io.snapcraft.Launcher.service", "[D-BUS Service]\nName=io.snapcraft.Launcher"}, - {"usr/share/dbus-1/services/io.snapcraft.Settings.service", "[D-BUS Service]\nName=io.snapcraft.Settings"}, - {"usr/share/dbus-1/services/io.snapcraft.SessionAgent.service", "[D-BUS Service]\nName=io.snapcraft.SessionAgent"}, + {"usr/share/dbus-1/services/io.snapcraft.Launcher.service", "" + + "[D-BUS Service]\n" + + "Name=io.snapcraft.Launcher"}, + {"usr/share/dbus-1/services/io.snapcraft.Settings.service", "" + + "[D-BUS Service]\n" + + "Name=io.snapcraft.Settings", + }, + {"usr/share/dbus-1/services/io.snapcraft.SessionAgent.service", "" + + "[D-BUS Service]\n" + + "Name=io.snapcraft.SessionAgent", + }, // desktop files - {"usr/share/applications/io.snapcraft.SessionAgent.desktop", "[Desktop Entry]\nName=Snapd User Session Agent"}, - {"usr/share/applications/snap-handle-link.desktop", "[Desktop Entry]\nName=Handler for snap:// URIs"}, - }) + {"usr/share/applications/io.snapcraft.SessionAgent.desktop", "" + + "[Desktop Entry]\n" + + "Name=Snapd User Session Agent", + }, + {"usr/share/applications/snap-handle-link.desktop", "" + + "[Desktop Entry]\n" + + "Name=Handler for snap:// URIs", + }, + } + + content := append(defaultContent, extra...) + + info := snaptest.MockSnapWithFiles(c, metaSnapYaml, &snap.SideInfo{Revision: snap.R(1)}, content) return info } @@ -207,6 +264,7 @@ WantedBy=snapd.service {"show", "--property=ActiveState", "usr-lib-snapd.mount"}, {"start", "usr-lib-snapd.mount"}, {"daemon-reload"}, + {"is-enabled", "snapd.apparmor.service"}, {"is-enabled", "snapd.autoimport.service"}, {"is-enabled", "snapd.service"}, {"is-enabled", "snapd.snap-repair.timer"}, @@ -231,6 +289,152 @@ WantedBy=snapd.service }) } +func (s *servicesTestSuite) TestAddSnapServicesOperationsWithQuirksOlderThan_2_62(c *C) { + var quirkySnapdYaml = ` +name: snapd +version: 2.58.2 +type: snapd +`[1:] + + extras := [][]string{ + // special case for snapd versions < 2.62, where snapd.apparmor.service did not have the no-restart tag + {"lib/systemd/system/snapd.apparmor.service", "" + + "[Unit]\n[Service]\n" + + "ExecStart=/usr/lib/snapd/snapd-apparmor start\n", + }, + } + + // snapd.apparmor is not restarted, even though it has no do-not-start tag + expectedOps := [][]string{ + {"daemon-reload"}, + {"--no-reload", "enable", "usr-lib-snapd.mount"}, + {"stop", "usr-lib-snapd.mount"}, + {"show", "--property=ActiveState", "usr-lib-snapd.mount"}, + {"start", "usr-lib-snapd.mount"}, + {"daemon-reload"}, + {"is-enabled", "snapd.apparmor.service"}, + {"is-enabled", "snapd.autoimport.service"}, + {"is-enabled", "snapd.service"}, + {"is-enabled", "snapd.snap-repair.timer"}, + {"is-enabled", "snapd.socket"}, + {"--no-reload", "enable", "snapd.socket"}, + {"is-enabled", "snapd.system-shutdown.service"}, + {"is-active", "snapd.snap-repair.timer"}, + {"stop", "snapd.snap-repair.timer"}, + {"show", "--property=ActiveState", "snapd.snap-repair.timer"}, + {"start", "snapd.snap-repair.timer"}, + {"is-active", "snapd.socket"}, + {"--user", "--global", "--no-reload", "disable", "snapd.session-agent.service"}, + {"--user", "--global", "--no-reload", "enable", "snapd.session-agent.service"}, + {"--user", "--global", "--no-reload", "disable", "snapd.session-agent.socket"}, + {"--user", "--global", "--no-reload", "enable", "snapd.session-agent.socket"}, + {"--user", "daemon-reload"}, + {"start", "--no-block", "snapd.apparmor.service"}, + {"start", "--no-block", "snapd.service"}, + {"start", "--no-block", "snapd.seeded.service"}, + {"start", "--no-block", "snapd.autoimport.service"}, + } + + s.testAddSnapServicesOperationsWithQuirks(c, quirkySnapdYaml, extras, expectedOps) +} + +func (s *servicesTestSuite) TestAddSnapServicesOperationsWithQuirksNewerThan_2_62(c *C) { + var quirkySnapdYaml = ` +name: snapd +version: 2.62 +type: snapd +`[1:] + + extras := [][]string{ + // with 2.62+ the presence/absence of do-not-start tag is respected + {"lib/systemd/system/snapd.apparmor.service", "" + + "[Unit]\n[Service]\n" + + // no tag, triggers restart + "ExecStart=/usr/lib/snapd/snapd-apparmor start\n", + }, + } + + expectedOps := [][]string{ + {"daemon-reload"}, + {"--no-reload", "enable", "usr-lib-snapd.mount"}, + {"stop", "usr-lib-snapd.mount"}, + {"show", "--property=ActiveState", "usr-lib-snapd.mount"}, + {"start", "usr-lib-snapd.mount"}, + {"daemon-reload"}, + {"is-enabled", "snapd.apparmor.service"}, + {"is-enabled", "snapd.autoimport.service"}, + {"is-enabled", "snapd.service"}, + {"is-enabled", "snapd.snap-repair.timer"}, + {"is-enabled", "snapd.socket"}, + {"--no-reload", "enable", "snapd.socket"}, + {"is-enabled", "snapd.system-shutdown.service"}, + // snapd.apparmor.service is restarted + {"is-active", "snapd.apparmor.service"}, + {"stop", "snapd.apparmor.service"}, + {"show", "--property=ActiveState", "snapd.apparmor.service"}, + {"start", "snapd.apparmor.service"}, + + {"is-active", "snapd.snap-repair.timer"}, + {"stop", "snapd.snap-repair.timer"}, + {"show", "--property=ActiveState", "snapd.snap-repair.timer"}, + {"start", "snapd.snap-repair.timer"}, + {"is-active", "snapd.socket"}, + {"--user", "--global", "--no-reload", "disable", "snapd.session-agent.service"}, + {"--user", "--global", "--no-reload", "enable", "snapd.session-agent.service"}, + {"--user", "--global", "--no-reload", "disable", "snapd.session-agent.socket"}, + {"--user", "--global", "--no-reload", "enable", "snapd.session-agent.socket"}, + {"--user", "daemon-reload"}, + {"start", "--no-block", "snapd.apparmor.service"}, + {"start", "--no-block", "snapd.service"}, + {"start", "--no-block", "snapd.seeded.service"}, + {"start", "--no-block", "snapd.autoimport.service"}, + } + + s.testAddSnapServicesOperationsWithQuirks(c, quirkySnapdYaml, extras, expectedOps) +} + +func (s *servicesTestSuite) testAddSnapServicesOperationsWithQuirks( + c *C, metaYaml string, extraContent [][]string, expectedOps [][]string, +) { + restore := release.MockOnClassic(false) + defer restore() + + restore = release.MockReleaseInfo(&release.OS{ID: "ubuntu"}) + defer restore() + + // reset root dir + dirs.SetRootDir(s.tempdir) + + systemctlRestorer := systemd.MockSystemctl(func(cmd ...string) ([]byte, error) { + s.sysdLog = append(s.sysdLog, cmd) + if cmd[0] == "show" && cmd[1] == "--property=Id,ActiveState,UnitFileState,Type" { + s := fmt.Sprintf("Type=oneshot\nId=%s\nActiveState=inactive\nUnitFileState=enabled\n", cmd[2]) + return []byte(s), nil + } + if len(cmd) == 2 && cmd[0] == "is-enabled" { + // pretend snapd.socket is disabled + if cmd[1] == "snapd.socket" { + return []byte("disabled"), &mockSystemctlError{msg: "disabled", exitCode: 1} + } + return []byte("enabled"), nil + } + return []byte("ActiveState=inactive\n"), nil + }) + defer systemctlRestorer() + + info := makeMockSnapdSnapWithOverrides(c, metaYaml, extraContent) + // add the snapd service + restart, err := wrappers.AddSnapdSnapServices(info, nil, progress.Null) + c.Assert(err, IsNil) + if restart != nil { + err = restart.Restart() + c.Assert(err, IsNil) + } + + // check the systemctl calls + c.Check(s.sysdLog, DeepEquals, expectedOps) +} + func (s *servicesTestSuite) TestAddSnapServicesForSnapdOnCorePreseeding(c *C) { restore := release.MockOnClassic(false) defer restore() @@ -317,6 +521,7 @@ WantedBy=snapd.service // check the systemctl calls c.Check(s.sysdLog, DeepEquals, [][]string{ {"--root", s.tempdir, "enable", "usr-lib-snapd.mount"}, + {"--root", s.tempdir, "enable", "snapd.apparmor.service"}, {"--root", s.tempdir, "enable", "snapd.autoimport.service"}, {"--root", s.tempdir, "enable", "snapd.service"}, {"--root", s.tempdir, "enable", "snapd.snap-repair.timer"}, diff --git a/wrappers/services_test.go b/wrappers/services_test.go index ca5c1eb8bed..293686d600f 100644 --- a/wrappers/services_test.go +++ b/wrappers/services_test.go @@ -2751,7 +2751,7 @@ func (s *servicesTestSuite) TestAddSnapServicesAndRemoveUserDaemons(c *C) { } var snapdYaml = `name: snapd -version: 1.0 +version: 2.62 type: snapd ` From 000ff910dfc493f5fd5d7ea27fc3a2fbd7dae91f Mon Sep 17 00:00:00 2001 From: Maciej Borzecki Date: Tue, 7 Jan 2025 12:56:49 +0100 Subject: [PATCH 7/9] osutil/user: look up getent executable in known host directories (#14792) * osutil/user: search for getent executable in known host directories It is possible that `snap run` may be called with a PATH set such that it does not include a directory containing getent. To workaround this, try to search for getent in a number of known locations on the host. Fixes: https://bugs.launchpad.net/snapd/+bug/2090938 Signed-off-by: Maciej Borzecki * osutil/user: allow overriding getent search path Signed-off-by: Maciej Borzecki * osutil: update user lookup tests for possible build with snapdusergo tag The snapdusergo tag disables use of cgo/Go based user.Lookup() (and similar) code, and instead provides alternative implementations that use getent under the hood. Adjust the tests to account for this. Signed-off-by: Maciej Borzecki --------- Signed-off-by: Maciej Borzecki --- osutil/group.go | 1 + osutil/group_test.go | 20 +++++++++++++++++++ osutil/user.go | 2 +- osutil/user/export_test.go | 8 ++++++++ osutil/user/getent.go | 39 +++++++++++++++++++++++++++++++++++++- osutil/user/getent_test.go | 9 +++++++++ osutil/user_test.go | 23 ++++++++++++++++++++++ 7 files changed, 100 insertions(+), 2 deletions(-) diff --git a/osutil/group.go b/osutil/group.go index 906858fe3c1..fdc47e76933 100644 --- a/osutil/group.go +++ b/osutil/group.go @@ -53,6 +53,7 @@ func MockFindGid(f func(string) (uint64, error)) (restore func()) { // getent returns the identifier of the given UNIX user or group name as // determined by the specified database +// TODO use a single implementation provided by osutil/user func getent(database, name string) (uint64, error) { if database != "passwd" && database != "group" { return 0, fmt.Errorf(`unsupported getent database "%q"`, database) diff --git a/osutil/group_test.go b/osutil/group_test.go index de9e19dd5e6..f33dbd4c0cc 100644 --- a/osutil/group_test.go +++ b/osutil/group_test.go @@ -53,6 +53,11 @@ exit 2`) s.mockGetent = testutil.MockCommand(c, "getent", ` exit 2`) } + + user.OverrideGetentSearchPath("/foo:/bar:" + s.mockGetent.BinDir()) + s.AddCleanup(func() { + user.OverrideGetentSearchPath(user.DefaultGetentSearchPath) + }) } func (s *findUserGroupSuite) TearDownTest(c *check.C) { @@ -137,6 +142,8 @@ func (s *findUserGroupSuite) TestFindUidGetentOtherErrFromFindUid(c *check.C) { func (s *findUserGroupSuite) TestFindUidGetentMockedOtherError(c *check.C) { s.mockGetent = testutil.MockCommand(c, "getent", "exit 3") + // clean up is done in TearDownTest + user.OverrideGetentSearchPath(s.mockGetent.BinDir()) uid, err := osutil.FindUidWithGetentFallback("lakatos") c.Assert(err, check.ErrorMatches, "getent failed with: exit status 3") @@ -244,6 +251,8 @@ func (s *findUserGroupSuite) TestFindGidGetentNonexistent(c *check.C) { func (s *findUserGroupSuite) TestFindGidGetentMockedOtherError(c *check.C) { s.mockGetent = testutil.MockCommand(c, "getent", "exit 3") + // clean up is done in TearDownTest + user.OverrideGetentSearchPath(s.mockGetent.BinDir()) gid, err := osutil.FindGidWithGetentFallback("lakatos") c.Assert(err, check.ErrorMatches, "getent failed with: exit status 3") @@ -263,6 +272,8 @@ func (s *findUserGroupSuite) TestFindGidGetentMockedOtherError(c *check.C) { func (s *findUserGroupSuite) TestFindGidGetentMocked(c *check.C) { s.mockGetent = testutil.MockCommand(c, "getent", "echo lakatos:x:1234:") + // clean up is done in TearDownTest + user.OverrideGetentSearchPath(s.mockGetent.BinDir()) gid, err := osutil.FindGidWithGetentFallback("lakatos") c.Assert(err, check.IsNil) @@ -272,6 +283,15 @@ func (s *findUserGroupSuite) TestFindGidGetentMocked(c *check.C) { }) } +func (s *findUserGroupSuite) TestFindNoGetentMocked(c *check.C) { + // clean up is done in TearDownTest + user.OverrideGetentSearchPath("/foo:/bar") + + uid, err := osutil.FindUidWithGetentFallback("lakatos") + c.Assert(err, check.ErrorMatches, "user: unknown user lakatos") + c.Check(uid, check.Equals, uint64(0)) +} + func (s *findUserGroupSuite) TestIsUnknownUser(c *check.C) { c.Check(osutil.IsUnknownUser(nil), check.Equals, false) c.Check(osutil.IsUnknownUser(fmt.Errorf("something else")), check.Equals, false) diff --git a/osutil/user.go b/osutil/user.go index 3d0eb93a816..f1a0a9b8f2e 100644 --- a/osutil/user.go +++ b/osutil/user.go @@ -370,7 +370,7 @@ func UserMaybeSudoUser() (*user.User, error) { return cur, nil } - real, err := user.Lookup(realName) + real, err := userLookup(realName) // This is a best effort, see the comment in findGidNoGetentFallback in // group.go. // diff --git a/osutil/user/export_test.go b/osutil/user/export_test.go index c9580079c52..edd34361c7f 100644 --- a/osutil/user/export_test.go +++ b/osutil/user/export_test.go @@ -19,6 +19,10 @@ package user +import ( + "github.com/snapcore/snapd/testutil" +) + var ( LookupGroupFromGetent = lookupGroupFromGetent LookupUserFromGetent = lookupUserFromGetent @@ -26,3 +30,7 @@ var ( UserMatchUsername = userMatchUsername GroupMatchGroupname = groupMatchGroupname ) + +func MockGetentSearchPath(p string) (restore func()) { + return testutil.Mock(&getentSearchPath, p) +} diff --git a/osutil/user/getent.go b/osutil/user/getent.go index cacef50e094..c1db043d545 100644 --- a/osutil/user/getent.go +++ b/osutil/user/getent.go @@ -24,13 +24,42 @@ import ( "bytes" "errors" "fmt" + "os" "os/exec" + "path/filepath" "strconv" "strings" ) +const ( + DefaultGetentSearchPath = "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin" +) + +var ( + getentSearchPath = DefaultGetentSearchPath +) + +func findGetent(searchPath string) (string, error) { + // try to look for getent in a couple of places, such that even when running + // with modified PATH we still can locate the executable + for _, dir := range filepath.SplitList(searchPath) { + p := filepath.Join(dir, "getent") + if fi, err := os.Stat(p); err == nil { + if !fi.IsDir() && fi.Mode().Perm()&0111 != 0 { + return p, nil + } + } + } + return "", errors.New("cannot locate getent executable") +} + func getEnt(params ...string) ([]byte, error) { - cmd := exec.Command("getent", params...) + getentCmd, err := findGetent(getentSearchPath) + if err != nil { + return nil, err + } + + cmd := exec.Command(getentCmd, params...) cmd.Stdin = nil outBuf, err := cmd.Output() @@ -194,3 +223,11 @@ func lookupUserFromGetent(matcher userMatcher) (*User, error) { HomeDir: components[5], }, nil } + +// OverrideGetentSearchPath allows overriding getent search path. Its only +// purpose is to be used in tests. +func OverrideGetentSearchPath(p string) { + // TODO should use osutil.MustBeTestBinary() but we cannot import due to + // cyclic dependencies + getentSearchPath = p +} diff --git a/osutil/user/getent_test.go b/osutil/user/getent_test.go index b21ba37427d..20e746cf79a 100644 --- a/osutil/user/getent_test.go +++ b/osutil/user/getent_test.go @@ -54,6 +54,7 @@ if [ -f "${base}.exit" ]; then exit "$(cat "${base}.exit")" fi `, s.getentDir)) + s.AddCleanup(user.MockGetentSearchPath(s.mockGetent.BinDir() + ":" + user.DefaultGetentSearchPath)) s.AddCleanup(s.mockGetent.Restore) } @@ -177,3 +178,11 @@ func (s *getentSuite) TestLookupGroupByNameMissing(c *C) { c.Assert(err, IsNil) c.Assert(grp, IsNil) } + +func (s *getentSuite) TestNoGetentBinary(c *C) { + defer user.MockGetentSearchPath("/foo:/bar")() + + usr, err := user.LookupUserFromGetent(user.UserMatchUid(1000)) + c.Assert(err, ErrorMatches, "cannot locate getent executable") + c.Assert(usr, IsNil) +} diff --git a/osutil/user_test.go b/osutil/user_test.go index 616c0a57774..9d82f8db42f 100644 --- a/osutil/user_test.go +++ b/osutil/user_test.go @@ -267,6 +267,29 @@ func (s *createUserSuite) TestUserMaybeSudoUser(c *check.C) { }, nil }) defer restore() + restore = osutil.MockUserLookup(func(username string) (*user.User, error) { + switch username { + case "guy": + return &user.User{ + Uid: "1000", + Gid: "1000", + Username: username, + Name: "guy", + HomeDir: "~", + }, nil + case "root": + return &user.User{ + Uid: "0", + Gid: "0", + Username: username, + Name: "root", + HomeDir: "/", + }, nil + default: + return nil, fmt.Errorf("unexpected username in test: %s", username) + } + }) + defer restore() os.Setenv("SUDO_USER", t.SudoUsername) cur, err := osutil.UserMaybeSudoUser() From f44bca2abb421f33763064a88e1381c05f9e43a1 Mon Sep 17 00:00:00 2001 From: Oliver Calder Date: Fri, 29 Nov 2024 09:30:55 -0600 Subject: [PATCH 8/9] i/p/patterns: disallow /./ and /../ in path patterns (#14774) * i/p/patterns: disallow /./ and /../ in path patterns Since patterns like /foo/./bar don't match paths paths like /foo/bar, throw an error if the client tries to reply or create a rule with such a construction clearly in the pattern. That is, if the pattern contains `/./` or `/../`, or if it ends with `/.` or `/..`. Notably, we do this on the raw pattern string, similarly to how we validate that the pattern begins with a `/`: we don't consider whether all alts in a leading group happen to start with `/`, we simply throw an error if the first character is not `/`. In this case, we take a more lenient (but similarly lazy) approach: if there's a literal `/./` or `/../`, or trailing `/.` or `/..`, then an error is thrown. As long as there's something interrupting the `/` or end of the pattern string (e.g. a group), we consider that fine. For example, we allow `/foo/.{,bar}` even though technically this renders to the variants `/foo/.` and `/foo/.bar`, where the former is undesirable. We may reconsider this in the future, but checking whether any rendered variant contains `/./` or `/../` in general requires fully rendering each variant and checking each one, which we do not do at parse time, and one nearly always has at least one pattern which is capable of matching something valid (an exception being the pattern `/foo/.{,}`, for example). But the user must fairly deliberately shoot themself in the foot to end up in that situation. Again, the worst case if a pattern which is "bad" in this way gets past validation is that we end up with a path pattern which is incapable of matching any paths. This is undesirable, but not problematic. Signed-off-by: Oliver Calder * i/p/requestprompts: clarify comment for the relative path regexp Co-authored-by: Zeyad Yasser --------- Signed-off-by: Oliver Calder Co-authored-by: Zeyad Yasser --- .../prompting/patterns/patterns_test.go | 22 +++++++++++++++++++ interfaces/prompting/patterns/scan.go | 7 ++++++ .../prompting/patterns/scan_internal_test.go | 16 ++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/interfaces/prompting/patterns/patterns_test.go b/interfaces/prompting/patterns/patterns_test.go index 94d8cc15708..3cf6d5bd21e 100644 --- a/interfaces/prompting/patterns/patterns_test.go +++ b/interfaces/prompting/patterns/patterns_test.go @@ -133,6 +133,12 @@ func (s *patternsSuite) TestParsePathPatternHappy(c *C) { "/foo/{a,b}{c,d}{e,f}{g,h,i,j,k}{l,m,n,o,p}{q,r,s,t,u},1,2,3", // expands to 1000, with commas outside groups "/" + strings.Repeat("{a,", 999) + "a" + strings.Repeat("}", 999), "/" + strings.Repeat("{", 999) + "a" + strings.Repeat(",a}", 999), + "/foo/.../bar", + "/foo/...", + "/foo/.{bar,baz}", + "/foo/..{bar,baz}", + "/foo/{bar,baz}.", + "/foo/{bar,baz}..", } { _, err := patterns.ParsePathPattern(pattern) c.Check(err, IsNil, Commentf("valid path pattern %q was incorrectly not allowed", pattern)) @@ -152,6 +158,22 @@ func (s *patternsSuite) TestParsePathPatternUnhappy(c *C) { `file.txt`, `invalid path pattern: pattern must start with '/': "file.txt"`, }, + { + `/foo/./bar`, + `invalid path pattern: pattern cannot contain '/./' or '/../': .*`, + }, + { + `/foo/../bar`, + `invalid path pattern: pattern cannot contain '/./' or '/../': .*`, + }, + { + `/foo/.`, + `invalid path pattern: pattern cannot contain '/./' or '/../': .*`, + }, + { + `/foo/..`, + `invalid path pattern: pattern cannot contain '/./' or '/../': .*`, + }, { `{/,/foo}`, `invalid path pattern: pattern must start with '/': .*`, diff --git a/interfaces/prompting/patterns/scan.go b/interfaces/prompting/patterns/scan.go index 00483d3b64c..f734399f4bc 100644 --- a/interfaces/prompting/patterns/scan.go +++ b/interfaces/prompting/patterns/scan.go @@ -23,6 +23,7 @@ import ( "errors" "fmt" "io" + "regexp" "strings" ) @@ -59,6 +60,9 @@ type token struct { text string } +// relpathFinder matches `/./` and `/../` along with their trailing variants `/.` and `/..` in path patterns. +var relpathFinder = regexp.MustCompile(`/\.(\.)?(/|$)`) + func scan(text string) (tokens []token, err error) { if len(text) == 0 { return nil, errors.New("pattern has length 0") @@ -66,6 +70,9 @@ func scan(text string) (tokens []token, err error) { if text[0] != '/' { return nil, errors.New("pattern must start with '/'") } + if relpathFinder.MatchString(text) { + return nil, errors.New("pattern cannot contain '/./' or '/../'") + } var runes []rune diff --git a/interfaces/prompting/patterns/scan_internal_test.go b/interfaces/prompting/patterns/scan_internal_test.go index 6282ff17d6b..26039203671 100644 --- a/interfaces/prompting/patterns/scan_internal_test.go +++ b/interfaces/prompting/patterns/scan_internal_test.go @@ -123,6 +123,22 @@ func (s *scanSuite) TestScanUnhappy(c *C) { `foo`, `pattern must start with '/'`, }, + { + `/foo/./bar`, + `pattern cannot contain '/./' or '/../'`, + }, + { + `/foo/../bar`, + `pattern cannot contain '/./' or '/../'`, + }, + { + `/foo/.`, + `pattern cannot contain '/./' or '/../'`, + }, + { + `/foo/..`, + `pattern cannot contain '/./' or '/../'`, + }, { `/foo\`, `trailing unescaped '\\' character`, From 81254bd2a22a96a998e0edf314a552fbec9afd99 Mon Sep 17 00:00:00 2001 From: Ernest Lotter Date: Wed, 15 Jan 2025 22:03:36 +0200 Subject: [PATCH 9/9] release: 2.67.1 --- NEWS.md | 9 +++++++++ packaging/arch/PKGBUILD | 2 +- packaging/debian-sid/changelog | 17 +++++++++++++++++ packaging/fedora/snapd.spec | 16 +++++++++++++++- packaging/opensuse/snapd.changes | 5 +++++ packaging/opensuse/snapd.spec | 2 +- packaging/ubuntu-14.04/changelog | 17 +++++++++++++++++ packaging/ubuntu-16.04/changelog | 17 +++++++++++++++++ 8 files changed, 82 insertions(+), 3 deletions(-) diff --git a/NEWS.md b/NEWS.md index 7417d45439d..6bc3cb9af66 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,3 +1,12 @@ +# New in snapd 2.67.1 +* Fix apparmor permissions to allow snaps access to kernel modules and firmware on UC24, which also fixes the kernel-modules-control interface on UC24 +* AppArmor prompting (experimental): disallow /./ and /../ in path patterns +* Fix 'snap run' getent based user lookup in case of bad PATH +* Fix snapd using the incorrect AppArmor version during undo of an refresh for regenerating snap profiles +* Add new syscalls to base templates +* hardware-observe interface: allow riscv_hwprobe syscall +* mount-observe interface: allow listmount and statmount syscalls + # New in snapd 2.67 * AppArmor prompting (experimental): allow overlapping rules * Registry view (experimental): Changes to registry data (from both users and snaps) can be validated and saved by custodian snaps diff --git a/packaging/arch/PKGBUILD b/packaging/arch/PKGBUILD index 2f97d9c3d74..78873953a3c 100644 --- a/packaging/arch/PKGBUILD +++ b/packaging/arch/PKGBUILD @@ -11,7 +11,7 @@ pkgdesc="Service and tools for management of snap packages." depends=('squashfs-tools' 'libseccomp' 'libsystemd' 'apparmor') optdepends=('bash-completion: bash completion support' 'xdg-desktop-portal: desktop integration') -pkgver=2.67 +pkgver=2.67.1 pkgrel=1 arch=('x86_64' 'i686' 'armv7h' 'aarch64') url="https://github.com/snapcore/snapd" diff --git a/packaging/debian-sid/changelog b/packaging/debian-sid/changelog index 66064ac9386..df651d93379 100644 --- a/packaging/debian-sid/changelog +++ b/packaging/debian-sid/changelog @@ -1,3 +1,20 @@ +snapd (2.67.1-1) unstable; urgency=medium + + * New upstream release, LP: #2089691 + - Fix apparmor permissions to allow snaps access to kernel modules + and firmware on UC24, which also fixes the kernel-modules-control + interface on UC24 + - AppArmor prompting (experimental): disallow /./ and /../ in path + patterns + - Fix 'snap run' getent based user lookup in case of bad PATH + - Fix snapd using the incorrect AppArmor version during undo of an + refresh for regenerating snap profiles + - Add new syscalls to base templates + - hardware-observe interface: allow riscv_hwprobe syscall + - mount-observe interface: allow listmount and statmount syscalls + + -- Ernest Lotter Wed, 15 Jan 2025 22:02:37 +0200 + snapd (2.67-1) unstable; urgency=medium * New upstream release, LP: #2089691 diff --git a/packaging/fedora/snapd.spec b/packaging/fedora/snapd.spec index acfbcdf690c..2326b43b13a 100644 --- a/packaging/fedora/snapd.spec +++ b/packaging/fedora/snapd.spec @@ -104,7 +104,7 @@ %endif Name: snapd -Version: 2.67 +Version: 2.67.1 Release: 0%{?dist} Summary: A transactional software package manager License: GPL-3.0-only @@ -1003,6 +1003,20 @@ fi %changelog +* Wed Jan 15 2025 Ernest Lotter +- New upstream release 2.67.1 + - Fix apparmor permissions to allow snaps access to kernel modules + and firmware on UC24, which also fixes the kernel-modules-control + interface on UC24 + - AppArmor prompting (experimental): disallow /./ and /../ in path + patterns + - Fix 'snap run' getent based user lookup in case of bad PATH + - Fix snapd using the incorrect AppArmor version during undo of an + refresh for regenerating snap profiles + - Add new syscalls to base templates + - hardware-observe interface: allow riscv_hwprobe syscall + - mount-observe interface: allow listmount and statmount syscalls + * Mon Dec 02 2024 Ernest Lotter - New upstream release 2.67 - AppArmor prompting (experimental): allow overlapping rules diff --git a/packaging/opensuse/snapd.changes b/packaging/opensuse/snapd.changes index 68612cd92b5..4da1b2ccb9b 100644 --- a/packaging/opensuse/snapd.changes +++ b/packaging/opensuse/snapd.changes @@ -1,3 +1,8 @@ +------------------------------------------------------------------- +Wed Jan 15 20:02:37 UTC 2025 - ernest.lotter@canonical.com + +- Update to upstream release 2.67.1 + ------------------------------------------------------------------- Mon Dec 02 21:14:24 UTC 2024 - ernest.lotter@canonical.com diff --git a/packaging/opensuse/snapd.spec b/packaging/opensuse/snapd.spec index 5ed51ae32ed..78155a0582b 100644 --- a/packaging/opensuse/snapd.spec +++ b/packaging/opensuse/snapd.spec @@ -82,7 +82,7 @@ Name: snapd -Version: 2.67 +Version: 2.67.1 Release: 0 Summary: Tools enabling systems to work with .snap files License: GPL-3.0 diff --git a/packaging/ubuntu-14.04/changelog b/packaging/ubuntu-14.04/changelog index e06ea1f8c5a..e6b2c569c6f 100644 --- a/packaging/ubuntu-14.04/changelog +++ b/packaging/ubuntu-14.04/changelog @@ -1,3 +1,20 @@ +snapd (2.67.1~14.04) trusty; urgency=medium + + * New upstream release, LP: #2089691 + - Fix apparmor permissions to allow snaps access to kernel modules + and firmware on UC24, which also fixes the kernel-modules-control + interface on UC24 + - AppArmor prompting (experimental): disallow /./ and /../ in path + patterns + - Fix 'snap run' getent based user lookup in case of bad PATH + - Fix snapd using the incorrect AppArmor version during undo of an + refresh for regenerating snap profiles + - Add new syscalls to base templates + - hardware-observe interface: allow riscv_hwprobe syscall + - mount-observe interface: allow listmount and statmount syscalls + + -- Ernest Lotter Wed, 15 Jan 2025 22:02:37 +0200 + snapd (2.67~14.04) trusty; urgency=medium * New upstream release, LP: #2089691 diff --git a/packaging/ubuntu-16.04/changelog b/packaging/ubuntu-16.04/changelog index 881ad9cb687..074aad08f3e 100644 --- a/packaging/ubuntu-16.04/changelog +++ b/packaging/ubuntu-16.04/changelog @@ -1,3 +1,20 @@ +snapd (2.67.1) xenial; urgency=medium + + * New upstream release, LP: #2089691 + - Fix apparmor permissions to allow snaps access to kernel modules + and firmware on UC24, which also fixes the kernel-modules-control + interface on UC24 + - AppArmor prompting (experimental): disallow /./ and /../ in path + patterns + - Fix 'snap run' getent based user lookup in case of bad PATH + - Fix snapd using the incorrect AppArmor version during undo of an + refresh for regenerating snap profiles + - Add new syscalls to base templates + - hardware-observe interface: allow riscv_hwprobe syscall + - mount-observe interface: allow listmount and statmount syscalls + + -- Ernest Lotter Wed, 15 Jan 2025 22:02:37 +0200 + snapd (2.67) xenial; urgency=medium * New upstream release, LP: #2089691