-
Notifications
You must be signed in to change notification settings - Fork 574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
c/snap-confine, i/udev, i/ifacetest: update snap-confine and snap-device-helper to understand component hook security tags #13775
Changes from all commits
c4fc2d4
9048d15
b0c8a3e
7643859
2f7eab6
3f21dac
b7ad65e
5415317
6cabf43
5a00500
0bb442b
ab7d138
76555de
ebff4c1
fc05b1b
c9a216b
b1dc0d6
98e8fa9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
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_name) | ||
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 = | ||
"^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])*)$"; | ||
regex_t re; | ||
if (regcomp(&re, whitelist_re, REG_EXTENDED) != 0) | ||
die("can not compile regex %s", whitelist_re); | ||
|
||
// first capture is for verifying the full security tag, second capture | ||
// for verifying the snap_name is correct for this security tag | ||
regmatch_t matches[2]; | ||
// 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nitpick, this should probably be just const int, enum feels not much like C. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.c Alfonso suggested the enum over a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL |
||
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,10 +63,39 @@ bool sc_security_tag_validate(const char *security_tag, const char *snap_name) | |
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_name) | ||
&& strncmp(security_tag + matches[1].rm_so, snap_name, len) == 0; | ||
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) | ||
|
@@ -102,6 +142,94 @@ static int skip_one_char(const char **p, char c) | |
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; | ||
while (*p != '\0') { | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 @@ void sc_instance_key_validate(const char *instance_key, sc_error **errorp) | |
sc_error_forward(errorp, err); | ||
} | ||
|
||
void sc_snap_name_validate(const char *snap_name, sc_error **errorp) | ||
void sc_snap_component_validate(const char *snap_component, | ||
const char *snap_instance, 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 (snap_name == NULL) { | ||
err = sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_NAME, | ||
"snap name cannot be 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; | ||
} | ||
// 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 = snap_name; | ||
if (skip_one_char(&p, '-')) { | ||
err = sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_NAME, | ||
"snap name cannot start with a dash"); | ||
|
||
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; | ||
} | ||
bool got_letter = false; | ||
int n = 0, m; | ||
for (; *p != '\0';) { | ||
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, | ||
SC_SNAP_INVALID_NAME, | ||
"snap name cannot end with a dash"); | ||
goto out; | ||
} | ||
if (skip_one_char(&p, '-') > 0) { | ||
err = | ||
sc_error_init(SC_SNAP_DOMAIN, | ||
SC_SNAP_INVALID_NAME, | ||
"snap name cannot contain two consecutive dashes"); | ||
goto out; | ||
} | ||
continue; | ||
} | ||
err = sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_NAME, | ||
"snap name must use lower case letters, digits or dashes"); | ||
|
||
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; | ||
} | ||
if (!got_letter) { | ||
err = sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_NAME, | ||
"snap name must contain at least one letter"); | ||
|
||
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; | ||
} | ||
if (n < 2) { | ||
err = sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_NAME, | ||
"snap name must be longer than 1 character"); | ||
|
||
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; | ||
} | ||
if (n > SNAP_NAME_LEN) { | ||
err = sc_error_init(SC_SNAP_DOMAIN, SC_SNAP_INVALID_NAME, | ||
"snap name must be shorter than 40 characters"); | ||
|
||
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 @@ void sc_snap_split_instance_name(const char *instance_name, char *snap_name, | |
size_t snap_name_size, char *instance_key, | ||
size_t instance_key_size) | ||
{ | ||
if (instance_name == NULL) { | ||
die("internal error: cannot split instance name when it is unset"); | ||
} | ||
if (snap_name == NULL && instance_key == NULL) { | ||
die("internal error: cannot split instance name when both snap name and instance key are unset"); | ||
} | ||
|
||
const char *pos = strchr(instance_name, '_'); | ||
const char *instance_key_start = ""; | ||
size_t snap_name_len = 0; | ||
size_t instance_key_len = 0; | ||
if (pos == NULL) { | ||
snap_name_len = strlen(instance_name); | ||
} else { | ||
snap_name_len = pos - instance_name; | ||
instance_key_start = pos + 1; | ||
instance_key_len = strlen(instance_key_start); | ||
} | ||
|
||
if (snap_name != NULL) { | ||
if (snap_name_len >= snap_name_size) { | ||
die("snap name buffer too small"); | ||
} | ||
|
||
memcpy(snap_name, instance_name, snap_name_len); | ||
snap_name[snap_name_len] = '\0'; | ||
} | ||
sc_string_split(instance_name, '_', snap_name, snap_name_size, | ||
instance_key, instance_key_size); | ||
} | ||
|
||
if (instance_key != NULL) { | ||
if (instance_key_len >= instance_key_size) { | ||
die("instance key buffer too small"); | ||
} | ||
memcpy(instance_key, instance_key_start, instance_key_len); | ||
instance_key[instance_key_len] = '\0'; | ||
} | ||
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); | ||
} |
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).