From fc6eedfd9fefacc298e450db836c4b1ba9a7f8aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:31 +0100 Subject: [PATCH 01/14] selinux: avoid nontransitive comparison MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avoid using nontransitive comparison to prevent unexpected sorting results due to (well-defined) overflows. See https://www.qualys.com/2024/01/30/qsort.txt for a related issue in glibc's qsort(3). Signed-off-by: Christian Göttsche --- v3: rename macro to cmp_int() --- security/selinux/ss/policydb.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 91df3db6a88cd..4de7ae4804f44 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -37,6 +37,8 @@ #include "mls.h" #include "services.h" +#define cmp_int(a, b) (((a) > (b)) - ((a) < (b))) + #ifdef CONFIG_SECURITY_SELINUX_DEBUG /* clang-format off */ static const char *const symtab_name[SYM_NUM] = { @@ -429,11 +431,11 @@ static int filenametr_cmp(const void *k1, const void *k2) const struct filename_trans_key *ft2 = k2; int v; - v = ft1->ttype - ft2->ttype; + v = cmp_int(ft1->ttype, ft2->ttype); if (v) return v; - v = ft1->tclass - ft2->tclass; + v = cmp_int(ft1->tclass, ft2->tclass); if (v) return v; @@ -464,15 +466,15 @@ static int rangetr_cmp(const void *k1, const void *k2) const struct range_trans *key1 = k1, *key2 = k2; int v; - v = key1->source_type - key2->source_type; + v = cmp_int(key1->source_type, key2->source_type); if (v) return v; - v = key1->target_type - key2->target_type; + v = cmp_int(key1->target_type, key2->target_type); if (v) return v; - v = key1->target_class - key2->target_class; + v = cmp_int(key1->target_class, key2->target_class); return v; } @@ -501,15 +503,15 @@ static int role_trans_cmp(const void *k1, const void *k2) const struct role_trans_key *key1 = k1, *key2 = k2; int v; - v = key1->role - key2->role; + v = cmp_int(key1->role, key2->role); if (v) return v; - v = key1->type - key2->type; + v = cmp_int(key1->type, key2->type); if (v) return v; - return key1->tclass - key2->tclass; + return cmp_int(key1->tclass, key2->tclass); } static const struct hashtab_key_params roletr_key_params = { From fa3edfb149cbba8fe8f2b5c7896d4405b85a816a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:35 +0100 Subject: [PATCH 02/14] selinux: use u16 for security classes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security class identifiers are limited to 2^16, thus use the appropriate type u16 consistently. Signed-off-by: Christian Göttsche --- v3: only change type, move the validation (> U16_MAX) to the subsequent patch --- security/selinux/ss/policydb.c | 5 +++-- security/selinux/ss/policydb.h | 10 +++++----- security/selinux/ss/services.c | 2 +- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 4de7ae4804f44..fab8c74cae3bb 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -934,7 +934,7 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) return 0; } -int policydb_class_isvalid(struct policydb *p, unsigned int class) +int policydb_class_isvalid(struct policydb *p, u16 class) { if (!class || class > p->p_classes.nprim) return 0; @@ -2010,7 +2010,8 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp struct filename_trans_key *ft = NULL; struct filename_trans_datum **dst, *datum, *first = NULL; char *name = NULL; - u32 len, ttype, tclass, ndatum, i; + u32 len, ttype, ndatum, i; + u16 tclass; __le32 buf[3]; int rc; diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 89a180b1742fb..a49275d1168d8 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -48,7 +48,7 @@ struct common_datum { /* Class attributes */ struct class_datum { - u32 value; /* class value */ + u16 value; /* class value */ char *comkey; /* common name */ struct common_datum *comdatum; /* common datum */ struct symtab permissions; /* class-specific permission symbol table */ @@ -82,7 +82,7 @@ struct role_datum { struct role_trans_key { u32 role; /* current role */ u32 type; /* program executable type, or new object type */ - u32 tclass; /* process class, or new object class */ + u16 tclass; /* process class, or new object class */ }; struct role_trans_datum { @@ -139,7 +139,7 @@ struct cat_datum { struct range_trans { u32 source_type; u32 target_type; - u32 target_class; + u16 target_class; }; /* Boolean data type */ @@ -195,7 +195,7 @@ struct ocontext { } ibendport; } u; union { - u32 sclass; /* security class for genfs */ + u16 sclass; /* security class for genfs */ u32 behavior; /* labeling behavior for fs_use */ } v; struct context context[2]; /* security context(s) */ @@ -322,7 +322,7 @@ struct policy_file { extern void policydb_destroy(struct policydb *p); extern int policydb_load_isids(struct policydb *p, struct sidtab *s); extern int policydb_context_isvalid(struct policydb *p, struct context *c); -extern int policydb_class_isvalid(struct policydb *p, unsigned int class); +extern int policydb_class_isvalid(struct policydb *p, u16 class); extern int policydb_type_isvalid(struct policydb *p, unsigned int type); extern int policydb_role_isvalid(struct policydb *p, unsigned int role); extern int policydb_read(struct policydb *p, struct policy_file *fp); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 13fc712d59231..b7cc6e8d79fa2 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -3413,7 +3413,7 @@ static int get_classes_callback(void *k, void *d, void *args) { struct class_datum *datum = d; char *name = k, **classes = args; - u32 value = datum->value - 1; + u16 value = datum->value - 1; classes[value] = kstrdup(name, GFP_ATOMIC); if (!classes[value]) From 0ef60354fcda2fa10e9bb066f620c1aed3113d1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:36 +0100 Subject: [PATCH 03/14] selinux: more strict policy parsing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Be more strict during parsing of policies and reject invalid values. Add some error messages in the case of policy parse failures, to enhance debugging, either on a malformed policy or a too strict check. Signed-off-by: Christian Göttsche --- v3: - incorporate the overflow checks on security classes from the previous patch, and permit U16_MAX as class ID - minimize the usage of magic values, by using macros or trivial helper functions v2: accept unknown xperm specifiers to support backwards compatibility for future ones, suggested by Thiébaud --- security/selinux/include/security.h | 1 + security/selinux/ss/avtab.c | 35 ++++- security/selinux/ss/avtab.h | 13 ++ security/selinux/ss/conditional.c | 18 +-- security/selinux/ss/constraint.h | 1 + security/selinux/ss/policydb.c | 196 +++++++++++++++++++++++----- security/selinux/ss/policydb.h | 23 +++- security/selinux/ss/services.c | 6 +- 8 files changed, 233 insertions(+), 60 deletions(-) diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h index d1f16d7f684de..3fd8e83a2b79d 100644 --- a/security/selinux/include/security.h +++ b/security/selinux/include/security.h @@ -236,6 +236,7 @@ int security_read_policy(void **data, size_t *len); int security_read_state_kernel(void **data, size_t *len); int security_policycap_supported(unsigned int req_cap); +/* Maximum supported number of permissions per class */ #define SEL_VEC_MAX 32 struct av_decision { u32 allowed; diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index d12ca337e6498..fb67b36001566 100644 --- a/security/selinux/ss/avtab.c +++ b/security/selinux/ss/avtab.c @@ -316,7 +316,7 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po struct avtab_extended_perms xperms; __le32 buf32[ARRAY_SIZE(xperms.perms.p)]; int rc; - unsigned int set, vers = pol->policyvers; + unsigned int vers = pol->policyvers; memset(&key, 0, sizeof(struct avtab_key)); memset(&datum, 0, sizeof(struct avtab_datum)); @@ -327,9 +327,12 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po pr_err("SELinux: avtab: truncated entry\n"); return rc; } + /* Read five or more items: source type, target type, + * target class, AV type, and at least one datum. + */ items2 = le32_to_cpu(buf32[0]); - if (items2 > ARRAY_SIZE(buf32)) { - pr_err("SELinux: avtab: entry overflow\n"); + if (items2 < 5 || items2 > ARRAY_SIZE(buf32)) { + pr_err("SELinux: avtab: invalid item count\n"); return -EINVAL; } rc = next_entry(buf32, fp, sizeof(u32) * items2); @@ -358,6 +361,13 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po return -EINVAL; } + if (!policydb_type_isvalid(pol, key.source_type) || + !policydb_type_isvalid(pol, key.target_type) || + !policydb_class_isvalid(pol, key.target_class)) { + pr_err("SELinux: avtab: invalid type or class\n"); + return -EINVAL; + } + val = le32_to_cpu(buf32[items++]); enabled = (val & AVTAB_ENABLED_OLD) ? AVTAB_ENABLED : 0; @@ -376,6 +386,11 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po for (i = 0; i < ARRAY_SIZE(spec_order); i++) { if (val & spec_order[i]) { + if (items >= items2) { + pr_err("SELinux: avtab: entry has too many items (%d/%d)\n", + items + 1, items2); + return -EINVAL; + } key.specified = spec_order[i] | enabled; datum.u.data = le32_to_cpu(buf32[items++]); rc = insertf(a, &key, &datum, p); @@ -411,9 +426,13 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po return -EINVAL; } - set = hweight16(key.specified & (AVTAB_XPERMS | AVTAB_TYPE | AVTAB_AV)); - if (!set || set > 1) { - pr_err("SELinux: avtab: more than one specifier\n"); + if (hweight16(key.specified & ~AVTAB_ENABLED) != 1) { + pr_err("SELinux: avtab: not exactly one specifier\n"); + return -EINVAL; + } + + if (key.specified & ~AVTAB_SPECIFIER_MASK) { + pr_err("SELinux: avtab: invalid specifier\n"); return -EINVAL; } @@ -438,6 +457,10 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po pr_err("SELinux: avtab: truncated entry\n"); return rc; } + if (!avtab_is_valid_xperm_specified(xperms.specified)) + pr_warn_once_policyload(pol, + "SELinux: avtab: unsupported xperm specifier %#x\n", + xperms.specified); rc = next_entry(&xperms.driver, fp, sizeof(u8)); if (rc) { pr_err("SELinux: avtab: truncated entry\n"); diff --git a/security/selinux/ss/avtab.h b/security/selinux/ss/avtab.h index 850b3453f2598..1de4cce288a76 100644 --- a/security/selinux/ss/avtab.h +++ b/security/selinux/ss/avtab.h @@ -44,6 +44,7 @@ struct avtab_key { AVTAB_XPERMS_DONTAUDIT) #define AVTAB_ENABLED_OLD 0x80000000 /* reserved for used in cond_avtab */ #define AVTAB_ENABLED 0x8000 /* reserved for used in cond_avtab */ +#define AVTAB_SPECIFIER_MASK (AVTAB_AV | AVTAB_TYPE | AVTAB_XPERMS | AVTAB_ENABLED) u16 specified; /* what field is specified */ }; @@ -68,6 +69,18 @@ struct avtab_extended_perms { struct extended_perms_data perms; }; +static inline bool avtab_is_valid_xperm_specified(u8 specified) +{ + switch (specified) { + case AVTAB_XPERMS_IOCTLFUNCTION: + case AVTAB_XPERMS_IOCTLDRIVER: + case AVTAB_XPERMS_NLMSG: + return true; + default: + return false; + } +} + struct avtab_datum { union { u32 data; /* access vector or type value */ diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 1bebfcb9c6a12..db30462ed6a32 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -199,19 +199,12 @@ int cond_index_bool(void *key, void *datum, void *datap) return 0; } -static int bool_isvalid(struct cond_bool_datum *b) -{ - if (!(b->state == 0 || b->state == 1)) - return 0; - return 1; -} - int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp) { char *key = NULL; struct cond_bool_datum *booldatum; __le32 buf[3]; - u32 len; + u32 len, val; int rc; booldatum = kzalloc(sizeof(*booldatum), GFP_KERNEL); @@ -223,11 +216,12 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp) goto err; booldatum->value = le32_to_cpu(buf[0]); - booldatum->state = le32_to_cpu(buf[1]); + val = le32_to_cpu(buf[1]); rc = -EINVAL; - if (!bool_isvalid(booldatum)) + if (!val_is_boolean(val)) goto err; + booldatum->state = (int)val; len = le32_to_cpu(buf[2]); @@ -241,6 +235,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp) return 0; err: + pr_err("SELinux: conditional: failed to read boolean\n"); cond_destroy_bool(key, booldatum, NULL); return rc; } @@ -362,7 +357,8 @@ static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr) return 0; } - if (expr->boolean > p->p_bools.nprim) { + if (expr->expr_type == COND_BOOL && + (expr->boolean == 0 || expr->boolean > p->p_bools.nprim)) { pr_err("SELinux: conditional expressions uses unknown bool.\n"); return 0; } diff --git a/security/selinux/ss/constraint.h b/security/selinux/ss/constraint.h index 203033cfad672..1d75a8a044df9 100644 --- a/security/selinux/ss/constraint.h +++ b/security/selinux/ss/constraint.h @@ -50,6 +50,7 @@ struct constraint_expr { u32 op; /* operator */ struct ebitmap names; /* names */ + /* internally unused, only forwarded via policydb_write() */ struct type_set *type_names; struct constraint_expr *next; /* next expression */ diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index fab8c74cae3bb..10c997d5c41a9 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -640,13 +640,11 @@ static int sens_index(void *key, void *datum, void *datap) levdatum = datum; p = datap; - if (!levdatum->isalias) { - if (!levdatum->level.sens || - levdatum->level.sens > p->p_levels.nprim) - return -EINVAL; + if (!levdatum->level.sens || levdatum->level.sens > p->p_levels.nprim) + return -EINVAL; + if (!levdatum->isalias) p->sym_val_to_name[SYM_LEVELS][levdatum->level.sens - 1] = key; - } return 0; } @@ -659,12 +657,11 @@ static int cat_index(void *key, void *datum, void *datap) catdatum = datum; p = datap; - if (!catdatum->isalias) { - if (!catdatum->value || catdatum->value > p->p_cats.nprim) - return -EINVAL; + if (!catdatum->value || catdatum->value > p->p_cats.nprim) + return -EINVAL; + if (!catdatum->isalias) p->sym_val_to_name[SYM_CATS][catdatum->value - 1] = key; - } return 0; } @@ -1143,6 +1140,9 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f len = le32_to_cpu(buf[0]); perdatum->value = le32_to_cpu(buf[1]); + rc = -EINVAL; + if (perdatum->value < 1 || perdatum->value > SEL_VEC_MAX) + goto bad; rc = str_read(&key, GFP_KERNEL, fp, len); if (rc) @@ -1177,6 +1177,9 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file len = le32_to_cpu(buf[0]); comdatum->value = le32_to_cpu(buf[1]); nel = le32_to_cpu(buf[3]); + rc = -EINVAL; + if (nel > SEL_VEC_MAX) + goto bad; rc = symtab_init(&comdatum->permissions, nel); if (rc) @@ -1328,7 +1331,7 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file * char *key = NULL; struct class_datum *cladatum; __le32 buf[6]; - u32 i, len, len2, ncons, nel; + u32 i, len, len2, ncons, nel, val; int rc; cladatum = kzalloc(sizeof(*cladatum), GFP_KERNEL); @@ -1341,8 +1344,16 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file * len = le32_to_cpu(buf[0]); len2 = le32_to_cpu(buf[1]); - cladatum->value = le32_to_cpu(buf[2]); nel = le32_to_cpu(buf[4]); + rc = -EINVAL; + if (nel > SEL_VEC_MAX) + goto bad; + + val = le32_to_cpu(buf[2]); + rc = -EINVAL; + if (val > U16_MAX) + goto bad; + cladatum->value = val; rc = symtab_init(&cladatum->permissions, nel); if (rc) @@ -1398,16 +1409,59 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file * if (rc) goto bad; - cladatum->default_user = le32_to_cpu(buf[0]); - cladatum->default_role = le32_to_cpu(buf[1]); - cladatum->default_range = le32_to_cpu(buf[2]); + rc = -EINVAL; + val = le32_to_cpu(buf[0]); + switch (val) { + case 0: + case DEFAULT_SOURCE: + case DEFAULT_TARGET: + cladatum->default_user = val; + break; + default: + goto bad; + } + val = le32_to_cpu(buf[1]); + switch (val) { + case 0: + case DEFAULT_SOURCE: + case DEFAULT_TARGET: + cladatum->default_role = val; + break; + default: + goto bad; + } + val = le32_to_cpu(buf[2]); + switch (val) { + case 0: + case DEFAULT_SOURCE_LOW: + case DEFAULT_SOURCE_HIGH: + case DEFAULT_SOURCE_LOW_HIGH: + case DEFAULT_TARGET_LOW: + case DEFAULT_TARGET_HIGH: + case DEFAULT_TARGET_LOW_HIGH: + case DEFAULT_GLBLUB: + cladatum->default_range = val; + break; + default: + goto bad; + } } if (p->policyvers >= POLICYDB_VERSION_DEFAULT_TYPE) { rc = next_entry(buf, fp, sizeof(u32) * 1); if (rc) goto bad; - cladatum->default_type = le32_to_cpu(buf[0]); + rc = -EINVAL; + val = le32_to_cpu(buf[0]); + switch (val) { + case 0: + case DEFAULT_TARGET: + case DEFAULT_SOURCE: + cladatum->default_type = val; + break; + default: + goto bad; + } } rc = symtab_insert(s, key, cladatum); @@ -1417,6 +1471,8 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file * return 0; bad: cls_destroy(key, cladatum, NULL); + if (rc) + pr_err("SELinux: invalid class\n"); return rc; } @@ -1608,7 +1664,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f struct level_datum *levdatum; int rc; __le32 buf[2]; - u32 len; + u32 len, val; levdatum = kzalloc(sizeof(*levdatum), GFP_KERNEL); if (!levdatum) @@ -1619,7 +1675,11 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f goto bad; len = le32_to_cpu(buf[0]); - levdatum->isalias = le32_to_cpu(buf[1]); + val = le32_to_cpu(buf[1]); + rc = -EINVAL; + if (!val_is_boolean(val)) + goto bad; + levdatum->isalias = val; rc = str_read(&key, GFP_KERNEL, fp, len); if (rc) @@ -1635,6 +1695,8 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f return 0; bad: sens_destroy(key, levdatum, NULL); + if (rc) + pr_err("SELinux: invalid sensitivity\n"); return rc; } @@ -1644,7 +1706,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp struct cat_datum *catdatum; int rc; __le32 buf[3]; - u32 len; + u32 len, val; catdatum = kzalloc(sizeof(*catdatum), GFP_KERNEL); if (!catdatum) @@ -1656,7 +1718,11 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp len = le32_to_cpu(buf[0]); catdatum->value = le32_to_cpu(buf[1]); - catdatum->isalias = le32_to_cpu(buf[2]); + val = le32_to_cpu(buf[2]); + rc = -EINVAL; + if (!val_is_boolean(val)) + goto bad; + catdatum->isalias = val; rc = str_read(&key, GFP_KERNEL, fp, len); if (rc) @@ -1668,6 +1734,8 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp return 0; bad: cat_destroy(key, catdatum, NULL); + if (rc) + pr_err("SELinux: invalid category\n"); return rc; } @@ -1849,7 +1917,7 @@ static int range_read(struct policydb *p, struct policy_file *fp) struct mls_range *r = NULL; int rc; __le32 buf[2]; - u32 i, nel; + u32 i, nel, val; if (p->policyvers < POLICYDB_VERSION_MLS) return 0; @@ -1880,7 +1948,11 @@ static int range_read(struct policydb *p, struct policy_file *fp) rc = next_entry(buf, fp, sizeof(u32)); if (rc) goto out; - rt->target_class = le32_to_cpu(buf[0]); + rc = -EINVAL; + val = le32_to_cpu(buf[0]); + if (val > U16_MAX) + goto out; + rt->target_class = val; } else rt->target_class = p->process_class; @@ -1917,6 +1989,8 @@ static int range_read(struct policydb *p, struct policy_file *fp) out: kfree(rt); kfree(r); + if (rc) + pr_err("SELinux: invalid range\n"); return rc; } @@ -1925,7 +1999,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f struct filename_trans_key key, *ft = NULL; struct filename_trans_datum *last, *datum = NULL; char *name = NULL; - u32 len, stype, otype; + u32 len, stype, otype, val; __le32 buf[4]; int rc; @@ -1944,9 +2018,17 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f if (rc) goto out; + rc = -EINVAL; stype = le32_to_cpu(buf[0]); + if (!policydb_type_isvalid(p, stype)) + goto out; key.ttype = le32_to_cpu(buf[1]); - key.tclass = le32_to_cpu(buf[2]); + if (!policydb_type_isvalid(p, key.ttype)) + goto out; + val = le32_to_cpu(buf[2]); + if (val > U16_MAX || !policydb_class_isvalid(p, val)) + goto out; + key.tclass = val; key.name = name; otype = le32_to_cpu(buf[3]); @@ -2002,6 +2084,9 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f kfree(ft); kfree(name); kfree(datum); + + if (rc) + pr_err("SELinux: invalid compat filename transition\n"); return rc; } @@ -2010,7 +2095,7 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp struct filename_trans_key *ft = NULL; struct filename_trans_datum **dst, *datum, *first = NULL; char *name = NULL; - u32 len, ttype, ndatum, i; + u32 len, ttype, ndatum, i, val; u16 tclass; __le32 buf[3]; int rc; @@ -2030,8 +2115,15 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp if (rc) goto out; + rc = -EINVAL; ttype = le32_to_cpu(buf[0]); - tclass = le32_to_cpu(buf[1]); + if (!policydb_type_isvalid(p, ttype)) + goto out; + val = le32_to_cpu(buf[1]); + rc = -EINVAL; + if (val > U16_MAX || !policydb_class_isvalid(p, val)) + goto out; + tclass = val; ndatum = le32_to_cpu(buf[2]); if (ndatum == 0) { @@ -2061,6 +2153,10 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp datum->otype = le32_to_cpu(buf[0]); + rc = -EINVAL; + if (!policydb_type_isvalid(p, datum->otype)) + goto out; + dst = &datum->next; } @@ -2092,6 +2188,9 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp ebitmap_destroy(&datum->stypes); kfree(datum); } + + if (rc) + pr_err("SELinux: invalid filename transition\n"); return rc; } @@ -2139,7 +2238,7 @@ static int filename_trans_read(struct policydb *p, struct policy_file *fp) static int genfs_read(struct policydb *p, struct policy_file *fp) { int rc; - u32 i, j, nel, nel2, len, len2; + u32 i, j, nel, nel2, len, len2, val; __le32 buf[1]; struct ocontext *l, *c; struct ocontext *newc = NULL; @@ -2209,7 +2308,11 @@ static int genfs_read(struct policydb *p, struct policy_file *fp) if (rc) goto out; - newc->v.sclass = le32_to_cpu(buf[0]); + rc = -EINVAL; + val = le32_to_cpu(buf[0]); + if (val > U16_MAX || (val != 0 && !policydb_class_isvalid(p, val))) + goto out; + newc->v.sclass = val; rc = context_read_and_validate(&newc->context[0], p, fp); if (rc) @@ -2246,6 +2349,9 @@ static int genfs_read(struct policydb *p, struct policy_file *fp) } ocontext_destroy(newc, OCON_FSUSE); + if (rc) + pr_err("SELinux: invalid genfs\n"); + return rc; } @@ -2254,7 +2360,7 @@ static int ocontext_read(struct policydb *p, { int rc; unsigned int i; - u32 j, nel, len; + u32 j, nel, len, val; __be64 prefixbuf[1]; __le32 buf[3]; struct ocontext *l, *c; @@ -2318,11 +2424,25 @@ static int ocontext_read(struct policydb *p, rc = next_entry(buf, fp, sizeof(u32) * 3); if (rc) goto out; - c->u.port.protocol = le32_to_cpu(buf[0]); - c->u.port.low_port = le32_to_cpu(buf[1]); - c->u.port.high_port = le32_to_cpu(buf[2]); - rc = context_read_and_validate(&c->context[0], - p, fp); + + rc = -EINVAL; + val = le32_to_cpu(buf[0]); + if (val > U8_MAX) + goto out; + c->u.port.protocol = val; + val = le32_to_cpu(buf[1]); + if (val > U16_MAX) + goto out; + c->u.port.low_port = val; + val = le32_to_cpu(buf[2]); + if (val > U16_MAX) + goto out; + c->u.port.high_port = val; + if (c->u.port.low_port == 0 || + c->u.port.low_port > c->u.port.high_port) + goto out; + + rc = context_read_and_validate(&c->context[0], p, fp); if (rc) goto out; break; @@ -2440,6 +2560,8 @@ static int ocontext_read(struct policydb *p, } rc = 0; out: + if (rc) + pr_err("SELinux: invalid ocon\n"); return rc; } @@ -2454,7 +2576,7 @@ int policydb_read(struct policydb *p, struct policy_file *fp) struct role_trans_datum *rtd = NULL; int rc; __le32 buf[4]; - u32 i, j, len, nprim, nel, perm; + u32 i, j, len, nprim, nel, perm, val; char *policydb_str; const struct policydb_compat_info *info; @@ -2646,7 +2768,11 @@ int policydb_read(struct policydb *p, struct policy_file *fp) rc = next_entry(buf, fp, sizeof(u32)); if (rc) goto bad; - rtk->tclass = le32_to_cpu(buf[0]); + rc = -EINVAL; + val = le32_to_cpu(buf[0]); + if (val > U16_MAX) + goto bad; + rtk->tclass = val; } else rtk->tclass = p->process_class; diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index a49275d1168d8..fd3473986643d 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -74,7 +74,7 @@ struct class_datum { /* Role attributes */ struct role_datum { u32 value; /* internal role value */ - u32 bounds; /* boundary of role */ + u32 bounds; /* boundary of role, 0 for none */ struct ebitmap dominates; /* set of roles dominated by this role */ struct ebitmap types; /* set of authorized types for role */ }; @@ -110,7 +110,8 @@ struct role_allow { /* Type attributes */ struct type_datum { u32 value; /* internal type value */ - u32 bounds; /* boundary of type */ + u32 bounds; /* boundary of type, 0 for none */ + /* internally unused, only forwarded via policydb_write() */ unsigned char primary; /* primary name? */ unsigned char attribute; /* attribute ?*/ }; @@ -118,7 +119,7 @@ struct type_datum { /* User attributes */ struct user_datum { u32 value; /* internal user value */ - u32 bounds; /* bounds of user */ + u32 bounds; /* bounds of user, 0 for none */ struct ebitmap roles; /* set of authorized roles for user */ struct mls_range range; /* MLS range (min - max) for user */ struct mls_level dfltlevel; /* default login MLS level for user */ @@ -195,7 +196,7 @@ struct ocontext { } ibendport; } u; union { - u16 sclass; /* security class for genfs */ + u16 sclass; /* security class for genfs (can be 0 for wildcard) */ u32 behavior; /* labeling behavior for fs_use */ } v; struct context context[2]; /* security context(s) */ @@ -388,9 +389,23 @@ static inline char *sym_name(struct policydb *p, unsigned int sym_num, return p->sym_val_to_name[sym_num][element_nr]; } +static inline bool val_is_boolean(u32 value) +{ + return value == 0 || value == 1; +} + extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len); extern u16 string_to_security_class(struct policydb *p, const char *name); extern u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name); +#define pr_warn_once_policyload(policy, fmt, ...) \ + do { \ + static const void *prev_policy__; \ + if (prev_policy__ != policy) { \ + printk(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__); \ + prev_policy__ = policy; \ + } \ + } while (0) + #endif /* _SS_POLICYDB_H_ */ diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index b7cc6e8d79fa2..56a4652c8ee80 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -446,8 +446,6 @@ static int dump_masked_av_helper(void *k, void *d, void *args) struct perm_datum *pdatum = d; char **permission_names = args; - BUG_ON(pdatum->value < 1 || pdatum->value > 32); - permission_names[pdatum->value - 1] = (char *)k; return 0; @@ -466,7 +464,7 @@ static void security_dump_masked_av(struct policydb *policydb, char *tclass_name; char *scontext_name = NULL; char *tcontext_name = NULL; - char *permission_names[32]; + char *permission_names[SEL_VEC_MAX]; int index; u32 length; bool need_comma = false; @@ -507,7 +505,7 @@ static void security_dump_masked_av(struct policydb *policydb, "scontext=%s tcontext=%s tclass=%s perms=", reason, scontext_name, tcontext_name, tclass_name); - for (index = 0; index < 32; index++) { + for (index = 0; index < SEL_VEC_MAX; index++) { u32 mask = (1 << index); if ((mask & permissions) == 0) From 198f5b1b0d1254c347e4e7ac3dd8ac16ed0c99d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:37 +0100 Subject: [PATCH 04/14] selinux: check length fields in policies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In multiple places the binary policy announces how many items of some kind are to be expected next. Before reading them the kernel already allocates enough memory for that announced size. Validate that the remaining input size can actually fit the announced items, to avoid OOM issues on malformed binary policies. Signed-off-by: Christian Göttsche --- v3: - fix error branch by returning directly instead of jumping to goto label, see https://lore.kernel.org/all/202412241405.LK8YTZqp-lkp@intel.com/ - rename to size_check() - add comments for magic values --- security/selinux/ss/avtab.c | 5 +++++ security/selinux/ss/conditional.c | 17 ++++++++++++++++ security/selinux/ss/policydb.c | 32 +++++++++++++++++++++++++++++++ security/selinux/ss/policydb.h | 13 +++++++++++++ 4 files changed, 67 insertions(+) diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index fb67b36001566..b708e2c9acdda 100644 --- a/security/selinux/ss/avtab.c +++ b/security/selinux/ss/avtab.c @@ -515,6 +515,11 @@ int avtab_read(struct avtab *a, struct policy_file *fp, struct policydb *pol) goto bad; } + /* avtab_read_item() reads at least 96 bytes for any valid entry */ + rc = size_check(3 * sizeof(u32), nel, fp); + if (rc) + goto bad; + rc = avtab_alloc(a, nel); if (rc) goto bad; diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index db30462ed6a32..92ed4f23a217e 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -12,6 +12,7 @@ #include "security.h" #include "conditional.h" +#include "policydb.h" #include "services.h" /* @@ -329,6 +330,11 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp, if (len == 0) return 0; + /* avtab_read_item() reads at least 96 bytes for any valid entry */ + rc = size_check(3 * sizeof(u32), len, fp); + if (rc) + return rc; + list->nodes = kcalloc(len, sizeof(*list->nodes), GFP_KERNEL); if (!list->nodes) return -ENOMEM; @@ -379,6 +385,12 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, struct pol /* expr */ len = le32_to_cpu(buf[1]); + + /* we will read 64 bytes per node */ + rc = size_check(2 * sizeof(u32), len, fp); + if (rc) + return rc; + node->expr.nodes = kcalloc(len, sizeof(*node->expr.nodes), GFP_KERNEL); if (!node->expr.nodes) return -ENOMEM; @@ -417,6 +429,11 @@ int cond_read_list(struct policydb *p, struct policy_file *fp) len = le32_to_cpu(buf[0]); + /* cond_read_node() reads at least 128 bytes for any valid node */ + rc = size_check(4 * sizeof(u32), len, fp); + if (rc) + return rc; + p->cond_list = kcalloc(len, sizeof(*p->cond_list), GFP_KERNEL); if (!p->cond_list) return -ENOMEM; diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 10c997d5c41a9..67984ba8dfc4a 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1107,6 +1107,9 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len) if ((len == 0) || (len == (u32)-1)) return -EINVAL; + if (size_check(sizeof(char), len, fp)) + return -EINVAL; + str = kmalloc(len + 1, flags | __GFP_NOWARN); if (!str) return -ENOMEM; @@ -1181,6 +1184,11 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file if (nel > SEL_VEC_MAX) goto bad; + /* perm_read() reads at least 64 bytes for any valid permission */ + rc = size_check(2 * sizeof(u32), nel, fp); + if (rc) + goto bad; + rc = symtab_init(&comdatum->permissions, nel); if (rc) goto bad; @@ -1355,6 +1363,11 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file * goto bad; cladatum->value = val; + /* perm_read() reads at least 64 bytes for any valid permission */ + rc = size_check(2 * sizeof(u32), nel, fp); + if (rc) + goto bad; + rc = symtab_init(&cladatum->permissions, nel); if (rc) goto bad; @@ -1928,6 +1941,13 @@ static int range_read(struct policydb *p, struct policy_file *fp) nel = le32_to_cpu(buf[0]); + /* we read at least 64 bytes and mls_read_range_helper() 32 bytes + * for any valid range-transition + */ + rc = size_check(3 * sizeof(u32), nel, fp); + if (rc) + return rc; + rc = hashtab_init(&p->range_tr, nel); if (rc) return rc; @@ -2702,6 +2722,13 @@ int policydb_read(struct policydb *p, struct policy_file *fp) nprim = le32_to_cpu(buf[0]); nel = le32_to_cpu(buf[1]); + /* every read_f() implementation reads at least 128 bytes + * for any valid entry + */ + rc = size_check(4 * sizeof(u32), nel, fp); + if (rc) + goto out; + rc = symtab_init(&p->symtab[i], nel); if (rc) goto out; @@ -2743,6 +2770,11 @@ int policydb_read(struct policydb *p, struct policy_file *fp) goto bad; nel = le32_to_cpu(buf[0]); + /* we read at least 96 bytes for any valid role-transition */ + rc = size_check(3 * sizeof(u32), nel, fp); + if (rc) + goto bad; + rc = hashtab_init(&p->role_tr, nel); if (rc) goto bad; diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index fd3473986643d..8b9f33046f2df 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -355,6 +355,19 @@ struct policy_data { struct policy_file *fp; }; +static inline int size_check(size_t bytes, size_t num, const struct policy_file *fp) +{ + size_t len; + + if (unlikely(check_mul_overflow(bytes, num, &len))) + return -EINVAL; + + if (unlikely(len > fp->len)) + return -EINVAL; + + return 0; +} + static inline int next_entry(void *buf, struct policy_file *fp, size_t bytes) { if (bytes > fp->len) From 6cd63af524f937c56fd610eb9b44f2dbd470a249 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:38 +0100 Subject: [PATCH 05/14] selinux: validate constraints MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate constraint expressions during reading the policy. Avoid the usage of BUG() on constraint evaluation, to mitigate malformed policies halting the system. Closes: https://github.com/SELinuxProject/selinux-testsuite/issues/76 Signed-off-by: Christian Göttsche --- security/selinux/ss/policydb.c | 61 ++++++++++++++++++++++++++++++++-- security/selinux/ss/services.c | 55 +++++++++++++++--------------- 2 files changed, 88 insertions(+), 28 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 67984ba8dfc4a..0c2b061c45516 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1264,6 +1264,8 @@ static int read_cons_helper(struct policydb *p, struct constraint_node **nodep, return rc; c->permissions = le32_to_cpu(buf[0]); nexpr = le32_to_cpu(buf[1]); + if (nexpr == 0) + return -EINVAL; le = NULL; depth = -1; for (j = 0; j < nexpr; j++) { @@ -1295,15 +1297,70 @@ static int read_cons_helper(struct policydb *p, struct constraint_node **nodep, depth--; break; case CEXPR_ATTR: - if (depth == (CEXPR_MAXDEPTH - 1)) + if (depth >= (CEXPR_MAXDEPTH - 1)) return -EINVAL; depth++; break; + + switch (e->op) { + case CEXPR_EQ: + case CEXPR_NEQ: + break; + case CEXPR_DOM: + case CEXPR_DOMBY: + case CEXPR_INCOMP: + if ((e->attr & CEXPR_USER) || (e->attr & CEXPR_TYPE)) + return -EINVAL; + break; + default: + return -EINVAL; + } + + switch (e->attr) { + case CEXPR_USER: + case CEXPR_ROLE: + case CEXPR_TYPE: + case CEXPR_L1L2: + case CEXPR_L1H2: + case CEXPR_H1L2: + case CEXPR_H1H2: + case CEXPR_L1H1: + case CEXPR_L2H2: + break; + default: + return -EINVAL; + } + + break; case CEXPR_NAMES: if (!allowxtarget && (e->attr & CEXPR_XTARGET)) return -EINVAL; - if (depth == (CEXPR_MAXDEPTH - 1)) + if (depth >= (CEXPR_MAXDEPTH - 1)) + return -EINVAL; + + switch (e->op) { + case CEXPR_EQ: + case CEXPR_NEQ: + break; + default: + return -EINVAL; + } + + switch (e->attr) { + case CEXPR_USER: + case CEXPR_USER | CEXPR_TARGET: + case CEXPR_USER | CEXPR_XTARGET: + case CEXPR_ROLE: + case CEXPR_ROLE | CEXPR_TARGET: + case CEXPR_ROLE | CEXPR_XTARGET: + case CEXPR_TYPE: + case CEXPR_TYPE | CEXPR_TARGET: + case CEXPR_TYPE | CEXPR_XTARGET: + break; + default: return -EINVAL; + } + depth++; rc = ebitmap_read(&e->names, fp); if (rc) diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 56a4652c8ee80..a600cb0edecbb 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -279,22 +279,25 @@ static int constraint_expr_eval(struct policydb *policydb, for (e = cexpr; e; e = e->next) { switch (e->expr_type) { case CEXPR_NOT: - BUG_ON(sp < 0); + if (unlikely(sp < 0)) + goto invalid; s[sp] = !s[sp]; break; case CEXPR_AND: - BUG_ON(sp < 1); + if (unlikely(sp < 1)) + goto invalid; sp--; s[sp] &= s[sp + 1]; break; case CEXPR_OR: - BUG_ON(sp < 1); + if (unlikely(sp < 1)) + goto invalid; sp--; s[sp] |= s[sp + 1]; break; case CEXPR_ATTR: - if (sp == (CEXPR_MAXDEPTH - 1)) - return 0; + if (unlikely(sp >= (CEXPR_MAXDEPTH - 1))) + goto invalid; switch (e->attr) { case CEXPR_USER: val1 = scontext->user; @@ -370,13 +373,11 @@ static int constraint_expr_eval(struct policydb *policydb, s[++sp] = mls_level_incomp(l2, l1); continue; default: - BUG(); - return 0; + goto invalid; } break; default: - BUG(); - return 0; + goto invalid; } switch (e->op) { @@ -387,22 +388,19 @@ static int constraint_expr_eval(struct policydb *policydb, s[++sp] = (val1 != val2); break; default: - BUG(); - return 0; + goto invalid; } break; case CEXPR_NAMES: - if (sp == (CEXPR_MAXDEPTH-1)) - return 0; + if (unlikely(sp >= (CEXPR_MAXDEPTH-1))) + goto invalid; c = scontext; if (e->attr & CEXPR_TARGET) c = tcontext; else if (e->attr & CEXPR_XTARGET) { c = xcontext; - if (!c) { - BUG(); - return 0; - } + if (unlikely(!c)) + goto invalid; } if (e->attr & CEXPR_USER) val1 = c->user; @@ -410,10 +408,8 @@ static int constraint_expr_eval(struct policydb *policydb, val1 = c->role; else if (e->attr & CEXPR_TYPE) val1 = c->type; - else { - BUG(); - return 0; - } + else + goto invalid; switch (e->op) { case CEXPR_EQ: @@ -423,18 +419,25 @@ static int constraint_expr_eval(struct policydb *policydb, s[++sp] = !ebitmap_get_bit(&e->names, val1 - 1); break; default: - BUG(); - return 0; + goto invalid; } break; default: - BUG(); - return 0; + goto invalid; } } - BUG_ON(sp != 0); + if (unlikely(sp != 0)) + goto invalid; + return s[0]; + +invalid: + /* Should *never* be reached, cause malformed constraints should + * have been filtered by read_cons_helper(). + */ + WARN_ONCE(true, "SELinux: invalid constraint passed validation\n"); + return 0; } /* From 1059304fce023ced7cec4813d6adc7d77eb49f89 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:40 +0100 Subject: [PATCH 06/14] selinux: pre-validate conditional expressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate conditional expressions while reading the policy, to avoid unexpected access decisions on malformed policies. Signed-off-by: Christian Göttsche --- security/selinux/ss/conditional.c | 116 ++++++++++++++++++++---------- security/selinux/ss/policydb.c | 7 ++ security/selinux/ss/policydb.h | 1 + 3 files changed, 88 insertions(+), 36 deletions(-) diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 92ed4f23a217e..ce0281cce739f 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -21,65 +21,119 @@ * or undefined (-1). Undefined occurs when the expression * exceeds the stack depth of COND_EXPR_MAXDEPTH. */ -static int cond_evaluate_expr(struct policydb *p, struct cond_expr *expr) +static int cond_evaluate_expr(const struct policydb *p, const struct cond_expr *expr) { u32 i; int s[COND_EXPR_MAXDEPTH]; int sp = -1; - if (expr->len == 0) - return -1; + if (unlikely(expr->len == 0)) + goto invalid; for (i = 0; i < expr->len; i++) { - struct cond_expr_node *node = &expr->nodes[i]; + const struct cond_expr_node *node = &expr->nodes[i]; switch (node->expr_type) { case COND_BOOL: - if (sp == (COND_EXPR_MAXDEPTH - 1)) - return -1; + if (unlikely(sp >= (COND_EXPR_MAXDEPTH - 1))) + goto invalid; sp++; s[sp] = p->bool_val_to_struct[node->boolean - 1]->state; break; case COND_NOT: - if (sp < 0) - return -1; + if (unlikely(sp < 0)) + goto invalid; s[sp] = !s[sp]; break; case COND_OR: - if (sp < 1) - return -1; + if (unlikely(sp < 1)) + goto invalid; sp--; s[sp] |= s[sp + 1]; break; case COND_AND: - if (sp < 1) - return -1; + if (unlikely(sp < 1)) + goto invalid; sp--; s[sp] &= s[sp + 1]; break; case COND_XOR: - if (sp < 1) - return -1; + if (unlikely(sp < 1)) + goto invalid; sp--; s[sp] ^= s[sp + 1]; break; case COND_EQ: - if (sp < 1) - return -1; + if (unlikely(sp < 1)) + goto invalid; sp--; s[sp] = (s[sp] == s[sp + 1]); break; case COND_NEQ: - if (sp < 1) - return -1; + if (unlikely(sp < 1)) + goto invalid; sp--; s[sp] = (s[sp] != s[sp + 1]); break; default: - return -1; + goto invalid; } } + + if (unlikely(sp != 0)) + goto invalid; + return s[0]; + +invalid: + /* Should *never* be reached, cause malformed expressions should + * have been filtered by cond_validate_expr(). + */ + WARN_ONCE(true, "SELinux: invalid conditional expression passed validation\n"); + return -1; +} + +static int cond_validate_expr(const struct policydb *p, const struct cond_expr *expr) +{ + u32 i; + int depth = -1; + + if (expr->len == 0) + return -EINVAL; + + for (i = 0; i < expr->len; i++) { + const struct cond_expr_node *node = &expr->nodes[i]; + + switch (node->expr_type) { + case COND_BOOL: + if (depth >= (COND_EXPR_MAXDEPTH - 1)) + return -EINVAL; + depth++; + if (!policydb_boolean_isvalid(p, node->boolean)) + return -EINVAL; + break; + case COND_NOT: + if (depth < 0) + return -EINVAL; + break; + case COND_OR: + case COND_AND: + case COND_XOR: + case COND_EQ: + case COND_NEQ: + if (depth < 1) + return -EINVAL; + depth--; + break; + default: + return -EINVAL; + } + } + + if (depth != 0) + return -EINVAL; + + return 0; } /* @@ -356,21 +410,6 @@ static int cond_read_av_list(struct policydb *p, struct policy_file *fp, return 0; } -static int expr_node_isvalid(struct policydb *p, struct cond_expr_node *expr) -{ - if (expr->expr_type <= 0 || expr->expr_type > COND_LAST) { - pr_err("SELinux: conditional expressions uses unknown operator.\n"); - return 0; - } - - if (expr->expr_type == COND_BOOL && - (expr->boolean == 0 || expr->boolean > p->p_bools.nprim)) { - pr_err("SELinux: conditional expressions uses unknown bool.\n"); - return 0; - } - return 1; -} - static int cond_read_node(struct policydb *p, struct cond_node *node, struct policy_file *fp) { __le32 buf[2]; @@ -385,6 +424,8 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, struct pol /* expr */ len = le32_to_cpu(buf[1]); + if (len == 0) + return -EINVAL; /* we will read 64 bytes per node */ rc = size_check(2 * sizeof(u32), len, fp); @@ -406,9 +447,12 @@ static int cond_read_node(struct policydb *p, struct cond_node *node, struct pol expr->expr_type = le32_to_cpu(buf[0]); expr->boolean = le32_to_cpu(buf[1]); + } - if (!expr_node_isvalid(p, expr)) - return -EINVAL; + rc = cond_validate_expr(p, &node->expr); + if (rc) { + pr_err("SELinux: invalid conditional expression\n"); + return rc; } rc = cond_read_av_list(p, fp, &node->true_list, NULL); diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 0c2b061c45516..26b8b57969694 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -952,6 +952,13 @@ int policydb_type_isvalid(struct policydb *p, unsigned int type) return 1; } +int policydb_boolean_isvalid(const struct policydb *p, u32 boolean) +{ + if (!boolean || boolean > p->p_bools.nprim) + return 0; + return 1; +} + /* * Return 1 if the fields in the security context * structure `c' are valid. Return 0 otherwise. diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 8b9f33046f2df..812bdaaf4dd85 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -326,6 +326,7 @@ extern int policydb_context_isvalid(struct policydb *p, struct context *c); extern int policydb_class_isvalid(struct policydb *p, u16 class); extern int policydb_type_isvalid(struct policydb *p, unsigned int type); extern int policydb_role_isvalid(struct policydb *p, unsigned int role); +extern int policydb_boolean_isvalid(const struct policydb *p, u32 boolean); extern int policydb_read(struct policydb *p, struct policy_file *fp); extern int policydb_write(struct policydb *p, struct policy_file *fp); From 45df0fcc4a3f7021eb8a83c5f6c6fc03b0d3f10d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:42 +0100 Subject: [PATCH 07/14] selinux: check type attr map overflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate that no types with an invalid too high ID are present in the attribute map. Gaps are still not checked. Signed-off-by: Christian Göttsche --- v3: squash with previous patch ("selinux: introduce ebitmap_highest_set_bit()") --- security/selinux/ss/ebitmap.c | 27 +++++++++++++++++++++++++++ security/selinux/ss/ebitmap.h | 1 + security/selinux/ss/policydb.c | 5 +++++ 3 files changed, 33 insertions(+) diff --git a/security/selinux/ss/ebitmap.c b/security/selinux/ss/ebitmap.c index 43bc19e219609..5d6b5b72b3e56 100644 --- a/security/selinux/ss/ebitmap.c +++ b/security/selinux/ss/ebitmap.c @@ -257,6 +257,33 @@ int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2, return 1; } +u32 ebitmap_highest_set_bit(const struct ebitmap *e) +{ + const struct ebitmap_node *n; + unsigned long unit; + u32 pos = 0; + + n = e->node; + if (!n) + return 0; + + while (n->next) + n = n->next; + + for (unsigned int i = EBITMAP_UNIT_NUMS; i > 0; i--) { + unit = n->maps[i - 1]; + if (unit == 0) + continue; + + pos = (i - 1) * EBITMAP_UNIT_SIZE; + while (unit >>= 1) + pos++; + break; + } + + return n->startbit + pos; +} + int ebitmap_get_bit(const struct ebitmap *e, u32 bit) { const struct ebitmap_node *n; diff --git a/security/selinux/ss/ebitmap.h b/security/selinux/ss/ebitmap.h index c9569998f287d..12bb359e83ff9 100644 --- a/security/selinux/ss/ebitmap.h +++ b/security/selinux/ss/ebitmap.h @@ -126,6 +126,7 @@ int ebitmap_and(struct ebitmap *dst, const struct ebitmap *e1, const struct ebitmap *e2); int ebitmap_contains(const struct ebitmap *e1, const struct ebitmap *e2, u32 last_e2bit); +u32 ebitmap_highest_set_bit(const struct ebitmap *e); int ebitmap_get_bit(const struct ebitmap *e, u32 bit); int ebitmap_set_bit(struct ebitmap *e, u32 bit, int value); void ebitmap_destroy(struct ebitmap *e); diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 26b8b57969694..2f725fa2b010e 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -2968,6 +2968,11 @@ int policydb_read(struct policydb *p, struct policy_file *fp) if (rc) goto bad; } + + rc = -EINVAL; + if (ebitmap_highest_set_bit(e) >= p->p_types.nprim) + goto bad; + /* add the type itself as the degenerate case */ rc = ebitmap_set_bit(e, i, 1); if (rc) From 35116ce631aa0ade92722559318c852f13adbe43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:43 +0100 Subject: [PATCH 08/14] selinux: reorder policydb_index() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Index as soon as possible to enable isvalid() checks to fail on gaps. Signed-off-by: Christian Göttsche --- security/selinux/ss/policydb.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 2f725fa2b010e..74f06f11b084a 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -734,7 +734,6 @@ static int policydb_index(struct policydb *p) pr_debug("SELinux: %d classes, %d rules\n", p->p_classes.nprim, p->te_avtab.nel); - avtab_hash_eval(&p->te_avtab, "rules"); symtab_hash_eval(p->symtab); p->class_val_to_struct = kcalloc(p->p_classes.nprim, @@ -2812,6 +2811,10 @@ int policydb_read(struct policydb *p, struct policy_file *fp) p->symtab[i].nprim = nprim; } + rc = policydb_index(p); + if (rc) + goto bad; + rc = -EINVAL; p->process_class = string_to_security_class(p, "process"); if (!p->process_class) { @@ -2823,6 +2826,8 @@ int policydb_read(struct policydb *p, struct policy_file *fp) if (rc) goto bad; + avtab_hash_eval(&p->te_avtab, "rules"); + if (p->policyvers >= POLICYDB_VERSION_BOOL) { rc = cond_read_list(p, fp); if (rc) @@ -2920,10 +2925,6 @@ int policydb_read(struct policydb *p, struct policy_file *fp) if (rc) goto bad; - rc = policydb_index(p); - if (rc) - goto bad; - rc = -EINVAL; perm = string_to_av_perm(p, p->process_class, "transition"); if (!perm) { From d5ef3c16192bcb48e4a5673245b7f8e6c8f8b069 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:44 +0100 Subject: [PATCH 09/14] selinux: beef up isvalid checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check that an ID does not refer to a gap in the global array of definitions. Constify parameters of isvalid() function and change return type to bool. Signed-off-by: Christian Göttsche --- security/selinux/ss/hashtab.h | 4 +-- security/selinux/ss/mls.c | 66 +++++++++++++++++++++------------- security/selinux/ss/mls.h | 6 ++-- security/selinux/ss/policydb.c | 56 ++++++++++++++++------------- security/selinux/ss/policydb.h | 12 +++---- security/selinux/ss/services.c | 2 +- security/selinux/ss/symtab.c | 2 +- security/selinux/ss/symtab.h | 2 +- 8 files changed, 88 insertions(+), 62 deletions(-) diff --git a/security/selinux/ss/hashtab.h b/security/selinux/ss/hashtab.h index deba82d78c3ae..c641fb12916b0 100644 --- a/security/selinux/ss/hashtab.h +++ b/security/selinux/ss/hashtab.h @@ -94,11 +94,11 @@ static inline int hashtab_insert(struct hashtab *h, void *key, void *datum, * Returns NULL if no entry has the specified key or * the datum of the entry otherwise. */ -static inline void *hashtab_search(struct hashtab *h, const void *key, +static inline void *hashtab_search(const struct hashtab *h, const void *key, struct hashtab_key_params key_params) { u32 hvalue; - struct hashtab_node *cur; + const struct hashtab_node *cur; if (!h->size) return NULL; diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c index a6e49269f5351..3cd36e2015fa2 100644 --- a/security/selinux/ss/mls.c +++ b/security/selinux/ss/mls.c @@ -32,7 +32,7 @@ int mls_compute_context_len(struct policydb *p, struct context *context) { int i, l, len, head, prev; - char *nm; + const char *nm; struct ebitmap *e; struct ebitmap_node *node; @@ -86,7 +86,8 @@ int mls_compute_context_len(struct policydb *p, struct context *context) void mls_sid_to_context(struct policydb *p, struct context *context, char **scontext) { - char *scontextp, *nm; + const char *nm; + char *scontextp; int i, l, head, prev; struct ebitmap *e; struct ebitmap_node *node; @@ -155,27 +156,44 @@ void mls_sid_to_context(struct policydb *p, struct context *context, *scontext = scontextp; } -int mls_level_isvalid(struct policydb *p, struct mls_level *l) +bool mls_level_isvalid(const struct policydb *p, const struct mls_level *l) { - struct level_datum *levdatum; + const char *name; + const struct level_datum *levdatum; + struct ebitmap_node *node; + u32 bit; + int rc; if (!l->sens || l->sens > p->p_levels.nprim) - return 0; - levdatum = symtab_search(&p->p_levels, - sym_name(p, SYM_LEVELS, l->sens - 1)); + return false; + + name = sym_name(p, SYM_LEVELS, l->sens - 1); + if (!name) + return false; + + levdatum = symtab_search(&p->p_levels, name); if (!levdatum) - return 0; + return false; /* - * Return 1 iff all the bits set in l->cat are also be set in + * Validate that all bits set in l->cat are also be set in * levdatum->level->cat and no bit in l->cat is larger than * p->p_cats.nprim. */ - return ebitmap_contains(&levdatum->level.cat, &l->cat, - p->p_cats.nprim); + rc = ebitmap_contains(&levdatum->level.cat, &l->cat, + p->p_cats.nprim); + if (!rc) + return false; + + ebitmap_for_each_positive_bit(&levdatum->level.cat, node, bit) { + if (!sym_name(p, SYM_CATS, bit)) + return false; + } + + return true; } -int mls_range_isvalid(struct policydb *p, struct mls_range *r) +bool mls_range_isvalid(const struct policydb *p, const struct mls_range *r) { return (mls_level_isvalid(p, &r->level[0]) && mls_level_isvalid(p, &r->level[1]) && @@ -183,32 +201,32 @@ int mls_range_isvalid(struct policydb *p, struct mls_range *r) } /* - * Return 1 if the MLS fields in the security context + * Return true if the MLS fields in the security context * structure `c' are valid. Return 0 otherwise. */ -int mls_context_isvalid(struct policydb *p, struct context *c) +bool mls_context_isvalid(const struct policydb *p, const struct context *c) { - struct user_datum *usrdatum; + const struct user_datum *usrdatum; if (!p->mls_enabled) - return 1; + return true; if (!mls_range_isvalid(p, &c->range)) - return 0; + return false; if (c->role == OBJECT_R_VAL) - return 1; + return true; /* * User must be authorized for the MLS range. */ if (!c->user || c->user > p->p_users.nprim) - return 0; + return false; usrdatum = p->user_val_to_struct[c->user - 1]; - if (!mls_range_contains(usrdatum->range, c->range)) - return 0; /* user may not be associated with range */ + if (!usrdatum || !mls_range_contains(usrdatum->range, c->range)) + return false; /* user may not be associated with range */ - return 1; + return true; } /* @@ -449,8 +467,8 @@ int mls_convert_context(struct policydb *oldp, struct policydb *newp, return 0; for (l = 0; l < 2; l++) { - char *name = sym_name(oldp, SYM_LEVELS, - oldc->range.level[l].sens - 1); + const char *name = sym_name(oldp, SYM_LEVELS, + oldc->range.level[l].sens - 1); levdatum = symtab_search(&newp->p_levels, name); diff --git a/security/selinux/ss/mls.h b/security/selinux/ss/mls.h index 07980636751fa..93cde1b22992c 100644 --- a/security/selinux/ss/mls.h +++ b/security/selinux/ss/mls.h @@ -27,9 +27,9 @@ int mls_compute_context_len(struct policydb *p, struct context *context); void mls_sid_to_context(struct policydb *p, struct context *context, char **scontext); -int mls_context_isvalid(struct policydb *p, struct context *c); -int mls_range_isvalid(struct policydb *p, struct mls_range *r); -int mls_level_isvalid(struct policydb *p, struct mls_level *l); +bool mls_context_isvalid(const struct policydb *p, const struct context *c); +bool mls_range_isvalid(const struct policydb *p, const struct mls_range *r); +bool mls_level_isvalid(const struct policydb *p, const struct mls_level *l); int mls_context_to_sid(struct policydb *p, char oldc, char *scontext, struct context *context, struct sidtab *s, u32 def_sid); diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 74f06f11b084a..5df43a042615d 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -930,51 +930,59 @@ int policydb_load_isids(struct policydb *p, struct sidtab *s) return 0; } -int policydb_class_isvalid(struct policydb *p, u16 class) +bool policydb_class_isvalid(const struct policydb *p, u16 class) { if (!class || class > p->p_classes.nprim) - return 0; - return 1; + return false; + if (!p->sym_val_to_name[SYM_CLASSES][class - 1]) + return false; + return true; } -int policydb_role_isvalid(struct policydb *p, unsigned int role) +bool policydb_role_isvalid(const struct policydb *p, u32 role) { if (!role || role > p->p_roles.nprim) - return 0; - return 1; + return false; + if (!p->sym_val_to_name[SYM_ROLES][role - 1]) + return false; + return true; } -int policydb_type_isvalid(struct policydb *p, unsigned int type) +bool policydb_type_isvalid(const struct policydb *p, u32 type) { if (!type || type > p->p_types.nprim) - return 0; - return 1; + return false; + if (!p->sym_val_to_name[SYM_TYPES][type - 1]) + return false; + return true; } -int policydb_boolean_isvalid(const struct policydb *p, u32 boolean) +bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean) { if (!boolean || boolean > p->p_bools.nprim) - return 0; - return 1; + return false; + if (!p->sym_val_to_name[SYM_BOOLS][boolean - 1]) + return false; + return true; } /* - * Return 1 if the fields in the security context + * Return true if the fields in the security context * structure `c' are valid. Return 0 otherwise. */ -int policydb_context_isvalid(struct policydb *p, struct context *c) +bool policydb_context_isvalid(const struct policydb *p, const struct context *c) { - struct role_datum *role; - struct user_datum *usrdatum; + const struct role_datum *role; + const struct user_datum *usrdatum; if (!c->role || c->role > p->p_roles.nprim) - return 0; + return false; if (!c->user || c->user > p->p_users.nprim) - return 0; + return false; if (!c->type || c->type > p->p_types.nprim) - return 0; + return false; if (c->role != OBJECT_R_VAL) { /* @@ -983,24 +991,24 @@ int policydb_context_isvalid(struct policydb *p, struct context *c) role = p->role_val_to_struct[c->role - 1]; if (!role || !ebitmap_get_bit(&role->types, c->type - 1)) /* role may not be associated with type */ - return 0; + return false; /* * User must be authorized for the role. */ usrdatum = p->user_val_to_struct[c->user - 1]; if (!usrdatum) - return 0; + return false; if (!ebitmap_get_bit(&usrdatum->roles, c->role - 1)) /* user may not be associated with role */ - return 0; + return false; } if (!mls_context_isvalid(p, c)) - return 0; + return false; - return 1; + return true; } /* diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 812bdaaf4dd85..44e1f32a34b7f 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -322,11 +322,11 @@ struct policy_file { extern void policydb_destroy(struct policydb *p); extern int policydb_load_isids(struct policydb *p, struct sidtab *s); -extern int policydb_context_isvalid(struct policydb *p, struct context *c); -extern int policydb_class_isvalid(struct policydb *p, u16 class); -extern int policydb_type_isvalid(struct policydb *p, unsigned int type); -extern int policydb_role_isvalid(struct policydb *p, unsigned int role); -extern int policydb_boolean_isvalid(const struct policydb *p, u32 boolean); +extern bool policydb_context_isvalid(const struct policydb *p, const struct context *c); +extern bool policydb_class_isvalid(const struct policydb *p, u16 class); +extern bool policydb_type_isvalid(const struct policydb *p, u32 type); +extern bool policydb_role_isvalid(const struct policydb *p, u32 role); +extern bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean); extern int policydb_read(struct policydb *p, struct policy_file *fp); extern int policydb_write(struct policydb *p, struct policy_file *fp); @@ -397,7 +397,7 @@ static inline int put_entry(const void *buf, size_t bytes, size_t num, return 0; } -static inline char *sym_name(struct policydb *p, unsigned int sym_num, +static inline const char *sym_name(const struct policydb *p, unsigned int sym_num, unsigned int element_nr) { return p->sym_val_to_name[sym_num][element_nr]; diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index a600cb0edecbb..20c8515369086 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -464,7 +464,7 @@ static void security_dump_masked_av(struct policydb *policydb, struct common_datum *common_dat; struct class_datum *tclass_dat; struct audit_buffer *ab; - char *tclass_name; + const char *tclass_name; char *scontext_name = NULL; char *tcontext_name = NULL; char *permission_names[SEL_VEC_MAX]; diff --git a/security/selinux/ss/symtab.c b/security/selinux/ss/symtab.c index 832660fd84a96..a756554e7f1d3 100644 --- a/security/selinux/ss/symtab.c +++ b/security/selinux/ss/symtab.c @@ -50,7 +50,7 @@ int symtab_insert(struct symtab *s, char *name, void *datum) return hashtab_insert(&s->table, name, datum, symtab_key_params); } -void *symtab_search(struct symtab *s, const char *name) +void *symtab_search(const struct symtab *s, const char *name) { return hashtab_search(&s->table, name, symtab_key_params); } diff --git a/security/selinux/ss/symtab.h b/security/selinux/ss/symtab.h index 8e667cdbf38fc..7cfa3b44953ab 100644 --- a/security/selinux/ss/symtab.h +++ b/security/selinux/ss/symtab.h @@ -21,6 +21,6 @@ struct symtab { int symtab_init(struct symtab *s, u32 size); int symtab_insert(struct symtab *s, char *name, void *datum); -void *symtab_search(struct symtab *s, const char *name); +void *symtab_search(const struct symtab *s, const char *name); #endif /* _SS_SYMTAB_H_ */ From 59fa1e46d8d43d0d8c1fc782d83450122c910e9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:46 +0100 Subject: [PATCH 10/14] selinux: validate symbols MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Some symbol tables need to be validated after indexing, since during indexing their referenced entries might not yet have been indexed. Signed-off-by: Christian Göttsche --- security/selinux/ss/policydb.c | 94 ++++++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 5df43a042615d..085c7236929b5 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -679,6 +679,84 @@ static int (*const index_f[SYM_NUM])(void *key, void *datum, void *datap) = { }; /* clang-format on */ +static int role_validate(void *key, void *datum, void *datap) +{ + const struct policydb *p = datap; + const struct role_datum *role = datum; + struct ebitmap_node *node; + u32 i; + + ebitmap_for_each_positive_bit(&role->dominates, node, i) { + if (!policydb_role_isvalid(p, i)) + goto bad; + } + + ebitmap_for_each_positive_bit(&role->types, node, i) { + if (!policydb_type_isvalid(p, i + 1)) + goto bad; + } + + return 0; + +bad: + pr_err("SELinux: invalid role %s\n", sym_name(p, SYM_ROLES, role->value - 1)); + return -EINVAL; +} + +static int user_validate(void *key, void *datum, void *datap) +{ + const struct policydb *p = datap; + const struct user_datum *usrdatum = datum; + struct ebitmap_node *node; + u32 i; + + ebitmap_for_each_positive_bit(&usrdatum->roles, node, i) { + if (!policydb_role_isvalid(p, i)) + goto bad; + } + + if (!mls_range_isvalid(p, &usrdatum->range)) + goto bad; + + if (!mls_level_isvalid(p, &usrdatum->dfltlevel)) + goto bad; + + return 0; + +bad: + pr_err("SELinux: invalid user %s\n", sym_name(p, SYM_USERS, usrdatum->value - 1)); + return -EINVAL; +} + +static int sens_validate(void *key, void *datum, void *datap) +{ + const struct policydb *p = datap; + const struct level_datum *levdatum = datum; + + if (!mls_level_isvalid(p, &levdatum->level)) + goto bad; + + return 0; + +bad: + pr_err("SELinux: invalid sensitivity\n"); + return -EINVAL; +} + + +/* clang-format off */ +static int (*const validate_f[SYM_NUM])(void *key, void *datum, void *datap) = { + NULL, /* Everything validated in common_read() and common_index() */ + NULL, /* Everything validated in class_read() and class_index() */ + role_validate, + NULL, /* Everything validated in type_read(), type_index() and type_bounds_sanity_check() */ + user_validate, + NULL, /* Everything validated in cond_read_bool() and cond_index_bool() */ + sens_validate, + NULL, /* Everything validated in cat_read() and cat_index() */ +}; +/* clang-format on */ + #ifdef CONFIG_SECURITY_SELINUX_DEBUG static void hash_eval(struct hashtab *h, const char *hash_name, const char *hash_details) @@ -771,6 +849,16 @@ static int policydb_index(struct policydb *p) if (rc) goto out; } + + for (i = 0; i < SYM_NUM; i++) { + if (!validate_f[i]) + continue; + + rc = hashtab_map(&p->symtab[i].table, validate_f[i], p); + if (rc) + goto out; + } + rc = 0; out: return rc; @@ -1094,6 +1182,12 @@ static int context_read_and_validate(struct context *c, struct policydb *p, pr_err("SELinux: error reading MLS range of context\n"); goto out; } + + rc = -EINVAL; + if (!mls_range_isvalid(p, &c->range)) { + pr_warn("SELinux: invalid range in security context\n"); + goto out; + } } rc = -EINVAL; From 8ccea76cf6d1a9be8f6ad1672141aca7644f4c19 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:47 +0100 Subject: [PATCH 11/14] selinux: more strict bounds check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate the types used in bounds checks. Replace the usage of BUG(), to avoid halting the system on malformed polices. Signed-off-by: Christian Göttsche --- security/selinux/ss/policydb.c | 29 +++++++++++++++++++++++++++-- security/selinux/ss/policydb.h | 1 + security/selinux/ss/services.c | 3 +++ 3 files changed, 31 insertions(+), 2 deletions(-) diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 085c7236929b5..9cc79827ce8e9 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1027,6 +1027,15 @@ bool policydb_class_isvalid(const struct policydb *p, u16 class) return true; } +bool policydb_user_isvalid(const struct policydb *p, u32 user) +{ + if (!user || user > p->p_roles.nprim) + return false; + if (!p->sym_val_to_name[SYM_USERS][user - 1]) + return false; + return true; +} + bool policydb_role_isvalid(const struct policydb *p, u32 role) { if (!role || role > p->p_roles.nprim) @@ -1949,6 +1958,12 @@ static int user_bounds_sanity_check(void *key, void *datum, void *datap) return -EINVAL; } + if (!policydb_user_isvalid(p, upper->bounds)) { + pr_err("SELinux: user %s: invalid boundary id %d\n", + (char *) key, upper->bounds); + return -EINVAL; + } + upper = p->user_val_to_struct[upper->bounds - 1]; ebitmap_for_each_positive_bit(&user->roles, node, bit) { @@ -1986,6 +2001,12 @@ static int role_bounds_sanity_check(void *key, void *datum, void *datap) return -EINVAL; } + if (!policydb_role_isvalid(p, upper->bounds)) { + pr_err("SELinux: role %s: invalid boundary id %d\n", + (char *) key, upper->bounds); + return -EINVAL; + } + upper = p->role_val_to_struct[upper->bounds - 1]; ebitmap_for_each_positive_bit(&role->types, node, bit) { @@ -2020,9 +2041,13 @@ static int type_bounds_sanity_check(void *key, void *datum, void *datap) return -EINVAL; } - upper = p->type_val_to_struct[upper->bounds - 1]; - BUG_ON(!upper); + if (!policydb_type_isvalid(p, upper->bounds)) { + pr_err("SELinux: type %s: invalid boundary id %d\n", + (char *) key, upper->bounds); + return -EINVAL; + } + upper = p->type_val_to_struct[upper->bounds - 1]; if (upper->attribute) { pr_err("SELinux: type %s: " "bounded by attribute %s\n", diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 44e1f32a34b7f..91145e1916a64 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -326,6 +326,7 @@ extern bool policydb_context_isvalid(const struct policydb *p, const struct cont extern bool policydb_class_isvalid(const struct policydb *p, u16 class); extern bool policydb_type_isvalid(const struct policydb *p, u32 type); extern bool policydb_role_isvalid(const struct policydb *p, u32 role); +extern bool policydb_user_isvalid(const struct policydb *p, u32 user); extern bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean); extern int policydb_read(struct policydb *p, struct policy_file *fp); extern int policydb_write(struct policydb *p, struct policy_file *fp); diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index 20c8515369086..d2a2dc55f4876 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -718,6 +718,9 @@ static void context_struct_compute_av(struct policydb *policydb, * If the given source and target types have boundary * constraint, lazy checks have to mask any violated * permission and notice it to userspace via audit. + * + * Infinite recursion is avoided via a depth pre-check in + * type_bounds_sanity_check(). */ type_attribute_bounds_av(policydb, scontext, tcontext, tclass, avd); From 8ec98414f65fd670eddaf10a19aff3b5677ede78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:48 +0100 Subject: [PATCH 12/14] selinux: check for simple types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate that the target of AVTAB_TYPE rules and file transitions are simple types and not attributes. Signed-off-by: Christian Göttsche --- security/selinux/ss/avtab.c | 9 ++++++++- security/selinux/ss/policydb.c | 23 +++++++++++++++++++++-- security/selinux/ss/policydb.h | 1 + 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/security/selinux/ss/avtab.c b/security/selinux/ss/avtab.c index b708e2c9acdda..0f94edd01f696 100644 --- a/security/selinux/ss/avtab.c +++ b/security/selinux/ss/avtab.c @@ -393,6 +393,13 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po } key.specified = spec_order[i] | enabled; datum.u.data = le32_to_cpu(buf32[items++]); + + if ((key.specified & AVTAB_TYPE) && + !policydb_simpletype_isvalid(pol, datum.u.data)) { + pr_err("SELinux: avtab: invalid type\n"); + return -EINVAL; + } + rc = insertf(a, &key, &datum, p); if (rc) return rc; @@ -484,7 +491,7 @@ int avtab_read_item(struct avtab *a, struct policy_file *fp, struct policydb *po datum.u.data = le32_to_cpu(*buf32); } if ((key.specified & AVTAB_TYPE) && - !policydb_type_isvalid(pol, datum.u.data)) { + !policydb_simpletype_isvalid(pol, datum.u.data)) { pr_err("SELinux: avtab: invalid type\n"); return -EINVAL; } diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index 9cc79827ce8e9..e31c1045758bf 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -692,7 +692,7 @@ static int role_validate(void *key, void *datum, void *datap) } ebitmap_for_each_positive_bit(&role->types, node, i) { - if (!policydb_type_isvalid(p, i + 1)) + if (!policydb_simpletype_isvalid(p, i + 1)) goto bad; } @@ -1054,6 +1054,23 @@ bool policydb_type_isvalid(const struct policydb *p, u32 type) return true; } +bool policydb_simpletype_isvalid(const struct policydb *p, u32 type) +{ + const struct type_datum *datum; + + if (!type || type > p->p_types.nprim) + return false; + + datum = p->type_val_to_struct[type - 1]; + if (!datum) + return false; + + if (datum->attribute) + return false; + + return true; +} + bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean) { if (!boolean || boolean > p->p_bools.nprim) @@ -2242,6 +2259,8 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f key.name = name; otype = le32_to_cpu(buf[3]); + if (!policydb_simpletype_isvalid(p, otype)) + goto out; last = NULL; datum = policydb_filenametr_search(p, &key); @@ -2364,7 +2383,7 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp datum->otype = le32_to_cpu(buf[0]); rc = -EINVAL; - if (!policydb_type_isvalid(p, datum->otype)) + if (!policydb_simpletype_isvalid(p, datum->otype)) goto out; dst = &datum->next; diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index 91145e1916a64..a3cf3da36364e 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -325,6 +325,7 @@ extern int policydb_load_isids(struct policydb *p, struct sidtab *s); extern bool policydb_context_isvalid(const struct policydb *p, const struct context *c); extern bool policydb_class_isvalid(const struct policydb *p, u16 class); extern bool policydb_type_isvalid(const struct policydb *p, u32 type); +extern bool policydb_simpletype_isvalid(const struct policydb *p, u32 type); extern bool policydb_role_isvalid(const struct policydb *p, u32 role); extern bool policydb_user_isvalid(const struct policydb *p, u32 user); extern bool policydb_boolean_isvalid(const struct policydb *p, u32 boolean); From be354b5139e0837ad3797fb15b902461ed20f091 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Thu, 14 Nov 2024 16:31:49 +0100 Subject: [PATCH 13/14] selinux: restrict policy strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validate the characters and the lengths of strings parsed from binary policies. * Disallow control characters * Limit characters of identifiers to alphanumeric, underscore, dash, and dot * Limit identifiers in length to 64, expect types to 1024, sensitivities to 32 and categories to 16, characters (excluding NUL-terminator) Signed-off-by: Christian Göttsche --- v3: - introduce a central limits.h header - add limits for all kinds of string: filesystem names, filetrans keys, genfs paths, infiniband device names v2: - add wrappers for str_read() to minimize the usage of magic numbers - limit sensitivities to a length of 32, to match categories, suggested by Daniel --- security/selinux/include/limits.h | 90 +++++++++++++++++++++++++++++++ security/selinux/ss/conditional.c | 5 +- security/selinux/ss/conditional.h | 2 - security/selinux/ss/constraint.h | 2 - security/selinux/ss/policydb.c | 78 ++++++++++++++++++--------- security/selinux/ss/policydb.h | 51 +++++++++++++++++- 6 files changed, 196 insertions(+), 32 deletions(-) create mode 100644 security/selinux/include/limits.h diff --git a/security/selinux/include/limits.h b/security/selinux/include/limits.h new file mode 100644 index 0000000000000..d267c0c64f491 --- /dev/null +++ b/security/selinux/include/limits.h @@ -0,0 +1,90 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Limits for various policy database elements. + */ + +/* + * Maximum supported depth of conditional expressions. + */ +#define COND_EXPR_MAXDEPTH 10 + +/* + * Maximum supported depth for constraint expressions. + */ +#define CEXPR_MAXDEPTH 5 + +/* + * Maximum supported identifier value. + * + * Reasoning: The most used symbols are types and they need to fit into + * an u16 for the avtab entries. Keep U16_MAX as special value + * and U16_MAX-1 to avoid accidental overflows into U16_MAX. + */ +#define IDENTIFIER_MAXVALUE (U16_MAX - 2) + +/* + * Maximum supported length of security context strings. + * + * Reasoning: The string must fir into a PAGE_SIZE. + */ +#define CONTEXT_MAXLENGTH 4000 + +/* + * Maximum supported boolean name length. + */ +#define BOOLEAN_NAME_MAXLENGTH 64 + +/* + * Maximum supported security class and common class name length. + */ +#define CLASS_NAME_MAXLENGTH 64 + +/* + * Maximum supported permission name length. + */ +#define PERMISSION_NAME_MAXLENGTH 64 + +/* + * Maximum supported user name length. + */ +#define USER_NAME_MAXLENGTH 64 + +/* + * Maximum supported role name length. + */ +#define ROLE_NAME_MAXLENGTH 64 + +/* + * Maximum supported type name length. + */ +#define TYPE_NAME_MAXLENGTH 1024 + +/* + * Maximum supported sensitivity name length. + */ +#define SENSITIVITY_NAME_MAXLENGTH 32 + +/* + * Maximum supported category name length. + */ +#define CATEGORY_NAME_MAXLENGTH 16 + +/* + * Maximum supported path name length for keys in filename transitions. + */ +#define FILETRANSKEY_NAME_MAXLENGTH 1024 + +/* + * Maximum supported filesystem name length. + */ +#define FILESYSTEM_NAME_MAXLENGTH 128 + +/* + * Maximum supported path prefix length for genfs statements. + */ +#define GENFS_PATH_MAXLENGTH 1024 + +/* + * Maximum supported Infiniband device name length. + */ +#define INFINIBAND_DEVNAME_MAXLENGTH 256 diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index ce0281cce739f..c0a2814dafdba 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -245,7 +245,8 @@ int cond_index_bool(void *key, void *datum, void *datap) booldatum = datum; p = datap; - if (!booldatum->value || booldatum->value > p->p_bools.nprim) + if (!booldatum->value || booldatum->value > p->p_bools.nprim || + booldatum->value > IDENTIFIER_MAXVALUE) return -EINVAL; p->sym_val_to_name[SYM_BOOLS][booldatum->value - 1] = key; @@ -280,7 +281,7 @@ int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp) len = le32_to_cpu(buf[2]); - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_bool(&key, GFP_KERNEL, fp, len); if (rc) goto err; diff --git a/security/selinux/ss/conditional.h b/security/selinux/ss/conditional.h index 468e98ad3ea14..d5aefcbaa1eb1 100644 --- a/security/selinux/ss/conditional.h +++ b/security/selinux/ss/conditional.h @@ -12,8 +12,6 @@ #include "policydb.h" #include "../include/conditional.h" -#define COND_EXPR_MAXDEPTH 10 - /* * A conditional expression is a list of operators and operands * in reverse polish notation. diff --git a/security/selinux/ss/constraint.h b/security/selinux/ss/constraint.h index 1d75a8a044df9..f986156de8569 100644 --- a/security/selinux/ss/constraint.h +++ b/security/selinux/ss/constraint.h @@ -19,8 +19,6 @@ #include "ebitmap.h" -#define CEXPR_MAXDEPTH 5 - struct constraint_expr { #define CEXPR_NOT 1 /* not expr */ #define CEXPR_AND 2 /* expr and expr */ diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c index e31c1045758bf..c711d3fac8514 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -558,7 +558,8 @@ static int common_index(void *key, void *datum, void *datap) comdatum = datum; p = datap; - if (!comdatum->value || comdatum->value > p->p_commons.nprim) + if (!comdatum->value || comdatum->value > p->p_commons.nprim || + comdatum->value > IDENTIFIER_MAXVALUE) return -EINVAL; p->sym_val_to_name[SYM_COMMONS][comdatum->value - 1] = key; @@ -573,7 +574,8 @@ static int class_index(void *key, void *datum, void *datap) cladatum = datum; p = datap; - if (!cladatum->value || cladatum->value > p->p_classes.nprim) + if (!cladatum->value || cladatum->value > p->p_classes.nprim || + cladatum->value > IDENTIFIER_MAXVALUE) return -EINVAL; p->sym_val_to_name[SYM_CLASSES][cladatum->value - 1] = key; @@ -589,6 +591,7 @@ static int role_index(void *key, void *datum, void *datap) role = datum; p = datap; if (!role->value || role->value > p->p_roles.nprim || + role->value > IDENTIFIER_MAXVALUE || role->bounds > p->p_roles.nprim) return -EINVAL; @@ -607,6 +610,7 @@ static int type_index(void *key, void *datum, void *datap) if (typdatum->primary) { if (!typdatum->value || typdatum->value > p->p_types.nprim || + typdatum->value > IDENTIFIER_MAXVALUE || typdatum->bounds > p->p_types.nprim) return -EINVAL; p->sym_val_to_name[SYM_TYPES][typdatum->value - 1] = key; @@ -624,6 +628,7 @@ static int user_index(void *key, void *datum, void *datap) usrdatum = datum; p = datap; if (!usrdatum->value || usrdatum->value > p->p_users.nprim || + usrdatum->value > IDENTIFIER_MAXVALUE || usrdatum->bounds > p->p_users.nprim) return -EINVAL; @@ -640,7 +645,8 @@ static int sens_index(void *key, void *datum, void *datap) levdatum = datum; p = datap; - if (!levdatum->level.sens || levdatum->level.sens > p->p_levels.nprim) + if (!levdatum->level.sens || levdatum->level.sens > p->p_levels.nprim || + levdatum->level.sens > IDENTIFIER_MAXVALUE) return -EINVAL; if (!levdatum->isalias) @@ -657,7 +663,8 @@ static int cat_index(void *key, void *datum, void *datap) catdatum = datum; p = datap; - if (!catdatum->value || catdatum->value > p->p_cats.nprim) + if (!catdatum->value || catdatum->value > p->p_cats.nprim || + catdatum->value > IDENTIFIER_MAXVALUE) return -EINVAL; if (!catdatum->isalias) @@ -1233,8 +1240,9 @@ static int context_read_and_validate(struct context *c, struct policydb *p, * binary representation file. */ -int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, int kind, u32 max_len) { + u32 i; int rc; char *str; @@ -1244,19 +1252,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len) if (size_check(sizeof(char), len, fp)) return -EINVAL; + if (len > max_len) + return -EINVAL; + str = kmalloc(len + 1, flags | __GFP_NOWARN); if (!str) return -ENOMEM; rc = next_entry(str, fp, len); - if (rc) { - kfree(str); - return rc; + if (rc) + goto bad_str; + + rc = -EINVAL; + for (i = 0; i < len; i++) { + if (iscntrl(str[i])) + goto bad_str; + + if (kind == STR_IDENTIFIER && + !(isalnum(str[i]) || str[i] == '_' || str[i] == '-' || str[i] == '.')) + goto bad_str; + } str[len] = '\0'; *strp = str; return 0; + +bad_str: + kfree(str); + return rc; } static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp) @@ -1281,7 +1305,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f if (perdatum->value < 1 || perdatum->value > SEL_VEC_MAX) goto bad; - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_perm(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -1328,7 +1352,7 @@ static int common_read(struct policydb *p, struct symtab *s, struct policy_file goto bad; comdatum->permissions.nprim = le32_to_cpu(buf[2]); - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_class(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -1566,12 +1590,12 @@ static int class_read(struct policydb *p, struct symtab *s, struct policy_file * ncons = le32_to_cpu(buf[5]); - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_class(&key, GFP_KERNEL, fp, len); if (rc) goto bad; if (len2) { - rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2); + rc = str_read_class(&cladatum->comkey, GFP_KERNEL, fp, len2); if (rc) goto bad; @@ -1705,7 +1729,7 @@ static int role_read(struct policydb *p, struct symtab *s, struct policy_file *f if (p->policyvers >= POLICYDB_VERSION_BOUNDARY) role->bounds = le32_to_cpu(buf[2]); - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_role(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -1772,7 +1796,7 @@ static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f typdatum->primary = le32_to_cpu(buf[2]); } - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_type(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -1836,7 +1860,7 @@ static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f if (p->policyvers >= POLICYDB_VERSION_BOUNDARY) usrdatum->bounds = le32_to_cpu(buf[2]); - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_user(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -1885,7 +1909,7 @@ static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f goto bad; levdatum->isalias = val; - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_sens(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -1928,7 +1952,7 @@ static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp goto bad; catdatum->isalias = val; - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_cat(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -2237,7 +2261,7 @@ static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f len = le32_to_cpu(buf[0]); /* path component string */ - rc = str_read(&name, GFP_KERNEL, fp, len); + rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, FILETRANSKEY_NAME_MAXLENGTH); if (rc) return rc; @@ -2336,7 +2360,7 @@ static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp len = le32_to_cpu(buf[0]); /* path component string */ - rc = str_read(&name, GFP_KERNEL, fp, len); + rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, FILETRANSKEY_NAME_MAXLENGTH); if (rc) return rc; @@ -2490,7 +2514,7 @@ static int genfs_read(struct policydb *p, struct policy_file *fp) if (!newgenfs) goto out; - rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len); + rc = str_read_fsname(&newgenfs->fstype, GFP_KERNEL, fp, len); if (rc) goto out; @@ -2529,7 +2553,8 @@ static int genfs_read(struct policydb *p, struct policy_file *fp) if (!newc) goto out; - rc = str_read(&newc->u.name, GFP_KERNEL, fp, len); + rc = str_read(&newc->u.name, GFP_KERNEL, fp, len, + STR_UNCONSTRAINT, GENFS_PATH_MAXLENGTH); if (rc) goto out; @@ -2632,7 +2657,7 @@ static int ocontext_read(struct policydb *p, goto out; len = le32_to_cpu(buf[0]); - rc = str_read(&c->u.name, GFP_KERNEL, fp, len); + rc = str_read_fsname(&c->u.name, GFP_KERNEL, fp, len); if (rc) goto out; @@ -2700,7 +2725,7 @@ static int ocontext_read(struct policydb *p, goto out; len = le32_to_cpu(buf[1]); - rc = str_read(&c->u.name, GFP_KERNEL, fp, len); + rc = str_read_fsname(&c->u.name, GFP_KERNEL, fp, len); if (rc) goto out; @@ -2766,7 +2791,9 @@ static int ocontext_read(struct policydb *p, len = le32_to_cpu(buf[0]); rc = str_read(&c->u.ibendport.dev_name, - GFP_KERNEL, fp, len); + GFP_KERNEL, fp, len, + STR_UNCONSTRAINT, + INFINIBAND_DEVNAME_MAXLENGTH); if (rc) goto out; @@ -2834,7 +2861,8 @@ int policydb_read(struct policydb *p, struct policy_file *fp) goto bad; } - rc = str_read(&policydb_str, GFP_KERNEL, fp, len); + rc = str_read(&policydb_str, GFP_KERNEL, fp, len, + STR_UNCONSTRAINT, strlen(POLICYDB_STRING)); if (rc) { if (rc == -ENOMEM) { pr_err("SELinux: unable to allocate memory for policydb string of length %d\n", diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h index a3cf3da36364e..83727e61ad904 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -27,6 +27,7 @@ #include "mls_types.h" #include "context.h" #include "constraint.h" +#include "limits.h" /* * A datum type is defined for each kind of symbol @@ -410,7 +411,55 @@ static inline bool val_is_boolean(u32 value) return value == 0 || value == 1; } -extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len); +#define STR_UNCONSTRAINT 0 +#define STR_IDENTIFIER 1 +extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, + int kind, u32 max_len); + +static inline int str_read_bool(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, BOOLEAN_NAME_MAXLENGTH); +} + +static inline int str_read_cat(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, CATEGORY_NAME_MAXLENGTH); +} + +static inline int str_read_class(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, CLASS_NAME_MAXLENGTH); +} + +static inline int str_read_perm(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, PERMISSION_NAME_MAXLENGTH); +} + +static inline int str_read_role(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, ROLE_NAME_MAXLENGTH); +} + +static inline int str_read_sens(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, SENSITIVITY_NAME_MAXLENGTH); +} + +static inline int str_read_type(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, TYPE_NAME_MAXLENGTH); +} + +static inline int str_read_user(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, USER_NAME_MAXLENGTH); +} + +static inline int str_read_fsname(char **strp, gfp_t flags, struct policy_file *fp, u32 len) +{ + return str_read(strp, flags, fp, len, STR_IDENTIFIER, FILESYSTEM_NAME_MAXLENGTH); +} extern u16 string_to_security_class(struct policydb *p, const char *name); extern u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name); From 90aa43418da6fdaeb6c4a5e15668c199dcd1e905 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20G=C3=B6ttsche?= Date: Sun, 11 May 2025 17:52:37 +0200 Subject: [PATCH 14/14] selinux: harden MLS context string generation against overflows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Check the length accumulator for the MLS component of security contexts does not overflow in mls_compute_context_len() resulting in out-of-buffer writes in mls_sid_to_context(). Signed-off-by: Christian Göttsche --- v3: add patch --- security/selinux/ss/mls.c | 18 +++++++++----- security/selinux/ss/services.c | 43 +++++++++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 12 deletions(-) diff --git a/security/selinux/ss/mls.c b/security/selinux/ss/mls.c index 3cd36e2015fa2..aa25724f0b0fb 100644 --- a/security/selinux/ss/mls.c +++ b/security/selinux/ss/mls.c @@ -42,7 +42,8 @@ int mls_compute_context_len(struct policydb *p, struct context *context) len = 1; /* for the beginning ":" */ for (l = 0; l < 2; l++) { u32 index_sens = context->range.level[l].sens; - len += strlen(sym_name(p, SYM_LEVELS, index_sens - 1)); + if (check_add_overflow(len, strlen(sym_name(p, SYM_LEVELS, index_sens - 1)), &len)) + return -EOVERFLOW; /* categories */ head = -2; @@ -54,24 +55,29 @@ int mls_compute_context_len(struct policydb *p, struct context *context) /* one or more negative bits are skipped */ if (head != prev) { nm = sym_name(p, SYM_CATS, prev); - len += strlen(nm) + 1; + if (check_add_overflow(len, strlen(nm) + 1, &len)) + return -EOVERFLOW; } nm = sym_name(p, SYM_CATS, i); - len += strlen(nm) + 1; + if (check_add_overflow(len, strlen(nm) + 1, &len)) + return -EOVERFLOW; head = i; } prev = i; } if (prev != head) { nm = sym_name(p, SYM_CATS, prev); - len += strlen(nm) + 1; + if (check_add_overflow(len, strlen(nm) + 1, &len)) + return -EOVERFLOW; } if (l == 0) { if (mls_level_eq(&context->range.level[0], &context->range.level[1])) break; - else - len++; + else { + if (check_add_overflow(len, 1, &len)) + return -EOVERFLOW; + } } } diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c index d2a2dc55f4876..1db28fd058659 100644 --- a/security/selinux/ss/services.c +++ b/security/selinux/ss/services.c @@ -1267,10 +1267,12 @@ static int context_struct_to_string(struct policydb *p, char **scontext, u32 *scontext_len) { char *scontextp; + size_t len; + int mls_len; if (scontext) *scontext = NULL; - *scontext_len = 0; + len = 0; if (context->len) { *scontext_len = context->len; @@ -1283,16 +1285,45 @@ static int context_struct_to_string(struct policydb *p, } /* Compute the size of the context. */ - *scontext_len += strlen(sym_name(p, SYM_USERS, context->user - 1)) + 1; - *scontext_len += strlen(sym_name(p, SYM_ROLES, context->role - 1)) + 1; - *scontext_len += strlen(sym_name(p, SYM_TYPES, context->type - 1)) + 1; - *scontext_len += mls_compute_context_len(p, context); + len += strlen(sym_name(p, SYM_USERS, context->user - 1)) + 1; + len += strlen(sym_name(p, SYM_ROLES, context->role - 1)) + 1; + len += strlen(sym_name(p, SYM_TYPES, context->type - 1)) + 1; + + mls_len = mls_compute_context_len(p, context); + if (unlikely(mls_len < 0)) { + pr_warn_ratelimited("SELinux: %s: MLS security context component too large [%s:%s:%s[:[%s:%d]-[%s:%d]]]\n", + __func__, + sym_name(p, SYM_USERS, context->user - 1), + sym_name(p, SYM_ROLES, context->role - 1), + sym_name(p, SYM_TYPES, context->type - 1), + sym_name(p, SYM_LEVELS, context->range.level[0].sens - 1), + ebitmap_length(&context->range.level[0].cat), + sym_name(p, SYM_LEVELS, context->range.level[1].sens - 1), + ebitmap_length(&context->range.level[1].cat)); + return -EOVERFLOW; + } + + if (unlikely(check_add_overflow(len, mls_len, &len) || len > CONTEXT_MAXLENGTH)) { + pr_warn_ratelimited("SELinux: %s: security context string of length %zu too large [%s:%s:%s[:[%s:%d]-[%s:%d]]]\n", + __func__, + len, + sym_name(p, SYM_USERS, context->user - 1), + sym_name(p, SYM_ROLES, context->role - 1), + sym_name(p, SYM_TYPES, context->type - 1), + sym_name(p, SYM_LEVELS, context->range.level[0].sens - 1), + ebitmap_length(&context->range.level[0].cat), + sym_name(p, SYM_LEVELS, context->range.level[1].sens - 1), + ebitmap_length(&context->range.level[1].cat)); + return -EOVERFLOW; + } + + *scontext_len = len; if (!scontext) return 0; /* Allocate space for the context; caller must free this space. */ - scontextp = kmalloc(*scontext_len, GFP_ATOMIC); + scontextp = kmalloc(len, GFP_ATOMIC); if (!scontextp) return -ENOMEM; *scontext = scontextp;