interfaces/utils: allow commas in filepaths

[olivercalder]

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 wish 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.

[olivercalder]

Currently, this change affects all uses of interfaces/utils/path_patterns.go:NewPathPattern(). I decided to make the change in this way so there would be consistent rules regarding comma usage in filepaths. If, however, this change should be confined to the custom_device interface, then I would propose a change similar to the attached diff, where the custom_device interface would use NewPathPatternAllowCommas and NewPathPattern would remain unchanged for all other callers. This is slightly more complicated and requires some duplicate testing, but it avoids changing behavior for anything besides the custom_device interface.

I'm very open to any comments or suggestions. Thanks!

restricted_change.txt

[alexmurray]

I was initially quite concerned that by allowing a comma it would then allow malicious snaps to inject arbitrary AppArmor rules and hence escape confinement as the comma is used to delimit separate AppArmor rules within AppArmor profiles.

However, there are a number of things that prevent this currently:

1. NewPathPattern() appears to only be used in the mount-control and custom-device interfaces, both of which need a matching snap declaration from the store to be able to be used, and so a snap publisher can only achieve the ability to inject content with the help of a store admin to grant a matching snap declaration
2. Both of these interfaces quote the generated path with "" which ensures that AppArmor treats the supplied argument as a complete path name rather than as say two AppArmor rules, separated by a comma.

This change does however remove an existing defensive measure to stop possible rule injection, so it will require careful attention to any new future users of NewPathPattern() to ensure they ultimately quote their final argument when emitting AppArmor rules.

So from a security point of view I am not opposed to this change as clearly it is required to allow snaps to specify a custom device path containing a comma which is a real-world use-case.

I wonder if perhaps though it would be worth parameterising NewPathPatter() with a boolean to allow / deny the use of a comma (and hence pass this parameter on to createRegex()) so that this new behaviour can be restricted to just the custom-device interface for now rather than to all / any callers of NewPathPattern() to keep things safer.

[olivercalder]

After a bit more discussion, it seems that there are three options regarding when to allow commas:

1. Allow commas in paths always (current state of the PR)
   • At the moment, with the only callers of NewPathPattern() being the mount-control and custom-device interfaces, this might be fine since both are privileged and presumably need to allow commas in filepaths
   • It is important to vet any future callers of NewPathPattern()
2. Add a boolean parameter to NewPathPattern() and createRegex() which toggles whether commas are allowed in filepaths
   • Since the only callers of NewPathPattern() are mount-control and custom-device, this change would be minimally disruptive
3. Make a separate function NewPathPatternWithCommas() and add the boolean parameter to createRegex(), thus making the default still disallow commas and the new separate function needed to allow them
   • This is probably not necessary given that there are currently only two callers, so widespread changes are not a concern
   • This would involve duplicate testing, which is undesirable

[olivercalder]

I opted for the second option of those listed above. I think a boolean parameter is the most flexible while minimizing duplicate code or execution paths.

One question is 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, I decided to leave it as false. Please reply if this should be changed.

[pedronis]

did a pass

[pedronis]

when we rarely use boolean args we use constants to help reading:

const allowCommas = true
... utils.NewPathPattern(path, allowCommas) ...

[pedronis]

this needs a corresponding unit test

[ernestl]

Looks good.
Left two minor comments.

For the mount control use case, do we currently need comma path support for more than functionfs?

[olivercalder]

Thanks for the review and suggestions @ernestl

Since commas are allowed in filepaths on Linux generally, I don't see why it wouldn't be the case that a user of the mount-control interface might need to mount a device with a comma in its path or mount to a mount point with a comma in its path, regardless of filesystem type. However, I don't think this has yet been a problem in the field. I'm happy to restrict it to the custom-device interface, if that's desirable (particularly with respect to security review).

[pedronis]

this looks good but needs matching unit test extensions

[pedronis]

thanks