interfaces/utils: allow commas in filepaths (snapcore#12697)

* interfaces/utils: allow commas in filepaths

Some device paths contain commas outside of groups (i.e. {a,b}) or
classes (i.e. [,.:;'"]).  For example, `/dev/foo,bar` is a valid device
path which one might with to use with the custom-device interface.

Most filesystems allow commas in filepaths, as does apparmor:
https://gitlab.com/apparmor/apparmor/-/blob/master/parser/parser_regex.c#L340

Previously, createRegex() would throw an error if a comma was used
outside of a group or class.  This commit removes that error and instead
treats commas outside of groups and classes as literal commas.  The
accompanying tests are also adjusted to reflect this change.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* interfaces/utils: added argument to allow commas in filepaths

Rather than allowing any caller of `NewPathPattern()` to successfully
validate paths containing commas, this change adds a boolean argument
which explicitly specifies whether commas should be allowed in the
filepath.

There are some risks involved with allowing commas in filepaths (see
discussion at snapcore#12697), so it is
desirable to restrict when commas are allowed based on the caller.  In
particular, superprivileged interfaces (such as `custom-device` and
`mount-control`) have valid needs for commas in filepaths, and users of
these interfaces are individually verified, so it is safe for them to
use `NewPathPattern()` with commas allowed.  Other callers (particularly
unprivileged interfaces) should probably not allow commas.

I was unsure whether `overlord/hookstate/ctlcmd/mount.go` should call
`NewPathPattern()` with commas allowed or not, but since commas had
previously been disallowed and tests continue to pass with
`allowCommas=false`, then I decided to leave it as `false`.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* interfaces/{builtin,utils}: added named variables for allowCommas

Also, switched `overlord/hookstate/ctlcmd/mount.go` to allow commas
(previously did not, but this should match what is allowed in
`interfaces/builtin/mount_control.go`.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* interfaces/utils: added unit tests for commas in paths

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* interfaces/utils: remove `QuoteMeta` when adding `","` to path regex

Since `,` is not a regex special character, the `QuoteMeta` call is
unnecessary.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* interfaces/utils: renamed TestCommasInRegex to TestCreateRegexWithCommas

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* many: added unit tests for callers of NewPathPattern with allowCommas=true

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

---------

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>
Co-authored-by: Michael Vogt <mvo@ubuntu.com>

interfaces/builtin/custom_device.go

@@ -77,7 +77,8 @@ func (iface *customDeviceInterface) validateFilePath(path string, attrName strin
 		return fmt.Errorf(`custom-device %q path is not clean: %q`, attrName, path)
 	}
 
-	if _, err := utils.NewPathPattern(path); err != nil {
+	const allowCommas = true
+	if _, err := utils.NewPathPattern(path, allowCommas); err != nil {
 		return fmt.Errorf(`custom-device %q path cannot be used: %v`, attrName, err)
 	}
 

interfaces/builtin/custom_device_test.go

@@ -409,6 +409,7 @@ slots:
   devices:
     - /dev/input/event[0-9]
     - /dev/input/mice
+    - /dev/dma_heap/qcom,qseecom
   read-devices:
     - /dev/js*
   %s
@@ -428,6 +429,7 @@ apps:
 				{`KERNEL`: `"input/event[0-9]"`},
 				{`KERNEL`: `"input/mice"`},
 				{`KERNEL`: `"js*"`},
+				{`KERNEL`: `"dma_heap/qcom,qseecom"`},
 			},
 		},
 		{
@@ -436,6 +438,7 @@ apps:
 				{`KERNEL`: `"input/event[0-9]"`},
 				{`KERNEL`: `"input/mice"`, `SUBSYSTEM`: `"input"`},
 				{`KERNEL`: `"js*"`},
+				{`KERNEL`: `"dma_heap/qcom,qseecom"`},
 			},
 		},
 		{
@@ -450,6 +453,7 @@ apps:
 				{`KERNEL`: `"input/event[0-9]"`},
 				{`KERNEL`: `"input/mice"`, `SUBSYSTEM`: `"input"`},
 				{`KERNEL`: `"js*"`, `ATTR{attr1}`: `"one"`, `ATTR{attr2}`: `"two"`},
+				{`KERNEL`: `"dma_heap/qcom,qseecom"`},
 			},
 		},
 		{
@@ -471,27 +475,25 @@ apps:
 				},
 				{`KERNEL`: `"input/mice"`, `ATTR{wheel}`: `"true"`},
 				{`KERNEL`: `"js*"`},
+				{`KERNEL`: `"dma_heap/qcom,qseecom"`},
 			},
 		},
 		{
-			`udev-tagging:
-   - kernel: mice
-     attributes:
-      wheel: "true"
-   - kernel: event[0-9]
-     subsystem: input
-     environment:
-      env1: first
-      env2: second|other`,
+			"udev-tagging:\n   - kernel: dma_heap/qcom,qseecom",
 			[]map[string]string{
-				{
-					`KERNEL`:    `"event[0-9]"`,
-					`SUBSYSTEM`: `"input"`,
-					`ENV{env1}`: `"first"`,
-					`ENV{env2}`: `"second|other"`,
-				},
-				{`KERNEL`: `"mice"`, `ATTR{wheel}`: `"true"`},
+				{`KERNEL`: `"input/event[0-9]"`},
+				{`KERNEL`: `"input/mice"`},
+				{`KERNEL`: `"js*"`},
+				{`KERNEL`: `"dma_heap/qcom,qseecom"`},
+			},
+		},
+		{
+			"udev-tagging:\n   - kernel: qcom,qseecom",
+			[]map[string]string{
+				{`KERNEL`: `"input/event[0-9]"`},
+				{`KERNEL`: `"input/mice"`},
 				{`KERNEL`: `"js*"`},
+				{`KERNEL`: `"qcom,qseecom"`},
 			},
 		},
 	}

interfaces/builtin/mount_control.go

@@ -367,7 +367,8 @@ func validateWhatAttr(mountInfo *MountInfo) error {
 		return fmt.Errorf(`mount-control "what" pattern is not clean: %q`, what)
 	}
 
-	if _, err := utils.NewPathPattern(what); err != nil {
+	const allowCommas = true
+	if _, err := utils.NewPathPattern(what, allowCommas); err != nil {
 		return fmt.Errorf(`mount-control "what" setting cannot be used: %v`, err)
 	}
 
@@ -393,7 +394,8 @@ func validateWhereAttr(where string) error {
 		return fmt.Errorf(`mount-control "where" pattern is not clean: %q`, where)
 	}
 
-	if _, err := utils.NewPathPattern(where); err != nil {
+	const allowCommas = true
+	if _, err := utils.NewPathPattern(where, allowCommas); err != nil {
 		return fmt.Errorf(`mount-control "where" setting cannot be used: %v`, err)
 	}
 

interfaces/builtin/mount_control_test.go

@@ -404,3 +404,17 @@ func (s *MountControlInterfaceSuite) TestFunctionfsValidates(c *C) {
 	err := interfaces.BeforeConnectPlug(s.iface, plug)
 	c.Check(err, IsNil)
 }
+
+func (s *MountControlInterfaceSuite) TestMountDevicePathWithCommas(c *C) {
+	plugYaml := `
+  mount:
+  - persistent: true
+    what: /dev/dma_heap/qcom,qseecom
+    where: /mnt/foo,bar
+    options: [rw]
+`
+	snapYaml := fmt.Sprintf(mountControlYaml, plugYaml)
+	plug, _ := MockConnectedPlug(c, snapYaml, nil, "mntctl")
+	err := interfaces.BeforeConnectPlug(s.iface, plug)
+	c.Check(err, IsNil)
+}

interfaces/utils/path_patterns.go

@@ -41,7 +41,7 @@ const (
 // createRegex converts the apparmor-like glob sequence into a regex. Loosely
 // using this as reference:
 // https://gitlab.com/apparmor/apparmor/-/blob/master/parser/parser_regex.c#L107
-func createRegex(pattern string, glob GlobFlags) (string, error) {
+func createRegex(pattern string, glob GlobFlags, allowCommas bool) (string, error) {
 	regex := "^"
 
 	appendGlob := func(defaultGlob, nullGlob string) {
@@ -137,6 +137,9 @@ func createRegex(pattern string, glob GlobFlags) (string, error) {
 			if currentGroupLevel > 0 {
 				itemCountInGroup[currentGroupLevel]++
 				regex += "|"
+			} else if allowCommas {
+				// treat commas outside of groups as literal commas
+				regex += ","
 			} else {
 				return "", fmt.Errorf("cannot use ',' outside of group or character class")
 			}
@@ -160,8 +163,8 @@ func createRegex(pattern string, glob GlobFlags) (string, error) {
 	return regex, nil
 }
 
-func NewPathPattern(pattern string) (*PathPattern, error) {
-	regexPattern, err := createRegex(pattern, globDefault)
+func NewPathPattern(pattern string, allowCommas bool) (*PathPattern, error) {
+	regexPattern, err := createRegex(pattern, globDefault, allowCommas)
 	if err != nil {
 		return nil, err
 	}

interfaces/utils/path_patterns_test.go

@@ -57,14 +57,17 @@ func (s *pathPatternsSuite) TestRegexCreationHappy(c *C) {
 		{`/quoted/bracket/[ab\]c]`, d, `^/quoted/bracket/[ab\]c]$`},
 		{`{[,],}`, d, `^([,]|)$`},
 		{`/path/with/comma[,]`, d, `^/path/with/comma[,]$`},
+		{`/path/with/commas,\,,`, d, `^/path/with/commas,,,$`},
+		{`/path/with/comma,{and[,]group,with\,comma}`, d, `^/path/with/comma,(and[,]group|with,comma)$`},
 		{`/$pecial/c^aracters`, d, `^/\$pecial/c\^aracters$`},
 		{`/in/char/class[^$]`, d, `^/in/char/class[^$]$`},
 	}
 
 	for _, testData := range data {
 		pattern := testData.pattern
 		expectedRegex := testData.expectedRegex
-		regex, err := utils.CreateRegex(pattern, testData.glob)
+		const allowCommas = true
+		regex, err := utils.CreateRegex(pattern, testData.glob, allowCommas)
 		c.Assert(err, IsNil, Commentf("%s", pattern))
 		c.Assert(regex, Equals, expectedRegex, Commentf("%s", pattern))
 		// Also, make sure that the obtained regex is valid
@@ -92,12 +95,50 @@ func (s *pathPatternsSuite) TestRegexCreationUnhappy(c *C) {
 	for _, testData := range data {
 		pattern := testData.pattern
 		expectedError := testData.expectedError
-		pathPattern, err := utils.NewPathPattern(pattern)
+		const allowCommas = false
+		pathPattern, err := utils.NewPathPattern(pattern, allowCommas)
 		c.Assert(pathPattern, IsNil, Commentf("%s", pattern))
 		c.Assert(err, ErrorMatches, expectedError, Commentf("%s", pattern))
 	}
 }
 
+func (s *pathPatternsSuite) TestCreateRegexWithCommas(c *C) {
+	data := []struct {
+		pattern        string
+		allowCommas    bool
+		shouldError    bool
+		expectedString string
+	}{
+		{`/dev/sd{a,b,c}`, false, false, `^/dev/sd(a|b|c)$`},
+		{`/dev/sd{a,b,c}`, true, false, `^/dev/sd(a|b|c)$`},
+		{`/dev/dma_heap/qcom,qseecom`, false, true, `cannot use ',' outside of group or character class`},
+		{`/dev/dma_heap/qcom,qseecom`, true, false, `^/dev/dma_heap/qcom,qseecom$`},
+		{`/path/with/comma[,]`, false, false, `^/path/with/comma[,]$`},
+		{`/path/with/comma[,]`, true, false, `^/path/with/comma[,]$`},
+		{`/path/with/commas,\,,`, false, true, `cannot use ',' outside of group or character class`},
+		{`/path/with/commas,\,,`, true, false, `^/path/with/commas,,,$`},
+		{`/path/with/comma,{and[,]group,with\,comma}`, false, true, `cannot use ',' outside of group or character class`},
+		{`/path/with/comma,{and[,]group,with\,comma}`, true, false, `^/path/with/comma,(and[,]group|with,comma)$`},
+		{`/path/with/empty{}group`, false, true, `invalid number of items between {}:.*`},
+		{`/path/with/empty{}group`, true, true, `invalid number of items between {}:.*`},
+	}
+
+	for _, testData := range data {
+		pattern := testData.pattern
+		allowCommas := testData.allowCommas
+		shouldError := testData.shouldError
+		expectedString := testData.expectedString
+		regex, err := utils.CreateRegex(pattern, utils.GlobDefault, allowCommas)
+		if shouldError {
+			c.Assert(regex, Equals, "", Commentf("%s", pattern))
+			c.Assert(err, ErrorMatches, expectedString, Commentf("%s", pattern))
+		} else {
+			c.Assert(regex, Equals, expectedString, Commentf("%s", pattern))
+			c.Assert(err, IsNil, Commentf("%s", pattern))
+		}
+	}
+}
+
 func (s *pathPatternsSuite) TestPatternMatches(c *C) {
 	data := []struct {
 		pattern       string
@@ -126,7 +167,41 @@ func (s *pathPatternsSuite) TestPatternMatches(c *C) {
 		pattern := testData.pattern
 		testPath := testData.testPath
 		expectedMatch := testData.expectedMatch
-		pathPattern, err := utils.NewPathPattern(pattern)
+		const allowCommas = false
+		pathPattern, err := utils.NewPathPattern(pattern, allowCommas)
+		c.Assert(err, IsNil, Commentf("%s", pattern))
+		c.Assert(pathPattern.Matches(testPath), Equals, expectedMatch, Commentf("%s", pattern))
+	}
+}
+
+func (s *pathPatternsSuite) TestPatternWithCommasMatches(c *C) {
+	data := []struct {
+		pattern       string
+		testPath      string
+		expectedMatch bool
+	}{
+		{`/dev/dma_heap/qcom,qseecom`, `/dev/dma_heap/qcom,qseecom`, true},
+		{`/dev/dma_heap/qcom,[,]qseecom`, `/dev/dma_heap/qcom,qseecom`, false},
+		{`/dev/dma_heap/qcom,[,]qseecom`, `/dev/dma_heap/qcom,,qseecom`, true},
+		{`/dev/dma_heap/qcom,{,[,]}qseecom`, `/dev/dma_heap/qcom,qseecom`, true},
+		{`/dev/dma_heap/qcom,{,[,]}qseecom`, `/dev/dma_heap/qcom,,qseecom`, true},
+		{`/dev/dma_heap/qcom,{\,,}qseecom`, `/dev/dma_heap/qcom,qseecom`, true},
+		{`/dev/dma_heap/qcom,{\,,}qseecom`, `/dev/dma_heap/qcom,,qseecom`, true},
+		{`/dev/dma_heap/qcom,{\,,[,]}qseecom`, `/dev/dma_heap/qcom,,,qseecom`, false},
+	}
+
+	const commaError = `cannot use ',' outside of group or character class`
+
+	for _, testData := range data {
+		pattern := testData.pattern
+		testPath := testData.testPath
+		expectedMatch := testData.expectedMatch
+		const allowCommasFalse = false
+		pathPattern, err := utils.NewPathPattern(pattern, allowCommasFalse)
+		c.Assert(pathPattern, IsNil, Commentf("%s", pattern))
+		c.Assert(err, ErrorMatches, commaError, Commentf("%s", pattern))
+		const allowCommasTrue = true
+		pathPattern, err = utils.NewPathPattern(pattern, allowCommasTrue)
 		c.Assert(err, IsNil, Commentf("%s", pattern))
 		c.Assert(pathPattern.Matches(testPath), Equals, expectedMatch, Commentf("%s", pattern))
 	}

overlord/hookstate/ctlcmd/mount.go

@@ -68,7 +68,8 @@ func matchMountPathAttribute(path string, attribute interface{}, snapInfo *snap.
 
 	expandedPattern := snapInfo.ExpandSnapVariables(pattern)
 
-	pp, err := utils.NewPathPattern(expandedPattern)
+	const allowCommas = true
+	pp, err := utils.NewPathPattern(expandedPattern, allowCommas)
 	return err == nil && pp.Matches(path)
 }
 

overlord/hookstate/ctlcmd/mount_test.go

@@ -145,6 +145,12 @@ func (s *mountSuite) SetUpTest(c *C) {
 					"options":    []string{"bind", "ro"},
 					"persistent": false,
 				},
+				map[string]interface{}{
+					"what":       "/dev/dma_heap/qcom,qseecom",
+					"where":      "/dest,with,commas",
+					"options":    []string{"ro"},
+					"persistent": false,
+				},
 			},
 		},
 	}
@@ -369,3 +375,27 @@ func (s *mountSuite) TestHappyWithVariableExpansion(c *C) {
 		{"/path/unit.mount"},
 	})
 }
+
+func (s *mountSuite) TestHappyWithCommasInPath(c *C) {
+	s.injectSnapWithProperPlug(c)
+
+	s.sysd.EnsureMountUnitFileWithOptionsResult = ResultForEnsureMountUnitFileWithOptions{"/path/unit.mount", nil}
+
+	// Now try with commas in the paths
+	_, _, err := ctlcmd.Run(s.mockContext, []string{"mount", "-o", "ro", "/dev/dma_heap/qcom,qseecom", "/dest,with,commas"}, 0)
+	c.Check(err, IsNil)
+	c.Check(s.sysd.EnsureMountUnitFileWithOptionsCalls, DeepEquals, []*systemd.MountUnitOptions{
+		{
+			Lifetime: systemd.Transient,
+			SnapName: "snap1",
+			Revision: "1",
+			What:     "/dev/dma_heap/qcom,qseecom",
+			Where:    "/dest,with,commas",
+			Options:  []string{"ro"},
+			Origin:   "mount-control",
+		},
+	})
+	c.Check(s.sysd.StartCalls, DeepEquals, [][]string{
+		{"/path/unit.mount"},
+	})
+}