cmd/snap-confine: handle CURRENT_TAGS on systems that support it #10540

bboozzoo · 2021-07-16T12:12:44Z

Try to determine whether the libudev/libsystemd supports CURRENT_TAGS which has
been introduced in systemd v247:
https://github.com/systemd/systemd/blob/f6278558da0304ec6b646bb172ce4688c7f162a5/NEWS#L1037-L1119
This comes in pair with TAGS becoming sticky, what means that our checks no
longer behave correctly as the tags do not go away.

If the library supports current tags, use the dynamically collected symbol to
double check that the device is really currently tagged for a snap. The
implementation introduced in v247 correctly checks whether the running systemd
supports current tags and otherwise falls back to looking at TAGS property.

Try to determine whether the libudev/libsystemd supports CURRENT_TAGS which has been introduced in systemd v247: https://github.com/systemd/systemd/blob/f6278558da0304ec6b646bb172ce4688c7f162a5/NEWS#L1037-L1119 This comes in pair with TAGS becoming sticky, what means that our checks no longer behave correctly as the tags do not go away. If the library supports current tags, use the dynamically collected symbol to double check that the device is really currently tagged for a snap. The implementation introduced in v247 correctly checks whether the running systemd supports current tags and otherwise falls back to looking at TAGS property. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

…sticky udev TAGS Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

alexmurray

From a security perspective, this looks fine - snap-confine is setuid root and dlopen does not consult LD_LIBRARY_PATH for setuid binaries so this should be safe from someone trying to supply their own udev_device_has_current_tag in place of the real function from libudev.

Also the use of udev_device_has_current_tag looks fine from what I can see too.

Instead of dlopen() and looking for the symbol ourselves, define a weak symbol and let the dynamic linker do the work. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mardy

LGTM, just comments about comments :-)

mardy · 2021-08-02T07:53:46Z

cmd/snap-confine/udev-support.c

+		 * and on slow devices we may indeed observe a device that no longer
+		 * exists.
+		 *
+		 * Similar debug + continue pattern repeats in all the udev calls in


Thanks for the comment! I was exactly wondering if all those debug() calls in sc_udev_allow_assigned_device() should be something more serious, but this clarifies it.

mardy · 2021-08-02T07:54:58Z

tests/main/security-device-cgroups-serial-port/task.yaml

-    if [ "$SPREAD_REBOOT" = 0 ]; then
-        echo "Given a snap is installed"
-        "$TESTSTOOLS"/snaps-state install-local test-snapd-sh
+    echo "Given a snap is installed"


I see that you didn't originally write this, but maybe it's a good occasion to fix this comment (I guess that it should be "given snap is installed").

mardy · 2021-08-02T07:57:13Z

tests/main/security-device-cgroups-serial-port/task.yaml

-        if [ "$(systemctl --version | awk '/systemd [0-9]+/ { print $2 }')" -ge 247 ]; then
-            REBOOT
-        fi
+    # Reboot needed just on systemd 247+


But now we are not rebooting anymore. Should this comment be removed?

mardy · 2021-08-02T07:57:55Z

tests/main/security-device-cgroups/task.yaml

-    if [ "$SPREAD_REBOOT" = 0 ]; then
-        echo "Given a snap is installed"
-        "$TESTSTOOLS"/snaps-state install-local test-snapd-sh
+    echo "Given a snap is installed"


same as for the other test

anonymouse64

lgtm, one comment about how we can expand the comments in the code and also I agree with @mardy on the spread tests

anonymouse64 · 2021-08-11T02:26:58Z

cmd/snap-confine/udev-support.c

+			debug("cannot find device from syspath %s", path);
+			continue;
+		}
+		if (udev_device_has_current_tag != NULL) {


I think this deserves a comment too, something like

Suggested change

if (udev_device_has_current_tag != NULL) {

// If we are able to query if the device has a current tag,

// do so and if there are no current tags, continue to prevent

// allowing assigned devices to the cgroup - this has the net

// desired effect of not re-creating device cgroups that were

// previously created/setup but should no longer be setup due

// to interface disconnection, etc.

if (udev_device_has_current_tag != NULL) {

…ent-tags

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2021-08-17T13:00:54Z

Add a blocked label again. The testing with a simple binary was showing that the 'weak' symbol is getting properly resolved. However, when I tried with the snap artifact from our github job, the symbol isn't even listed in readelf output:

root@ubuntu:/home/guest# readelf -a -W /snap/snapd/x1/usr/lib/snapd/snap-confine |grep udev_device
000000000061b108  0000002000000007 R_X86_64_JUMP_SLOT     0000000000000000 udev_device_get_devnode@LIBUDEV_183 + 0
000000000061b1b0  0000003500000007 R_X86_64_JUMP_SLOT     0000000000000000 udev_device_new_from_syspath@LIBUDEV_183 + 0
000000000061b2e8  0000005d00000007 R_X86_64_JUMP_SLOT     0000000000000000 udev_device_get_syspath@LIBUDEV_183 + 0
000000000061b390  0000007200000007 R_X86_64_JUMP_SLOT     0000000000000000 udev_device_get_devnum@LIBUDEV_183 + 0
000000000061b460  0000008d00000007 R_X86_64_JUMP_SLOT     0000000000000000 udev_device_unref@LIBUDEV_183 + 0
    32: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND udev_device_get_devnode@LIBUDEV_183 (7)
    53: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND udev_device_new_from_syspath@LIBUDEV_183 (7)
    93: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND udev_device_get_syspath@LIBUDEV_183 (7)                                                                                                                                                                                                                                                                                             
   114: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND udev_device_get_devnum@LIBUDEV_183 (7)
   141: 0000000000000000     0 FUNC    GLOBAL DEFAULT  UND udev_device_unref@LIBUDEV_183 (7)

while building locally:

$ readelf -a -W snap-confine/snap-confine|grep udev_device_has
0000000000018fe8  0000009900000006 R_X86_64_GLOB_DAT      0000000000000000 udev_device_has_current_tag@LIBUDEV_247 + 0
   153: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND udev_device_has_current_tag@LIBUDEV_247 (12)
   328: 0000000000000000     0 FUNC    WEAK   DEFAULT  UND udev_device_has_current_tag@LIBUDEV_247

bboozzoo · 2021-08-17T14:12:39Z

Another data point. A binary built on 20.04 has the weak symbol correctly resolved on Arch, and correctly does not find the symbol on 16.04, although I can clearly see that ld.so tries to look it up:

guest@ubuntu:~/udevtest$ LD_DEBUG=all ./a.out |& grep udev_dev
      6942:     symbol=udev_device_has_current_tag;  lookup in file=./a.out [0]
      6942:     symbol=udev_device_has_current_tag;  lookup in file=/lib/x86_64-linux-gnu/libudev.so.1 [0]
      6942:     symbol=udev_device_has_current_tag;  lookup in file=/lib/x86_64-linux-gnu/libc.so.6 [0]
      6942:     symbol=udev_device_has_current_tag;  lookup in file=/lib/x86_64-linux-gnu/librt.so.1 [0]
      6942:     symbol=udev_device_has_current_tag;  lookup in file=/lib/x86_64-linux-gnu/libpthread.so.0 [0]
      6942:     symbol=udev_device_has_current_tag;  lookup in file=/lib64/ld-linux-x86-64.so.2 [0]

Now a binary built on 16.04, has the weak symbol:

guest@ubuntu:~/udevtest$ nm -a ./a.out |grep udev_dev
                 w udev_device_has_current_tag

but it is not correctly resolved anywhere, neither on 20.04 or Arch, see:

guest@ubuntu:~/udevtest$ ./a.out 
no symbol
guest@ubuntu:~/udevtest$ LD_DEBUG=all ./a.out |& grep udev_dev
guest@ubuntu:~/udevtest$ LD_DEBUG=all ./a.out |& grep udev_
     11685:     symbol=udev_new;  lookup in file=./a.out [0]
     11685:     symbol=udev_new;  lookup in file=/lib/x86_64-linux-gnu/libudev.so.1 [0]
     11685:     binding file ./a.out [0] to /lib/x86_64-linux-gnu/libudev.so.1 [0]: normal symbol `udev_new' [LIBUDEV_183]
     11685:     symbol=udev_unref;  lookup in file=./a.out [0]
     11685:     symbol=udev_unref;  lookup in file=/lib/x86_64-linux-gnu/libudev.so.1 [0]
     11685:     binding file ./a.out [0] to /lib/x86_64-linux-gnu/libudev.so.1 [0]: normal symbol `udev_unref' [LIBUDEV_183]
guest@ubuntu:~/udevtest$

So somehow, the Xenial toolchain breaks how symbols are resolved, and that's why I was not able to observe current tags being checked when I used the snapd snap artifact from our testing.

…ent-tags

This reverts commit 35d28c4.

Note that we could try to define udev_device_has_current_tag with a weak attribute, which should in the normal case be the filled by ld.so when loading snap-confined. However this was observed to work in practice only when the binary itself is build with recent enough toolchain (eg. gcc & binutils on Ubuntu 20.04). In case when the snapd snap needs to be built on 16.04, we have observed that the weak symbol was indeed produced, but when resolving libraries and symbols by ld.so on a recent host, the symbol would not be populated. The patch revers to the original proposed behavior. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo · 2021-09-10T12:49:02Z

I will restore the original proposed behavior to unblock this PR.

bboozzoo · 2021-09-10T13:04:05Z

Pushed an update restoring dlopen() and dropped the 'blocked' label.

codecov-commenter · 2021-09-10T13:23:06Z

Codecov Report

Merging #10540 (76e8529) into master (cfea6ca) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           master   #10540    +/-   ##
========================================
  Coverage   78.34%   78.35%            
========================================
  Files         888      888            
  Lines       99871   100091   +220     
========================================
+ Hits        78247    78425   +178     
- Misses      16718    16748    +30     
- Partials     4906     4918    +12

Flag	Coverage Δ
unittests	`78.35% <ø> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
store/cache.go	`69.23% <0.00%> (-1.93%)`	⬇️
systemd/systemd.go	`85.94% <0.00%> (-0.46%)`	⬇️
overlord/snapstate/storehelpers.go	`79.22% <0.00%> (-0.36%)`	⬇️
overlord/snapstate/handlers.go	`70.80% <0.00%> (-0.01%)`	⬇️
dirs/dirs.go	`94.62% <0.00%> (+0.02%)`	⬆️
overlord/ifacestate/helpers.go	`77.52% <0.00%> (+0.49%)`	⬆️
overlord/hookstate/ctlcmd/refresh.go	`71.64% <0.00%> (+0.76%)`	⬆️
systemd/emulation.go	`40.18% <0.00%> (+3.34%)`	⬆️
overlord/snapstate/backend/mountunit.go	`88.88% <0.00%> (+8.88%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfea6ca...76e8529. Read the comment docs.

…n 14.04 Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

anonymouse64

one question about the check in the spread test, but still lgtm overall

tests/main/security-device-cgroups-serial-port/task.yaml

tests/main/security-device-cgroups/task.yaml

mvo5 · 2021-09-13T15:39:54Z

Fwiw, looks good overall from a (not too deep) look.

anonymouse64

still lgtm

mvo5 · 2021-09-14T08:48:02Z

I removed the 2.52 milestone, I think we want this in 2.53 instead.

bboozzoo added 2 commits July 16, 2021 13:30

tests/main/security-device-cgroups: tweak the tests to better handle …

ff39b70

…sticky udev TAGS Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added the Needs security review Can only be merged once security gave a :+1: label Jul 16, 2021

bboozzoo marked this pull request as draft July 16, 2021 12:12

anonymouse64 self-requested a review July 16, 2021 14:39

bboozzoo mentioned this pull request Jul 20, 2021

cmd/snap-confine: refactor device cgroup handling to enable easier v2 integration #10547

Merged

alexmurray approved these changes Jul 21, 2021

View reviewed changes

alexmurray removed the Needs security review Can only be merged once security gave a :+1: label Jul 21, 2021

bboozzoo marked this pull request as ready for review July 27, 2021 07:20

cmd/snap-confine: let the linker fill the symbol

b281ff1

Instead of dlopen() and looking for the symbol ourselves, define a weak symbol and let the dynamic linker do the work. Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mardy reviewed Aug 2, 2021

View reviewed changes

anonymouse64 approved these changes Aug 11, 2021

View reviewed changes

bboozzoo added 4 commits August 17, 2021 12:11

Merge remote-tracking branch 'upstream/master' into bboozzoo/s-c-curr…

e933a43

…ent-tags

cmd: snap-confine does not need to link with libdl anymore

35d28c4

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

cmd/snap-confine: tweak comment about current tags

99cc3f6

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

tests/main/security-device-cgroup: tweak comments and messages

3493907

Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

bboozzoo added the ⛔ Blocked label Aug 17, 2021

bboozzoo added 3 commits September 10, 2021 14:13

Merge remote-tracking branch 'upstream/master' into bboozzoo/s-c-curr…

8d15bee

…ent-tags

Revert "cmd: snap-confine does not need to link with libdl anymore"

28dd6a0

This reverts commit 35d28c4.

bboozzoo removed the ⛔ Blocked label Sep 10, 2021

mvo5 added this to the 2.52 milestone Sep 13, 2021

tests/main/security-device-cgroups-serial-port: do not restart udev o…

76e8529

…n 14.04 Signed-off-by: Maciej Borzecki <maciej.zenon.borzecki@canonical.com>

mvo5 added the Squash-merge Please squash this PR when merging. label Sep 13, 2021

anonymouse64 reviewed Sep 13, 2021

View reviewed changes

tests/main/security-device-cgroups-serial-port/task.yaml Show resolved Hide resolved

tests/main/security-device-cgroups/task.yaml Show resolved Hide resolved

anonymouse64 approved these changes Sep 13, 2021

View reviewed changes

mvo5 merged commit a125a37 into snapcore:master Sep 14, 2021

mvo5 removed this from the 2.52 milestone Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cmd/snap-confine: handle CURRENT_TAGS on systems that support it #10540

cmd/snap-confine: handle CURRENT_TAGS on systems that support it #10540

bboozzoo commented Jul 16, 2021

alexmurray left a comment

mardy left a comment

mardy Aug 2, 2021

mardy Aug 2, 2021

mardy Aug 2, 2021

mardy Aug 2, 2021

anonymouse64 left a comment

anonymouse64 Aug 11, 2021

bboozzoo commented Aug 17, 2021

bboozzoo commented Aug 17, 2021

bboozzoo commented Sep 10, 2021

bboozzoo commented Sep 10, 2021

codecov-commenter commented Sep 10, 2021 •

edited

anonymouse64 left a comment

mvo5 commented Sep 13, 2021

anonymouse64 left a comment

mvo5 commented Sep 14, 2021

-		if (udev_device_has_current_tag != NULL) {
+                // If we are able to query if the device has a current tag,
+                // do so and if there are no current tags, continue to prevent
+                // allowing assigned devices to the cgroup - this has the net
+                // desired effect of not re-creating device cgroups that were
+                // previously created/setup but should no longer be setup due
+                // to interface disconnection, etc.
+		if (udev_device_has_current_tag != NULL) {

cmd/snap-confine: handle CURRENT_TAGS on systems that support it #10540

cmd/snap-confine: handle CURRENT_TAGS on systems that support it #10540

Conversation

bboozzoo commented Jul 16, 2021

alexmurray left a comment

Choose a reason for hiding this comment

mardy left a comment

Choose a reason for hiding this comment

mardy Aug 2, 2021

Choose a reason for hiding this comment

mardy Aug 2, 2021

Choose a reason for hiding this comment

mardy Aug 2, 2021

Choose a reason for hiding this comment

mardy Aug 2, 2021

Choose a reason for hiding this comment

anonymouse64 left a comment

Choose a reason for hiding this comment

anonymouse64 Aug 11, 2021

Choose a reason for hiding this comment

bboozzoo commented Aug 17, 2021

bboozzoo commented Aug 17, 2021

bboozzoo commented Sep 10, 2021

bboozzoo commented Sep 10, 2021

codecov-commenter commented Sep 10, 2021 • edited

Codecov Report

anonymouse64 left a comment

Choose a reason for hiding this comment

mvo5 commented Sep 13, 2021

anonymouse64 left a comment

Choose a reason for hiding this comment

mvo5 commented Sep 14, 2021

codecov-commenter commented Sep 10, 2021 •

edited