snapd icon indicating copy to clipboard operation
snapd copied to clipboard

c/snap-confine, i/udev, i/ifacetest: update snap-confine and snap-device-helper to understand component hook security tags

Open andrewphelpsj opened this issue 10 months ago • 7 comments

This PR teaches snap-confine and snap-device-helper how to parse security tags that represent component hooks.

An example of a security tag from a component hook would be: snap.name+comp.hook.install And one with an instance key: snap.name_instance+comp.hook.install

Something important to note is how these are encoded as udev tags. Currently, when converting a security tag to a udev tag, we replace all . characters in the tag with _ characters because systemd limits udev tags to having only alphanumeric characters, with the addition of the characters - and _. Since security tags can now contain + characters, those will be encoded as two consecutive _ characters.

For example:

"snap.name+comp.hook.install" -> "snap_name__comp_hook_install"
"snap.name_instance+comp.hook.install" -> "snap_name_instance__comp_hook_install"

This allows the conversion to maintain its reversibility.

andrewphelpsj avatar Apr 01 '24 18:04 andrewphelpsj

shellcheck appears to be unhappy:


In tests/main/component-hooks/comp-with-install-hook/meta/hooks/install line 5:
printf "GET /ip HTTP/1.0\r\n\r\n" | nc httpbin.org 80 2>&1 > "${SNAP_COMMON}/install-logs" 
                                                      ^--^ SC2069 (warning): To redirect stdout+stderr, 2>&1 must be last (or use '{ cmd > file; } 2>&1' to clarify).

bboozzoo avatar Apr 02 '24 08:04 bboozzoo

Codecov Report

Attention: Patch coverage is 5.26316% with 18 lines in your changes missing coverage. Please review.

Project coverage is 78.71%. Comparing base (c01787f) to head (a6de4f3). Report is 27 commits behind head on master.

Files Patch % Lines
interfaces/ifacetest/app_set.go 0.00% 18 Missing :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #13775      +/-   ##
==========================================
- Coverage   78.72%   78.71%   -0.02%     
==========================================
  Files        1053     1053              
  Lines      137711   138127     +416     
==========================================
+ Hits       108419   108731     +312     
- Misses      22494    22580      +86     
- Partials     6798     6816      +18     
Flag Coverage Δ
unittests 78.71% <5.26%> (-0.02%) :arrow_down:

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

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Apr 03 '24 02:04 codecov-commenter

This has conflicts now

alfonsosanchezbeato avatar Jun 20 '24 14:06 alfonsosanchezbeato

There are some memory leaks. Please hold on merging this before those are addressed.

==598353== 
==598353== HEAP SUMMARY:
==598353==     in use at exit: 24,755 bytes in 41 blocks
==598353==   total heap usage: 56,850 allocs, 56,809 frees, 22,733,906 bytes allocated
==598353== 
==598353== 72 (24 direct, 48 indirect) bytes in 1 blocks are definitely lost in loss record 22 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x11D1AB: validate_as_snap_or_component_name (snap.c:208)
==598353==    by 0x120E16: sc_snap_component_validate (snap.c:372)
==598353==    by 0x12120D: test_sc_snap_component_validate (snap-test.c:618)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 80 (24 direct, 56 indirect) bytes in 1 blocks are definitely lost in loss record 23 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x120EA2: sc_snap_component_validate (snap.c:386)
==598353==    by 0x121038: test_sc_snap_component_validate (snap-test.c:579)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 80 (24 direct, 56 indirect) bytes in 1 blocks are definitely lost in loss record 24 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x120EA2: sc_snap_component_validate (snap.c:386)
==598353==    by 0x121074: test_sc_snap_component_validate (snap-test.c:583)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 80 (24 direct, 56 indirect) bytes in 1 blocks are definitely lost in loss record 25 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x11D1AB: validate_as_snap_or_component_name (snap.c:208)
==598353==    by 0x120DC3: sc_snap_component_validate (snap.c:366)
==598353==    by 0x12120D: test_sc_snap_component_validate (snap-test.c:618)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 85 (24 direct, 61 indirect) bytes in 1 blocks are definitely lost in loss record 26 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x11D12C: validate_as_snap_or_component_name (snap.c:202)
==598353==    by 0x120E16: sc_snap_component_validate (snap.c:372)
==598353==    by 0x12120D: test_sc_snap_component_validate (snap-test.c:618)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 93 (24 direct, 69 indirect) bytes in 1 blocks are definitely lost in loss record 27 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x11D12C: validate_as_snap_or_component_name (snap.c:202)
==598353==    by 0x120DC3: sc_snap_component_validate (snap.c:366)
==598353==    by 0x1210B3: test_sc_snap_component_validate (snap-test.c:591)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 93 (24 direct, 69 indirect) bytes in 1 blocks are definitely lost in loss record 28 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x11D12C: validate_as_snap_or_component_name (snap.c:202)
==598353==    by 0x120DC3: sc_snap_component_validate (snap.c:366)
==598353==    by 0x1210EB: test_sc_snap_component_validate (snap-test.c:596)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 110 (48 direct, 62 indirect) bytes in 2 blocks are definitely lost in loss record 32 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x120EC7: sc_snap_component_validate (snap.c:339)
==598353==    by 0x12120D: test_sc_snap_component_validate (snap-test.c:618)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== 143 (48 direct, 95 indirect) bytes in 2 blocks are definitely lost in loss record 33 of 37
==598353==    at 0x484D953: calloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==598353==    by 0x110DFD: sc_error_initv (error.c:34)
==598353==    by 0x111252: sc_error_init (error.c:50)
==598353==    by 0x120EA2: sc_snap_component_validate (snap.c:386)
==598353==    by 0x12120D: test_sc_snap_component_validate (snap-test.c:618)
==598353==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==598353==    by 0x10E00C: main (unit-tests-main.c:21)
==598353== 
==598353== LEAK SUMMARY:
==598353==    definitely lost: 264 bytes in 11 blocks
==598353==    indirectly lost: 572 bytes in 11 blocks
==598353==      possibly lost: 0 bytes in 0 blocks
==598353==    still reachable: 23,919 bytes in 19 blocks
==598353==         suppressed: 0 bytes in 0 blocks
==598353== Reachable blocks (those to which a pointer was found) are not shown.
==598353== To see them, rerun with: --leak-check=full --show-leak-kinds=all

zyga avatar Jul 08 '24 12:07 zyga

@zyga thanks for the catch, fixed here: 57e087f3867e431095a351edbcae696d85b6c033.

andrewphelpsj avatar Jul 08 '24 12:07 andrewphelpsj

@andrewphelpsj there are some more:

# End of invocation tests
==603908== 
==603908== HEAP SUMMARY:
==603908==     in use at exit: 23,561 bytes in 18 blocks
==603908==   total heap usage: 6,499 allocs, 6,481 frees, 2,896,109 bytes allocated
==603908== 
==603908== 9 bytes in 1 blocks are definitely lost in loss record 2 of 18
==603908==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==603908==    by 0x11E441: sc_strdup (string-utils.c:72)
==603908==    by 0x11819C: sc_init_invocation (snap-confine-invocation.c:88)
==603908==    by 0x11874A: test_sc_invocation_component (snap-confine-invocation-test.c:130)
==603908==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x10E68C: main (unit-tests-main.c:21)
==603908== 
==603908== 9 bytes in 1 blocks are definitely lost in loss record 3 of 18
==603908==    at 0x4846828: malloc (in /usr/libexec/valgrind/vgpreload_memcheck-amd64-linux.so)
==603908==    by 0x11E441: sc_strdup (string-utils.c:72)
==603908==    by 0x11819C: sc_init_invocation (snap-confine-invocation.c:88)
==603908==    by 0x11831A: test_sc_invocation_component_instance_key (snap-confine-invocation-test.c:154)
==603908==    by 0x48F9287: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x48F91A2: ??? (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x48F9799: g_test_run_suite (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x48F982F: g_test_run (in /usr/lib/x86_64-linux-gnu/libglib-2.0.so.0.8000.3)
==603908==    by 0x10E68C: main (unit-tests-main.c:21)
==603908== 
==603908== LEAK SUMMARY:
==603908==    definitely lost: 18 bytes in 2 blocks
==603908==    indirectly lost: 0 bytes in 0 blocks
==603908==      possibly lost: 0 bytes in 0 blocks
==603908==    still reachable: 23,543 bytes in 16 blocks
==603908==         suppressed: 0 bytes in 0 blocks
==603908== Reachable blocks (those to which a pointer was found) are not shown.
==603908== To see them, rerun with: --leak-check=full --show-leak-kinds=all

zyga avatar Jul 08 '24 12:07 zyga

It looks like the inclusive naming check is picking up some of the snap-confine files that were changed. I think that we probably don't want to include a bunch of renames in this PR, but we don't have a way to skip that check right now. I could add a tag (and a check in the action for that tag) that would allow us to skip it if we want.

andrewphelpsj avatar Jul 08 '24 13:07 andrewphelpsj