i/builtin: allow @ in custom-device filepaths #14651

olivercalder · 2024-10-18T20:52:24Z

Allow the @ symbol to occur in filepaths for the custom-device interface. This is common in paths under /sys/, such as /sys/devices/platform/soc@0/soc@0:bus@30000000/30350000.ocotp-ctrl/imx-ocotp0/nvmem.

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-32542

codecov · 2024-10-18T21:12:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.01%. Comparing base (96ea7b0) to head (8cf28da).
Report is 68 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14651      +/-   ##
==========================================
+ Coverage   78.95%   79.01%   +0.06%     
==========================================
  Files        1084     1085       +1     
  Lines      146638   147512     +874     
==========================================
+ Hits       115773   116558     +785     
- Misses      23667    23725      +58     
- Partials     7198     7229      +31

Flag	Coverage Δ
unittests	`79.01% <ø> (+0.06%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

olivercalder · 2024-10-18T21:14:56Z

Need to modify the slot so that the @ is not treated as a variable marker in AppArmor profiles.

alexmurray · 2024-10-20T23:23:49Z

CC @jslarraz we might need to update review-tools to also allow this

bboozzoo · 2024-10-21T06:20:48Z

interfaces/builtin/custom_device.go


 	// Validating regexp for udev device names.
 	// We forbid:
 	// - `|`: it's valid for udev, but more work for us
 	// - `{}`: have a special meaning for AppArmor
+	// - `@`: denotes a variable in AppArmor


I am not sure this is entirely correct, looking at apparmor.d manpage, a variable is defined to have this format:

VARIABLE = '@{' ALPHA [ ( ALPHANUMERIC | '_' ) ... ] '}'

I am also told that folks are using system-files as a workaround, which means that AppArmor parser may actually be happy with paths containing @ as long as it's not @{..}. I'm guessing, it's likely @ does not need escaping at all in this case, since the profiles worked for system-files? At the same time, I suspect that system-files may simply not validate the path correctly, so it'd be great to align both interface implementations to use common code for validation.

@alexmurray @jslarraz do you know what are the rules around @ in profiles is it's not part of a variable reference nor denotes an abstract unit socket path?

My motivation behind prohibiting @ in device paths was to preserve the existing behavior, while allowing @ for filepaths. But there's not a technical reason for this, I just wanted to minimize the change in behavior.

Regarding usage of @, I think there are cases where @ has a special meaning in profiles without being followed by {. Some examples from the manpage:

the @ character, similar to how they are reported (as paths) by netstat -x. The address then follows and may contain pattern matching and any characters including the null character. In apparmor null characters must be specified by using an escape sequence \000 or \x00. The pattern matching is the same as is used by file path matching so * will not match / even though it has no special meaning with in an abstract socket name. Eg. unix addr=@*,

# Allow SOCK_STREAM connect, receive and send on an abstract socket @bar # with peer running under profile '/foo' unix (connect, receive, send) type=stream peer=(label=/foo,addr="@bar"), # Allow accepting connections from and receiving from peer running under # profile '/bar' on abstract socket '@foo' unix (accept, receive) addr=@foo peer=(label=/bar),

But these examples all pertain to unix sockets, not filepaths, so probably it would be fine to not escape @ in filepaths? Though then we need to ensure that filepaths don't contain @{, as right now they can contain {} characters (though device paths cannot). I do think it is safe to universally escape @ symbols, if that would be simpler. I'd be interested to hear what the security people think :)

I think abstract sockets have somewhat unusual apparmor names, at least in audit messages. I don't recall how this behaves in the parser though. I could check quickly and report back.

ernestl · 2024-10-21T13:52:14Z

If feels like we need a test to actually prove whatever we end up doing works?

zyga

I left a pair of comments but ultimately we need to see what happens in the parser.

zyga · 2024-10-21T15:59:38Z

interfaces/builtin/custom_device.go


 	// Validating regexp for udev device names.
 	// We forbid:
 	// - `|`: it's valid for udev, but more work for us
 	// - `{}`: have a special meaning for AppArmor
+	// - `@`: denotes a variable in AppArmor


I think abstract sockets have somewhat unusual apparmor names, at least in audit messages. I don't recall how this behaves in the parser though. I could check quickly and report back.

olivercalder · 2024-11-02T01:08:34Z

Spotted by @ZeyadYasser and myself today, there's a bug in common-files-based interfaces (system-files and personal-files) at the moment. These interfaces allow paths which contain @ characters. This includes paths which end in @. However, filepaths in common-files have {,/,/**} appended to them, which can result in filepaths of the form /foo@{,/,/**}, where the trailing @{,/,/**} looks like an AppArmor variable even though it's not. I'm not yet sure how the AppArmor parser deals with this, when there's no such variable defined.

In a separate PR, I think we should add validation that common-files doesn't allow trailing unescaped @ characters in filepaths.

olivercalder · 2024-11-02T01:10:16Z

Before merging, the "escape @ ..." commit and its revert should be purged from the git log, since the associated messages are no longer correct.

bboozzoo

Thanks

andrewphelpsj

Thanks!

zyga

I had a look at the parser and at this change. I think the conservative approach works well. LGTM!

alexmurray

LGTM - this is the correct approach IMO - allow @ but only if not followed by {. Thanks.

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

AppArmor variables take the form @{foo} in rules, so we cannot allow a specified filepath to contain substrings of this form. Such paths should never be necessary. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

* master: (44 commits) wrappers: do not reload activation units (canonical#14724) gadget/install: support for no{exec,dev,suid} mount flags interfaces/builtin/mount_control: add support for nfs mounts (canonical#14694) tests: use gojq - part 1 (canonical#14686) interfaces/desktop-legacy: allow DBus access to com.canonical.dbusmenu interfaces/builtin/fwupd.go: allow access to nvmem for thunderbolt plugin tests: removing uc16 executions (canonical#14575) tests: Added arm github runner to build snapd (canonical#14504) tests: no need to run spread when there are not tests matching the filter (canonical#14728) tests/lib/tools/store-state: exit on errors, update relevant tests (canonical#14725) tests: udpate the github workflow to run tests suggested by spread-filter tool (canonical#14519) testtime: add mockable timers for use in tests (canonical#14672) interface/screen_inhibit_control: Improve screen inhibit control for use on core (canonical#14134) tests: use images with 20G disk in openstack (canonical#14720) i/builtin: allow @ in custom-device filepaths (canonical#14651) tests: refactor test-snapd-desktop-layout-with-content tests: fix broken app definition tests: capitalize sentences in comments tests: wrap very long shell line tests: fix raciness in async startup and sync install ...

olivercalder added this to the 2.67 milestone Oct 18, 2024

olivercalder requested review from bboozzoo and andrewphelpsj October 18, 2024 20:52

bboozzoo reviewed Oct 21, 2024

View reviewed changes

zyga reviewed Oct 21, 2024

View reviewed changes

olivercalder removed this from the 2.67 milestone Oct 24, 2024

olivercalder force-pushed the SNAPDENG-32542-allow-at-in-paths branch from 53deba2 to 27757ed Compare November 2, 2024 01:04

olivercalder requested review from zyga and bboozzoo November 2, 2024 01:04

olivercalder added the Needs security review Can only be merged once security gave a :+1: label Nov 2, 2024

bboozzoo requested a review from alexmurray November 4, 2024 11:28

bboozzoo approved these changes Nov 4, 2024

View reviewed changes

andrewphelpsj approved these changes Nov 6, 2024

View reviewed changes

olivercalder mentioned this pull request Nov 6, 2024

i/builtin: prohibit trailing '@' in common-files filepaths #14696

Merged

zyga approved these changes Nov 8, 2024

View reviewed changes

olivercalder force-pushed the SNAPDENG-32542-allow-at-in-paths branch from 27757ed to 4b963b0 Compare November 13, 2024 15:04

alexmurray approved these changes Nov 13, 2024

View reviewed changes

olivercalder added 2 commits November 13, 2024 17:27

i/builtin: allow @ in custom-device filepaths

4f2755d

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

i/builtin: disallow @{ in custom-device file paths

8cf28da

AppArmor variables take the form @{foo} in rules, so we cannot allow a specified filepath to contain substrings of this form. Such paths should never be necessary. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder force-pushed the SNAPDENG-32542-allow-at-in-paths branch from 213abe7 to 8cf28da Compare November 13, 2024 23:27

olivercalder merged commit db0e079 into canonical:master Nov 14, 2024
54 of 57 checks passed

olivercalder added this to the 2.67 milestone Nov 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i/builtin: allow @ in custom-device filepaths #14651

i/builtin: allow @ in custom-device filepaths #14651

olivercalder commented Oct 18, 2024

codecov bot commented Oct 18, 2024 •

edited

Loading

olivercalder commented Oct 18, 2024

alexmurray commented Oct 20, 2024

bboozzoo Oct 21, 2024

bboozzoo Oct 21, 2024 •

edited

Loading

olivercalder Oct 21, 2024 •

edited

Loading

zyga Oct 21, 2024

ernestl commented Oct 21, 2024

zyga left a comment

zyga Oct 21, 2024

olivercalder commented Nov 2, 2024

olivercalder commented Nov 2, 2024

bboozzoo left a comment

andrewphelpsj left a comment

zyga left a comment

alexmurray left a comment

i/builtin: allow @ in custom-device filepaths #14651

i/builtin: allow @ in custom-device filepaths #14651

Conversation

olivercalder commented Oct 18, 2024

codecov bot commented Oct 18, 2024 • edited Loading

Codecov Report

olivercalder commented Oct 18, 2024

alexmurray commented Oct 20, 2024

bboozzoo Oct 21, 2024

Choose a reason for hiding this comment

bboozzoo Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

olivercalder Oct 21, 2024 • edited Loading

Choose a reason for hiding this comment

zyga Oct 21, 2024

Choose a reason for hiding this comment

ernestl commented Oct 21, 2024

zyga left a comment

Choose a reason for hiding this comment

zyga Oct 21, 2024

Choose a reason for hiding this comment

olivercalder commented Nov 2, 2024

olivercalder commented Nov 2, 2024

bboozzoo left a comment

Choose a reason for hiding this comment

andrewphelpsj left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

alexmurray left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 18, 2024 •

edited

Loading

bboozzoo Oct 21, 2024 •

edited

Loading

olivercalder Oct 21, 2024 •

edited

Loading