| Message ID | 20231101163725.177237-1-cgzones@googlemail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b1b3467a476b |
| Delegated to: | Petr Lautrbach |
| Headers | show |
| Series | [v2,1/2] libsepol: reject avtab entries with invalid specifier | expand |
On Wed, Nov 1, 2023 at 12:38 PM Christian Göttsche <cgzones@googlemail.com> wrote: > > Neverallow avtab entries are not supported (normal and extended). Reject > them to avoid lookup confusions via avtab_search(), e.g. when searching > for a invalid key of AVTAB_TRANSITION|AVTAB_NEVERALLOW and the result of > only AVTAB_NEVERALLOW has no transition value. > > Simplify the check for the number of specifiers by using the compiler > popcount builtin (already used in libsepol). > > Reported-by: oss-fuzz (issue 60568), caused at the time by the filetrans > prefix proposal > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> For these two patches: Acked-by: James Carter <jwcart2@gmail.com> > --- > v2: > rebase after revert of filename prefix proposal > --- > libsepol/src/avtab.c | 13 ++++++------- > 1 file changed, 6 insertions(+), 7 deletions(-) > > diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c > index 6ab49c5e..1ef5ee00 100644 > --- a/libsepol/src/avtab.c > +++ b/libsepol/src/avtab.c > @@ -441,7 +441,6 @@ int avtab_read_item(struct policy_file *fp, uint32_t vers, avtab_t * a, > avtab_key_t key; > avtab_datum_t datum; > avtab_extended_perms_t xperms; > - unsigned set; > unsigned int i; > int rc; > > @@ -535,13 +534,13 @@ int avtab_read_item(struct policy_file *fp, uint32_t vers, avtab_t * a, > key.target_class = le16_to_cpu(buf16[items++]); > key.specified = le16_to_cpu(buf16[items++]); > > - set = 0; > - for (i = 0; i < ARRAY_SIZE(spec_order); i++) { > - if (key.specified & spec_order[i]) > - set++; > + if (key.specified & ~(AVTAB_AV | AVTAB_TYPE | AVTAB_XPERMS | AVTAB_ENABLED)) { > + ERR(fp->handle, "invalid specifier"); > + return -1; > } > - if (!set || set > 1) { > - ERR(fp->handle, "more than one specifier"); > + > + if (__builtin_popcount(key.specified & ~AVTAB_ENABLED) != 1) { > + ERR(fp->handle, "not exactly one specifier"); > return -1; > } > > -- > 2.42.0 >
On Thu, Nov 2, 2023 at 11:17 AM James Carter <jwcart2@gmail.com> wrote: > > On Wed, Nov 1, 2023 at 12:38 PM Christian Göttsche > <cgzones@googlemail.com> wrote: > > > > Neverallow avtab entries are not supported (normal and extended). Reject > > them to avoid lookup confusions via avtab_search(), e.g. when searching > > for a invalid key of AVTAB_TRANSITION|AVTAB_NEVERALLOW and the result of > > only AVTAB_NEVERALLOW has no transition value. > > > > Simplify the check for the number of specifiers by using the compiler > > popcount builtin (already used in libsepol). > > > > Reported-by: oss-fuzz (issue 60568), caused at the time by the filetrans > > prefix proposal > > Signed-off-by: Christian Göttsche <cgzones@googlemail.com> > > For these two patches: > Acked-by: James Carter <jwcart2@gmail.com> > These two patches have been merged. Thanks, Jim > > --- > > v2: > > rebase after revert of filename prefix proposal > > --- > > libsepol/src/avtab.c | 13 ++++++------- > > 1 file changed, 6 insertions(+), 7 deletions(-) > > > > diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c > > index 6ab49c5e..1ef5ee00 100644 > > --- a/libsepol/src/avtab.c > > +++ b/libsepol/src/avtab.c > > @@ -441,7 +441,6 @@ int avtab_read_item(struct policy_file *fp, uint32_t vers, avtab_t * a, > > avtab_key_t key; > > avtab_datum_t datum; > > avtab_extended_perms_t xperms; > > - unsigned set; > > unsigned int i; > > int rc; > > > > @@ -535,13 +534,13 @@ int avtab_read_item(struct policy_file *fp, uint32_t vers, avtab_t * a, > > key.target_class = le16_to_cpu(buf16[items++]); > > key.specified = le16_to_cpu(buf16[items++]); > > > > - set = 0; > > - for (i = 0; i < ARRAY_SIZE(spec_order); i++) { > > - if (key.specified & spec_order[i]) > > - set++; > > + if (key.specified & ~(AVTAB_AV | AVTAB_TYPE | AVTAB_XPERMS | AVTAB_ENABLED)) { > > + ERR(fp->handle, "invalid specifier"); > > + return -1; > > } > > - if (!set || set > 1) { > > - ERR(fp->handle, "more than one specifier"); > > + > > + if (__builtin_popcount(key.specified & ~AVTAB_ENABLED) != 1) { > > + ERR(fp->handle, "not exactly one specifier"); > > return -1; > > } > > > > -- > > 2.42.0 > >
diff --git a/libsepol/src/avtab.c b/libsepol/src/avtab.c index 6ab49c5e..1ef5ee00 100644 --- a/libsepol/src/avtab.c +++ b/libsepol/src/avtab.c @@ -441,7 +441,6 @@ int avtab_read_item(struct policy_file *fp, uint32_t vers, avtab_t * a, avtab_key_t key; avtab_datum_t datum; avtab_extended_perms_t xperms; - unsigned set; unsigned int i; int rc; @@ -535,13 +534,13 @@ int avtab_read_item(struct policy_file *fp, uint32_t vers, avtab_t * a, key.target_class = le16_to_cpu(buf16[items++]); key.specified = le16_to_cpu(buf16[items++]); - set = 0; - for (i = 0; i < ARRAY_SIZE(spec_order); i++) { - if (key.specified & spec_order[i]) - set++; + if (key.specified & ~(AVTAB_AV | AVTAB_TYPE | AVTAB_XPERMS | AVTAB_ENABLED)) { + ERR(fp->handle, "invalid specifier"); + return -1; } - if (!set || set > 1) { - ERR(fp->handle, "more than one specifier"); + + if (__builtin_popcount(key.specified & ~AVTAB_ENABLED) != 1) { + ERR(fp->handle, "not exactly one specifier"); return -1; }
Neverallow avtab entries are not supported (normal and extended). Reject them to avoid lookup confusions via avtab_search(), e.g. when searching for a invalid key of AVTAB_TRANSITION|AVTAB_NEVERALLOW and the result of only AVTAB_NEVERALLOW has no transition value. Simplify the check for the number of specifiers by using the compiler popcount builtin (already used in libsepol). Reported-by: oss-fuzz (issue 60568), caused at the time by the filetrans prefix proposal Signed-off-by: Christian Göttsche <cgzones@googlemail.com> --- v2: rebase after revert of filename prefix proposal --- libsepol/src/avtab.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)