| Message ID | 20231207165319.63889-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [RFC] libsepol: validate permission identifier length | expand |
On Thu, Dec 7, 2023 at 11:53 AM Christian Göttsche <cgzones@googlemail.com> wrote: > > Limit the maximum length of permission identifiers. Otherwise > formatting an access vector might fail in the common used helper > sepol_av_to_string(). > > The current longest permission within the Reference Policy is > x_application_data { paste_after_confirm } with a length of 19. Android has longer permission names. I'd rather just fix sepol_av_to_string() than impose some arbitrary limit here. > > Reported-by: oss-fuzz (issue 64832) > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > --- > The oss-fuzz issue got closed by the latest fuzzer updates (due to > unrelated changes that invalid the current reproducer), but the issue is > still valid. > --- > libsepol/src/policydb_validate.c | 20 ++++++++++++++++++++ > 1 file changed, 20 insertions(+) > > diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c > index bd8e9f8f..72681120 100644 > --- a/libsepol/src/policydb_validate.c > +++ b/libsepol/src/policydb_validate.c > @@ -366,12 +366,30 @@ bad: > return -1; > } > > +static int validate_permission_wrapper(hashtab_key_t k, __attribute__((unused)) hashtab_datum_t d, void *args) > +{ > + sepol_handle_t *handle = args; > + const char *name = k; > + size_t len = strlen(name); > + > + if (len == 0 || len >= 32) > + goto bad; > + > + return 0; > + > +bad: > + ERR(handle, "Invalid permission"); > + return -1; > +} > + > static int validate_common_datum(sepol_handle_t *handle, const common_datum_t *common, validate_t flavors[]) > { > if (validate_value(common->s.value, &flavors[SYM_COMMONS])) > goto bad; > if (common->permissions.table->nel == 0 || common->permissions.nprim > PERM_SYMTAB_SIZE) > goto bad; > + if (hashtab_map(common->permissions.table, validate_permission_wrapper, handle)) > + goto bad; > > return 0; > > @@ -395,6 +413,8 @@ static int validate_class_datum(sepol_handle_t *handle, const class_datum_t *cla > goto bad; > if (class->permissions.nprim > PERM_SYMTAB_SIZE) > goto bad; > + if (hashtab_map(class->permissions.table, validate_permission_wrapper, handle)) > + goto bad; > if (validate_constraint_nodes(handle, class->permissions.nprim, class->constraints, flavors)) > goto bad; > if (validate_constraint_nodes(handle, 0, class->validatetrans, flavors)) > -- > 2.43.0 > >
On Fri, Dec 8, 2023 at 11:05 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Dec 7, 2023 at 11:53 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Limit the maximum length of permission identifiers. Otherwise > > formatting an access vector might fail in the common used helper > > sepol_av_to_string(). > > > > The current longest permission within the Reference Policy is > > x_application_data { paste_after_confirm } with a length of 19. > > Android has longer permission names. I'd rather just fix > sepol_av_to_string() than impose some arbitrary limit here. Looking at AOSP master, it appears that the longest permission is 32-bytes but since we haven't imposed such a limit in the past, we have no way of knowing what any particular version of refpolicy, AOSP policy, or downstream policies might have used.
On Fri, Dec 8, 2023 at 11:06 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Thu, Dec 7, 2023 at 11:53 AM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Limit the maximum length of permission identifiers. Otherwise > > formatting an access vector might fail in the common used helper > > sepol_av_to_string(). > > > > The current longest permission within the Reference Policy is > > x_application_data { paste_after_confirm } with a length of 19. > > Android has longer permission names. I'd rather just fix > sepol_av_to_string() than impose some arbitrary limit here. > I agree. Christian, something like your recent patch to avoid a fixed size format buffer for xperms would be better. Thanks for all your work. Jim > > > > Reported-by: oss-fuzz (issue 64832) > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > --- > > The oss-fuzz issue got closed by the latest fuzzer updates (due to > > unrelated changes that invalid the current reproducer), but the issue is > > still valid. > > --- > > libsepol/src/policydb_validate.c | 20 ++++++++++++++++++++ > > 1 file changed, 20 insertions(+) > > > > diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c > > index bd8e9f8f..72681120 100644 > > --- a/libsepol/src/policydb_validate.c > > +++ b/libsepol/src/policydb_validate.c > > @@ -366,12 +366,30 @@ bad: > > return -1; > > } > > > > +static int validate_permission_wrapper(hashtab_key_t k, __attribute__((unused)) hashtab_datum_t d, void *args) > > +{ > > + sepol_handle_t *handle = args; > > + const char *name = k; > > + size_t len = strlen(name); > > + > > + if (len == 0 || len >= 32) > > + goto bad; > > + > > + return 0; > > + > > +bad: > > + ERR(handle, "Invalid permission"); > > + return -1; > > +} > > + > > static int validate_common_datum(sepol_handle_t *handle, const common_datum_t *common, validate_t flavors[]) > > { > > if (validate_value(common->s.value, &flavors[SYM_COMMONS])) > > goto bad; > > if (common->permissions.table->nel == 0 || common->permissions.nprim > PERM_SYMTAB_SIZE) > > goto bad; > > + if (hashtab_map(common->permissions.table, validate_permission_wrapper, handle)) > > + goto bad; > > > > return 0; > > > > @@ -395,6 +413,8 @@ static int validate_class_datum(sepol_handle_t *handle, const class_datum_t *cla > > goto bad; > > if (class->permissions.nprim > PERM_SYMTAB_SIZE) > > goto bad; > > + if (hashtab_map(class->permissions.table, validate_permission_wrapper, handle)) > > + goto bad; > > if (validate_constraint_nodes(handle, class->permissions.nprim, class->constraints, flavors)) > > goto bad; > > if (validate_constraint_nodes(handle, 0, class->validatetrans, flavors)) > > -- > > 2.43.0 > > > > >
diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c index bd8e9f8f..72681120 100644 --- a/libsepol/src/policydb_validate.c +++ b/libsepol/src/policydb_validate.c @@ -366,12 +366,30 @@ bad: return -1; } +static int validate_permission_wrapper(hashtab_key_t k, __attribute__((unused)) hashtab_datum_t d, void *args) +{ + sepol_handle_t *handle = args; + const char *name = k; + size_t len = strlen(name); + + if (len == 0 || len >= 32) + goto bad; + + return 0; + +bad: + ERR(handle, "Invalid permission"); + return -1; +} + static int validate_common_datum(sepol_handle_t *handle, const common_datum_t *common, validate_t flavors[]) { if (validate_value(common->s.value, &flavors[SYM_COMMONS])) goto bad; if (common->permissions.table->nel == 0 || common->permissions.nprim > PERM_SYMTAB_SIZE) goto bad; + if (hashtab_map(common->permissions.table, validate_permission_wrapper, handle)) + goto bad; return 0; @@ -395,6 +413,8 @@ static int validate_class_datum(sepol_handle_t *handle, const class_datum_t *cla goto bad; if (class->permissions.nprim > PERM_SYMTAB_SIZE) goto bad; + if (hashtab_map(class->permissions.table, validate_permission_wrapper, handle)) + goto bad; if (validate_constraint_nodes(handle, class->permissions.nprim, class->constraints, flavors)) goto bad; if (validate_constraint_nodes(handle, 0, class->validatetrans, flavors))
Limit the maximum length of permission identifiers. Otherwise formatting an access vector might fail in the common used helper sepol_av_to_string(). The current longest permission within the Reference Policy is x_application_data { paste_after_confirm } with a length of 19. Reported-by: oss-fuzz (issue 64832) Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- The oss-fuzz issue got closed by the latest fuzzer updates (due to unrelated changes that invalid the current reproducer), but the issue is still valid. --- libsepol/src/policydb_validate.c | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)