From 17eccc7ac861d594f8f8429cb4ba81762e847032 Mon Sep 17 00:00:00 2001 From: Zygmunt Bazyli Krynicki Date: Fri, 24 May 2024 13:48:46 +0200 Subject: [PATCH] [PATCH 03/18] Merge pull request from GHSA-p9v8-q5m4-pf46 * o/hookstate: recognize "--" in snapctl argument parser When parsing snapctl argument vector recognize the "--" as an option terminator, so that dash-options are not recognized afterwards. Fixes: https://bugs.launchpad.net/snapd/+bug/2065077 Signed-off-by: Zygmunt Krynicki * tests: add regression test for lp-2065077 Signed-off-by: Zygmunt Krynicki --------- Signed-off-by: Zygmunt Krynicki Gbp-Pq: Name 0005-PATCH-03-18-Merge-pull-request-from-GHSA-p9v8-q5m4-p.patch --- overlord/hookstate/ctlcmd/ctlcmd.go | 40 ++++++++++++++----- overlord/hookstate/ctlcmd/ctlcmd_test.go | 7 ++++ tests/regression/lp-2065077/task.yaml | 26 ++++++++++++ .../lp-2065077/test-snapd-sh/bin/sh | 3 ++ .../lp-2065077/test-snapd-sh/meta/snap.yaml | 15 +++++++ 5 files changed, 82 insertions(+), 9 deletions(-) create mode 100644 tests/regression/lp-2065077/task.yaml create mode 100755 tests/regression/lp-2065077/test-snapd-sh/bin/sh create mode 100644 tests/regression/lp-2065077/test-snapd-sh/meta/snap.yaml diff --git a/overlord/hookstate/ctlcmd/ctlcmd.go b/overlord/hookstate/ctlcmd/ctlcmd.go index b663420a..a4a717fd 100644 --- a/overlord/hookstate/ctlcmd/ctlcmd.go +++ b/overlord/hookstate/ctlcmd/ctlcmd.go @@ -180,14 +180,36 @@ func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr return stdoutBuffer.Bytes(), stderrBuffer.Bytes(), err } +// isAllowedToRun returns true if the user with the given UID can run the given snapctl command vector. +// +// Commands still need valid context and snaps can only access own config. func isAllowedToRun(uid uint32, args []string) bool { - // A command can run if any of the following are true: - // * It runs as root - // * It's contained in nonRootAllowed - // * It's used with the -h or --help flags - // note: commands still need valid context and snaps can only access own config. - return uid == 0 || - strutil.ListContains(nonRootAllowed, args[0]) || - strutil.ListContains(args, "-h") || - strutil.ListContains(args, "--help") + // Root can run all snapctl commands. + if uid == 0 { + return true + } + + for idx, arg := range args { + // A number of sub-commands are allowed to be executed by non-root users. + if idx == 0 && strutil.ListContains(nonRootAllowed, arg) { + return true + } + + // Invoking help is always allowed. + if arg == "-h" || arg == "--help" { + return true + } + + // Note that we are not interrupting parsing after the first non-option + // argument (POSIX style), because we want to cater to the use case of + // the user appending --help or -h at the end of the command and still + // getting something useful. The only exception is the condition below. + + // The explicit termination argument terminates parsing. + if arg == "--" { + break + } + } + + return false } diff --git a/overlord/hookstate/ctlcmd/ctlcmd_test.go b/overlord/hookstate/ctlcmd/ctlcmd_test.go index 28cc6f33..d9c60696 100644 --- a/overlord/hookstate/ctlcmd/ctlcmd_test.go +++ b/overlord/hookstate/ctlcmd/ctlcmd_test.go @@ -121,6 +121,13 @@ func (s *ctlcmdSuite) TestRootRequiredCommandFailure(c *C) { c.Check(err.Error(), Equals, `cannot use "start" with uid 1000, try with sudo`) } +func (s *ctlcmdSuite) TestRootRequiredCommandFailureParsingTweak(c *C) { + _, _, err := ctlcmd.Run(s.mockContext, []string{"start", "--", "--help"}, 1000) + + c.Check(err, FitsTypeOf, &ctlcmd.ForbiddenCommandError{}) + c.Check(err.Error(), Equals, `cannot use "start" with uid 1000, try with sudo`) +} + func (s *ctlcmdSuite) TestRunNoArgsFailure(c *C) { _, _, err := ctlcmd.Run(s.mockContext, []string{}, 0) c.Check(err, NotNil) diff --git a/tests/regression/lp-2065077/task.yaml b/tests/regression/lp-2065077/task.yaml new file mode 100644 index 00000000..4f94fe15 --- /dev/null +++ b/tests/regression/lp-2065077/task.yaml @@ -0,0 +1,26 @@ +summary: Ensure that snapctl argument parser handles -- + +details: | + Snapctl argument parser used to have a flaw related to parsing -- which + might have lead to incorrect interpretation of the following arguments. + +systems: + - -ubuntu-14.04-* # systemd is too old for generated mount units + +prepare: | + "$TESTSTOOLS"/snaps-state install-local test-snapd-sh + tests.cleanup defer snap remove --purge test-snapd-sh + + snap connect test-snapd-sh:mount-control + + mkdir -p /var/snap/test-snapd-sh/common/base-files + echo 'snapctl mount -o ro,bind,noatime,noexec /usr/share/base-files /var/snap/test-snapd-sh/common/base-files' | snap run --shell test-snapd-sh.sh + mountpoint /var/snap/test-snapd-sh/common/base-files + tests.cleanup defer umount /var/snap/test-snapd-sh/common/base-files + + tests.session prepare -u test + tests.cleanup defer tests.session restore -u test + +execute: | + tests.session -u test exec snap run --shell test-snapd-sh.sh -c 'snapctl umount /var/snap/test-snapd-sh/common/base-files -- --help' 2>&1 | MATCH 'error: cannot use "umount" with uid 12345, try with sudo' + mountpoint /var/snap/test-snapd-sh/common/base-files diff --git a/tests/regression/lp-2065077/test-snapd-sh/bin/sh b/tests/regression/lp-2065077/test-snapd-sh/bin/sh new file mode 100755 index 00000000..0f845e07 --- /dev/null +++ b/tests/regression/lp-2065077/test-snapd-sh/bin/sh @@ -0,0 +1,3 @@ +#!/bin/sh +PS1='$ ' +exec /bin/sh "$@" diff --git a/tests/regression/lp-2065077/test-snapd-sh/meta/snap.yaml b/tests/regression/lp-2065077/test-snapd-sh/meta/snap.yaml new file mode 100644 index 00000000..9d138570 --- /dev/null +++ b/tests/regression/lp-2065077/test-snapd-sh/meta/snap.yaml @@ -0,0 +1,15 @@ +name: test-snapd-sh +version: 1.0 +apps: + sh: + command: bin/sh +plugs: + mount-control: + mount: + - what: /usr/share/base-files + where: $SNAP_COMMON/base-files + options: + - ro + - bind + - noatime + - noexec -- 2.30.2