| Message ID | 20210204220131.229753-1-jwcart2@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libsepol/cil: Fix integer overflow in the handling of hll line marks | expand |
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
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 >
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;