* [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks
@ 2021-02-05 14:44 James Carter
2021-02-08 21:03 ` Nicolas Iooss
0 siblings, 1 reply; 6+ messages in thread
From: James Carter @ 2021-02-05 14:44 UTC (permalink / raw)
To: selinux; +Cc: James Carter, Nicolass Iooss, James Carter
From: James Carter <jwcart2@tycho.nsa.gov>
Nicolass Iooss reports:
OSS-Fuzz found an integer overflow when compiling the following
(empty) CIL policy:
;;*lms 2147483647 a
; (empty line)
Change hll_lineno to uint32_t which is the type of the field hll_line
in struct cil_tree_node where the line number will be stored eventually.
Read the line number into an unsigned long variable using strtoul()
instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, return
an error if the value is larger than UINT32_MAX.
Also change hll_expand to uint32_t, since its value will be either
0 or 1 and there is no need for it to be signed.
Reported-by: Nicolass Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
libsepol/cil/src/cil_parser.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
index 0038eed6..a9306218 100644
--- a/libsepol/cil/src/cil_parser.c
+++ b/libsepol/cil/src/cil_parser.c
@@ -47,11 +47,11 @@ char *CIL_KEY_HLL_LMX;
char *CIL_KEY_HLL_LME;
struct hll_info {
- int hll_lineno;
- int hll_expand;
+ uint32_t hll_lineno;
+ uint32_t hll_expand;
};
-static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expand)
+static void push_hll_info(struct cil_stack *stack, uint32_t hll_lineno, uint32_t hll_expand)
{
struct hll_info *new = cil_malloc(sizeof(*new));
@@ -61,7 +61,7 @@ static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expan
cil_stack_push(stack, CIL_NONE, new);
}
-static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expand)
+static void pop_hll_info(struct cil_stack *stack, uint32_t *hll_lineno, uint32_t *hll_expand)
{
struct cil_stack_item *curr = cil_stack_pop(stack);
struct cil_stack_item *prev = cil_stack_peek(stack);
@@ -70,8 +70,8 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
free(curr->data);
if (!prev) {
- *hll_lineno = -1;
- *hll_expand = -1;
+ *hll_lineno = 0;
+ *hll_expand = 0;
} else {
old = prev->data;
*hll_lineno = old->hll_lineno;
@@ -79,7 +79,7 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
}
}
-static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, int line, int hll_line, void *value)
+static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, uint32_t line, uint32_t hll_line, void *value)
{
cil_tree_node_init(node);
(*node)->parent = current;
@@ -99,13 +99,14 @@ static void insert_node(struct cil_tree_node *node, struct cil_tree_node *curren
current->cl_tail = node;
}
-static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int *hll_expand, struct cil_stack *stack, char *path)
+static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno, uint32_t *hll_expand, struct cil_stack *stack, char *path)
{
char *hll_type;
struct cil_tree_node *node;
struct token tok;
char *hll_file;
char *end = NULL;
+ unsigned long val;
cil_lexer_next(&tok);
hll_type = cil_strpool_add(tok.value);
@@ -141,11 +142,19 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
cil_log(CIL_ERR, "Invalid line mark syntax\n");
goto exit;
}
- *hll_lineno = strtol(tok.value, &end, 10);
+
+ val = strtoul(tok.value, &end, 10);
if (errno == ERANGE || *end != '\0') {
cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
goto exit;
}
+#if ULONG_MAX > UINT32_MAX
+ if (val > UINT32_MAX) {
+ cil_log(CIL_ERR, "Line mark line number > UINT32_MAX\n");
+ goto exit;
+ }
+#endif
+ *hll_lineno = val;
push_hll_info(stack, *hll_lineno, *hll_expand);
@@ -175,7 +184,7 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
return SEPOL_OK;
exit:
- cil_log(CIL_ERR, "Problem with high-level line mark at line %d of %s\n", tok.line, path);
+ cil_log(CIL_ERR, "Problem with high-level line mark at line %u of %s\n", tok.line, path);
return SEPOL_ERR;
}
@@ -207,8 +216,8 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
struct cil_tree_node *current = NULL;
char *path = cil_strpool_add(_path);
struct cil_stack *stack;
- int hll_lineno = -1;
- int hll_expand = -1;
+ uint32_t hll_lineno = 0;
+ uint32_t hll_expand = 0;
struct token tok;
int rc = SEPOL_OK;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks
2021-02-05 14:44 [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks James Carter
@ 2021-02-08 21:03 ` Nicolas Iooss
2021-02-10 13:11 ` James Carter
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Iooss @ 2021-02-08 21:03 UTC (permalink / raw)
To: James Carter; +Cc: SElinux list, James Carter
On Fri, Feb 5, 2021 at 3:44 PM James Carter <jwcart2@gmail.com> wrote:
>
> From: James Carter <jwcart2@tycho.nsa.gov>
>
> Nicolass Iooss reports:
> OSS-Fuzz found an integer overflow when compiling the following
> (empty) CIL policy:
>
> ;;*lms 2147483647 a
> ; (empty line)
>
> Change hll_lineno to uint32_t which is the type of the field hll_line
> in struct cil_tree_node where the line number will be stored eventually.
> Read the line number into an unsigned long variable using strtoul()
> instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, return
> an error if the value is larger than UINT32_MAX.
>
> Also change hll_expand to uint32_t, since its value will be either
> 0 or 1 and there is no need for it to be signed.
>
> Reported-by: Nicolass Iooss <nicolas.iooss@m4x.org>
> Signed-off-by: James Carter <jwcart2@gmail.com>
Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
By the way this week I have infrequent access to the system I use to
work on SELinux stuff, so I will not be able to review/test/apply
patches until next week-end. So feel free to merge this without
waiting for me.
Thanks,
Nicolas
> ---
> libsepol/cil/src/cil_parser.c | 33 +++++++++++++++++++++------------
> 1 file changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> index 0038eed6..a9306218 100644
> --- a/libsepol/cil/src/cil_parser.c
> +++ b/libsepol/cil/src/cil_parser.c
> @@ -47,11 +47,11 @@ char *CIL_KEY_HLL_LMX;
> char *CIL_KEY_HLL_LME;
>
> struct hll_info {
> - int hll_lineno;
> - int hll_expand;
> + uint32_t hll_lineno;
> + uint32_t hll_expand;
> };
>
> -static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expand)
> +static void push_hll_info(struct cil_stack *stack, uint32_t hll_lineno, uint32_t hll_expand)
> {
> struct hll_info *new = cil_malloc(sizeof(*new));
>
> @@ -61,7 +61,7 @@ static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expan
> cil_stack_push(stack, CIL_NONE, new);
> }
>
> -static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expand)
> +static void pop_hll_info(struct cil_stack *stack, uint32_t *hll_lineno, uint32_t *hll_expand)
> {
> struct cil_stack_item *curr = cil_stack_pop(stack);
> struct cil_stack_item *prev = cil_stack_peek(stack);
> @@ -70,8 +70,8 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
> free(curr->data);
>
> if (!prev) {
> - *hll_lineno = -1;
> - *hll_expand = -1;
> + *hll_lineno = 0;
> + *hll_expand = 0;
> } else {
> old = prev->data;
> *hll_lineno = old->hll_lineno;
> @@ -79,7 +79,7 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
> }
> }
>
> -static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, int line, int hll_line, void *value)
> +static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, uint32_t line, uint32_t hll_line, void *value)
> {
> cil_tree_node_init(node);
> (*node)->parent = current;
> @@ -99,13 +99,14 @@ static void insert_node(struct cil_tree_node *node, struct cil_tree_node *curren
> current->cl_tail = node;
> }
>
> -static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int *hll_expand, struct cil_stack *stack, char *path)
> +static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno, uint32_t *hll_expand, struct cil_stack *stack, char *path)
> {
> char *hll_type;
> struct cil_tree_node *node;
> struct token tok;
> char *hll_file;
> char *end = NULL;
> + unsigned long val;
>
> cil_lexer_next(&tok);
> hll_type = cil_strpool_add(tok.value);
> @@ -141,11 +142,19 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
> cil_log(CIL_ERR, "Invalid line mark syntax\n");
> goto exit;
> }
> - *hll_lineno = strtol(tok.value, &end, 10);
> +
> + val = strtoul(tok.value, &end, 10);
> if (errno == ERANGE || *end != '\0') {
> cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
> goto exit;
> }
> +#if ULONG_MAX > UINT32_MAX
> + if (val > UINT32_MAX) {
> + cil_log(CIL_ERR, "Line mark line number > UINT32_MAX\n");
> + goto exit;
> + }
> +#endif
> + *hll_lineno = val;
>
> push_hll_info(stack, *hll_lineno, *hll_expand);
>
> @@ -175,7 +184,7 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
> return SEPOL_OK;
>
> exit:
> - cil_log(CIL_ERR, "Problem with high-level line mark at line %d of %s\n", tok.line, path);
> + cil_log(CIL_ERR, "Problem with high-level line mark at line %u of %s\n", tok.line, path);
> return SEPOL_ERR;
> }
>
> @@ -207,8 +216,8 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
> struct cil_tree_node *current = NULL;
> char *path = cil_strpool_add(_path);
> struct cil_stack *stack;
> - int hll_lineno = -1;
> - int hll_expand = -1;
> + uint32_t hll_lineno = 0;
> + uint32_t hll_expand = 0;
> struct token tok;
> int rc = SEPOL_OK;
>
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks
2021-02-08 21:03 ` Nicolas Iooss
@ 2021-02-10 13:11 ` James Carter
0 siblings, 0 replies; 6+ messages in thread
From: James Carter @ 2021-02-10 13:11 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: SElinux list, James Carter
On Mon, Feb 8, 2021 at 4:03 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Fri, Feb 5, 2021 at 3:44 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > From: James Carter <jwcart2@tycho.nsa.gov>
> >
> > Nicolass Iooss reports:
> > OSS-Fuzz found an integer overflow when compiling the following
> > (empty) CIL policy:
> >
> > ;;*lms 2147483647 a
> > ; (empty line)
> >
> > Change hll_lineno to uint32_t which is the type of the field hll_line
> > in struct cil_tree_node where the line number will be stored eventually.
> > Read the line number into an unsigned long variable using strtoul()
> > instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, return
> > an error if the value is larger than UINT32_MAX.
> >
> > Also change hll_expand to uint32_t, since its value will be either
> > 0 or 1 and there is no need for it to be signed.
> >
> > Reported-by: Nicolass Iooss <nicolas.iooss@m4x.org>
> > Signed-off-by: James Carter <jwcart2@gmail.com>
>
> Acked-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
This has been applied (with your name corrected)
Jim
> By the way this week I have infrequent access to the system I use to
> work on SELinux stuff, so I will not be able to review/test/apply
> patches until next week-end. So feel free to merge this without
> waiting for me.
>
> Thanks,
> Nicolas
>
> > ---
> > libsepol/cil/src/cil_parser.c | 33 +++++++++++++++++++++------------
> > 1 file changed, 21 insertions(+), 12 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> > index 0038eed6..a9306218 100644
> > --- a/libsepol/cil/src/cil_parser.c
> > +++ b/libsepol/cil/src/cil_parser.c
> > @@ -47,11 +47,11 @@ char *CIL_KEY_HLL_LMX;
> > char *CIL_KEY_HLL_LME;
> >
> > struct hll_info {
> > - int hll_lineno;
> > - int hll_expand;
> > + uint32_t hll_lineno;
> > + uint32_t hll_expand;
> > };
> >
> > -static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expand)
> > +static void push_hll_info(struct cil_stack *stack, uint32_t hll_lineno, uint32_t hll_expand)
> > {
> > struct hll_info *new = cil_malloc(sizeof(*new));
> >
> > @@ -61,7 +61,7 @@ static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expan
> > cil_stack_push(stack, CIL_NONE, new);
> > }
> >
> > -static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expand)
> > +static void pop_hll_info(struct cil_stack *stack, uint32_t *hll_lineno, uint32_t *hll_expand)
> > {
> > struct cil_stack_item *curr = cil_stack_pop(stack);
> > struct cil_stack_item *prev = cil_stack_peek(stack);
> > @@ -70,8 +70,8 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
> > free(curr->data);
> >
> > if (!prev) {
> > - *hll_lineno = -1;
> > - *hll_expand = -1;
> > + *hll_lineno = 0;
> > + *hll_expand = 0;
> > } else {
> > old = prev->data;
> > *hll_lineno = old->hll_lineno;
> > @@ -79,7 +79,7 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
> > }
> > }
> >
> > -static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, int line, int hll_line, void *value)
> > +static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, uint32_t line, uint32_t hll_line, void *value)
> > {
> > cil_tree_node_init(node);
> > (*node)->parent = current;
> > @@ -99,13 +99,14 @@ static void insert_node(struct cil_tree_node *node, struct cil_tree_node *curren
> > current->cl_tail = node;
> > }
> >
> > -static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int *hll_expand, struct cil_stack *stack, char *path)
> > +static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno, uint32_t *hll_expand, struct cil_stack *stack, char *path)
> > {
> > char *hll_type;
> > struct cil_tree_node *node;
> > struct token tok;
> > char *hll_file;
> > char *end = NULL;
> > + unsigned long val;
> >
> > cil_lexer_next(&tok);
> > hll_type = cil_strpool_add(tok.value);
> > @@ -141,11 +142,19 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
> > cil_log(CIL_ERR, "Invalid line mark syntax\n");
> > goto exit;
> > }
> > - *hll_lineno = strtol(tok.value, &end, 10);
> > +
> > + val = strtoul(tok.value, &end, 10);
> > if (errno == ERANGE || *end != '\0') {
> > cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
> > goto exit;
> > }
> > +#if ULONG_MAX > UINT32_MAX
> > + if (val > UINT32_MAX) {
> > + cil_log(CIL_ERR, "Line mark line number > UINT32_MAX\n");
> > + goto exit;
> > + }
> > +#endif
> > + *hll_lineno = val;
> >
> > push_hll_info(stack, *hll_lineno, *hll_expand);
> >
> > @@ -175,7 +184,7 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
> > return SEPOL_OK;
> >
> > exit:
> > - cil_log(CIL_ERR, "Problem with high-level line mark at line %d of %s\n", tok.line, path);
> > + cil_log(CIL_ERR, "Problem with high-level line mark at line %u of %s\n", tok.line, path);
> > return SEPOL_ERR;
> > }
> >
> > @@ -207,8 +216,8 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
> > struct cil_tree_node *current = NULL;
> > char *path = cil_strpool_add(_path);
> > struct cil_stack *stack;
> > - int hll_lineno = -1;
> > - int hll_expand = -1;
> > + uint32_t hll_lineno = 0;
> > + uint32_t hll_expand = 0;
> > struct token tok;
> > int rc = SEPOL_OK;
> >
> > --
> > 2.26.2
> >
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks
2021-02-05 10:28 ` Nicolas Iooss
@ 2021-02-05 14:26 ` James Carter
0 siblings, 0 replies; 6+ messages in thread
From: James Carter @ 2021-02-05 14:26 UTC (permalink / raw)
To: Nicolas Iooss; +Cc: James Carter, SElinux list, William Roberts
On Fri, Feb 5, 2021 at 5:28 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Thu, Feb 4, 2021 at 11:01 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > From: James Carter <jwcart2@tycho.nsa.gov>
> >
> > Nicolass Iooss reports:
>
> There is only one S in my first name ;)
>
> > OSS-Fuzz found an integer overflow when compiling the following
> > (empty) CIL policy:
> >
> > ;;*lms 2147483647 a
> > ; (empty line)
> >
> > Change hll_lineno to uint32_t which is the type of the field hll_line
> > in struct cil_tree_node where the line number will be stored eventually.
> > Read the line number into an unsigned long variable using strtoul()
> > instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, set the
> > value to 0 and print a warning if it is larger than UINT32_MAX before
> > storing it in hll_lineno.
> >
> > Also change hll_expand to uint32_t, since its value will be either
> > 0 or 1 and there is no need for it to be signed.
> >
> > Reported-by: Nicolass Iooss <nicolas.iooss@m4x.org>
> > Signed-off-by: James Carter <jwcart2@gmail.com>
> > ---
> > libsepol/cil/src/cil_parser.c | 31 ++++++++++++++++++++-----------
> > 1 file changed, 20 insertions(+), 11 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> > index b62043b9..9d3bd580 100644
> >[...]
> > @@ -140,11 +141,19 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
> > cil_log(CIL_ERR, "Invalid line mark syntax\n");
> > goto exit;
> > }
> > - *hll_lineno = strtol(tok.value, &end, 10);
> > +
> > + val = strtoul(tok.value, &end, 10);
> > if (errno == ERANGE || *end != '\0') {
> > cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
> > goto exit;
> > }
> > +#if ULONG_MAX > UINT32_MAX
> > + if (val > UINT32_MAX) {
> > + cil_log(CIL_WARN, "Line mark line number > UINT32_MAX\n");
> > + val = 0;
> > + }
> > +#endif
> > + *hll_lineno = val;
>
> Using both cil_log(CIL_ERR, "Problem parsing line number for line
> mark\n"); and cil_log(CIL_WARN, "Line mark line number >
> UINT32_MAX\n"); is inconsistent (if a CIL file is processed on a
> 32-bit system and contains a line mark with a number greater than
> UINT32_MAX, the compilation will fail due to the first if).
>
> In my humble opinion, the compiler should output an error if val >
> UINT32_MAX here, while accepting to (silently) wrap around UINT32_MAX
> when the line number is later incremented (which is the standard
> behavior with unsigned integers in C, if I remember correctly). This
> way, numbers greater than UINT32_MAX are forbidden in CIL source
> files, both on 32-bit and 64-bit systems. If you disagree and want to
> accept line mark with any line numbers, the first if block needs to be
> changed to use "cil_log(CIL_WARN, "Problem parsing line number for
> line mark\n");val=0;".
>
I agree with your reasoning. Really if there is a number greater than
UINT32_MAX in the source files, it most likely is an error.
Yes, it is not undefined behavior for an unsigned value to wrap.
> Another issue: function add_hll_linemark() currently ends with:
>
> exit:
> cil_log(CIL_ERR, "Problem with high-level line mark at line %d of
> %s\n", tok.line, path);
> return SEPOL_ERR;
>
> The %d needs to be replaced with %u in the message. Moreover if you
> want to keep cil_log(CIL_WARN), it would be very useful to have ("...
> at line %u of %s\n", tok.line, path) in the message.
>
Good catch, I send out a revised patch.
Thanks for the review,
Jim
> Thanks,
> Nicolas
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks
2021-02-04 22:01 James Carter
@ 2021-02-05 10:28 ` Nicolas Iooss
2021-02-05 14:26 ` James Carter
0 siblings, 1 reply; 6+ messages in thread
From: Nicolas Iooss @ 2021-02-05 10:28 UTC (permalink / raw)
To: James Carter, James Carter; +Cc: SElinux list, William Roberts
On Thu, Feb 4, 2021 at 11:01 PM James Carter <jwcart2@gmail.com> wrote:
>
> From: James Carter <jwcart2@tycho.nsa.gov>
>
> Nicolass Iooss reports:
There is only one S in my first name ;)
> OSS-Fuzz found an integer overflow when compiling the following
> (empty) CIL policy:
>
> ;;*lms 2147483647 a
> ; (empty line)
>
> Change hll_lineno to uint32_t which is the type of the field hll_line
> in struct cil_tree_node where the line number will be stored eventually.
> Read the line number into an unsigned long variable using strtoul()
> instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, set the
> value to 0 and print a warning if it is larger than UINT32_MAX before
> storing it in hll_lineno.
>
> Also change hll_expand to uint32_t, since its value will be either
> 0 or 1 and there is no need for it to be signed.
>
> Reported-by: Nicolass Iooss <nicolas.iooss@m4x.org>
> Signed-off-by: James Carter <jwcart2@gmail.com>
> ---
> libsepol/cil/src/cil_parser.c | 31 ++++++++++++++++++++-----------
> 1 file changed, 20 insertions(+), 11 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
> index b62043b9..9d3bd580 100644
>[...]
> @@ -140,11 +141,19 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
> cil_log(CIL_ERR, "Invalid line mark syntax\n");
> goto exit;
> }
> - *hll_lineno = strtol(tok.value, &end, 10);
> +
> + val = strtoul(tok.value, &end, 10);
> if (errno == ERANGE || *end != '\0') {
> cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
> goto exit;
> }
> +#if ULONG_MAX > UINT32_MAX
> + if (val > UINT32_MAX) {
> + cil_log(CIL_WARN, "Line mark line number > UINT32_MAX\n");
> + val = 0;
> + }
> +#endif
> + *hll_lineno = val;
Using both cil_log(CIL_ERR, "Problem parsing line number for line
mark\n"); and cil_log(CIL_WARN, "Line mark line number >
UINT32_MAX\n"); is inconsistent (if a CIL file is processed on a
32-bit system and contains a line mark with a number greater than
UINT32_MAX, the compilation will fail due to the first if).
In my humble opinion, the compiler should output an error if val >
UINT32_MAX here, while accepting to (silently) wrap around UINT32_MAX
when the line number is later incremented (which is the standard
behavior with unsigned integers in C, if I remember correctly). This
way, numbers greater than UINT32_MAX are forbidden in CIL source
files, both on 32-bit and 64-bit systems. If you disagree and want to
accept line mark with any line numbers, the first if block needs to be
changed to use "cil_log(CIL_WARN, "Problem parsing line number for
line mark\n");val=0;".
Another issue: function add_hll_linemark() currently ends with:
exit:
cil_log(CIL_ERR, "Problem with high-level line mark at line %d of
%s\n", tok.line, path);
return SEPOL_ERR;
The %d needs to be replaced with %u in the message. Moreover if you
want to keep cil_log(CIL_WARN), it would be very useful to have ("...
at line %u of %s\n", tok.line, path) in the message.
Thanks,
Nicolas
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks
@ 2021-02-04 22:01 James Carter
2021-02-05 10:28 ` Nicolas Iooss
0 siblings, 1 reply; 6+ messages in thread
From: James Carter @ 2021-02-04 22:01 UTC (permalink / raw)
To: selinux; +Cc: bill.c.roberts, James Carter, Nicolass Iooss, James Carter
From: James Carter <jwcart2@tycho.nsa.gov>
Nicolass Iooss reports:
OSS-Fuzz found an integer overflow when compiling the following
(empty) CIL policy:
;;*lms 2147483647 a
; (empty line)
Change hll_lineno to uint32_t which is the type of the field hll_line
in struct cil_tree_node where the line number will be stored eventually.
Read the line number into an unsigned long variable using strtoul()
instead of strtol(). On systems where ULONG_MAX > UINT32_MAX, set the
value to 0 and print a warning if it is larger than UINT32_MAX before
storing it in hll_lineno.
Also change hll_expand to uint32_t, since its value will be either
0 or 1 and there is no need for it to be signed.
Reported-by: Nicolass Iooss <nicolas.iooss@m4x.org>
Signed-off-by: James Carter <jwcart2@gmail.com>
---
libsepol/cil/src/cil_parser.c | 31 ++++++++++++++++++++-----------
1 file changed, 20 insertions(+), 11 deletions(-)
diff --git a/libsepol/cil/src/cil_parser.c b/libsepol/cil/src/cil_parser.c
index b62043b9..9d3bd580 100644
--- a/libsepol/cil/src/cil_parser.c
+++ b/libsepol/cil/src/cil_parser.c
@@ -46,11 +46,11 @@ char *CIL_KEY_HLL_LMX;
char *CIL_KEY_HLL_LME;
struct hll_info {
- int hll_lineno;
- int hll_expand;
+ uint32_t hll_lineno;
+ uint32_t hll_expand;
};
-static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expand)
+static void push_hll_info(struct cil_stack *stack, uint32_t hll_lineno, uint32_t hll_expand)
{
struct hll_info *new = cil_malloc(sizeof(*new));
@@ -60,7 +60,7 @@ static void push_hll_info(struct cil_stack *stack, int hll_lineno, int hll_expan
cil_stack_push(stack, CIL_NONE, new);
}
-static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expand)
+static void pop_hll_info(struct cil_stack *stack, uint32_t *hll_lineno, uint32_t *hll_expand)
{
struct cil_stack_item *curr = cil_stack_pop(stack);
struct cil_stack_item *prev = cil_stack_peek(stack);
@@ -69,8 +69,8 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
free(curr->data);
if (!prev) {
- *hll_lineno = -1;
- *hll_expand = -1;
+ *hll_lineno = 0;
+ *hll_expand = 0;
} else {
old = prev->data;
*hll_lineno = old->hll_lineno;
@@ -78,7 +78,7 @@ static void pop_hll_info(struct cil_stack *stack, int *hll_lineno, int *hll_expa
}
}
-static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, int line, int hll_line, void *value)
+static void create_node(struct cil_tree_node **node, struct cil_tree_node *current, uint32_t line, uint32_t hll_line, void *value)
{
cil_tree_node_init(node);
(*node)->parent = current;
@@ -98,13 +98,14 @@ static void insert_node(struct cil_tree_node *node, struct cil_tree_node *curren
current->cl_tail = node;
}
-static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int *hll_expand, struct cil_stack *stack, char *path)
+static int add_hll_linemark(struct cil_tree_node **current, uint32_t *hll_lineno, uint32_t *hll_expand, struct cil_stack *stack, char *path)
{
char *hll_type;
struct cil_tree_node *node;
struct token tok;
char *hll_file;
char *end = NULL;
+ unsigned long val;
cil_lexer_next(&tok);
hll_type = cil_strpool_add(tok.value);
@@ -140,11 +141,19 @@ static int add_hll_linemark(struct cil_tree_node **current, int *hll_lineno, int
cil_log(CIL_ERR, "Invalid line mark syntax\n");
goto exit;
}
- *hll_lineno = strtol(tok.value, &end, 10);
+
+ val = strtoul(tok.value, &end, 10);
if (errno == ERANGE || *end != '\0') {
cil_log(CIL_ERR, "Problem parsing line number for line mark\n");
goto exit;
}
+#if ULONG_MAX > UINT32_MAX
+ if (val > UINT32_MAX) {
+ cil_log(CIL_WARN, "Line mark line number > UINT32_MAX\n");
+ val = 0;
+ }
+#endif
+ *hll_lineno = val;
push_hll_info(stack, *hll_lineno, *hll_expand);
@@ -206,8 +215,8 @@ int cil_parser(const char *_path, char *buffer, uint32_t size, struct cil_tree *
struct cil_tree_node *current = NULL;
char *path = cil_strpool_add(_path);
struct cil_stack *stack;
- int hll_lineno = -1;
- int hll_expand = -1;
+ uint32_t hll_lineno = 0;
+ uint32_t hll_expand = 0;
struct token tok;
int rc = SEPOL_OK;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-02-10 13:12 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 14:44 [PATCH] libsepol/cil: Fix integer overflow in the handling of hll line marks James Carter
2021-02-08 21:03 ` Nicolas Iooss
2021-02-10 13:11 ` James Carter
-- strict thread matches above, loose matches on Subject: below --
2021-02-04 22:01 James Carter
2021-02-05 10:28 ` Nicolas Iooss
2021-02-05 14:26 ` James Carter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).