On Thu, Oct 15, 2020 at 7:00 PM James Carter <jwcart2@xxxxxxxxx> wrote:
>
> On Sat, Oct 3, 2020 at 3:40 PM Nicolas Iooss <nicolas.iooss@xxxxxxx> wrote:
> >
> > Contrary to Linux kernel, BUG_ON() does not halt the execution, in
> > libsepol/src/services.c. Instead it displays an error message and
> > continues the execution.
> >
> > This means that this code does not prevent an out-of-bound write from
> > happening:
> >
> > case CEXPR_AND:
> > BUG_ON(sp < 1);
> > sp--;
> > s[sp] &= s[sp + 1];
> >
> > Use if(...){BUG();rc=-EINVAL;goto out;} constructions instead, to make
> > sure that the array access is always in-bound.
> >
> > This issue has been found using clang's static analyzer:
> > https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-50a861.html#EndPath
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@xxxxxxx>
>
> Acked-by: James Carter <jwcart2@xxxxxxxxx>
Thanks. Merged.
Nicolas
>
> > ---
> > libsepol/src/services.c | 19 +++++++++++++++----
> > 1 file changed, 15 insertions(+), 4 deletions(-)
> >
> > diff --git a/libsepol/src/services.c b/libsepol/src/services.c
> > index 90da1f4efef3..beb0711f6680 100644
> > --- a/libsepol/src/services.c
> > +++ b/libsepol/src/services.c
> > @@ -67,7 +67,6 @@
> > #include "flask.h"
> >
> > #define BUG() do { ERR(NULL, "Badness at %s:%d", __FILE__, __LINE__); } while (0)
> > -#define BUG_ON(x) do { if (x) ERR(NULL, "Badness at %s:%d", __FILE__, __LINE__); } while (0)
> >
> > static int selinux_enforcing = 1;
> >
> > @@ -469,18 +468,30 @@ static int constraint_expr_eval_reason(context_struct_t *scontext,
> > /* Now process each expression of the constraint */
> > switch (e->expr_type) {
> > case CEXPR_NOT:
> > - BUG_ON(sp < 0);
> > + if (sp < 0) {
> > + BUG();
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > s[sp] = !s[sp];
> > cat_expr_buf(expr_list[expr_counter], "not");
> > break;
> > case CEXPR_AND:
> > - BUG_ON(sp < 1);
> > + if (sp < 1) {
> > + BUG();
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > sp--;
> > s[sp] &= s[sp + 1];
> > cat_expr_buf(expr_list[expr_counter], "and");
> > break;
> > case CEXPR_OR:
> > - BUG_ON(sp < 1);
> > + if (sp < 1) {
> > + BUG();
> > + rc = -EINVAL;
> > + goto out;
> > + }
> > sp--;
> > s[sp] |= s[sp + 1];
> > cat_expr_buf(expr_list[expr_counter], "or");
> > --
> > 2.28.0
> >
