From 8fc5be2ad78388d8b37527b9c250e1a99d9afc38 Mon Sep 17 00:00:00 2001 From: Valentin David Date: Tue, 15 Aug 2023 12:03:55 +0200 Subject: [PATCH] kernel/fde: simplify invocation of fde-reveal-key --- kernel/fde/cmd_helper.go | 133 +++---------------------------- kernel/fde/export_test.go | 16 ---- kernel/fde/fde_test.go | 156 ++++--------------------------------- kernel/fde/reveal_key.go | 6 +- secboot/secboot_sb_test.go | 4 +- systemd/systemd.go | 7 ++ 6 files changed, 41 insertions(+), 281 deletions(-) diff --git a/kernel/fde/cmd_helper.go b/kernel/fde/cmd_helper.go index 7976bb4a7a2..4ceb529752d 100644 --- a/kernel/fde/cmd_helper.go +++ b/kernel/fde/cmd_helper.go @@ -22,137 +22,28 @@ package fde import ( "bytes" "fmt" - "io/ioutil" - "os" - "os/exec" - "path/filepath" "time" - "github.com/snapcore/snapd/dirs" - "github.com/snapcore/snapd/logger" - "github.com/snapcore/snapd/osutil" + "github.com/snapcore/snapd/systemd" ) // fdeInitramfsHelperRuntimeMax is the maximum runtime a helper can execute // XXX: what is a reasonable default here? var fdeInitramfsHelperRuntimeMax = 2 * time.Minute -// 50 ms means we check at a frequency 20 Hz, fast enough to not hold -// up boot, but not too fast that we are hogging the CPU from the -// thing we are waiting to finish running -var fdeInitramfsHelperPollWait = 50 * time.Millisecond - -// fdeInitramfsHelperPollWaitParanoiaFactor controls much longer we wait -// then fdeInitramfsHelperRuntimeMax before stopping to poll for results -var fdeInitramfsHelperPollWaitParanoiaFactor = 2 - -// overridden in tests -var fdeInitramfsHelperCommandExtra []string - func runFDEinitramfsHelper(name string, stdin []byte) (output []byte, err error) { - runDir := filepath.Join(dirs.GlobalRootDir, "/run", name) - if err := os.MkdirAll(runDir, 0700); err != nil { - return nil, fmt.Errorf("cannot create tmp dir for %s: %v", name, err) - } - - // delete and re-create the std{in,out,err} stream files that we use for the - // hook to be robust against bugs where the files are created with too - // permissive permissions or not properly deleted afterwards since the hook - // will be invoked multiple times during the initrd and we want to be really - // careful since the stdout file will contain the unsealed encryption key - for _, stream := range []string{"stdin", "stdout", "stderr"} { - streamFile := filepath.Join(runDir, name+"."+stream) - // we want to make sure that the file permissions for stdout are always - // 0600, so to ensure this is the case and be robust against bugs, we - // always delete the file and re-create it with 0600 - - // note that if the file already exists, WriteFile will not change the - // permissions, so deleting first is the right thing to do - os.Remove(streamFile) - if stream == "stdin" { - err = os.WriteFile(streamFile, stdin, 0600) - } else { - err = os.WriteFile(streamFile, nil, 0600) - } - if err != nil { - return nil, fmt.Errorf("cannot create %s for %s: %v", name, stream, err) - } + command := []string{name} + + opts := &systemd.RunOptions{ + Properties: []string{ + "DefaultDependencies=no", + "SystemCallFilter=~@mount", + fmt.Sprintf("RuntimeMaxSec=%s", fdeInitramfsHelperRuntimeMax), + }, + Stdin: bytes.NewReader(stdin), } - // TODO: use the new systemd.Run() interface once it supports - // running without dbus (i.e. supports running without --pipe) - cmd := exec.Command( - "systemd-run", - "--collect", - "--service-type=exec", - "--quiet", - // ensure we get some result from the hook within a - // reasonable timeout and output from systemd if - // things go wrong - fmt.Sprintf("--property=RuntimeMaxSec=%s", fdeInitramfsHelperRuntimeMax), - // Do not allow mounting, this ensures hooks in initrd - // can not mess around with ubuntu-data. - // - // Note that this is not about perfect confinement, more about - // making sure that people using the hook know that we do not - // want them to mess around outside of just providing unseal. - "--property=SystemCallFilter=~@mount", - // We do not need any systemd unit dependencies for this - // call. - "--property=DefaultDependencies=no", - // WORKAROUNDS - // workaround the lack of "--pipe" - fmt.Sprintf("--property=StandardInput=file:%s/%s.stdin", runDir, name), - // NOTE: these files are manually created above with 0600 because by - // default systemd will create them 0644 and we want to be paranoid here - fmt.Sprintf("--property=StandardOutput=file:%s/%s.stdout", runDir, name), - fmt.Sprintf("--property=StandardError=file:%s/%s.stderr", runDir, name), - // this ensures we get useful output for e.g. segfaults - fmt.Sprintf(`--property=ExecStopPost=/bin/sh -c 'if [ "$EXIT_STATUS" = 0 ]; then touch %[1]s/%[2]s.success; else echo "service result: $SERVICE_RESULT" >%[1]s/%[2]s.failed; fi'`, runDir, name), - ) - if fdeInitramfsHelperCommandExtra != nil { - cmd.Args = append(cmd.Args, fdeInitramfsHelperCommandExtra...) - } - // "name" is what we actually need to run - cmd.Args = append(cmd.Args, name) - - // ensure we cleanup our tmp files - defer func() { - if err := os.RemoveAll(runDir); err != nil { - logger.Noticef("cannot remove tmp dir: %v", err) - } - }() - - // run the command - output, err = cmd.CombinedOutput() - if err != nil { - return output, err - } - - // This loop will be terminate by systemd-run, either because - // "name" exists or it gets killed when it reaches the - // fdeInitramfsHelperRuntimeMax defined above. - // - // However we are paranoid and exit this loop if systemd - // did not terminate the process after twice the allocated - // runtime - maxLoops := int(fdeInitramfsHelperRuntimeMax/fdeInitramfsHelperPollWait) * fdeInitramfsHelperPollWaitParanoiaFactor - for i := 0; i < maxLoops; i++ { - switch { - case osutil.FileExists(filepath.Join(runDir, name+".failed")): - stderr, _ := ioutil.ReadFile(filepath.Join(runDir, name+".stderr")) - systemdErr, _ := ioutil.ReadFile(filepath.Join(runDir, name+".failed")) - buf := bytes.NewBuffer(stderr) - buf.Write(systemdErr) - return buf.Bytes(), fmt.Errorf("%s failed", name) - case osutil.FileExists(filepath.Join(runDir, name+".success")): - return ioutil.ReadFile(filepath.Join(runDir, name+".stdout")) - default: - time.Sleep(fdeInitramfsHelperPollWait) - } - } + sysd := systemd.New(systemd.SystemMode, nil) - // this should never happen, the loop above should be terminated - // via systemd - return nil, fmt.Errorf("internal error: systemd-run did not honor RuntimeMax=%s setting", fdeInitramfsHelperRuntimeMax) + return sysd.Run(command, opts) } diff --git a/kernel/fde/export_test.go b/kernel/fde/export_test.go index dbd1003630f..d62d2632ca7 100644 --- a/kernel/fde/export_test.go +++ b/kernel/fde/export_test.go @@ -23,14 +23,6 @@ import ( "time" ) -func MockFdeInitramfsHelperCommandExtra(args []string) (restore func()) { - oldFdeRevealKeyCommandExtra := fdeInitramfsHelperCommandExtra - fdeInitramfsHelperCommandExtra = args - return func() { - fdeInitramfsHelperCommandExtra = oldFdeRevealKeyCommandExtra - } -} - func MockFdeRevealKeyRuntimeMax(d time.Duration) (restore func()) { oldFdeRevealKeyRuntimeMax := fdeInitramfsHelperRuntimeMax fdeInitramfsHelperRuntimeMax = d @@ -38,11 +30,3 @@ func MockFdeRevealKeyRuntimeMax(d time.Duration) (restore func()) { fdeInitramfsHelperRuntimeMax = oldFdeRevealKeyRuntimeMax } } - -func MockFdeRevealKeyPollWaitParanoiaFactor(n int) (restore func()) { - oldFdeRevealKeyPollWaitParanoiaFactor := fdeInitramfsHelperPollWaitParanoiaFactor - fdeInitramfsHelperPollWaitParanoiaFactor = n - return func() { - fdeInitramfsHelperPollWaitParanoiaFactor = oldFdeRevealKeyPollWaitParanoiaFactor - } -} diff --git a/kernel/fde/fde_test.go b/kernel/fde/fde_test.go index ab11d0ca54a..067270405fd 100644 --- a/kernel/fde/fde_test.go +++ b/kernel/fde/fde_test.go @@ -36,6 +36,7 @@ import ( "github.com/snapcore/snapd/dirs" "github.com/snapcore/snapd/kernel/fde" "github.com/snapcore/snapd/osutil" + "github.com/snapcore/snapd/systemd" "github.com/snapcore/snapd/testutil" ) @@ -43,6 +44,8 @@ func TestFde(t *testing.T) { TestingT(t) } type fdeSuite struct { testutil.BaseTest + + sysd systemd.Systemd } var _ = Suite(&fdeSuite{}) @@ -50,6 +53,11 @@ var _ = Suite(&fdeSuite{}) func (s *fdeSuite) SetUpTest(c *C) { dirs.SetRootDir(c.MkDir()) s.AddCleanup(func() { dirs.SetRootDir("") }) + + s.sysd = systemd.New(systemd.UserMode, nil) + s.AddCleanup(systemd.MockNewSystemd(func(be systemd.Backend, roodDir string, mode systemd.InstanceMode, meter systemd.Reporter) systemd.Systemd { + return s.sysd + })) } func (s *fdeSuite) TestHasRevealKey(c *C) { @@ -179,8 +187,6 @@ func checkSystemdRunOrSkip(c *C) { func (s *fdeSuite) TestLockSealedKeysCallsFdeReveal(c *C) { checkSystemdRunOrSkip(c) - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() fdeRevealKeyStdin := filepath.Join(c.MkDir(), "stdin") mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", fmt.Sprintf(` cat - > %s @@ -201,38 +207,14 @@ cat - > %s func (s *fdeSuite) TestLockSealedKeysHonorsRuntimeMax(c *C) { checkSystemdRunOrSkip(c) - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", "sleep 60") defer mockSystemdRun.Restore() - restore = fde.MockFdeRevealKeyPollWaitParanoiaFactor(100) - defer restore() - - restore = fde.MockFdeRevealKeyRuntimeMax(100 * time.Millisecond) + restore := fde.MockFdeRevealKeyRuntimeMax(100 * time.Millisecond) defer restore() err := fde.LockSealedKeys() - c.Assert(err, ErrorMatches, `cannot run fde-reveal-key "lock": service result: timeout`) -} - -func (s *fdeSuite) TestLockSealedKeysHonorsParanoia(c *C) { - checkSystemdRunOrSkip(c) - - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() - mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", "sleep 60") - defer mockSystemdRun.Restore() - - restore = fde.MockFdeRevealKeyPollWaitParanoiaFactor(1) - defer restore() - - // shorter than the fdeRevealKeyPollWait time - restore = fde.MockFdeRevealKeyRuntimeMax(1 * time.Millisecond) - defer restore() - - err := fde.LockSealedKeys() - c.Assert(err, ErrorMatches, `cannot run fde-reveal-key "lock": internal error: systemd-run did not honor RuntimeMax=1ms setting`) + c.Assert(err, ErrorMatches, `cannot run \["fde-reveal-key"\]: exit status 1`) } func (s *fdeSuite) TestReveal(c *C) { @@ -244,8 +226,6 @@ func (s *fdeSuite) TestReveal(c *C) { sealedKey := []byte("sealed-v2-payload") v2payload := []byte("unsealed-v2-payload") - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() fdeRevealKeyStdin := filepath.Join(c.MkDir(), "stdin") mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", fmt.Sprintf(` cat - > %s @@ -278,8 +258,6 @@ func (s *fdeSuite) TestRevealV1(c *C) { // fix randutil outcome rand.Seed(1) - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() fdeRevealKeyStdin := filepath.Join(c.MkDir(), "stdin") mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", fmt.Sprintf(` cat - > %s @@ -312,8 +290,6 @@ func (s *fdeSuite) TestRevealV2PayloadV1Hook(c *C) { sealedKey := []byte("sealed-v2-payload") v2payload := []byte("unsealed-v2-payload") - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() fdeRevealKeyStdin := filepath.Join(c.MkDir(), "stdin") mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", fmt.Sprintf(` cat - > %s @@ -348,8 +324,6 @@ func (s *fdeSuite) TestRevealV2BadJSON(c *C) { sealedKey := []byte("sealed-v2-payload") - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() fdeRevealKeyStdin := filepath.Join(c.MkDir(), "stdin") mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", fmt.Sprintf(` cat - > %s @@ -382,8 +356,6 @@ func (s *fdeSuite) TestRevealV1BadOutputSize(c *C) { // fix randutil outcome rand.Seed(1) - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() fdeRevealKeyStdin := filepath.Join(c.MkDir(), "stdin") mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", fmt.Sprintf(` cat - > %s @@ -401,95 +373,6 @@ printf "bad-size" c.Check(osutil.FileExists(filepath.Join(dirs.GlobalRootDir, "/run/fde-reveal-key")), Equals, false) } -func (s *fdeSuite) TestedRevealTruncatesStreamFiles(c *C) { - checkSystemdRunOrSkip(c) - - // fix randutil outcome - rand.Seed(1) - - // create the temporary output file streams with garbage data to ensure that - // by the time the hook runs the files are emptied and recreated with the - // right permissions - streamFiles := []string{} - for _, stream := range []string{"stdin", "stdout", "stderr"} { - streamFile := filepath.Join(dirs.GlobalRootDir, "/run/fde-reveal-key/fde-reveal-key."+stream) - streamFiles = append(streamFiles, streamFile) - // make the dir 0700 - err := os.MkdirAll(filepath.Dir(streamFile), 0700) - c.Assert(err, IsNil) - // but make the file world-readable as it should be reset to 0600 before - // the hook is run - err = os.WriteFile(streamFile, []byte("blah blah blah blah blah blah blah blah blah blah"), 0755) - c.Assert(err, IsNil) - } - - // the hook script only verifies that the stdout file is empty since we - // need to write to the stderr file for performing the test, but we still - // check the stderr file for correct permissions - mockSystemdRun := testutil.MockCommand(c, "fde-reveal-key", fmt.Sprintf(` -# check that stdin has the right sealed key content -if [ "$(cat %[1]s)" != "{\"op\":\"reveal\",\"sealed-key\":\"AQIDBA==\",\"key-name\":\"deprecated-pw7MpXh0JB4P\"}" ]; then - echo "test failed: stdin file has wrong content: $(cat %[1]s)" 1>&2 -else - echo "stdin file has correct content" 1>&2 -fi - -# check that stdout is empty -if [ -n "$(cat %[2]s)" ]; then - echo "test failed: stdout file is not empty: $(cat %[2]s)" 1>&2 -else - echo "stdout file is correctly empty" 1>&2 -fi - -# check that stdin has the right 600 perms -if [ "$(stat --format=%%a %[1]s)" != "600" ]; then - echo "test failed: stdin file has wrong permissions: $(stat --format=%%a %[1]s)" 1>&2 -else - echo "stdin file has correct 600 permissions" 1>&2 -fi - -# check that stdout has the right 600 perms -if [ "$(stat --format=%%a %[2]s)" != "600" ]; then - echo "test failed: stdout file has wrong permissions: $(stat --format=%%a %[2]s)" 1>&2 -else - echo "stdout file has correct 600 permissions" 1>&2 -fi - -# check that stderr has the right 600 perms -if [ "$(stat --format=%%a %[3]s)" != "600" ]; then - echo "test failed: stderr file has wrong permissions: $(stat --format=%%a %[3]s)" 1>&2 -else - echo "stderr file has correct 600 permissions" 1>&2 -fi - -echo "making the hook always fail for simpler test code" 1>&2 - -# always make the hook exit 1 for simpler test code -exit 1 -`, streamFiles[0], streamFiles[1], streamFiles[2])) - defer mockSystemdRun.Restore() - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() - - sealedKey := []byte{1, 2, 3, 4} - p := fde.RevealParams{ - SealedKey: sealedKey, - } - _, err := fde.Reveal(&p) - c.Assert(err, ErrorMatches, `(?s)cannot run fde-reveal-key "reveal": ------ -stdin file has correct content -stdout file is correctly empty -stdin file has correct 600 permissions -stdout file has correct 600 permissions -stderr file has correct 600 permissions -making the hook always fail for simpler test code -service result: exit-code ------`) - // ensure no tmp files are left behind - c.Check(osutil.FileExists(filepath.Join(dirs.GlobalRootDir, "/run/fde-reveal-key")), Equals, false) -} - func (s *fdeSuite) TestRevealErr(c *C) { checkSystemdRunOrSkip(c) @@ -498,29 +381,24 @@ func (s *fdeSuite) TestRevealErr(c *C) { mockSystemdRun := testutil.MockCommand(c, "systemd-run", `echo failed 1>&2; false`) defer mockSystemdRun.Restore() - restore := fde.MockFdeInitramfsHelperCommandExtra([]string{"--user"}) - defer restore() sealedKey := []byte{1, 2, 3, 4} p := fde.RevealParams{ SealedKey: sealedKey, } _, err := fde.Reveal(&p) - c.Assert(err, ErrorMatches, `(?s)cannot run fde-reveal-key "reveal": failed`) + c.Assert(err, ErrorMatches, `(?s)cannot run [[]"fde-reveal-key"[]]: .*failed.*`) - root := dirs.GlobalRootDir + //root := dirs.GlobalRootDir calls := mockSystemdRun.Calls() c.Check(calls, DeepEquals, [][]string{ { - "systemd-run", "--collect", "--service-type=exec", "--quiet", - "--property=RuntimeMaxSec=2m0s", - "--property=SystemCallFilter=~@mount", + "systemd-run", "--wait", "--pipe", "--collect", + "--service-type=exec", "--quiet", "--user", "--property=DefaultDependencies=no", - fmt.Sprintf("--property=StandardInput=file:%s/run/fde-reveal-key/fde-reveal-key.stdin", root), - fmt.Sprintf("--property=StandardOutput=file:%s/run/fde-reveal-key/fde-reveal-key.stdout", root), - fmt.Sprintf("--property=StandardError=file:%s/run/fde-reveal-key/fde-reveal-key.stderr", root), - fmt.Sprintf(`--property=ExecStopPost=/bin/sh -c 'if [ "$EXIT_STATUS" = 0 ]; then touch %[1]s/run/fde-reveal-key/fde-reveal-key.success; else echo "service result: $SERVICE_RESULT" >%[1]s/run/fde-reveal-key/fde-reveal-key.failed; fi'`, root), - "--user", + "--property=SystemCallFilter=~@mount", + "--property=RuntimeMaxSec=2m0s", + "--", "fde-reveal-key", }, }) diff --git a/kernel/fde/reveal_key.go b/kernel/fde/reveal_key.go index c669883b0a9..261740a94b4 100644 --- a/kernel/fde/reveal_key.go +++ b/kernel/fde/reveal_key.go @@ -71,8 +71,8 @@ func LockSealedKeys() error { req := &RevealKeyRequest{ Op: "lock", } - if output, err := runFDERevealKey(req); err != nil { - return fmt.Errorf(`cannot run fde-reveal-key "lock": %v`, osutil.OutputErr(output, err)) + if _, err := runFDERevealKey(req); err != nil { + return err } return nil @@ -111,7 +111,7 @@ func Reveal(params *RevealParams) (payload []byte, err error) { } output, err := runFDERevealKey(req) if err != nil { - return nil, fmt.Errorf(`cannot run fde-reveal-key "reveal": %v`, osutil.OutputErr(output, err)) + return nil, err } // We expect json output that fits the revealKeyResult json at // this point. However the "denver" project uses the old and diff --git a/secboot/secboot_sb_test.go b/secboot/secboot_sb_test.go index 793f4307fc8..465795969dd 100644 --- a/secboot/secboot_sb_test.go +++ b/secboot/secboot_sb_test.go @@ -1390,7 +1390,7 @@ func (s *secbootSuite) TestUnlockEncryptedVolumeUsingKeyErr(c *C) { func (s *secbootSuite) TestUnlockVolumeUsingSealedKeyIfEncryptedFdeRevealKeyErr(c *C) { restore := fde.MockRunFDERevealKey(func(req *fde.RevealKeyRequest) ([]byte, error) { - return nil, fmt.Errorf("helper error") + return nil, fmt.Errorf(`cannot run ["fde-reveal-key"]: helper error`) }) defer restore() @@ -1424,7 +1424,7 @@ func (s *secbootSuite) TestUnlockVolumeUsingSealedKeyIfEncryptedFdeRevealKeyErr( opts := &secboot.UnlockVolumeUsingSealedKeyOptions{} _, err := secboot.UnlockVolumeUsingSealedKeyIfEncrypted(mockDiskWithEncDev, defaultDevice, mockSealedKeyFile, opts) - c.Assert(err, ErrorMatches, `cannot unlock encrypted partition: cannot recover keys because of an unexpected error: cannot run fde-reveal-key "reveal": helper error`) + c.Assert(err, ErrorMatches, `cannot unlock encrypted partition: cannot recover keys because of an unexpected error: cannot run \["fde-reveal-key"\]: helper error`) } // this test that v1 hooks and raw binary v1 created sealedKey files still work diff --git a/systemd/systemd.go b/systemd/systemd.go index 2aaaf4c0aa2..5ab2711c534 100644 --- a/systemd/systemd.go +++ b/systemd/systemd.go @@ -441,6 +441,7 @@ type RunOptions struct { // and let the caller do the keyring setup but feels a bit loose KeyringMode KeyringMode Stdin io.Reader + Properties []string } // A Log is a single entry in the systemd journal. @@ -1730,9 +1731,15 @@ func (s *systemd) Run(command []string, opts *RunOptions) ([]byte, error) { "--service-type=exec", "--quiet", } + if s.mode == UserMode { + runArgs = append(runArgs, "--user") + } if opts.KeyringMode != "" { runArgs = append(runArgs, fmt.Sprintf("--property=KeyringMode=%v", opts.KeyringMode)) } + for _, p := range opts.Properties { + runArgs = append(runArgs, fmt.Sprintf("--property=%v", p)) + } runArgs = append(runArgs, "--") runArgs = append(runArgs, command...) cmd := exec.Command("systemd-run", runArgs...)