[PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[Nicolas Iooss]

OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying to
compile the following policy:

    (sid SID)
    (sidorder(SID))
    (filecon "\" any ())
    (filecon "" any ())

When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
terminator of the string. Fix this by returning when '\0' is read after
a backslash.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_post.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index a55df1ea5bb0..5f9cf4efd242 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -179,6 +179,12 @@ void cil_post_fc_fill_data(struct fc_data *fc, char *path)
 			break;
 		case '\\':
 			c++;
+			if (path[c] == '\0') {
+				if (!fc->meta) {
+					fc->stem_len++;
+				}
+				return;
+			}
 			/* FALLTHRU */
 		default:
 			if (!fc->meta) {
-- 
2.30.2

[PATCH 2/6] libsepol/cil: make cil_post_fc_fill_data static

[Nicolas Iooss]

cil_post_fc_fill_data() is not used outside of cil_post.c, and is not
exported in libsepol.so. Make it static, in order to ease the analysis
of static analyzers.

While at it, make its path argument "const char*" and the fields of
"struct fc_data" "unsigned int" or "size_t", in order to make the types
better match the values.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_post.c | 11 +++++++++--
 libsepol/cil/src/cil_post.h |  7 -------
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
index 5f9cf4efd242..783929e50df8 100644
--- a/libsepol/cil/src/cil_post.c
+++ b/libsepol/cil/src/cil_post.c
@@ -27,6 +27,7 @@
  * either expressed or implied, of Tresys Technology, LLC.
  */
 
+#include <stddef.h>
 #include <stdlib.h>
 #include <stdio.h>
 #include <string.h>
@@ -50,6 +51,12 @@
 #define GEN_REQUIRE_ATTR "cil_gen_require" /* Also in libsepol/src/module_to_cil.c */
 #define TYPEATTR_INFIX "_typeattr_"        /* Also in libsepol/src/module_to_cil.c */
 
+struct fc_data {
+	unsigned int meta;
+	size_t stem_len;
+	size_t str_len;
+};
+
 static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int max, struct cil_db *db);
 static int __cil_expr_list_to_bitmap(struct cil_list *expr_list, ebitmap_t *out, int max, struct cil_db *db);
 
@@ -156,9 +163,9 @@ static int cil_verify_is_list(struct cil_list *list, enum cil_flavor flavor)
 	return CIL_TRUE;
 }
 
-void cil_post_fc_fill_data(struct fc_data *fc, char *path)
+static void cil_post_fc_fill_data(struct fc_data *fc, const char *path)
 {
-	int c = 0;
+	size_t c = 0;
 	fc->meta = 0;
 	fc->stem_len = 0;
 	fc->str_len = 0;
diff --git a/libsepol/cil/src/cil_post.h b/libsepol/cil/src/cil_post.h
index 3d5415486b77..b1d2206f9ef6 100644
--- a/libsepol/cil/src/cil_post.h
+++ b/libsepol/cil/src/cil_post.h
@@ -30,13 +30,6 @@
 #ifndef CIL_POST_H_
 #define CIL_POST_H_
 
-struct fc_data {
-	int meta;
-	int stem_len;
-	int str_len;
-};
-
-void cil_post_fc_fill_data(struct fc_data *fc, char *path);
 int cil_post_filecon_compare(const void *a, const void *b);
 int cil_post_ibpkeycon_compare(const void *a, const void *b);
 int cil_post_portcon_compare(const void *a, const void *b);
-- 
2.30.2

[PATCH 3/6] libsepol/cil: remove stray printf

[Nicolas Iooss]

printf("%i\n", node->flavor); looks very much like a statement which was
added for debugging purpose and was unintentionally left.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_resolve_ast.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 0e07856133e5..47cdf0e7c0b9 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -1088,7 +1088,6 @@ int cil_resolve_roletransition(struct cil_tree_node *current, void *extra_args)
 	node = NODE(result_datum);
 	if (node->flavor != CIL_ROLE) {
 		rc = SEPOL_ERR;
-		printf("%i\n", node->flavor);
 		cil_log(CIL_ERR, "roletransition must result in a role, but %s is a %s\n", roletrans->result_str, cil_node_to_string(node));
 		goto exit;
 	}
-- 
2.30.2

[PATCH 4/6] libsepol/cil: replace printf with proper cil_tree_log

[Nicolas Iooss]

All functions of the CIL compiler use cil_log or cil_tree_log to report
errors, but in two places which still uses printf. Replace these printf
invocation with cil_tree_log.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_resolve_ast.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 47cdf0e7c0b9..2ea106d63505 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -2497,7 +2497,7 @@ int cil_resolve_in(struct cil_tree_node *current, void *extra_args)
 
 	rc = cil_copy_ast(db, current, block_node);
 	if (rc != SEPOL_OK) {
-		printf("Failed to copy in, rc: %d\n", rc);
+		cil_tree_log(current, CIL_ERR, "Failed to copy in-statement");
 		goto exit;
 	}
 
@@ -2788,7 +2788,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
 	macro_node = NODE(macro_datum);
 
 	if (macro_node->flavor != CIL_MACRO) {
-		printf("Failed to resolve %s to a macro\n", new_call->macro_str);
+		cil_tree_log(current, CIL_ERR, "Failed to resolve %s to a macro", new_call->macro_str);
 		rc = SEPOL_ERR;
 		goto exit;
 	}
-- 
2.30.2

[PATCH 5/6] libsepol/cil: fix NULL pointer dereference in __cil_insert_name

[Nicolas Iooss]

OSS-Fuzz found a Null-dereference in __cil_insert_name when trying to
compile the following policy:

    (macro MACRO ()
        (classmap CLASS (PERM))
        (type TYPE)
        (typetransition TYPE TYPE CLASS "name" TYPE)
    )
    (call MACRO)

When using a macro with no argument, macro->params is NULL and
cil_list_for_each(item, macro->params) dereferenced a NULL pointer.
Fix this by checking that macro->params is not NULL before using it.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28565
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_resolve_ast.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
index 2ea106d63505..63beed9230b9 100644
--- a/libsepol/cil/src/cil_resolve_ast.c
+++ b/libsepol/cil/src/cil_resolve_ast.c
@@ -82,7 +82,7 @@ static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key,
 	} else if (parent->flavor == CIL_MACRO) {
 		macro = parent->data;
 	}
-	if (macro != NULL) {
+	if (macro != NULL && macro->params != NULL) {
 		struct cil_list_item *item;
 		cil_list_for_each(item, macro->params) {
 			if (((struct cil_param*)item->data)->str == key) {
-- 
2.30.2

[PATCH 6/6] libsepol/cil: do not leak avrulex_ioctl_table memory when an error occurs

[Nicolas Iooss]

OSS-Fuzz found a memory leak when trying to compile the following
policy:

    (class CLASS (PERM ioctl))
    (classorder (CLASS))
    (sid SID)
    (sidorder (SID))
    (user USER)
    (role ROLE)
    (type TYPE)
    (category CAT)
    (categoryorder (CAT))
    (sensitivity SENS)
    (sensitivityorder (SENS))
    (sensitivitycategory SENS (CAT))
    (allow TYPE self (CLASS (PERM)))
    (roletype ROLE TYPE)
    (userrole USER ROLE)
    (userlevel USER (SENS))
    (userrange USER ((SENS)(SENS (CAT))))
    (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))

    (permissionx ioctl_test (ioctl CLASS (and (range 0x1600 0x19FF) (not (range 0x1750 0x175F)))))
    (allowx TYPE TYPE ioctl_test)

    (boolean BOOLEAN false)

    (booleanif (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (not (xor (eq BOOLEAN BOOLEAN)
                (and (eq BOOLEAN BOOLEAN) BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
                BOOLEAN ) ) )
        (true
            (allow TYPE TYPE (CLASS (PERM)))
        )
    )

When the CIL compiler reports "Conditional expression exceeded max
allowable depth" because of the loooooong expression in the booleanif
statement, cil_binary_create_allocated_pdb returns without freeing the
memory which was allocated to store the keys and values of hash table
avrulex_ioctl_table.

Fix this by moving the freeing logic to a dedicated destructor function
and calling it in the exit block of cil_binary_create_allocated_pdb.

Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28618
Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
 libsepol/cil/src/cil_binary.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
index f80d84679f85..18532aad9801 100644
--- a/libsepol/cil/src/cil_binary.c
+++ b/libsepol/cil/src/cil_binary.c
@@ -1668,14 +1668,6 @@ exit:
 		}
 		cil_list_destroy(&xperms_list, CIL_FALSE);
 	}
-
-	// hashtab_t does not have a way to free keys or datum since it doesn't
-	// know what they are. We won't need the keys/datum after this function, so
-	// clean them up here.
-	free(avtab_key);
-	ebitmap_destroy(datum);
-	free(datum);
-
 	return rc;
 }
 
@@ -1885,6 +1877,15 @@ exit:
 	return rc;
 }
 
+static int __cil_avrulex_ioctl_destroy(hashtab_key_t k, hashtab_datum_t datum, __attribute__((unused)) void *args)
+{
+	free(k);
+	ebitmap_destroy(datum);
+	free(datum);
+
+	return SEPOL_OK;
+}
+
 int __cil_cond_to_policydb_helper(struct cil_tree_node *node, __attribute__((unused)) uint32_t *finished, void *extra_args)
 {
 	int rc;
@@ -5037,6 +5038,7 @@ int cil_binary_create_allocated_pdb(const struct cil_db *db, sepol_policydb_t *p
 
 exit:
 	hashtab_destroy(role_trans_table);
+	hashtab_map(avrulex_ioctl_table, __cil_avrulex_ioctl_destroy, NULL);
 	hashtab_destroy(avrulex_ioctl_table);
 	free(type_value_to_cil);
 	free(class_value_to_cil);
-- 
2.30.2

Re: [PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[James Carter]

On Sun, Mar 14, 2021 at 4:23 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying to
> compile the following policy:
>
>     (sid SID)
>     (sidorder(SID))
>     (filecon "\" any ())
>     (filecon "" any ())
>
> When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
> terminator of the string. Fix this by returning when '\0' is read after
> a backslash.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> ---
>  libsepol/cil/src/cil_post.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> index a55df1ea5bb0..5f9cf4efd242 100644
> --- a/libsepol/cil/src/cil_post.c
> +++ b/libsepol/cil/src/cil_post.c
> @@ -179,6 +179,12 @@ void cil_post_fc_fill_data(struct fc_data *fc, char *path)
>                         break;
>                 case '\\':
>                         c++;

The patch below is fine, but I can't figure out the reason for the
line above. I guess it means that fc->str_len++ will be skipped, but
if that is the purpose, it is not very clear. Does anyone know if this
is correct?

Jim


> +                       if (path[c] == '\0') {
> +                               if (!fc->meta) {
> +                                       fc->stem_len++;
> +                               }
> +                               return;
> +                       }
>                         /* FALLTHRU */
>                 default:
>                         if (!fc->meta) {
> --
> 2.30.2
>

Re: [PATCH 2/6] libsepol/cil: make cil_post_fc_fill_data static

[James Carter]

On Sun, Mar 14, 2021 at 4:22 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> cil_post_fc_fill_data() is not used outside of cil_post.c, and is not
> exported in libsepol.so. Make it static, in order to ease the analysis
> of static analyzers.
>
> While at it, make its path argument "const char*" and the fields of
> "struct fc_data" "unsigned int" or "size_t", in order to make the types
> better match the values.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_post.c | 11 +++++++++--
>  libsepol/cil/src/cil_post.h |  7 -------
>  2 files changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> index 5f9cf4efd242..783929e50df8 100644
> --- a/libsepol/cil/src/cil_post.c
> +++ b/libsepol/cil/src/cil_post.c
> @@ -27,6 +27,7 @@
>   * either expressed or implied, of Tresys Technology, LLC.
>   */
>
> +#include <stddef.h>
>  #include <stdlib.h>
>  #include <stdio.h>
>  #include <string.h>
> @@ -50,6 +51,12 @@
>  #define GEN_REQUIRE_ATTR "cil_gen_require" /* Also in libsepol/src/module_to_cil.c */
>  #define TYPEATTR_INFIX "_typeattr_"        /* Also in libsepol/src/module_to_cil.c */
>
> +struct fc_data {
> +       unsigned int meta;
> +       size_t stem_len;
> +       size_t str_len;
> +};
> +
>  static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int max, struct cil_db *db);
>  static int __cil_expr_list_to_bitmap(struct cil_list *expr_list, ebitmap_t *out, int max, struct cil_db *db);
>
> @@ -156,9 +163,9 @@ static int cil_verify_is_list(struct cil_list *list, enum cil_flavor flavor)
>         return CIL_TRUE;
>  }
>
> -void cil_post_fc_fill_data(struct fc_data *fc, char *path)
> +static void cil_post_fc_fill_data(struct fc_data *fc, const char *path)
>  {
> -       int c = 0;
> +       size_t c = 0;
>         fc->meta = 0;
>         fc->stem_len = 0;
>         fc->str_len = 0;
> diff --git a/libsepol/cil/src/cil_post.h b/libsepol/cil/src/cil_post.h
> index 3d5415486b77..b1d2206f9ef6 100644
> --- a/libsepol/cil/src/cil_post.h
> +++ b/libsepol/cil/src/cil_post.h
> @@ -30,13 +30,6 @@
>  #ifndef CIL_POST_H_
>  #define CIL_POST_H_
>
> -struct fc_data {
> -       int meta;
> -       int stem_len;
> -       int str_len;
> -};
> -
> -void cil_post_fc_fill_data(struct fc_data *fc, char *path);
>  int cil_post_filecon_compare(const void *a, const void *b);
>  int cil_post_ibpkeycon_compare(const void *a, const void *b);
>  int cil_post_portcon_compare(const void *a, const void *b);
> --
> 2.30.2
>

Re: [PATCH 3/6] libsepol/cil: remove stray printf

[James Carter]

On Sun, Mar 14, 2021 at 4:22 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> printf("%i\n", node->flavor); looks very much like a statement which was
> added for debugging purpose and was unintentionally left.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_resolve_ast.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 0e07856133e5..47cdf0e7c0b9 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -1088,7 +1088,6 @@ int cil_resolve_roletransition(struct cil_tree_node *current, void *extra_args)
>         node = NODE(result_datum);
>         if (node->flavor != CIL_ROLE) {
>                 rc = SEPOL_ERR;
> -               printf("%i\n", node->flavor);
>                 cil_log(CIL_ERR, "roletransition must result in a role, but %s is a %s\n", roletrans->result_str, cil_node_to_string(node));
>                 goto exit;
>         }
> --
> 2.30.2
>

Re: [PATCH 4/6] libsepol/cil: replace printf with proper cil_tree_log

[James Carter]

On Sun, Mar 14, 2021 at 4:22 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> All functions of the CIL compiler use cil_log or cil_tree_log to report
> errors, but in two places which still uses printf. Replace these printf
> invocation with cil_tree_log.
>
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_resolve_ast.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 47cdf0e7c0b9..2ea106d63505 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -2497,7 +2497,7 @@ int cil_resolve_in(struct cil_tree_node *current, void *extra_args)
>
>         rc = cil_copy_ast(db, current, block_node);
>         if (rc != SEPOL_OK) {
> -               printf("Failed to copy in, rc: %d\n", rc);
> +               cil_tree_log(current, CIL_ERR, "Failed to copy in-statement");
>                 goto exit;
>         }
>
> @@ -2788,7 +2788,7 @@ int cil_resolve_call1(struct cil_tree_node *current, void *extra_args)
>         macro_node = NODE(macro_datum);
>
>         if (macro_node->flavor != CIL_MACRO) {
> -               printf("Failed to resolve %s to a macro\n", new_call->macro_str);
> +               cil_tree_log(current, CIL_ERR, "Failed to resolve %s to a macro", new_call->macro_str);
>                 rc = SEPOL_ERR;
>                 goto exit;
>         }
> --
> 2.30.2
>

Re: [PATCH 5/6] libsepol/cil: fix NULL pointer dereference in __cil_insert_name

[James Carter]

On Sun, Mar 14, 2021 at 4:22 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a Null-dereference in __cil_insert_name when trying to
> compile the following policy:
>
>     (macro MACRO ()
>         (classmap CLASS (PERM))
>         (type TYPE)
>         (typetransition TYPE TYPE CLASS "name" TYPE)
>     )
>     (call MACRO)
>
> When using a macro with no argument, macro->params is NULL and
> cil_list_for_each(item, macro->params) dereferenced a NULL pointer.
> Fix this by checking that macro->params is not NULL before using it.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28565
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_resolve_ast.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/libsepol/cil/src/cil_resolve_ast.c b/libsepol/cil/src/cil_resolve_ast.c
> index 2ea106d63505..63beed9230b9 100644
> --- a/libsepol/cil/src/cil_resolve_ast.c
> +++ b/libsepol/cil/src/cil_resolve_ast.c
> @@ -82,7 +82,7 @@ static struct cil_name * __cil_insert_name(struct cil_db *db, hashtab_key_t key,
>         } else if (parent->flavor == CIL_MACRO) {
>                 macro = parent->data;
>         }
> -       if (macro != NULL) {
> +       if (macro != NULL && macro->params != NULL) {
>                 struct cil_list_item *item;
>                 cil_list_for_each(item, macro->params) {
>                         if (((struct cil_param*)item->data)->str == key) {
> --
> 2.30.2
>

Re: [PATCH 6/6] libsepol/cil: do not leak avrulex_ioctl_table memory when an error occurs

[James Carter]

On Sun, Mar 14, 2021 at 4:22 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> OSS-Fuzz found a memory leak when trying to compile the following
> policy:
>
>     (class CLASS (PERM ioctl))
>     (classorder (CLASS))
>     (sid SID)
>     (sidorder (SID))
>     (user USER)
>     (role ROLE)
>     (type TYPE)
>     (category CAT)
>     (categoryorder (CAT))
>     (sensitivity SENS)
>     (sensitivityorder (SENS))
>     (sensitivitycategory SENS (CAT))
>     (allow TYPE self (CLASS (PERM)))
>     (roletype ROLE TYPE)
>     (userrole USER ROLE)
>     (userlevel USER (SENS))
>     (userrange USER ((SENS)(SENS (CAT))))
>     (sidcontext SID (USER ROLE TYPE ((SENS)(SENS))))
>
>     (permissionx ioctl_test (ioctl CLASS (and (range 0x1600 0x19FF) (not (range 0x1750 0x175F)))))
>     (allowx TYPE TYPE ioctl_test)
>
>     (boolean BOOLEAN false)
>
>     (booleanif (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (not (xor (eq BOOLEAN BOOLEAN)
>                 (and (eq BOOLEAN BOOLEAN) BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>                 BOOLEAN ) ) )
>         (true
>             (allow TYPE TYPE (CLASS (PERM)))
>         )
>     )
>
> When the CIL compiler reports "Conditional expression exceeded max
> allowable depth" because of the loooooong expression in the booleanif
> statement, cil_binary_create_allocated_pdb returns without freeing the
> memory which was allocated to store the keys and values of hash table
> avrulex_ioctl_table.
>
> Fix this by moving the freeing logic to a dedicated destructor function
> and calling it in the exit block of cil_binary_create_allocated_pdb.
>
> Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28618
> Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>

Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libsepol/cil/src/cil_binary.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/libsepol/cil/src/cil_binary.c b/libsepol/cil/src/cil_binary.c
> index f80d84679f85..18532aad9801 100644
> --- a/libsepol/cil/src/cil_binary.c
> +++ b/libsepol/cil/src/cil_binary.c
> @@ -1668,14 +1668,6 @@ exit:
>                 }
>                 cil_list_destroy(&xperms_list, CIL_FALSE);
>         }
> -
> -       // hashtab_t does not have a way to free keys or datum since it doesn't
> -       // know what they are. We won't need the keys/datum after this function, so
> -       // clean them up here.
> -       free(avtab_key);
> -       ebitmap_destroy(datum);
> -       free(datum);
> -
>         return rc;
>  }
>
> @@ -1885,6 +1877,15 @@ exit:
>         return rc;
>  }
>
> +static int __cil_avrulex_ioctl_destroy(hashtab_key_t k, hashtab_datum_t datum, __attribute__((unused)) void *args)
> +{
> +       free(k);
> +       ebitmap_destroy(datum);
> +       free(datum);
> +
> +       return SEPOL_OK;
> +}
> +
>  int __cil_cond_to_policydb_helper(struct cil_tree_node *node, __attribute__((unused)) uint32_t *finished, void *extra_args)
>  {
>         int rc;
> @@ -5037,6 +5038,7 @@ int cil_binary_create_allocated_pdb(const struct cil_db *db, sepol_policydb_t *p
>
>  exit:
>         hashtab_destroy(role_trans_table);
> +       hashtab_map(avrulex_ioctl_table, __cil_avrulex_ioctl_destroy, NULL);
>         hashtab_destroy(avrulex_ioctl_table);
>         free(type_value_to_cil);
>         free(class_value_to_cil);
> --
> 2.30.2
>

Re: [PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[Nicolas Iooss]

On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sun, Mar 14, 2021 at 4:23 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying to
> > compile the following policy:
> >
> >     (sid SID)
> >     (sidorder(SID))
> >     (filecon "\" any ())
> >     (filecon "" any ())
> >
> > When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
> > terminator of the string. Fix this by returning when '\0' is read after
> > a backslash.
> >
> > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > ---
> >  libsepol/cil/src/cil_post.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> > index a55df1ea5bb0..5f9cf4efd242 100644
> > --- a/libsepol/cil/src/cil_post.c
> > +++ b/libsepol/cil/src/cil_post.c
> > @@ -179,6 +179,12 @@ void cil_post_fc_fill_data(struct fc_data *fc, char *path)
> >                         break;
> >                 case '\\':
> >                         c++;
>
> The patch below is fine, but I can't figure out the reason for the
> line above. I guess it means that fc->str_len++ will be skipped, but
> if that is the purpose, it is not very clear. Does anyone know if this
> is correct?

Which line? "break;" ? In case you and/or other people are confused
about the code in cil_post_fc_fill_data, this "break;" exits the
switch(path[c]) block but still executes the lines right after
("fc->str_len++;" and "c++;"):

while (path[c] != '\0') {
    switch (path[c]) {
    case '.':
    /* ... */
    case '{':
        fc->meta = 1;
        break;
    case '\\':
        c++;
        /* FALLTHRU */
    default:
// This code is executed for every character before a special one
// (while "meta" is false)
// and "\c" counts as a single character, for c being anything.
        if (!fc->meta) {
            fc->stem_len++;
        }
        break;
    }
// These lines are executed for every character.
// "str_len" counts the number of unescaped characters
// ("\c" counts as a single character)
    fc->str_len++;
    c++;
}

In my opinion, the code looks correct, but this could be verified with
a new unit test which could computes str_len and stem_len for some
strings.

Cheers,
Nicolas

Re: [PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[James Carter]

On Mon, Mar 15, 2021 at 5:34 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > On Sun, Mar 14, 2021 at 4:23 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > >
> > > OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying to
> > > compile the following policy:
> > >
> > >     (sid SID)
> > >     (sidorder(SID))
> > >     (filecon "\" any ())
> > >     (filecon "" any ())
> > >
> > > When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
> > > terminator of the string. Fix this by returning when '\0' is read after
> > > a backslash.
> > >
> > > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
> > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > > ---
> > >  libsepol/cil/src/cil_post.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> > > index a55df1ea5bb0..5f9cf4efd242 100644
> > > --- a/libsepol/cil/src/cil_post.c
> > > +++ b/libsepol/cil/src/cil_post.c
> > > @@ -179,6 +179,12 @@ void cil_post_fc_fill_data(struct fc_data *fc, char *path)
> > >                         break;
> > >                 case '\\':
> > >                         c++;
> >
> > The patch below is fine, but I can't figure out the reason for the
> > line above. I guess it means that fc->str_len++ will be skipped, but
> > if that is the purpose, it is not very clear. Does anyone know if this
> > is correct?
>
> Which line? "break;" ? In case you and/or other people are confused
> about the code in cil_post_fc_fill_data, this "break;" exits the
> switch(path[c]) block but still executes the lines right after
> ("fc->str_len++;" and "c++;"):
>

Sorry, I wasn't very clear. I am wondering what the "c++" is doing
here because after the switch statement there is another "c++" (after
"fc->str_length++"), so this skips the character after the "/". Why
would one do that? My only thought is that maybe "/" is not supposed
to count towards the string length and the author thought not counting
the next character works just as well? Except, of course, it doesn't
if there is no next character.

Jim

> while (path[c] != '\0') {
>     switch (path[c]) {
>     case '.':
>     /* ... */
>     case '{':
>         fc->meta = 1;
>         break;
>     case '\\':
>         c++;
>         /* FALLTHRU */
>     default:
> // This code is executed for every character before a special one
> // (while "meta" is false)
> // and "\c" counts as a single character, for c being anything.
>         if (!fc->meta) {
>             fc->stem_len++;
>         }
>         break;
>     }
> // These lines are executed for every character.
> // "str_len" counts the number of unescaped characters
> // ("\c" counts as a single character)
>     fc->str_len++;
>     c++;
> }
>
> In my opinion, the code looks correct, but this could be verified with
> a new unit test which could computes str_len and stem_len for some
> strings.
>
> Cheers,
> Nicolas
>

Re: [PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[Nicolas Iooss]

On Tue, Mar 16, 2021 at 2:34 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Mon, Mar 15, 2021 at 5:34 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote:
> > >
> > > On Sun, Mar 14, 2021 at 4:23 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > > >
> > > > OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying to
> > > > compile the following policy:
> > > >
> > > >     (sid SID)
> > > >     (sidorder(SID))
> > > >     (filecon "\" any ())
> > > >     (filecon "" any ())
> > > >
> > > > When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
> > > > terminator of the string. Fix this by returning when '\0' is read after
> > > > a backslash.
> > > >
> > > > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
> > > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > > > ---
> > > >  libsepol/cil/src/cil_post.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > >
> > > > diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> > > > index a55df1ea5bb0..5f9cf4efd242 100644
> > > > --- a/libsepol/cil/src/cil_post.c
> > > > +++ b/libsepol/cil/src/cil_post.c
> > > > @@ -179,6 +179,12 @@ void cil_post_fc_fill_data(struct fc_data *fc, char *path)
> > > >                         break;
> > > >                 case '\\':
> > > >                         c++;
> > >
> > > The patch below is fine, but I can't figure out the reason for the
> > > line above. I guess it means that fc->str_len++ will be skipped, but
> > > if that is the purpose, it is not very clear. Does anyone know if this
> > > is correct?
> >
> > Which line? "break;" ? In case you and/or other people are confused
> > about the code in cil_post_fc_fill_data, this "break;" exits the
> > switch(path[c]) block but still executes the lines right after
> > ("fc->str_len++;" and "c++;"):
> >
>
> Sorry, I wasn't very clear. I am wondering what the "c++" is doing
> here because after the switch statement there is another "c++" (after
> "fc->str_length++"), so this skips the character after the "/". Why
> would one do that? My only thought is that maybe "/" is not supposed
> to count towards the string length and the author thought not counting
> the next character works just as well? Except, of course, it doesn't
> if there is no next character.

The matched character is a backslash ("\"), not a slash ("/"). I
understand that this implementation of "c++; without fc->str_len++;
nor fc->stem_len++;" is there in order to count sequences such as
"\(", "\.", "\["... as a single character. More precisely, when for
example the two-character sequence "\." is encountered in a path
(which happens often, as it is the way to escape dots in file context
patterns):

* c is increased twice ("c++;" in present twice in the while loop), in
order to go to the character next to the sequence ;
* fc->str_len is increased once (this sequence counts as a single
non-special character) ;
* if fc->meta is false (i.e. if no meta character such as ".", "(",
"["... has been encountered yet), fc->stem_len is increased once (this
sequence counts as a single non-special character in the "stem" of the
path)

The code I added in my patch made fc->stem_len increase when a path
ends with "\" (the character after the backslash character is a NUL
string terminator), before exiting cil_post_fc_fill_data. Now I am
wondering whether fc->str_len should also be increased, in order to
"count the backslash". In fact, finishing a path pattern with an
incomplete escape sequence is weird and I do not precisely know the
semantic of the length counters in such a case. What do you think?

Nicolas

> > while (path[c] != '\0') {
> >     switch (path[c]) {
> >     case '.':
> >     /* ... */
> >     case '{':
> >         fc->meta = 1;
> >         break;
> >     case '\\':
> >         c++;
> >         /* FALLTHRU */
> >     default:
> > // This code is executed for every character before a special one
> > // (while "meta" is false)
> > // and "\c" counts as a single character, for c being anything.
> >         if (!fc->meta) {
> >             fc->stem_len++;
> >         }
> >         break;
> >     }
> > // These lines are executed for every character.
> > // "str_len" counts the number of unescaped characters
> > // ("\c" counts as a single character)
> >     fc->str_len++;
> >     c++;
> > }
> >
> > In my opinion, the code looks correct, but this could be verified with
> > a new unit test which could computes str_len and stem_len for some
> > strings.
> >
> > Cheers,
> > Nicolas
> >

Re: [PATCH 2/6] libsepol/cil: make cil_post_fc_fill_data static

[Nicolas Iooss]

On Mon, Mar 15, 2021 at 10:03 PM James Carter <jwcart2@gmail.com> wrote:
>
> On Sun, Mar 14, 2021 at 4:22 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> >
> > cil_post_fc_fill_data() is not used outside of cil_post.c, and is not
> > exported in libsepol.so. Make it static, in order to ease the analysis
> > of static analyzers.
> >
> > While at it, make its path argument "const char*" and the fields of
> > "struct fc_data" "unsigned int" or "size_t", in order to make the types
> > better match the values.
> >
> > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
>
> Acked-by: James Carter <jwcart2@gmail.com>

I merged patches 2 to 6 of this series. Patch 1 still has discussions.

Thanks,
Nicolas

> > ---
> >  libsepol/cil/src/cil_post.c | 11 +++++++++--
> >  libsepol/cil/src/cil_post.h |  7 -------
> >  2 files changed, 9 insertions(+), 9 deletions(-)
> >
> > diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> > index 5f9cf4efd242..783929e50df8 100644
> > --- a/libsepol/cil/src/cil_post.c
> > +++ b/libsepol/cil/src/cil_post.c
> > @@ -27,6 +27,7 @@
> >   * either expressed or implied, of Tresys Technology, LLC.
> >   */
> >
> > +#include <stddef.h>
> >  #include <stdlib.h>
> >  #include <stdio.h>
> >  #include <string.h>
> > @@ -50,6 +51,12 @@
> >  #define GEN_REQUIRE_ATTR "cil_gen_require" /* Also in libsepol/src/module_to_cil.c */
> >  #define TYPEATTR_INFIX "_typeattr_"        /* Also in libsepol/src/module_to_cil.c */
> >
> > +struct fc_data {
> > +       unsigned int meta;
> > +       size_t stem_len;
> > +       size_t str_len;
> > +};
> > +
> >  static int __cil_expr_to_bitmap(struct cil_list *expr, ebitmap_t *out, int max, struct cil_db *db);
> >  static int __cil_expr_list_to_bitmap(struct cil_list *expr_list, ebitmap_t *out, int max, struct cil_db *db);
> >
> > @@ -156,9 +163,9 @@ static int cil_verify_is_list(struct cil_list *list, enum cil_flavor flavor)
> >         return CIL_TRUE;
> >  }
> >
> > -void cil_post_fc_fill_data(struct fc_data *fc, char *path)
> > +static void cil_post_fc_fill_data(struct fc_data *fc, const char *path)
> >  {
> > -       int c = 0;
> > +       size_t c = 0;
> >         fc->meta = 0;
> >         fc->stem_len = 0;
> >         fc->str_len = 0;
> > diff --git a/libsepol/cil/src/cil_post.h b/libsepol/cil/src/cil_post.h
> > index 3d5415486b77..b1d2206f9ef6 100644
> > --- a/libsepol/cil/src/cil_post.h
> > +++ b/libsepol/cil/src/cil_post.h
> > @@ -30,13 +30,6 @@
> >  #ifndef CIL_POST_H_
> >  #define CIL_POST_H_
> >
> > -struct fc_data {
> > -       int meta;
> > -       int stem_len;
> > -       int str_len;
> > -};
> > -
> > -void cil_post_fc_fill_data(struct fc_data *fc, char *path);
> >  int cil_post_filecon_compare(const void *a, const void *b);
> >  int cil_post_ibpkeycon_compare(const void *a, const void *b);
> >  int cil_post_portcon_compare(const void *a, const void *b);
> > --
> > 2.30.2
> >

Re: [PATCH 1/6] libsepol/cil: fix out-of-bound read of a file context pattern ending with "\"

[James Carter]

On Wed, Mar 17, 2021 at 3:45 AM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
>
> On Tue, Mar 16, 2021 at 2:34 PM James Carter <jwcart2@gmail.com> wrote:
> >
> > On Mon, Mar 15, 2021 at 5:34 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > >
> > > On Mon, Mar 15, 2021 at 10:02 PM James Carter <jwcart2@gmail.com> wrote:
> > > >
> > > > On Sun, Mar 14, 2021 at 4:23 PM Nicolas Iooss <nicolas.iooss@m4x.org> wrote:
> > > > >
> > > > > OSS-Fuzz found a Heap-buffer-overflow in the CIL compiler when trying to
> > > > > compile the following policy:
> > > > >
> > > > >     (sid SID)
> > > > >     (sidorder(SID))
> > > > >     (filecon "\" any ())
> > > > >     (filecon "" any ())
> > > > >
> > > > > When cil_post_fc_fill_data() processes "\", it goes beyond the NUL
> > > > > terminator of the string. Fix this by returning when '\0' is read after
> > > > > a backslash.
> > > > >
> > > > > Fixes: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=28484
> > > > > Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
> > > > > ---
> > > > >  libsepol/cil/src/cil_post.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/libsepol/cil/src/cil_post.c b/libsepol/cil/src/cil_post.c
> > > > > index a55df1ea5bb0..5f9cf4efd242 100644
> > > > > --- a/libsepol/cil/src/cil_post.c
> > > > > +++ b/libsepol/cil/src/cil_post.c
> > > > > @@ -179,6 +179,12 @@ void cil_post_fc_fill_data(struct fc_data *fc, char *path)
> > > > >                         break;
> > > > >                 case '\\':
> > > > >                         c++;
> > > >
> > > > The patch below is fine, but I can't figure out the reason for the
> > > > line above. I guess it means that fc->str_len++ will be skipped, but
> > > > if that is the purpose, it is not very clear. Does anyone know if this
> > > > is correct?
> > >
> > > Which line? "break;" ? In case you and/or other people are confused
> > > about the code in cil_post_fc_fill_data, this "break;" exits the
> > > switch(path[c]) block but still executes the lines right after
> > > ("fc->str_len++;" and "c++;"):
> > >
> >
> > Sorry, I wasn't very clear. I am wondering what the "c++" is doing
> > here because after the switch statement there is another "c++" (after
> > "fc->str_length++"), so this skips the character after the "/". Why
> > would one do that? My only thought is that maybe "/" is not supposed
> > to count towards the string length and the author thought not counting
> > the next character works just as well? Except, of course, it doesn't
> > if there is no next character.
>
> The matched character is a backslash ("\"), not a slash ("/"). I

I am an idiot. The code makes a lot more sense now. You only want to
count an escaped character as one character.

> understand that this implementation of "c++; without fc->str_len++;
> nor fc->stem_len++;" is there in order to count sequences such as
> "\(", "\.", "\["... as a single character. More precisely, when for
> example the two-character sequence "\." is encountered in a path
> (which happens often, as it is the way to escape dots in file context
> patterns):
>
> * c is increased twice ("c++;" in present twice in the while loop), in
> order to go to the character next to the sequence ;
> * fc->str_len is increased once (this sequence counts as a single
> non-special character) ;
> * if fc->meta is false (i.e. if no meta character such as ".", "(",
> "["... has been encountered yet), fc->stem_len is increased once (this
> sequence counts as a single non-special character in the "stem" of the
> path)
>
> The code I added in my patch made fc->stem_len increase when a path
> ends with "\" (the character after the backslash character is a NUL
> string terminator), before exiting cil_post_fc_fill_data. Now I am
> wondering whether fc->str_len should also be increased, in order to
> "count the backslash". In fact, finishing a path pattern with an
> incomplete escape sequence is weird and I do not precisely know the
> semantic of the length counters in such a case. What do you think?
>

I think that it is an invalid path, so maybe I need to add some sort
of verification to the path at some point.

I finally found the source of this code in
refpolicy/support/fc_sort.py where the function compute_diffdata() is
very similar. That function would increment str_len in this case, so
to be consistent with that fc->str_len should also be incremented.

Jim


> Nicolas
>
> > > while (path[c] != '\0') {
> > >     switch (path[c]) {
> > >     case '.':
> > >     /* ... */
> > >     case '{':
> > >         fc->meta = 1;
> > >         break;
> > >     case '\\':
> > >         c++;
> > >         /* FALLTHRU */
> > >     default:
> > > // This code is executed for every character before a special one
> > > // (while "meta" is false)
> > > // and "\c" counts as a single character, for c being anything.
> > >         if (!fc->meta) {
> > >             fc->stem_len++;
> > >         }
> > >         break;
> > >     }
> > > // These lines are executed for every character.
> > > // "str_len" counts the number of unescaped characters
> > > // ("\c" counts as a single character)
> > >     fc->str_len++;
> > >     c++;
> > > }
> > >
> > > In my opinion, the code looks correct, but this could be verified with
> > > a new unit test which could computes str_len and stem_len for some
> > > strings.
> > >
> > > Cheers,
> > > Nicolas
> > >
>