Skip to content

Commit

Permalink
c/snap-confine, i/udev, i/ifacetest: update snap-confine and snap-dev…
Browse files Browse the repository at this point in the history 
…ice-helper to understand component hook security tags (#13775)

* i/udev, c/snap-confine, c/libsnap-confine-private, c/snap-device-helper: update snap-confine to be able to handle security tags that come from 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.

* i/udev, i/ifacetest: create udev tags for component hooks that encode + as __

* c/libsnap-confine-private: update regmatch_t array to have space for all matches

* c/libsnap-confine-private: rename sc_snap_or_component_name_validate to validate_as_snap_or_component_name

* c/libsnap-confine-private: rework validate_as_snap_or_component_name to take in error details

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

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

* c/libsnap-confine-private: test a few more cases in sc_snap_component_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

* c/libsnap-confine-private: add missing sc_error_free calls to new unit tests

* c/snap-confine: add missing free in sc_cleanup_invocation for new struct field

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

* c/libsnap-confine-private: replace sizeof math with anonymous enum as array size

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

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

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

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

* .woke.yaml: add exclusions for files that contain pre-existing issues
  • Loading branch information
@andrewphelpsj
andrewphelpsj authored Jul 22, 2024
1 parent 30be453 commit 98b4e32
Show file tree
Hide file tree
Showing 16 changed files with 909 additions and 269 deletions.
5 changes: 5 additions & 0 deletions .woke.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,8 @@ ignore_files:
- packaging/debian-sid/changelog
- packaging/fedora/snapd.spec
- cmd/snap-confine/snap-confine.apparmor.in
- cmd/snap-confine/snap-confine.c
- cmd/libsnap-confine-private/string-utils.c
- cmd/libsnap-confine-private/string-utils-test.c
- cmd/libsnap-confine-private/string-utils-test.c
- cmd/libsnap-confine-private/snap.c
338 changes: 199 additions & 139 deletions cmd/libsnap-confine-private/snap-test.c
View file

Large diffs are not rendered by default.

303 changes: 204 additions & 99 deletions cmd/libsnap-confine-private/snap.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
enum { num_matches = 9 };
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);
Expand All @@ -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);

// 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)
Expand Down Expand Up @@ -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",
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
Expand Down Expand Up @@ -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");
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)
{
Expand All @@ -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);
}
Loading

0 comments on commit 98b4e32

Please sign in to comment.