CVE-2024-5138: Insufficient Information (NVD-CWE-noinfo)
value: The snapctl component within snapd allows a confined snap to interact with the snapd daemon to take certain privileged actions on behalf of the snap. It was found that snapctl did not properly parse command-line arguments, allowing an unprivileged user to trigger an authorised action on behalf of the snap that would normally require administrator privileges to perform. This could possibly allow an unprivileged user to perform a denial of service or similar.
CWE description: There is insufficient information about the issue to classify it; details are unkown or unspecified.
Published: 2024-05-31T21:15Z
Repository: https://github.com/snapcore/snapd
Language: YAML
Files changed: overlord/hookstate/ctlcmd/ctlcmd.go, overlord/hookstate/ctlcmd/ctlcmd_test.go, tests/regression/lp-2065077/task.yaml, tests/regression/lp-2065077/test-snapd-sh/bin/sh, tests/regression/lp-2065077/test-snapd-sh/meta/snap.yaml
Commit date: 2024-05-24 13:48:46 +0200
Commit message: 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 <zygmunt.krynicki@canonical.com>

* tests: add regression test for lp-2065077

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

---------

Signed-off-by: Zygmunt Krynicki <zygmunt.krynicki@canonical.com>
Diff: diff --git a/overlord/hookstate/ctlcmd/ctlcmd.go b/overlord/hookstate/ctlcmd/ctlcmd.go
index b663420a3fb..a4a717fd4da 100644
--- a/overlord/hookstate/ctlcmd/ctlcmd.go
+++ b/overlord/hookstate/ctlcmd/ctlcmd.go
@@ -178,16 +178,38 @@ func Run(context *hookstate.Context, args []string, uid uint32) (stdout, stderr
 
 	_, err = parser.ParseArgs(args)
 	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 28cc6f33710..d9c60696939 100644
--- a/overlord/hookstate/ctlcmd/ctlcmd_test.go
+++ b/overlord/hookstate/ctlcmd/ctlcmd_test.go
@@ -119,10 +119,17 @@ func (s *ctlcmdSuite) TestRootRequiredCommandFailure(c *C) {
 
 	c.Check(err, FitsTypeOf, &ctlcmd.ForbiddenCommandError{})
 	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 00000000000..4f94fe15074
--- /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 00000000000..0f845e07c5a
--- /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 00000000000..9d1385700e0
--- /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

Vulnerable code: 	// 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")
Fixed code: // 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.
	// 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
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`)
}

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
#!/bin/sh
PS1='$ '
exec /bin/sh "$@"
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