* [PATCH 1/2] libsepol: drop confusing BUG_ON macro
@ 2020-10-03 19:39 Nicolas Iooss
2020-10-03 19:39 ` [PATCH 2/2] libsepol: silence potential NULL pointer dereference warning Nicolas Iooss
2020-10-15 17:00 ` [PATCH 1/2] libsepol: drop confusing BUG_ON macro James Carter
0 siblings, 2 replies; 6+ messages in thread
From: Nicolas Iooss @ 2020-10-03 19:39 UTC (permalink / raw)
To: selinux
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@m4x.org>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] libsepol: silence potential NULL pointer dereference warning
2020-10-03 19:39 [PATCH 1/2] libsepol: drop confusing BUG_ON macro Nicolas Iooss
@ 2020-10-03 19:39 ` Nicolas Iooss
2020-10-15 17:00 ` James Carter
2020-10-15 17:00 ` [PATCH 1/2] libsepol: drop confusing BUG_ON macro James Carter
1 sibling, 1 reply; 6+ messages in thread
From: Nicolas Iooss @ 2020-10-03 19:39 UTC (permalink / raw)
To: selinux
When find_avtab_node() is called with key->specified & AVTAB_XPERMS and
xperms=NULL, xperms is being dereferenced. This is detected as a
"NULL pointer dereference issue" by static analyzers.
Even though it does not make much sense to call find_avtab_node() in a
way which triggers the NULL pointer dereference issue, static analyzers
have a hard time with calls such as:
node = find_avtab_node(handle, avtab, &avkey, cond, NULL);
... where xperms=NULL.
So, make the function report an error instead of crashing.
Here is an example of report from clang's static analyzer:
https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
libsepol/src/expand.c | 23 ++++++++++++++---------
1 file changed, 14 insertions(+), 9 deletions(-)
diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
index 19e48c507236..eac7e4507d02 100644
--- a/libsepol/src/expand.c
+++ b/libsepol/src/expand.c
@@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
/* AVTAB_XPERMS entries are not necessarily unique */
if (key->specified & AVTAB_XPERMS) {
- node = avtab_search_node(avtab, key);
- while (node) {
- if ((node->datum.xperms->specified == xperms->specified) &&
- (node->datum.xperms->driver == xperms->driver)) {
- match = 1;
- break;
+ if (xperms == NULL) {
+ ERR(handle, "searching xperms NULL");
+ node = NULL;
+ } else {
+ node = avtab_search_node(avtab, key);
+ while (node) {
+ if ((node->datum.xperms->specified == xperms->specified) &&
+ (node->datum.xperms->driver == xperms->driver)) {
+ match = 1;
+ break;
+ }
+ node = avtab_search_node_next(node, key->specified);
}
- node = avtab_search_node_next(node, key->specified);
+ if (!match)
+ node = NULL;
}
- if (!match)
- node = NULL;
} else {
node = avtab_search_node(avtab, key);
}
--
2.28.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] libsepol: drop confusing BUG_ON macro
2020-10-03 19:39 [PATCH 1/2] libsepol: drop confusing BUG_ON macro Nicolas Iooss
2020-10-03 19:39 ` [PATCH 2/2] libsepol: silence potential NULL pointer dereference warning Nicolas Iooss
@ 2020-10-15 17:00 ` James Carter
2020-10-19 21:28 ` Nicolas Iooss
1 sibling, 1 reply; 6+ messages in thread
From: James Carter @ 2020-10-15 17:00 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Sat, Oct 3, 2020 at 3:40 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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@m4x.org>
Acked-by: James Carter <jwcart2@gmail.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] libsepol: silence potential NULL pointer dereference warning
2020-10-03 19:39 ` [PATCH 2/2] libsepol: silence potential NULL pointer dereference warning Nicolas Iooss
@ 2020-10-15 17:00 ` James Carter
2020-10-19 21:29 ` Nicolas Iooss
0 siblings, 1 reply; 6+ messages in thread
From: James Carter @ 2020-10-15 17:00 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list
On Sat, Oct 3, 2020 at 3:41 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> When find_avtab_node() is called with key->specified & AVTAB_XPERMS and
> xperms=NULL, xperms is being dereferenced. This is detected as a
> "NULL pointer dereference issue" by static analyzers.
>
> Even though it does not make much sense to call find_avtab_node() in a
> way which triggers the NULL pointer dereference issue, static analyzers
> have a hard time with calls such as:
>
> node = find_avtab_node(handle, avtab, &avkey, cond, NULL);
>
> ... where xperms=NULL.
>
> So, make the function report an error instead of crashing.
>
> Here is an example of report from clang's static analyzer:
> https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
Acked-by: James Carter <jwcart2@gmail.com>
> ---
> libsepol/src/expand.c | 23 ++++++++++++++---------
> 1 file changed, 14 insertions(+), 9 deletions(-)
>
> diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> index 19e48c507236..eac7e4507d02 100644
> --- a/libsepol/src/expand.c
> +++ b/libsepol/src/expand.c
> @@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
>
> /* AVTAB_XPERMS entries are not necessarily unique */
> if (key->specified & AVTAB_XPERMS) {
> - node = avtab_search_node(avtab, key);
> - while (node) {
> - if ((node->datum.xperms->specified == xperms->specified) &&
> - (node->datum.xperms->driver == xperms->driver)) {
> - match = 1;
> - break;
> + if (xperms == NULL) {
> + ERR(handle, "searching xperms NULL");
> + node = NULL;
> + } else {
> + node = avtab_search_node(avtab, key);
> + while (node) {
> + if ((node->datum.xperms->specified == xperms->specified) &&
> + (node->datum.xperms->driver == xperms->driver)) {
> + match = 1;
> + break;
> + }
> + node = avtab_search_node_next(node, key->specified);
> }
> - node = avtab_search_node_next(node, key->specified);
> + if (!match)
> + node = NULL;
> }
> - if (!match)
> - node = NULL;
> } else {
> node = avtab_search_node(avtab, key);
> }
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] libsepol: drop confusing BUG_ON macro
2020-10-15 17:00 ` [PATCH 1/2] libsepol: drop confusing BUG_ON macro James Carter
@ 2020-10-19 21:28 ` Nicolas Iooss
0 siblings, 0 replies; 6+ messages in thread
From: Nicolas Iooss @ 2020-10-19 21:28 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
On Thu, Oct 15, 2020 at 7:00 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sat, Oct 3, 2020 at 3:40 PM Nicolas Iooss <nicolas.iooss@m4x.org> 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@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>
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
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] libsepol: silence potential NULL pointer dereference warning
2020-10-15 17:00 ` James Carter
@ 2020-10-19 21:29 ` Nicolas Iooss
0 siblings, 0 replies; 6+ messages in thread
From: Nicolas Iooss @ 2020-10-19 21:29 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list
On Thu, Oct 15, 2020 at 7:00 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sat, Oct 3, 2020 at 3:41 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > When find_avtab_node() is called with key->specified & AVTAB_XPERMS and
> > xperms=NULL, xperms is being dereferenced. This is detected as a
> > "NULL pointer dereference issue" by static analyzers.
> >
> > Even though it does not make much sense to call find_avtab_node() in a
> > way which triggers the NULL pointer dereference issue, static analyzers
> > have a hard time with calls such as:
> >
> > node = find_avtab_node(handle, avtab, &avkey, cond, NULL);
> >
> > ... where xperms=NULL.
> >
> > So, make the function report an error instead of crashing.
> >
> > Here is an example of report from clang's static analyzer:
> > https://558-118970575-gh.circle-artifacts.com/0/output-scan-build/2020-10-02-065849-6375-1/report-d86a57.html#EndPath
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>
Thanks. Merged.
Nicolas
> > ---
> > libsepol/src/expand.c | 23 ++++++++++++++---------
> > 1 file changed, 14 insertions(+), 9 deletions(-)
> >
> > diff --git a/libsepol/src/expand.c b/libsepol/src/expand.c
> > index 19e48c507236..eac7e4507d02 100644
> > --- a/libsepol/src/expand.c
> > +++ b/libsepol/src/expand.c
> > @@ -1570,17 +1570,22 @@ static avtab_ptr_t find_avtab_node(sepol_handle_t * handle,
> >
> > /* AVTAB_XPERMS entries are not necessarily unique */
> > if (key->specified & AVTAB_XPERMS) {
> > - node = avtab_search_node(avtab, key);
> > - while (node) {
> > - if ((node->datum.xperms->specified == xperms->specified) &&
> > - (node->datum.xperms->driver == xperms->driver)) {
> > - match = 1;
> > - break;
> > + if (xperms == NULL) {
> > + ERR(handle, "searching xperms NULL");
> > + node = NULL;
> > + } else {
> > + node = avtab_search_node(avtab, key);
> > + while (node) {
> > + if ((node->datum.xperms->specified == xperms->specified) &&
> > + (node->datum.xperms->driver == xperms->driver)) {
> > + match = 1;
> > + break;
> > + }
> > + node = avtab_search_node_next(node, key->specified);
> > }
> > - node = avtab_search_node_next(node, key->specified);
> > + if (!match)
> > + node = NULL;
> > }
> > - if (!match)
> > - node = NULL;
> > } else {
> > node = avtab_search_node(avtab, key);
> > }
> > --
> > 2.28.0
> >
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-19 21:38 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-03 19:39 [PATCH 1/2] libsepol: drop confusing BUG_ON macro Nicolas Iooss
2020-10-03 19:39 ` [PATCH 2/2] libsepol: silence potential NULL pointer dereference warning Nicolas Iooss
2020-10-15 17:00 ` James Carter
2020-10-19 21:29 ` Nicolas Iooss
2020-10-15 17:00 ` [PATCH 1/2] libsepol: drop confusing BUG_ON macro James Carter
2020-10-19 21:28 ` Nicolas Iooss
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.