| Message ID | 20241216164055.96267-22-cgoettsche@seltendoof.de (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Paul Moore |
| Headers | show |
| Series | [RFC,v2,01/22] selinux: supply missing field initializers | expand |
On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche <cgoettsche@seltendoof.de> wrote: > > From: Christian Göttsche <cgzones@googlemail.com> > > 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 128, expect types to 1024 and > categories to 32, characters (excluding NUL-terminator) One option if we are concerned about breaking backward compatibility with policies in the wild would be to make these restrictions conditional on whether the policy is being loaded into a non-init SELinux namespace, similar to: https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u That said, it seems hard to imagine real-world policies that would exceed these limits, and likely could make them even smaller. But as Daniel said, we should make them consistently enforced in both userspace and kernel, and potentially these should all be #define'd symbols in a uapi header that can be referenced by both. Looks like you left the type limit at 1024 despite Daniel's observation that CIL uses 2048 as the limit, but as you noted, given the page size limit on the entire context by various kernel interfaces, this is likely fine.
> On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche > <cgoettsche@seltendoof.de> wrote: >> >> From: Christian Göttsche <cgzones@googlemail.com> >> >> Validate the characters and the lengths of strings parsed from binary >> policies. Excellent idea. >> * Disallow control characters Fine here. >> * Limit characters of identifiers to alphanumeric, underscore, dash, >> and dot Fine again. >> * Limit identifiers in length to 128, Fine again, our longest - type is 51 characters - attribute is 31 - boolean is 46 - role is 12 >> expect types to 1024 and I don’t understand what this means. >> categories to 32, characters (excluding NUL-terminator) One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string. Joe > One option if we are concerned about breaking backward compatibility > with policies in the wild would be to make these restrictions > conditional on whether the policy is being loaded into a non-init > SELinux namespace, similar to: > https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u > > That said, it seems hard to imagine real-world policies that would > exceed these limits, and likely could make them even smaller. > But as Daniel said, we should make them consistently enforced in both > userspace and kernel, and potentially these should all be #define'd > symbols in a uapi header that can be referenced by both. > Looks like you left the type limit at 1024 despite Daniel's > observation that CIL uses 2048 as the limit, but as you noted, given > the page size limit on the entire context by various kernel > interfaces, > this is likely fine. >
On Mon, 6 Jan 2025 at 00:26, Joe Nall <joenall@gmail.com> wrote: > > > > > On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > > > On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche > > <cgoettsche@seltendoof.de> wrote: > >> > >> From: Christian Göttsche <cgzones@googlemail.com> > >> > >> Validate the characters and the lengths of strings parsed from binary > >> policies. > Excellent idea. > > >> * Disallow control characters > Fine here. > > >> * Limit characters of identifiers to alphanumeric, underscore, dash, > >> and dot > Fine again. > > >> * Limit identifiers in length to 128, > Fine again, our longest > - type is 51 characters > - attribute is 31 > - boolean is 46 > - role is 12 > > >> expect types to 1024 and > I don’t understand what this means. Similar to your list of the length in you policy boolean, role, user, class, and permission identifiers are limited to 128 charatcers (not including NUL), types (and attributes, which are just special types) are limited to 1024 characters, and individual sensitivities and categories are limited to 32 characters. > > >> categories to 32, characters (excluding NUL-terminator) > One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string. This only affects individual sensitivities and categories, like "s9" or "c1023", not whole MCS/MLS parts. > Joe > > > One option if we are concerned about breaking backward compatibility > > with policies in the wild would be to make these restrictions > > conditional on whether the policy is being loaded into a non-init > > SELinux namespace, similar to: > > https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u > > > > That said, it seems hard to imagine real-world policies that would > > exceed these limits, and likely could make them even smaller. > > But as Daniel said, we should make them consistently enforced in both > > userspace and kernel, and potentially these should all be #define'd > > symbols in a uapi header that can be referenced by both. > > Looks like you left the type limit at 1024 despite Daniel's > > observation that CIL uses 2048 as the limit, but as you noted, given > > the page size limit on the entire context by various kernel > > interfaces, > > this is likely fine. I interpreted Daniels comment more like a assessment what CIL currently constrains, not as a request for a change, maybe I misunderstood? Exporting the limits via a public headers seems reasonable. Maybe for a smooth transition one could introduce a build time configuration (CONFIG_SELINUX_STRICT_IDENTIFIERS?). This configuration can be disabled by default, leading to identifiers not being rejected only logged. Than after two releases the default can change to reject instead of log. And after the next LTS release the configuration can be dropped again. > > >
On 1/7/2025 9:04 AM, Christian Göttsche wrote: > On Mon, 6 Jan 2025 at 00:26, Joe Nall <joenall@gmail.com> wrote: >> >> >> >>> On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote: >>> >>> On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche >>> <cgoettsche@seltendoof.de> wrote: >>>> >>>> From: Christian Göttsche <cgzones@googlemail.com> >>>> >>>> Validate the characters and the lengths of strings parsed from binary >>>> policies. >> Excellent idea. >> >>>> * Disallow control characters >> Fine here. >> >>>> * Limit characters of identifiers to alphanumeric, underscore, dash, >>>> and dot >> Fine again. >> >>>> * Limit identifiers in length to 128, >> Fine again, our longest >> - type is 51 characters >> - attribute is 31 >> - boolean is 46 >> - role is 12 >> >>>> expect types to 1024 and >> I don’t understand what this means. > > Similar to your list of the length in you policy boolean, role, user, > class, and permission identifiers are limited to 128 charatcers (not > including NUL), types (and attributes, which are just special types) > are limited to 1024 characters, and individual sensitivities and > categories are limited to 32 characters. > >> >>>> categories to 32, characters (excluding NUL-terminator) >> One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string. > > This only affects individual sensitivities and categories, like "s9" > or "c1023", not whole MCS/MLS parts. > >> Joe >> >>> One option if we are concerned about breaking backward compatibility >>> with policies in the wild would be to make these restrictions >>> conditional on whether the policy is being loaded into a non-init >>> SELinux namespace, similar to: >>> https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u >>> >>> That said, it seems hard to imagine real-world policies that would >>> exceed these limits, and likely could make them even smaller. >>> But as Daniel said, we should make them consistently enforced in both >>> userspace and kernel, and potentially these should all be #define'd >>> symbols in a uapi header that can be referenced by both. >>> Looks like you left the type limit at 1024 despite Daniel's >>> observation that CIL uses 2048 as the limit, but as you noted, given >>> the page size limit on the entire context by various kernel >>> interfaces, >>> this is likely fine. > > I interpreted Daniels comment more like a assessment what CIL > currently constrains, not as a request for a change, maybe I > misunderstood? That is what I intended, yes. My related request was "I would think that we'd want to end up in a situation where the kernel is either equally restrictive or less restrictive than CIL". In isolation, my opinion is that the 1024 limit is fine, but I've been hoping James would chime in about the feasibility of dropping the CIL limit at some point to get them in sync. FWIW we have a few generated type names internally that subjectively feel long to humans, but are still under 100 characters. So 1024 is plenty of extra margin in my opinion. -Daniel > > Exporting the limits via a public headers seems reasonable. > > Maybe for a smooth transition one could introduce a build time > configuration (CONFIG_SELINUX_STRICT_IDENTIFIERS?). > This configuration can be disabled by default, leading to identifiers > not being rejected only logged. > Than after two releases the default can change to reject instead of log. > And after the next LTS release the configuration can be dropped again. > >>> >>
On Tue, Jan 7, 2025 at 11:12 AM Daniel Burgener <dburgener@linux.microsoft.com> wrote: > > On 1/7/2025 9:04 AM, Christian Göttsche wrote: > > On Mon, 6 Jan 2025 at 00:26, Joe Nall <joenall@gmail.com> wrote: > >> > >> > >> > >>> On Jan 3, 2025, at 2:12 PM, Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > >>> > >>> On Mon, Dec 16, 2024 at 11:42 AM Christian Göttsche > >>> <cgoettsche@seltendoof.de> wrote: > >>>> > >>>> From: Christian Göttsche <cgzones@googlemail.com> > >>>> > >>>> Validate the characters and the lengths of strings parsed from binary > >>>> policies. > >> Excellent idea. > >> > >>>> * Disallow control characters > >> Fine here. > >> > >>>> * Limit characters of identifiers to alphanumeric, underscore, dash, > >>>> and dot > >> Fine again. > >> > >>>> * Limit identifiers in length to 128, > >> Fine again, our longest > >> - type is 51 characters > >> - attribute is 31 > >> - boolean is 46 > >> - role is 12 > >> > >>>> expect types to 1024 and > >> I don’t understand what this means. > > > > Similar to your list of the length in you policy boolean, role, user, > > class, and permission identifiers are limited to 128 charatcers (not > > including NUL), types (and attributes, which are just special types) > > are limited to 1024 characters, and individual sensitivities and > > categories are limited to 32 characters. > > > >> > >>>> categories to 32, characters (excluding NUL-terminator) > >> One category or the whole category string? A single category longer than 7 characters seems pretty unlikely and 32 is wildly short for the whole string. > > > > This only affects individual sensitivities and categories, like "s9" > > or "c1023", not whole MCS/MLS parts. > > > >> Joe > >> > >>> One option if we are concerned about breaking backward compatibility > >>> with policies in the wild would be to make these restrictions > >>> conditional on whether the policy is being loaded into a non-init > >>> SELinux namespace, similar to: > >>> https://lore.kernel.org/selinux/20250102164509.25606-38-stephen.smalley.work@gmail.com/T/#u > >>> > >>> That said, it seems hard to imagine real-world policies that would > >>> exceed these limits, and likely could make them even smaller. > >>> But as Daniel said, we should make them consistently enforced in both > >>> userspace and kernel, and potentially these should all be #define'd > >>> symbols in a uapi header that can be referenced by both. > >>> Looks like you left the type limit at 1024 despite Daniel's > >>> observation that CIL uses 2048 as the limit, but as you noted, given > >>> the page size limit on the entire context by various kernel > >>> interfaces, > >>> this is likely fine. > > > > I interpreted Daniels comment more like a assessment what CIL > > currently constrains, not as a request for a change, maybe I > > misunderstood? > > That is what I intended, yes. My related request was "I would think > that we'd want to end up in a situation where the kernel is either > equally restrictive or less restrictive than CIL". In isolation, my > opinion is that the 1024 limit is fine, but I've been hoping James would > chime in about the feasibility of dropping the CIL limit at some point > to get them in sync. > The CIL limit of 2048 is arbitrary and could be changed to 1024 without a problem. Jim > FWIW we have a few generated type names internally that subjectively > feel long to humans, but are still under 100 characters. So 1024 is > plenty of extra margin in my opinion. > > -Daniel > > > > > Exporting the limits via a public headers seems reasonable. > > > > Maybe for a smooth transition one could introduce a build time > > configuration (CONFIG_SELINUX_STRICT_IDENTIFIERS?). > > This configuration can be disabled by default, leading to identifiers > > not being rejected only logged. > > Than after two releases the default can change to reject instead of log. > > And after the next LTS release the configuration can be dropped again. > > > >>> > >> > >
diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c index 481aa17a6f26..7688615a3934 100644 --- a/security/selinux/ss/conditional.c +++ b/security/selinux/ss/conditional.c @@ -280,7 +280,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/policydb.c b/security/selinux/ss/policydb.c index 34db40753654..c72182b91f49 100644 --- a/security/selinux/ss/policydb.c +++ b/security/selinux/ss/policydb.c @@ -1226,8 +1226,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; @@ -1237,19 +1238,35 @@ int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len) if (oom_check(sizeof(char), len, fp)) return -EINVAL; + if (max_len != 0 && 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) @@ -1274,7 +1291,7 @@ static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f if (perdatum->value < 1 || perdatum->value > 32) goto bad; - rc = str_read(&key, GFP_KERNEL, fp, len); + rc = str_read_perm(&key, GFP_KERNEL, fp, len); if (rc) goto bad; @@ -1320,7 +1337,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; @@ -1557,12 +1574,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; @@ -1696,7 +1713,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; @@ -1763,7 +1780,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; @@ -1827,7 +1844,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; @@ -1876,7 +1893,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; @@ -1919,7 +1936,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; @@ -2225,7 +2242,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, 0); if (rc) return rc; @@ -2324,7 +2341,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, 0); if (rc) return rc; @@ -2478,7 +2495,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(&newgenfs->fstype, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128); if (rc) goto out; @@ -2517,7 +2534,7 @@ 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, 0); if (rc) goto out; @@ -2620,7 +2637,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(&c->u.name, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128); if (rc) goto out; @@ -2688,7 +2705,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(&c->u.name, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128); if (rc) goto out; @@ -2754,7 +2771,7 @@ 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_IDENTIFIER, 128); if (rc) goto out; @@ -2822,7 +2839,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 d979965ef939..22e5e5adf73c 100644 --- a/security/selinux/ss/policydb.h +++ b/security/selinux/ss/policydb.h @@ -402,7 +402,50 @@ static inline const char *sym_name(const struct policydb *p, unsigned int sym_nu return p->sym_val_to_name[sym_num][element_nr]; } -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, 128); +} + +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, 32); +} + +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, 128); +} + +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, 128); +} + +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, 128); +} + +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, 32); +} + +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, 1024); +} + +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, 128); +} 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);