c/snap-confine, i/udev, i/ifacetest: update snap-confine and snap-device-helper to understand component hook security tags #13775
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,21 +28,32 @@ | |
| #include "string-utils.h" | ||
| #include "cleanup-funcs.h" | ||
|
|
||
| bool sc_security_tag_validate(const char *security_tag, | ||
| const char *snap_instance, | ||
| const char *component_name) | ||
| { | ||
| /* Don't even check overly long tags. */ | ||
| if (strlen(security_tag) > SNAP_SECURITY_TAG_MAX_LEN) { | ||
| return false; | ||
| } | ||
| const char *whitelist_re = | ||
|
Check failure on line 39 in cmd/libsnap-confine-private/snap.c
|
||
| "^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])*)$"; | ||
| regex_t re; | ||
| if (regcomp(&re, whitelist_re, REG_EXTENDED) != 0) | ||
|
Check failure on line 42 in cmd/libsnap-confine-private/snap.c
|
||
| die("can not compile regex %s", whitelist_re); | ||
|
Check failure on line 43 in cmd/libsnap-confine-private/snap.c
|
||
|
|
||
| // first capture is for verifying the full security tag, second capture | ||
| // for verifying the snap_name is correct for this security tag, eighth capture | ||
| // for verifying the component_name is correct for this security tag. the | ||
| // 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. | ||
alfonsosanchezbeato marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| enum { num_matches = 9 }; | ||
|
There was a problem hiding this comment. Nitpick, this should probably be just const int, enum feels not much like C. There was a problem hiding this comment. Using a 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.cAlfonso suggested the enum over a |
||
| regmatch_t matches[num_matches]; | ||
| if (num_matches != re.re_nsub) { | ||
| die("internal error: all regex capture groups not fully accounted for"); | ||
| } | ||
|
|
||
| int status = | ||
| regexec(&re, security_tag, sizeof matches / sizeof *matches, | ||
| matches, 0); | ||
|
|
@@ -52,20 +63,49 @@ | |
| if (status != 0 || matches[1].rm_so < 0) { | ||
| return false; | ||
| } | ||
| // if we expect a component name (a non-null string was passed in here), | ||
| // then we need to make sure that the regex captured a component name | ||
| if (component_name != NULL) { | ||
| // fail if the security tag doesn't contain a component name and we | ||
| // expected one | ||
| if (matches[7].rm_so < 0) { | ||
| return false; | ||
| } | ||
|
|
||
| size_t component_name_len = strlen(component_name); | ||
alfonsosanchezbeato marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| // don't allow empty component names, only allow NULL as an indication | ||
| // that we don't expect a component name. | ||
| if (component_name_len == 0) { | ||
| return false; | ||
| } | ||
|
|
||
| size_t len = matches[7].rm_eo - matches[7].rm_so; | ||
| if (len != component_name_len | ||
| || strncmp(security_tag + matches[7].rm_so, component_name, | ||
| len) != 0) { | ||
| return false; | ||
| } | ||
| } else if (matches[7].rm_so >= 0) { | ||
| // fail if the security tag contains a component name and we didn't | ||
| // expect one | ||
| return false; | ||
| } | ||
|
|
||
| size_t len = matches[1].rm_eo - matches[1].rm_so; | ||
| return len == strlen(snap_instance) | ||
| && strncmp(security_tag + matches[1].rm_so, snap_instance, | ||
| len) == 0; | ||
| } | ||
|
|
||
| bool sc_is_hook_security_tag(const char *security_tag) | ||
| { | ||
| const char *whitelist_re = | ||
|
Check failure on line 103 in cmd/libsnap-confine-private/snap.c
|
||
| "^snap\\.[a-z](-?[a-z0-9])*(_[a-z0-9]{1,10})?\\.(hook\\.[a-z](-?[a-z0-9])*)$"; | ||
|
|
||
| regex_t re; | ||
| if (regcomp(&re, whitelist_re, REG_EXTENDED | REG_NOSUB) != 0) | ||
|
Check failure on line 107 in cmd/libsnap-confine-private/snap.c
|
||
| die("can not compile regex %s", whitelist_re); | ||
|
Check failure on line 108 in cmd/libsnap-confine-private/snap.c
|
||
|
|
||
| int status = regexec(&re, security_tag, 0, NULL, 0); | ||
| regfree(&re); | ||
|
|
@@ -102,6 +142,94 @@ | |
| return 0; | ||
| } | ||
|
|
||
| static void validate_as_snap_or_component_name(const char *name, | ||
| int err_code, | ||
| const char *err_subject, | ||
| sc_error **errorp) | ||
| { | ||
| // NOTE: This function should be synchronized with the two other | ||
| // implementations: validate_snap_name and snap.ValidateName. | ||
| sc_error *err = NULL; | ||
|
|
||
| // Ensure that name is not NULL | ||
| if (name == NULL) { | ||
| err = sc_error_init(SC_SNAP_DOMAIN, err_code, | ||
| "%s cannot be NULL", err_subject); | ||
| goto out; | ||
| } | ||
| // This is a regexp-free routine hand-codes the following pattern: | ||
| // | ||
| // "^([a-z0-9]+-?)*[a-z](-?[a-z0-9])*$" | ||
| // | ||
| // The only motivation for not using regular expressions is so that we | ||
| // don't run untrusted input against a potentially complex regular | ||
| // expression engine. | ||
| const char *p = name; | ||
| if (skip_one_char(&p, '-')) { | ||
| err = sc_error_init(SC_SNAP_DOMAIN, err_code, | ||
| "%s cannot start with a dash", err_subject); | ||
| goto out; | ||
| } | ||
| bool got_letter = false; | ||
| int n = 0, m; | ||
| for (; *p != '\0';) { | ||
andrewphelpsj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if ((m = skip_lowercase_letters(&p)) > 0) { | ||
| n += m; | ||
| got_letter = true; | ||
| continue; | ||
| } | ||
| if ((m = skip_digits(&p)) > 0) { | ||
| n += m; | ||
| continue; | ||
| } | ||
| if (skip_one_char(&p, '-') > 0) { | ||
| n++; | ||
| if (*p == '\0') { | ||
| err = | ||
| sc_error_init(SC_SNAP_DOMAIN, | ||
| err_code, | ||
| "%s cannot end with a dash", | ||
| err_subject); | ||
| goto out; | ||
| } | ||
| if (skip_one_char(&p, '-') > 0) { | ||
| err = | ||
| sc_error_init(SC_SNAP_DOMAIN, | ||
| err_code, | ||
| "%s cannot contain two consecutive dashes", | ||
| err_subject); | ||
| goto out; | ||
| } | ||
| continue; | ||
| } | ||
| err = sc_error_init(SC_SNAP_DOMAIN, err_code, | ||
| "%s must use lower case letters, digits or dashes", | ||
| err_subject); | ||
| goto out; | ||
| } | ||
| if (!got_letter) { | ||
| err = sc_error_init(SC_SNAP_DOMAIN, err_code, | ||
| "%s must contain at least one letter", | ||
| err_subject); | ||
| goto out; | ||
| } | ||
| if (n < 2) { | ||
| err = sc_error_init(SC_SNAP_DOMAIN, err_code, | ||
| "%s must be longer than 1 character", | ||
| err_subject); | ||
| goto out; | ||
| } | ||
| if (n > SNAP_NAME_LEN) { | ||
| err = sc_error_init(SC_SNAP_DOMAIN, err_code, | ||
| "%s must be shorter than 40 characters", | ||
|
There was a problem hiding this comment. 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 |
||
| err_subject); | ||
| goto out; | ||
| } | ||
|
|
||
| out: | ||
| sc_error_forward(errorp, err); | ||
| } | ||
|
|
||
| void sc_instance_name_validate(const char *instance_name, sc_error **errorp) | ||
| { | ||
| // NOTE: This function should be synchronized with the two other | ||
|
|
@@ -195,85 +323,86 @@ | |
| sc_error_forward(errorp, err); | ||
| } | ||
|
|
||
| void sc_snap_component_validate(const char *snap_component, | ||
| const char *snap_instance, sc_error **errorp) | ||
| { | ||
| sc_error *err = NULL; | ||
|
|
||
| // ensure that name is not NULL | ||
| if (snap_component == NULL) { | ||
| err = | ||
| sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_COMPONENT, | ||
| "snap component cannot be NULL"); | ||
| goto out; | ||
| } | ||
|
|
||
| const char *pos = strchr(snap_component, '+'); | ||
| if (pos == NULL) { | ||
| err = | ||
| sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_COMPONENT, | ||
| "snap component must contain a +"); | ||
| goto out; | ||
| } | ||
|
|
||
| size_t snap_name_len = pos - snap_component; | ||
| if (snap_name_len > SNAP_NAME_LEN) { | ||
| err = | ||
| sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_COMPONENT, | ||
| "snap name must be shorter than 40 characters"); | ||
| goto out; | ||
| } | ||
|
|
||
| size_t component_name_len = strlen(pos + 1); | ||
| if (component_name_len > SNAP_NAME_LEN) { | ||
| err = | ||
| sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_COMPONENT, | ||
| "component name must be shorter than 40 characters"); | ||
| goto out; | ||
| } | ||
|
|
||
| char snap_name[SNAP_NAME_LEN + 1] = { 0 }; | ||
| strncpy(snap_name, snap_component, snap_name_len); | ||
|
|
||
| char component_name[SNAP_NAME_LEN + 1] = { 0 }; | ||
| strncpy(component_name, pos + 1, component_name_len); | ||
|
|
||
| validate_as_snap_or_component_name(snap_name, SC_SNAP_INVALID_COMPONENT, | ||
| "snap name in component", &err); | ||
| if (err != NULL) { | ||
| goto out; | ||
| } | ||
|
|
||
| validate_as_snap_or_component_name(component_name, | ||
| SC_SNAP_INVALID_COMPONENT, | ||
| "component name", &err); | ||
| if (err != NULL) { | ||
| goto out; | ||
| } | ||
|
|
||
| if (snap_instance != NULL) { | ||
| char snap_name_in_instance[SNAP_NAME_LEN + 1] = { 0 }; | ||
| sc_snap_drop_instance_key(snap_instance, snap_name_in_instance, | ||
| sizeof snap_name_in_instance); | ||
|
|
||
| if (strcmp(snap_name, snap_name_in_instance) != 0) { | ||
| err = | ||
| sc_error_init(SC_SNAP_DOMAIN, | ||
| SC_SNAP_INVALID_COMPONENT, | ||
| "snap name in component must match snap name in instance"); | ||
andrewphelpsj marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| goto out; | ||
| } | ||
| } | ||
|
|
||
| out: | ||
| sc_error_forward(errorp, err); | ||
| } | ||
|
|
||
| void sc_snap_name_validate(const char *snap_name, sc_error **errorp) | ||
| { | ||
| validate_as_snap_or_component_name(snap_name, SC_SNAP_INVALID_NAME, | ||
| "snap name", errorp); | ||
| } | ||
|
|
||
| void sc_snap_drop_instance_key(const char *instance_name, char *snap_name, | ||
| size_t snap_name_size) | ||
| { | ||
|
|
@@ -285,39 +414,15 @@ | |
| size_t snap_name_size, char *instance_key, | ||
| size_t instance_key_size) | ||
| { | ||
| sc_string_split(instance_name, '_', snap_name, snap_name_size, | ||
| instance_key, instance_key_size); | ||
| } | ||
|
|
||
| void sc_snap_split_snap_component(const char *snap_component, | ||
| char *snap_name, size_t snap_name_size, | ||
| char *component_name, | ||
| size_t component_name_size) | ||
| { | ||
| sc_string_split(snap_component, '+', snap_name, snap_name_size, | ||
| component_name, component_name_size); | ||
| } | ||
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).