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

andrewphelpsj · 2024-04-01T18:37:23Z

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.

bboozzoo · 2024-04-02T08:56:56Z

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

codecov-commenter · 2024-04-03T02:51:57Z

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 ⚠️

❗ 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%)`	⬇️

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.

…all matches

…to validate_as_snap_or_component_name

…to take in error details

…_validate Make sure it fails to validate component names with instance keys, test that we fail to validate a component name that matches against a wrong instance key.

…t tests

…uct field

andrewphelpsj · 2024-07-08T13:26:56Z

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.

alfonsosanchezbeato

Looks good, some comments but mainly nitpicks.

cmd/libsnap-confine-private/snap.h

cmd/libsnap-confine-private/snap.c

… array size

alfonsosanchezbeato

LGTM, thanks!

cmd/libsnap-confine-private/snap.c

jslarraz · 2024-07-12T10:58:12Z

cmd/libsnap-confine-private/string-utils.c

+		     char *prefix, size_t prefix_size,
+		     char *suffix, size_t suffix_size)
+{
+	if (string == NULL) {


Maybe those are too evident, but we may still want to add a couple of simple test cases for (string == NULL) and (prefix == NULL && suffix == NULL)

Thanks! Done in fc05b1b.

jslarraz

LGTM, just one minor comment about some extra tests, but feel free to ignore it if you think they do not add much value.

I think it will be worth giving @alexmurray a chance to take a look at it before merging just to be sure I'm not overlooking anything

alexmurray

LGTM!

zyga

Please check my comment on reverse_component_separator_encoding. I left some nitpicks in othe places.

zyga · 2024-07-08T13:07:56Z

cmd/libsnap-confine-private/snap.c

+	}
+	if (n > SNAP_NAME_LEN) {
+		err = sc_error_init(SC_SNAP_DOMAIN, err_code,
+				    "%s must be shorter than 40 characters",


You can use some preprocessor magic to inject SNAP_NAME_LEN as a value there, at compile time, with #SNAP_NAME_LEN. See https://gcc.gnu.org/onlinedocs/cpp/Stringizing.html

Alternatively just %d and pass the limit as an argument.

zyga · 2024-07-10T12:38:56Z

cmd/libsnap-confine-private/snap.h

+ *
+ * For example:
+ *   snap+component => "snap" & "component"
+ *


Please clarify that snap name and component name are copied into the given buffers.

zyga · 2024-07-10T12:40:37Z

cmd/libsnap-confine-private/string-utils.h

+ *
+ * The size of prefix must be large enough to hold the prefix part of the
+ * string, and the size of suffix must be large enough to hold the suffix part
+ * of the string.


I was thinking about this and I kind of think that it's "probably" better to just use strdup+strtok instead.

No need to fuss around with buffers and handle them by hand. One allocation for all the needs for handling typical tag names. Each token can be replaced with \0 for a ready-to-use string.

cmd/libsnap-confine-private/string-utils.c

zyga · 2024-07-12T13:16:48Z

cmd/libsnap-confine-private/snap.c

+	// expression currently contains 9 capture groups, but we only care about these 3,
+	// which unfortunately are not within the first 3 submatches, but rather group 1,
+	// 2, and 7, so for completeness capture all the groups.
+	enum { num_matches = 9 };


Nitpick, this should probably be just const int, enum feels not much like C.

Using a const int would result in this being a variable-length array, if you compile with -Wvla, you'll see a warning.

Example:

phelps@arch ~/tmp> cat main.c int main() { const int size = 10; int array[size]; return 0; } phelps@arch ~/tmp> gcc -Wvla -o main ./main.c ./main.c: In function ‘main’: ./main.c:3:5: warning: ISO C90 forbids variable length array ‘array’ [-Wvla] 3 | int array[size]; | ^~~ phelps@arch ~/tmp> cat other.c int main() { enum { size = 10 }; int array[size]; return 0; } phelps@arch ~/tmp> gcc -Wvla -o main ./other.c

Alfonso suggested the enum over a #define, but I don't really have a strong opinion.

zyga · 2024-07-19T12:46:39Z

cmd/libsnap-confine-private/snap.c

 {
 	/* Don't even check overly long tags. */
 	if (strlen(security_tag) > SNAP_SECURITY_TAG_MAX_LEN) {
 		return false;
 	}
 	const char *whitelist_re =
-	    "^snap\\.([a-z0-9](-?[a-z0-9])*(_[a-z0-9]{1,10})?)\\.([a-zA-Z0-9](-?[a-zA-Z0-9])*|hook\\.[a-z](-?[a-z0-9])*)$";
+	    "^snap\\.([a-z0-9](-?[a-z0-9])*(_[a-z0-9]{1,10})?)(\\.[a-zA-Z0-9](-?[a-zA-Z0-9])*|(\\+([a-z0-9](-?[a-z0-9])*))?\\.hook\\.[a-z](-?[a-z0-9])*)$";


We should clarify exactly what is the allowed pattern, as now there's a disagreement between snapd and snapcraft. Can you circle back to the spec and make sure it says what is allowed (digits).

cmd/libsnap-confine-private/snap.c

cmd/snap-device-helper/snap-device-helper.c

… clarity

zyga

LGTM

I'll follow up on some string operations but this is okay.

github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Apr 1, 2024

andrewphelpsj force-pushed the component-hooks-run branch from 6c5ad3b to acc399d Compare April 1, 2024 19:43

andrewphelpsj force-pushed the component-hooks-run branch 2 times, most recently from 5e4f87d to 236f2ea Compare April 2, 2024 19:59

andrewphelpsj requested review from alfonsosanchezbeato and pedronis April 4, 2024 15:22

andrewphelpsj force-pushed the component-hooks-run branch 3 times, most recently from c43b233 to 429cad9 Compare April 8, 2024 17:50

andrewphelpsj force-pushed the component-hooks-run branch 4 times, most recently from 035b839 to c5d6971 Compare April 17, 2024 15:22

andrewphelpsj force-pushed the component-hooks-run branch 3 times, most recently from eb91528 to c9a9f92 Compare May 7, 2024 14:42

andrewphelpsj force-pushed the component-hooks-run branch 2 times, most recently from 73fa729 to 6cd4807 Compare May 15, 2024 07:44

andrewphelpsj force-pushed the component-hooks-run branch 3 times, most recently from c3f7fdf to 1582077 Compare May 23, 2024 16:26

andrewphelpsj force-pushed the component-hooks-run branch 2 times, most recently from a7b0050 to cb379d3 Compare June 11, 2024 18:15

github-actions bot removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Jun 11, 2024

andrewphelpsj force-pushed the component-hooks-run branch 4 times, most recently from bcb8dbc to 686b2f6 Compare June 20, 2024 13:10

andrewphelpsj added 9 commits July 8, 2024 09:12

c/libsnap-confine-private: update regmatch_t array to have space for …

b0c8a3e

…all matches

c/libsnap-confine-private: rename sc_snap_or_component_name_validate …

7643859

…to validate_as_snap_or_component_name

c/libsnap-confine-private: rework validate_as_snap_or_component_name …

2f7eab6

…to take in error details

c/libsnap-confine-private: fix incorrectly named variable

3f21dac

c/libsnap-confine-private: assert size of regex match array

b7ad65e

c/libsnap-confine-private: test a few more cases in sc_snap_component…

5415317

…_validate Make sure it fails to validate component names with instance keys, test that we fail to validate a component name that matches against a wrong instance key.

c/snap-device-helper: fix variable naming inconsistency

6cabf43

c/libsnap-confine-private: add missing sc_error_free calls to new uni…

5a00500

…t tests

c/snap-confine: add missing free in sc_cleanup_invocation for new str…

0bb442b

…uct field

andrewphelpsj force-pushed the component-hooks-run branch from 57e087f to 0bb442b Compare July 8, 2024 13:15

alfonsosanchezbeato reviewed Jul 8, 2024

View reviewed changes

cmd/libsnap-confine-private/snap.h Outdated Show resolved Hide resolved

cmd/libsnap-confine-private/snap.c Outdated Show resolved Hide resolved

cmd/libsnap-confine-private/snap.c Show resolved Hide resolved

cmd/libsnap-confine-private/snap.c Show resolved Hide resolved

andrewphelpsj added 3 commits July 9, 2024 15:48

c/libsnap-confine-private: update comment on SC_SNAP_INVALID_COMPONENT

ab7d138

c/libsnap-confine-private: replace sizeof math with anonymous enum as…

76555de

… array size

c/libsnap-confine-private: only call strlen on component_name once

ebff4c1

andrewphelpsj requested a review from alfonsosanchezbeato July 9, 2024 20:33

alfonsosanchezbeato approved these changes Jul 9, 2024

View reviewed changes

cmd/libsnap-confine-private/snap.c Show resolved Hide resolved

ernestl requested a review from jslarraz July 11, 2024 13:40

jslarraz reviewed Jul 12, 2024

View reviewed changes

jslarraz approved these changes Jul 12, 2024

View reviewed changes

c/libsnap-confine-private: add tests for NULL checks in sc_string_split

fc05b1b

alexmurray approved these changes Jul 18, 2024

View reviewed changes

alexmurray removed Security-High Needs security review Can only be merged once security gave a :+1: labels Jul 18, 2024

zyga reviewed Jul 19, 2024

View reviewed changes

andrewphelpsj added 2 commits July 19, 2024 09:31

c/libsnap-confine-private: replace for with while

c9a216b

c/libsnap-confine-private: rename sc_string_split function params for…

b1dc0d6

… clarity

zyga approved these changes Jul 19, 2024

View reviewed changes

.woke.yaml: add exclusions for files that contain pre-existing issues

98e8fa9

alfonsosanchezbeato merged commit 98b4e32 into canonical:master Jul 22, 2024
45 of 53 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andrewphelpsj commented Apr 1, 2024 •

edited

Loading

bboozzoo commented Apr 2, 2024

codecov-commenter commented Apr 3, 2024 •

edited

Loading

andrewphelpsj commented Jul 8, 2024

alfonsosanchezbeato left a comment

alfonsosanchezbeato left a comment

jslarraz Jul 12, 2024

andrewphelpsj Jul 12, 2024

jslarraz left a comment

alexmurray left a comment

zyga left a comment

zyga Jul 8, 2024

zyga Jul 10, 2024

zyga Jul 10, 2024

zyga Jul 12, 2024

andrewphelpsj Jul 19, 2024

zyga Jul 19, 2024

zyga Jul 19, 2024

zyga left a comment

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

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

Conversation

andrewphelpsj commented Apr 1, 2024 • edited Loading

bboozzoo commented Apr 2, 2024

codecov-commenter commented Apr 3, 2024 • edited Loading

Codecov Report

andrewphelpsj commented Jul 8, 2024

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

alfonsosanchezbeato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslarraz left a comment

Choose a reason for hiding this comment

alexmurray left a comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zyga left a comment

Choose a reason for hiding this comment

andrewphelpsj commented Apr 1, 2024 •

edited

Loading

codecov-commenter commented Apr 3, 2024 •

edited

Loading