[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 <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>
Gbp-Pq: Name 0005-PATCH-03-18-Merge-pull-request-from-GHSA-p9v8-q5m4-p.patch

diff --git a/overlord/hookstate/ctlcmd/ctlcmd.go b/overlord/hookstate/ctlcmd/ctlcmd.go
index 8f3b67650353344bfa4e9c19ccecbccaf1c717a8..69f6f857ea1568e91a614f1814b535c05ee9dac6 100644 (file)
--- a/overlord/hookstate/ctlcmd/ctlcmd.go
+++ b/overlord/hookstate/ctlcmd/ctlcmd.go
@@ -172,14 +172,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 28cc6f3371034da31fd7186fc58f125debd82972..d9c606969390fecaf58cb7dc9ae68ce686ed83a2 100644 (file)
--- 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 (file)
index 0000000..4f94fe1
--- /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 (executable)
index 0000000..0f845e0
--- /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 (file)
index 0000000..9d13857
--- /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