[PATCH 00/13] checkpolicy improvements

[PATCH 00/13] checkpolicy improvements

[Christian Göttsche]

Miscellaneous improvements to the checkpolicy subproject, affecting the
traditional language compilers checkmodule(8) and checkpolicy(8).

Avoid implicit conversions, free leaked memory, resolve several compiler
warnings, use strict integer parsing.

The last patch is an adoption of a patch proposed by liwugang [1], with
the requested changes integrated.


[1]: https://patchwork.kernel.org/project/selinux/patch/20210601151704.2688389-1-liwugang@163.com/

Christian Göttsche (13):
  libsepol: avoid implicit conversions
  libsepol: free memory after policy validation
  checkpolicy: enclose macro argument in parentheses
  checkpolicy: misc checkmodule tweaks
  checkpolicy: misc checkpolicy tweaks
  checkpolicy: mark read-only parameters in module compiler const
  checkpolicy: mark file local functions in policy_define static
  checkpolicy: add missing function declarations
  checkpolicy: resolve dismod memory leaks
  checkpolicy: avoid implicit conversion
  checkpolicy: error out on parsing too big integers
  checkpolicy: print warning on source line overflow
  checkpolicy: free extended permission memory

 checkpolicy/checkmodule.c        | 18 +++----
 checkpolicy/checkpolicy.c        | 26 +++++-----
 checkpolicy/module_compiler.c    | 26 +++++-----
 checkpolicy/module_compiler.h    |  4 +-
 checkpolicy/policy_define.c      | 87 ++++++++++++++++----------------
 checkpolicy/policy_parse.y       | 16 +++++-
 checkpolicy/policy_scan.l        | 25 ++++++---
 checkpolicy/test/dismod.c        |  7 ++-
 libsepol/src/policydb_validate.c | 14 ++++-
 libsepol/src/util.c              |  2 +-
 10 files changed, 131 insertions(+), 94 deletions(-)

-- 
2.33.0

[PATCH 01/13] libsepol: avoid implicit conversions

[Christian Göttsche]

Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.

    util.c:95:15: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/libsepol/src/util.c b/libsepol/src/util.c
index a47cae87..902c63c5 100644
--- a/libsepol/src/util.c
+++ b/libsepol/src/util.c
@@ -92,7 +92,7 @@ char *sepol_av_to_string(policydb_t * policydbp, uint32_t tclass,
 	cladatum = policydbp->class_val_to_struct[tclass - 1];
 	p = avbuf;
 	for (i = 0; i < cladatum->permissions.nprim; i++) {
-		if (av & (1 << i)) {
+		if (av & (UINT32_C(1) << i)) {
 			v.val = i + 1;
 			rc = hashtab_map(cladatum->permissions.table,
 					 perm_name, &v);
-- 
2.33.0

[PATCH 02/13] libsepol: free memory after policy validation

[Christian Göttsche]

Found while running the checkpolicy/test/dispol binary.

    Direct leak of 24 byte(s) in 1 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
        #1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
        #2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
        #3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
        #4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
        #5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
        #6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
        #7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
        #8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16

    Indirect leak of 48 byte(s) in 2 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
        #1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
        #2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
        #3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
        #4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
        #5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
        #6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
        #7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
        #8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libsepol/src/policydb_validate.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
index 246aa6e3..f9a4b942 100644
--- a/libsepol/src/policydb_validate.c
+++ b/libsepol/src/policydb_validate.c
@@ -722,12 +722,21 @@ bad:
 	return -1;
 }
 
+static void validate_destroy(validate_t flavors[])
+{
+	unsigned int i;
+
+	for (i = 0; i < SYM_NUM; i++) {
+		ebitmap_destroy(&flavors[i].gaps);
+	}
+}
+
 /*
  * Validate policydb
  */
 int validate_policydb(sepol_handle_t *handle, policydb_t *p)
 {
-	validate_t flavors[SYM_NUM];
+	validate_t flavors[SYM_NUM] = {};
 
 	if (validate_array_init(p, flavors))
 		goto bad;
@@ -756,9 +765,12 @@ int validate_policydb(sepol_handle_t *handle, policydb_t *p)
 	if (validate_datum_arrays(handle, p, flavors))
 		goto bad;
 
+	validate_destroy(flavors);
+
 	return 0;
 
 bad:
 	ERR(handle, "Invalid policydb");
+	validate_destroy(flavors);
 	return -1;
 }
-- 
2.33.0

[PATCH 03/13] checkpolicy: enclose macro argument in parentheses

[Christian Göttsche]

Found by clang-tidy

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/policy_define.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 75a67d5c..7e7801d3 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -2143,13 +2143,13 @@ out:
 }
 
 /* index of the u32 containing the permission */
-#define XPERM_IDX(x) (x >> 5)
+#define XPERM_IDX(x) ((x) >> 5)
 /* set bits 0 through x-1 within the u32 */
-#define XPERM_SETBITS(x) ((1U << (x & 0x1f)) - 1)
+#define XPERM_SETBITS(x) ((1U << ((x) & 0x1f)) - 1)
 /* low value for this u32 */
-#define XPERM_LOW(x) (x << 5)
+#define XPERM_LOW(x) ((x) << 5)
 /* high value for this u32 */
-#define XPERM_HIGH(x) (((x + 1) << 5) - 1)
+#define XPERM_HIGH(x) ((((x) + 1) << 5) - 1)
 void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
 				av_extended_perms_t *xperms)
 {
@@ -2189,9 +2189,9 @@ int avrule_xperms_used(const av_extended_perms_t *xperms)
  * dir, size, driver, and function. Only the driver and function fields
  * are considered here
  */
-#define IOC_DRIV(x) (x >> 8)
-#define IOC_FUNC(x) (x & 0xff)
-#define IOC_CMD(driver, func) ((driver << 8) + func)
+#define IOC_DRIV(x) ((x) >> 8)
+#define IOC_FUNC(x) ((x) & 0xff)
+#define IOC_CMD(driver, func) (((driver) << 8) + (func))
 int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
 				av_extended_perms_t *complete_driver,
 				av_extended_perms_t **extended_perms)
-- 
2.33.0

[PATCH 04/13] checkpolicy: misc checkmodule tweaks

[Christian Göttsche]

Add missing argument in usage message.
Drop redundant includes `optarg` and `optind`, which are declared in
<getopt.h>.
Use consistent quit style by using `exit(1)`.
Mark read-only options struct const.
Check closing file streams after writing, which can signal a failed
write or sync to disk and should be checked.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/checkmodule.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/checkpolicy/checkmodule.c b/checkpolicy/checkmodule.c
index 316b2898..3432608b 100644
--- a/checkpolicy/checkmodule.c
+++ b/checkpolicy/checkmodule.c
@@ -34,9 +34,6 @@
 #include "checkpolicy.h"
 #include "parse_util.h"
 
-extern char *optarg;
-extern int optind;
-
 static sidtab_t sidtab;
 
 extern int mlspol;
@@ -126,7 +123,7 @@ static int write_binary_policy(policydb_t * p, FILE *outfp)
 
 static __attribute__((__noreturn__)) void usage(const char *progname)
 {
-	printf("usage:  %s [-h] [-V] [-b] [-C] [-E] [-U handle_unknown] [-m] [-M] [-o FILE] [INPUT]\n", progname);
+	printf("usage:  %s [-h] [-V] [-b] [-C] [-E] [-U handle_unknown] [-m] [-M] [-o FILE] [-c VERSION] [INPUT]\n", progname);
 	printf("Build base and policy modules.\n");
 	printf("Options:\n");
 	printf("  INPUT      build module from INPUT (else read from \"%s\")\n",
@@ -155,7 +152,7 @@ int main(int argc, char **argv)
 	int ch;
 	int show_version = 0;
 	policydb_t modpolicydb;
-	struct option long_options[] = {
+	const struct option long_options[] = {
 		{"help", no_argument, NULL, 'h'},
 		{"output", required_argument, NULL, 'o'},
 		{"binary", no_argument, NULL, 'b'},
@@ -271,7 +268,7 @@ int main(int argc, char **argv)
 	} else {
 		if (policydb_init(&modpolicydb)) {
 			fprintf(stderr, "%s: out of memory!\n", argv[0]);
-			return -1;
+			exit(1);
 		}
 
 		modpolicydb.policy_type = policy_type;
@@ -283,7 +280,7 @@ int main(int argc, char **argv)
 		}
 
 		if (hierarchy_check_constraints(NULL, &modpolicydb)) {
-			return -1;
+			exit(1);
 		}
 	}
 
@@ -336,7 +333,7 @@ int main(int argc, char **argv)
 		FILE *outfp = fopen(outfile, "w");
 
 		if (!outfp) {
-			perror(outfile);
+			fprintf(stderr, "%s:  error opening %s:  %s\n", argv[0], outfile, strerror(errno));
 			exit(1);
 		}
 
@@ -352,7 +349,10 @@ int main(int argc, char **argv)
 			}
 		}
 
-		fclose(outfp);
+		if (fclose(outfp)) {
+			fprintf(stderr, "%s:  error closing %s:  %s\n", argv[0], outfile, strerror(errno));
+			exit(1);
+		}
 	} else if (cil) {
 		fprintf(stderr, "%s:  No file to write CIL was specified\n", argv[0]);
 		exit(1);
-- 
2.33.0

[PATCH 05/13] checkpolicy: misc checkpolicy tweaks

[Christian Göttsche]

Add missing argument in usage message.
Drop redundant includes `optarg` and `optind`, which are declared in
<getopt.h>.
Mark file local functions static.
Drop unused function declaration.

Check closing file streams after writing, which can signal a failed
write or sync to disk and should be checked.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/checkpolicy.c | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/checkpolicy/checkpolicy.c b/checkpolicy/checkpolicy.c
index e6cfd337..9459486b 100644
--- a/checkpolicy/checkpolicy.c
+++ b/checkpolicy/checkpolicy.c
@@ -92,9 +92,6 @@
 #include "checkpolicy.h"
 #include "parse_util.h"
 
-extern char *optarg;
-extern int optind;
-
 static policydb_t policydb;
 static sidtab_t sidtab;
 
@@ -112,7 +109,7 @@ static __attribute__((__noreturn__)) void usage(const char *progname)
 {
 	printf
 	    ("usage:  %s [-b[F]] [-C] [-d] [-U handle_unknown (allow,deny,reject)] [-M] "
-	     "[-c policyvers (%d-%d)] [-o output_file|-] [-S] "
+	     "[-c policyvers (%d-%d)] [-o output_file|-] [-S] [-O]"
 	     "[-t target_platform (selinux,xen)] [-E] [-V] [input_file]\n",
 	     progname, POLICYDB_VERSION_MIN, POLICYDB_VERSION_MAX);
 	exit(1);
@@ -297,9 +294,7 @@ static int identify_equiv_types(void)
 }
 #endif
 
-extern char *av_to_string(uint32_t tclass, sepol_access_vector_t av);
-
-int display_bools(void)
+static int display_bools(void)
 {
 	uint32_t i;
 
@@ -310,10 +305,10 @@ int display_bools(void)
 	return 0;
 }
 
-void display_expr(cond_expr_t * exp)
+static void display_expr(const cond_expr_t * exp)
 {
 
-	cond_expr_t *cur;
+	const cond_expr_t *cur;
 	for (cur = exp; cur != NULL; cur = cur->next) {
 		switch (cur->expr_type) {
 		case COND_BOOL:
@@ -345,9 +340,9 @@ void display_expr(cond_expr_t * exp)
 	}
 }
 
-int display_cond_expressions(void)
+static int display_cond_expressions(void)
 {
-	cond_node_t *cur;
+	const cond_node_t *cur;
 
 	for (cur = policydbp->cond_list; cur != NULL; cur = cur->next) {
 		printf("expression: ");
@@ -357,7 +352,7 @@ int display_cond_expressions(void)
 	return 0;
 }
 
-int change_bool(char *name, int state)
+static int change_bool(const char *name, int state)
 {
 	cond_bool_datum_t *bool;
 
@@ -412,7 +407,7 @@ int main(int argc, char **argv)
 	unsigned int reason;
 	int flags;
 	struct policy_file pf;
-	struct option long_options[] = {
+	const struct option long_options[] = {
 		{"output", required_argument, NULL, 'o'},
 		{"target", required_argument, NULL, 't'},
 		{"binary", no_argument, NULL, 'b'},
@@ -706,7 +701,10 @@ int main(int argc, char **argv)
 		}
 
 		if (outfp != stdout) {
-			fclose(outfp);
+			if(fclose(outfp)) {
+				fprintf(stderr, "%s:  error closing %s:  %s\n", argv[0], outfile, strerror(errno));
+				exit(1);
+			}
 		}
 	} else if (cil) {
 		fprintf(stderr, "%s:  No file to write CIL was specified\n", argv[0]);
-- 
2.33.0

[PATCH 06/13] checkpolicy: mark read-only parameters in module compiler const

[Christian Göttsche]

Make it more obvious which parameters are read-only and not being
modified and allow callers to pass const pointers.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/module_compiler.c | 26 +++++++++++++-------------
 checkpolicy/module_compiler.h |  4 ++--
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/checkpolicy/module_compiler.c b/checkpolicy/module_compiler.c
index ada7cb2a..e8f15f4e 100644
--- a/checkpolicy/module_compiler.c
+++ b/checkpolicy/module_compiler.c
@@ -1104,14 +1104,14 @@ int require_cat(int pass)
 	return 0;
 }
 
-static int is_scope_in_stack(scope_datum_t * scope, scope_stack_t * stack)
+static int is_scope_in_stack(const scope_datum_t * scope, const scope_stack_t * stack)
 {
 	uint32_t i;
 	if (stack == NULL) {
 		return 0;	/* no matching scope found */
 	}
 	if (stack->type == 1) {
-		avrule_decl_t *decl = stack->decl;
+		const avrule_decl_t *decl = stack->decl;
 		for (i = 0; i < scope->decl_ids_len; i++) {
 			if (scope->decl_ids[i] == decl->decl_id) {
 				return 1;
@@ -1126,9 +1126,9 @@ static int is_scope_in_stack(scope_datum_t * scope, scope_stack_t * stack)
 	return is_scope_in_stack(scope, stack->parent);
 }
 
-int is_id_in_scope(uint32_t symbol_type, hashtab_key_t id)
+int is_id_in_scope(uint32_t symbol_type, const_hashtab_key_t id)
 {
-	scope_datum_t *scope =
+	const scope_datum_t *scope =
 	    (scope_datum_t *) hashtab_search(policydbp->scope[symbol_type].
 					     table, id);
 	if (scope == NULL) {
@@ -1138,7 +1138,7 @@ int is_id_in_scope(uint32_t symbol_type, hashtab_key_t id)
 }
 
 static int is_perm_in_scope_index(uint32_t perm_value, uint32_t class_value,
-				  scope_index_t * scope)
+				  const scope_index_t * scope)
 {
 	if (class_value > scope->class_perms_len) {
 		return 1;
@@ -1151,7 +1151,7 @@ static int is_perm_in_scope_index(uint32_t perm_value, uint32_t class_value,
 }
 
 static int is_perm_in_stack(uint32_t perm_value, uint32_t class_value,
-			    scope_stack_t * stack)
+			    const scope_stack_t * stack)
 {
 	if (stack == NULL) {
 		return 0;	/* no matching scope found */
@@ -1173,12 +1173,12 @@ static int is_perm_in_stack(uint32_t perm_value, uint32_t class_value,
 	return is_perm_in_stack(perm_value, class_value, stack->parent);
 }
 
-int is_perm_in_scope(hashtab_key_t perm_id, hashtab_key_t class_id)
+int is_perm_in_scope(const_hashtab_key_t perm_id, const_hashtab_key_t class_id)
 {
-	class_datum_t *cladatum =
+	const class_datum_t *cladatum =
 	    (class_datum_t *) hashtab_search(policydbp->p_classes.table,
 					     class_id);
-	perm_datum_t *perdatum;
+	const perm_datum_t *perdatum;
 	if (cladatum == NULL) {
 		return 1;
 	}
@@ -1361,17 +1361,17 @@ int begin_optional_else(int pass)
 	return 0;
 }
 
-static int copy_requirements(avrule_decl_t * dest, scope_stack_t * stack)
+static int copy_requirements(avrule_decl_t * dest, const scope_stack_t * stack)
 {
 	uint32_t i;
 	if (stack == NULL) {
 		return 0;
 	}
 	if (stack->type == 1) {
-		scope_index_t *src_scope = &stack->decl->required;
+		const scope_index_t *src_scope = &stack->decl->required;
 		scope_index_t *dest_scope = &dest->required;
 		for (i = 0; i < SYM_NUM; i++) {
-			ebitmap_t *src_bitmap = &src_scope->scope[i];
+			const ebitmap_t *src_bitmap = &src_scope->scope[i];
 			ebitmap_t *dest_bitmap = &dest_scope->scope[i];
 			if (ebitmap_union(dest_bitmap, src_bitmap)) {
 				yyerror("Out of memory!");
@@ -1397,7 +1397,7 @@ static int copy_requirements(avrule_decl_t * dest, scope_stack_t * stack)
 			    src_scope->class_perms_len;
 		}
 		for (i = 0; i < src_scope->class_perms_len; i++) {
-			ebitmap_t *src_bitmap = &src_scope->class_perms_map[i];
+			const ebitmap_t *src_bitmap = &src_scope->class_perms_map[i];
 			ebitmap_t *dest_bitmap =
 			    &dest_scope->class_perms_map[i];
 			if (ebitmap_union(dest_bitmap, src_bitmap)) {
diff --git a/checkpolicy/module_compiler.h b/checkpolicy/module_compiler.h
index 72c2d9bb..29b824b4 100644
--- a/checkpolicy/module_compiler.h
+++ b/checkpolicy/module_compiler.h
@@ -65,12 +65,12 @@ int require_cat(int pass);
 /* Check if an identifier is within the scope of the current
  * declaration or any of its parents.  Return 1 if it is, 0 if not.
  * If the identifier is not known at all then return 1 (truth).  */
-int is_id_in_scope(uint32_t symbol_type, hashtab_key_t id);
+int is_id_in_scope(uint32_t symbol_type, const_hashtab_key_t id);
 
 /* Check if a particular permission is within the scope of the current
  * declaration or any of its parents.  Return 1 if it is, 0 if not.
  * If the identifier is not known at all then return 1 (truth).  */
-int is_perm_in_scope(hashtab_key_t perm_id, hashtab_key_t class_id);
+int is_perm_in_scope(const_hashtab_key_t perm_id, const_hashtab_key_t class_id);
 
 /* Search the current avrules block for a conditional with the same
  * expression as 'cond'.  If the conditional does not exist then
-- 
2.33.0

[PATCH 07/13] checkpolicy: mark file local functions in policy_define static

[Christian Göttsche]

Also remove the unused function `avrule_ioctl_freeranges()`.

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/policy_define.c | 45 ++++++++++++++-----------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 7e7801d3..11707700 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -1605,7 +1605,7 @@ static int set_types(type_set_t * set, char *id, int *add, char starallowed)
 	return -1;
 }
 
-int define_compute_type_helper(int which, avrule_t ** rule)
+static int define_compute_type_helper(int which, avrule_t ** rule)
 {
 	char *id;
 	type_datum_t *datum;
@@ -1832,7 +1832,7 @@ struct av_ioctl_range_list {
 	struct av_ioctl_range_list *next;
 };
 
-int avrule_sort_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_sort_ioctls(struct av_ioctl_range_list **rangehead)
 {
 	struct av_ioctl_range_list *r, *r2, *sorted, *sortedhead = NULL;
 
@@ -1880,7 +1880,7 @@ error:
 	return -1;
 }
 
-int avrule_merge_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_merge_ioctls(struct av_ioctl_range_list **rangehead)
 {
 	struct av_ioctl_range_list *r, *tmp;
 	r = *rangehead;
@@ -1900,7 +1900,7 @@ int avrule_merge_ioctls(struct av_ioctl_range_list **rangehead)
 	return 0;
 }
 
-int avrule_read_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_read_ioctls(struct av_ioctl_range_list **rangehead)
 {
 	char *id;
 	struct av_ioctl_range_list *rnew, *r = NULL;
@@ -1953,7 +1953,7 @@ error:
 }
 
 /* flip to included ranges */
-int avrule_omit_ioctls(struct av_ioctl_range_list **rangehead)
+static int avrule_omit_ioctls(struct av_ioctl_range_list **rangehead)
 {
 	struct av_ioctl_range_list *rnew, *r, *newhead, *r2;
 
@@ -2001,7 +2001,7 @@ error:
 	return -1;
 }
 
-int avrule_ioctl_ranges(struct av_ioctl_range_list **rangelist)
+static int avrule_ioctl_ranges(struct av_ioctl_range_list **rangelist)
 {
 	struct av_ioctl_range_list *rangehead;
 	uint8_t omit;
@@ -2029,7 +2029,7 @@ int avrule_ioctl_ranges(struct av_ioctl_range_list **rangelist)
 	return 0;
 }
 
-int define_te_avtab_xperms_helper(int which, avrule_t ** rule)
+static int define_te_avtab_xperms_helper(int which, avrule_t ** rule)
 {
 	char *id;
 	class_perm_node_t *perms, *tail = NULL, *cur_perms = NULL;
@@ -2150,7 +2150,7 @@ out:
 #define XPERM_LOW(x) ((x) << 5)
 /* high value for this u32 */
 #define XPERM_HIGH(x) ((((x) + 1) << 5) - 1)
-void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
+static void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
 				av_extended_perms_t *xperms)
 {
 	unsigned int i;
@@ -2172,7 +2172,7 @@ void avrule_xperm_setrangebits(uint16_t low, uint16_t high,
 	}
 }
 
-int avrule_xperms_used(const av_extended_perms_t *xperms)
+static int avrule_xperms_used(const av_extended_perms_t *xperms)
 {
 	unsigned int i;
 
@@ -2192,7 +2192,7 @@ int avrule_xperms_used(const av_extended_perms_t *xperms)
 #define IOC_DRIV(x) ((x) >> 8)
 #define IOC_FUNC(x) ((x) & 0xff)
 #define IOC_CMD(driver, func) (((driver) << 8) + (func))
-int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
+static int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
 				av_extended_perms_t *complete_driver,
 				av_extended_perms_t **extended_perms)
 {
@@ -2231,7 +2231,7 @@ int avrule_ioctl_partialdriver(struct av_ioctl_range_list *rangelist,
 
 }
 
-int avrule_ioctl_completedriver(struct av_ioctl_range_list *rangelist,
+static int avrule_ioctl_completedriver(struct av_ioctl_range_list *rangelist,
 			av_extended_perms_t **extended_perms)
 {
 	struct av_ioctl_range_list *r;
@@ -2273,7 +2273,7 @@ int avrule_ioctl_completedriver(struct av_ioctl_range_list *rangelist,
 	return 0;
 }
 
-int avrule_ioctl_func(struct av_ioctl_range_list *rangelist,
+static int avrule_ioctl_func(struct av_ioctl_range_list *rangelist,
 		av_extended_perms_t **extended_perms, unsigned int driver)
 {
 	struct av_ioctl_range_list *r;
@@ -2323,18 +2323,7 @@ int avrule_ioctl_func(struct av_ioctl_range_list *rangelist,
 	return 0;
 }
 
-void avrule_ioctl_freeranges(struct av_ioctl_range_list *rangelist)
-{
-	struct av_ioctl_range_list *r, *tmp;
-	r = rangelist;
-	while (r) {
-		tmp = r;
-		r = r->next;
-		free(tmp);
-	}
-}
-
-unsigned int xperms_for_each_bit(unsigned int *bit, av_extended_perms_t *xperms)
+static unsigned int xperms_for_each_bit(unsigned int *bit, av_extended_perms_t *xperms)
 {
 	unsigned int i;
 	for (i = *bit; i < sizeof(xperms->perms)*8; i++) {
@@ -2347,7 +2336,7 @@ unsigned int xperms_for_each_bit(unsigned int *bit, av_extended_perms_t *xperms)
 	return 0;
 }
 
-int avrule_cpy(avrule_t *dest, const avrule_t *src)
+static int avrule_cpy(avrule_t *dest, const avrule_t *src)
 {
 	class_perm_node_t *src_perms;
 	class_perm_node_t *dest_perms, *dest_tail;
@@ -2395,7 +2384,7 @@ int avrule_cpy(avrule_t *dest, const avrule_t *src)
 	return 0;
 }
 
-int define_te_avtab_ioctl(const avrule_t *avrule_template)
+static int define_te_avtab_ioctl(const avrule_t *avrule_template)
 {
 	avrule_t *avrule;
 	struct av_ioctl_range_list *rangelist;
@@ -2490,7 +2479,7 @@ int define_te_avtab_extended_perms(int which)
 	return 0;
 }
 
-int define_te_avtab_helper(int which, avrule_t ** rule)
+static int define_te_avtab_helper(int which, avrule_t ** rule)
 {
 	char *id;
 	class_datum_t *cladatum;
@@ -5470,7 +5459,7 @@ int define_fs_use(int behavior)
 	return 0;
 }
 
-int define_genfs_context_helper(char *fstype, int has_type)
+static int define_genfs_context_helper(char *fstype, int has_type)
 {
 	struct genfs *genfs_p, *genfs, *newgenfs;
 	ocontext_t *newc, *c, *head, *p;
-- 
2.33.0

[PATCH 08/13] checkpolicy: add missing function declarations

[Christian Göttsche]

Ideally they should be declared in the corresponding header file, but
the overall include style in the checkpolicy code is quite messy.
Declare them for now in the source file before defining them to silence
related compiler warnings:

    policy_define.c:84:6: error: no previous prototype for function 'init_parser' [-Werror,-Wmissing-prototypes]
    void init_parser(int pass_number)
         ^
    policy_define.c:93:6: error: no previous prototype for function 'yyerror2' [-Werror,-Wmissing-prototypes]
    void yyerror2(const char *fmt, ...)
         ^

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/policy_define.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 11707700..c71e0571 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -60,6 +60,10 @@
 #include "module_compiler.h"
 #include "policy_define.h"
 
+extern void init_parser(int pass_number);
+__attribute__ ((format(printf, 1, 2)))
+extern void yyerror2(const char *fmt, ...);
+
 policydb_t *policydbp;
 queue_t id_queue = 0;
 unsigned int pass;
@@ -89,7 +93,6 @@ void init_parser(int pass_number)
 	pass = pass_number;
 }
 
-__attribute__ ((format(printf, 1, 2)))
 void yyerror2(const char *fmt, ...)
 {
 	va_list ap;
-- 
2.33.0

[PATCH 09/13] checkpolicy: resolve dismod memory leaks

[Christian Göttsche]

Example leak:

    Indirect leak of 4 byte(s) in 1 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dismod+0x49bacd)
        #1 0x58ae54 in add_i_to_a ./libsepol/src/util.c:55:21
        #2 0x53ea8e in symtab_insert ./libsepol/src/policydb.c:1729:6
        #3 0x536252 in roles_init ./libsepol/src/policydb.c:772:7
        #4 0x536252 in policydb_init ./libsepol/src/policydb.c:892:7
        #5 0x562ff1 in sepol_policydb_create ./libsepol/src/policydb_public.c:69:6
        #6 0x521a7c in module_package_init ./libsepol/src/module.c:96:6
        #7 0x521a7c in sepol_module_package_create ./libsepol/src/module.c:126:7
        #8 0x4cfb80 in read_policy ./checkpolicy/test/dismod.c:750:7
        #9 0x4cda10 in main ./checkpolicy/test/dismod.c:878:6
        #10 0x7f8538d01e49 in __libc_start_main csu/../csu/libc-start.c:314:16

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/test/dismod.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
index 90c29318..9550b999 100644
--- a/checkpolicy/test/dismod.c
+++ b/checkpolicy/test/dismod.c
@@ -751,12 +751,15 @@ static int read_policy(char *filename, policydb_t * policy)
 			fprintf(stderr, "%s:  Out of memory!\n", __FUNCTION__);
 			exit(1);
 		}
+		policydb_destroy((policydb_t *) package->policy);
+		free(package->policy);
 		package->policy = (sepol_policydb_t *) policy;
 		package->file_contexts = NULL;
 		retval =
 		    sepol_module_package_read(package,
 					      (sepol_policy_file_t *) & f, 1);
-		free(package->file_contexts);
+		package->policy = NULL;
+		sepol_module_package_free(package);
 	} else {
 		if (policydb_init(policy)) {
 			fprintf(stderr, "%s:  Out of memory!\n", __FUNCTION__);
-- 
2.33.0

[PATCH 10/13] checkpolicy: avoid implicit conversion

[Christian Göttsche]

Avoid implicit conversions from signed to unsigned values, found by
UB sanitizers, by using unsigned values in the first place.

    dismod.c:92:42: runtime error: left shift of 1 by 31 places cannot be represented in type 'int'

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/test/dismod.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
index 9550b999..792deb3a 100644
--- a/checkpolicy/test/dismod.c
+++ b/checkpolicy/test/dismod.c
@@ -89,7 +89,7 @@ static void render_access_bitmap(ebitmap_t * map, uint32_t class,
 	fprintf(fp, "{");
 	for (i = ebitmap_startbit(map); i < ebitmap_length(map); i++) {
 		if (ebitmap_get_bit(map, i)) {
-			perm = sepol_av_to_string(p, class, 1 << i);
+			perm = sepol_av_to_string(p, class, UINT32_C(1) << i);
 			if (perm)
 				fprintf(fp, " %s", perm);
 		}
-- 
2.33.0

[PATCH 11/13] checkpolicy: error out on parsing too big integers

[Christian Göttsche]

Error out instead of silently converting too big integer values in
policy sources.

    policy_parse.y:893:41: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/policy_parse.y | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/checkpolicy/policy_parse.y b/checkpolicy/policy_parse.y
index 6098eb50..e969d973 100644
--- a/checkpolicy/policy_parse.y
+++ b/checkpolicy/policy_parse.y
@@ -890,10 +890,22 @@ filename		: FILENAME
 			{ yytext[strlen(yytext) - 1] = '\0'; if (insert_id(yytext + 1,0)) return -1; }
 			;
 number			: NUMBER 
-			{ $$ = strtoul(yytext,NULL,0); }
+			{ unsigned long x;
+			  errno = 0;
+			  x = strtoul(yytext, NULL, 0);
+			  if (errno || x > UINT_MAX)
+			      return -1;
+			  $$ = (unsigned int) x;
+			}
 			;
 number64		: NUMBER
-			{ $$ = strtoull(yytext,NULL,0); }
+			{ unsigned long long x;
+			  errno = 0;
+			  x = strtoull(yytext, NULL, 0);
+			  if (errno)
+			      return -1;
+			  $$ = (uint64_t) x;
+			}
 			;
 ipv6_addr		: IPV6_ADDR
 			{ if (insert_id(yytext,0)) return -1; }
-- 
2.33.0

[PATCH 12/13] checkpolicy: print warning on source line overflow

[Christian Göttsche]

In case the source line value overflows or has a too big value in the
source policy print a warning.

    policy_scan.l:273:19: runtime error: implicit conversion from type 'int' of value -2 (32-bit, signed) to type 'unsigned long' changed the value to 18446744073709551614 (64-bit, unsigned)
    policy_scan.l:66:20: runtime error: unsigned integer overflow: 18446744073709551615 + 1 cannot be represented in type 'unsigned long'

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/policy_scan.l | 25 ++++++++++++++++++-------
 1 file changed, 18 insertions(+), 7 deletions(-)

diff --git a/checkpolicy/policy_scan.l b/checkpolicy/policy_scan.l
index 4067268b..129a8a2a 100644
--- a/checkpolicy/policy_scan.l
+++ b/checkpolicy/policy_scan.l
@@ -59,12 +59,17 @@ alnum   [a-zA-Z0-9]
 hexval	[0-9A-Fa-f]
 
 %%
-\n.*				{ strncpy(linebuf[lno], yytext+1, 255);
-                                  linebuf[lno][254] = 0;
-                                  lno = 1 - lno; 
-                                  policydb_lineno++;
-				  source_lineno++;
-                                  yyless(1); }
+\n.*				{
+				  strncpy(linebuf[lno], yytext+1, 255);
+				  linebuf[lno][254] = 0;
+				  lno = 1 - lno;
+				  policydb_lineno++;
+				  if (source_lineno == ULONG_MAX)
+				      yywarn("source line number overflow");
+				  else
+				      source_lineno++;
+				  yyless(1);
+				}
 CLONE |
 clone				{ return(CLONE); }
 COMMON |
@@ -270,7 +275,13 @@ GLBLUB				{ return(GLBLUB); }
 {hexval}{0,4}":"{hexval}{0,4}":"({hexval}|[:.])*  { return(IPV6_ADDR); }
 {digit}+(\.({alnum}|[_.])*)?    { return(VERSION_IDENTIFIER); }
 #line[ ]1[ ]\"[^\n]*\"		{ set_source_file(yytext+9); }
-#line[ ]{digit}+	        { source_lineno = atoi(yytext+6)-1; }
+#line[ ]{digit}+	        {
+				  errno = 0;
+				  source_lineno = strtoul(yytext+6, NULL, 10) - 1;
+				  if (errno) {
+				    yywarn("source line number too big");
+				  }
+				}
 #[^\n]*                         { /* delete comments */ }
 [ \t\f]+			{ /* delete whitespace */ }
 "==" 				{ return(EQUALS); }
-- 
2.33.0

[PATCH 13/13] checkpolicy: free extended permission memory

[Christian Göttsche]

define_te_avtab_xperms_helper() allocates memory for the avrule, while
define_te_avtab_ioctl() does not transfer any ownership of it.
Free the affected memory.

    Direct leak of 272 byte(s) in 2 object(s) allocated from:
        #0 0x49bb8d in __interceptor_malloc (./checkpolicy/checkmodule+0x49bb8d)
        #1 0x4f379c in define_te_avtab_xperms_helper ./checkpolicy/policy_define.c:2047:24
        #2 0x4f379c in define_te_avtab_extended_perms ./checkpolicy/policy_define.c:2469:6
        #3 0x4cf417 in yyparse ./checkpolicy/policy_parse.y:494:30
        #4 0x4eaf35 in read_source_policy ./checkpolicy/parse_util.c:63:6
        #5 0x50cccd in main ./checkpolicy/checkmodule.c:278:7
        #6 0x7fbfa455ce49 in __libc_start_main csu/../csu/libc-start.c:314:16

    Direct leak of 32 byte(s) in 2 object(s) allocated from:
        #0 0x49bb8d in __interceptor_malloc (./checkpolicy/checkmodule+0x49bb8d)
        #1 0x4f4a38 in avrule_sort_ioctls ./checkpolicy/policy_define.c:1844:12
        #2 0x4f4a38 in avrule_ioctl_ranges ./checkpolicy/policy_define.c:2021:6
        #3 0x4f4a38 in define_te_avtab_ioctl ./checkpolicy/policy_define.c:2399:6
        #4 0x4f4a38 in define_te_avtab_extended_perms ./checkpolicy/policy_define.c:2475:7
        #5 0x4cf417 in yyparse ./checkpolicy/policy_parse.y:494:30
        #6 0x4eaf35 in read_source_policy ./checkpolicy/parse_util.c:63:6
        #7 0x50cccd in main ./checkpolicy/checkmodule.c:278:7
        #8 0x7fbfa455ce49 in __libc_start_main csu/../csu/libc-start.c:314:16

Reported-by: liwugang <liwugang@163.com>
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/policy_define.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index c71e0571..185d5704 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -2390,7 +2390,7 @@ static int avrule_cpy(avrule_t *dest, const avrule_t *src)
 static int define_te_avtab_ioctl(const avrule_t *avrule_template)
 {
 	avrule_t *avrule;
-	struct av_ioctl_range_list *rangelist;
+	struct av_ioctl_range_list *rangelist, *r;
 	av_extended_perms_t *complete_driver, *partial_driver, *xperms;
 	unsigned int i;
 
@@ -2448,6 +2448,12 @@ done:
 	if (partial_driver)
 		free(partial_driver);
 
+	while (rangelist != NULL) {
+		r = rangelist;
+		rangelist = rangelist->next;
+		free(r);
+	}
+
 	return 0;
 }
 
@@ -2456,6 +2462,7 @@ int define_te_avtab_extended_perms(int which)
 	char *id;
 	unsigned int i;
 	avrule_t *avrule_template;
+	int rc = 0;
 
 	if (pass == 1) {
 		for (i = 0; i < 4; i++) {
@@ -2471,15 +2478,17 @@ int define_te_avtab_extended_perms(int which)
 
 	id = queue_remove(id_queue);
 	if (strcmp(id,"ioctl") == 0) {
-		free(id);
-		if (define_te_avtab_ioctl(avrule_template))
-			return -1;
+		rc = define_te_avtab_ioctl(avrule_template);
 	} else {
 		yyerror("only ioctl extended permissions are supported");
-		free(id);
-		return -1;
+		rc = -1;
 	}
-	return 0;
+
+	free(id);
+	avrule_destroy(avrule_template);
+	free(avrule_template);
+
+	return rc;
 }
 
 static int define_te_avtab_helper(int which, avrule_t ** rule)
-- 
2.33.0

Re: [PATCH 09/13] checkpolicy: resolve dismod memory leaks

[James Carter]

On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Example leak:
>
>     Indirect leak of 4 byte(s) in 1 object(s) allocated from:
>         #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dismod+0x49bacd)
>         #1 0x58ae54 in add_i_to_a ./libsepol/src/util.c:55:21
>         #2 0x53ea8e in symtab_insert ./libsepol/src/policydb.c:1729:6
>         #3 0x536252 in roles_init ./libsepol/src/policydb.c:772:7
>         #4 0x536252 in policydb_init ./libsepol/src/policydb.c:892:7
>         #5 0x562ff1 in sepol_policydb_create ./libsepol/src/policydb_public.c:69:6
>         #6 0x521a7c in module_package_init ./libsepol/src/module.c:96:6
>         #7 0x521a7c in sepol_module_package_create ./libsepol/src/module.c:126:7
>         #8 0x4cfb80 in read_policy ./checkpolicy/test/dismod.c:750:7
>         #9 0x4cda10 in main ./checkpolicy/test/dismod.c:878:6
>         #10 0x7f8538d01e49 in __libc_start_main csu/../csu/libc-start.c:314:16
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  checkpolicy/test/dismod.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
> index 90c29318..9550b999 100644
> --- a/checkpolicy/test/dismod.c
> +++ b/checkpolicy/test/dismod.c
> @@ -751,12 +751,15 @@ static int read_policy(char *filename, policydb_t * policy)
>                         fprintf(stderr, "%s:  Out of memory!\n", __FUNCTION__);
>                         exit(1);
>                 }
> +               policydb_destroy((policydb_t *) package->policy);
> +               free(package->policy);

Should use "sepol_policydb_free(package->policy)" here. It does both
of these lines.
Jim


>                 package->policy = (sepol_policydb_t *) policy;
>                 package->file_contexts = NULL;
>                 retval =
>                     sepol_module_package_read(package,
>                                               (sepol_policy_file_t *) & f, 1);
> -               free(package->file_contexts);
> +               package->policy = NULL;
> +               sepol_module_package_free(package);
>         } else {
>                 if (policydb_init(policy)) {
>                         fprintf(stderr, "%s:  Out of memory!\n", __FUNCTION__);
> --
> 2.33.0
>

Re: [PATCH 11/13] checkpolicy: error out on parsing too big integers

[James Carter]

On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Error out instead of silently converting too big integer values in
> policy sources.
>
>     policy_parse.y:893:41: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)
>
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
> ---
>  checkpolicy/policy_parse.y | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/checkpolicy/policy_parse.y b/checkpolicy/policy_parse.y
> index 6098eb50..e969d973 100644
> --- a/checkpolicy/policy_parse.y
> +++ b/checkpolicy/policy_parse.y
> @@ -890,10 +890,22 @@ filename          : FILENAME
>                         { yytext[strlen(yytext) - 1] = '\0'; if (insert_id(yytext + 1,0)) return -1; }
>                         ;
>  number                 : NUMBER
> -                       { $$ = strtoul(yytext,NULL,0); }
> +                       { unsigned long x;
> +                         errno = 0;
> +                         x = strtoul(yytext, NULL, 0);
> +                         if (errno || x > UINT_MAX)
> +                             return -1;

Some compilers will emit a warning if unsigned long is 32 bits. To
prevent this use:

if (errno)
   return -1;

#if ULONG_MAX > UINT_MAX
    if (val > UINT_MAX) {
        return -1;
    }
#endif

See commit b7ea65f547c67bfbae4ae133052583b090747e5a

And discussion:
https://lore.kernel.org/selinux/CAFftDdrGoQezmVSOnrFrPKaOnS3pejQXzYpfjwQ+QBHH_Pv02w@mail.gmail.com/

Jim



> +                         $$ = (unsigned int) x;
> +                       }
>                         ;
>  number64               : NUMBER
> -                       { $$ = strtoull(yytext,NULL,0); }
> +                       { unsigned long long x;
> +                         errno = 0;
> +                         x = strtoull(yytext, NULL, 0);
> +                         if (errno)
> +                             return -1;
> +                         $$ = (uint64_t) x;
> +                       }
>                         ;
>  ipv6_addr              : IPV6_ADDR
>                         { if (insert_id(yytext,0)) return -1; }
> --
> 2.33.0
>

[PATCH v2 02/13] libsepol: free memory after policy validation

[Christian Göttsche]

Found while running the checkpolicy/test/dispol binary.

    Direct leak of 24 byte(s) in 1 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
        #1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
        #2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
        #3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
        #4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
        #5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
        #6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
        #7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
        #8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16

    Indirect leak of 48 byte(s) in 2 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
        #1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
        #2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
        #3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
        #4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
        #5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
        #6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
        #7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
        #8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - name function validate_array_destroy(), similar to the allocating
    validate_array_init()


 libsepol/src/policydb_validate.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
index 246aa6e3..3ab072a8 100644
--- a/libsepol/src/policydb_validate.c
+++ b/libsepol/src/policydb_validate.c
@@ -722,12 +722,21 @@ bad:
 	return -1;
 }
 
+static void validate_array_destroy(validate_t flavors[])
+{
+	unsigned int i;
+
+	for (i = 0; i < SYM_NUM; i++) {
+		ebitmap_destroy(&flavors[i].gaps);
+	}
+}
+
 /*
  * Validate policydb
  */
 int validate_policydb(sepol_handle_t *handle, policydb_t *p)
 {
-	validate_t flavors[SYM_NUM];
+	validate_t flavors[SYM_NUM] = {};
 
 	if (validate_array_init(p, flavors))
 		goto bad;
@@ -756,9 +765,12 @@ int validate_policydb(sepol_handle_t *handle, policydb_t *p)
 	if (validate_datum_arrays(handle, p, flavors))
 		goto bad;
 
+        validate_array_destroy(flavors);
+
 	return 0;
 
 bad:
 	ERR(handle, "Invalid policydb");
+        validate_array_destroy(flavors);
 	return -1;
 }
-- 
2.33.0

[PATCH v2 09/13] checkpolicy: resolve dismod memory leaks

[Christian Göttsche]

Example leak:

    Indirect leak of 4 byte(s) in 1 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dismod+0x49bacd)
        #1 0x58ae54 in add_i_to_a ./libsepol/src/util.c:55:21
        #2 0x53ea8e in symtab_insert ./libsepol/src/policydb.c:1729:6
        #3 0x536252 in roles_init ./libsepol/src/policydb.c:772:7
        #4 0x536252 in policydb_init ./libsepol/src/policydb.c:892:7
        #5 0x562ff1 in sepol_policydb_create ./libsepol/src/policydb_public.c:69:6
        #6 0x521a7c in module_package_init ./libsepol/src/module.c:96:6
        #7 0x521a7c in sepol_module_package_create ./libsepol/src/module.c:126:7
        #8 0x4cfb80 in read_policy ./checkpolicy/test/dismod.c:750:7
        #9 0x4cda10 in main ./checkpolicy/test/dismod.c:878:6
        #10 0x7f8538d01e49 in __libc_start_main csu/../csu/libc-start.c:314:16

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - use sepol_policydb_free()


 checkpolicy/test/dismod.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/checkpolicy/test/dismod.c b/checkpolicy/test/dismod.c
index 90c29318..1ff161da 100644
--- a/checkpolicy/test/dismod.c
+++ b/checkpolicy/test/dismod.c
@@ -751,12 +751,14 @@ static int read_policy(char *filename, policydb_t * policy)
 			fprintf(stderr, "%s:  Out of memory!\n", __FUNCTION__);
 			exit(1);
 		}
+		sepol_policydb_free(package->policy);
 		package->policy = (sepol_policydb_t *) policy;
 		package->file_contexts = NULL;
 		retval =
 		    sepol_module_package_read(package,
 					      (sepol_policy_file_t *) & f, 1);
-		free(package->file_contexts);
+		package->policy = NULL;
+		sepol_module_package_free(package);
 	} else {
 		if (policydb_init(policy)) {
 			fprintf(stderr, "%s:  Out of memory!\n", __FUNCTION__);
-- 
2.33.0

[PATCH v2 11/13] checkpolicy: error out on parsing too big integers

[Christian Göttsche]

Error out instead of silently converting too big integer values in
policy sources.

    policy_parse.y:893:41: runtime error: implicit conversion from type 'unsigned long' of value 18446744073709551615 (64-bit, unsigned) to type 'unsigned int' changed the value to 4294967295 (32-bit, unsigned)

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v2:
  - only check usigned long against UINT_MAX, if ULONG_MAX is actually
    bigger


 checkpolicy/policy_parse.y | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/checkpolicy/policy_parse.y b/checkpolicy/policy_parse.y
index 6098eb50..45f973ff 100644
--- a/checkpolicy/policy_parse.y
+++ b/checkpolicy/policy_parse.y
@@ -890,10 +890,26 @@ filename		: FILENAME
 			{ yytext[strlen(yytext) - 1] = '\0'; if (insert_id(yytext + 1,0)) return -1; }
 			;
 number			: NUMBER 
-			{ $$ = strtoul(yytext,NULL,0); }
+			{ unsigned long x;
+			  errno = 0;
+			  x = strtoul(yytext, NULL, 0);
+			  if (errno)
+			      return -1;
+#if ULONG_MAX > UINT_MAX
+			  if (x > UINT_MAX)
+			      return -1;
+#endif
+			  $$ = (unsigned int) x;
+			}
 			;
 number64		: NUMBER
-			{ $$ = strtoull(yytext,NULL,0); }
+			{ unsigned long long x;
+			  errno = 0;
+			  x = strtoull(yytext, NULL, 0);
+			  if (errno)
+			      return -1;
+			  $$ = (uint64_t) x;
+			}
 			;
 ipv6_addr		: IPV6_ADDR
 			{ if (insert_id(yytext,0)) return -1; }
-- 
2.33.0

[PATCH v3 02/13] libsepol: free memory after policy validation

[Christian Göttsche]

Found while running the checkpolicy/test/dispol binary.

    Direct leak of 24 byte(s) in 1 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
        #1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
        #2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
        #3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
        #4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
        #5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
        #6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
        #7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
        #8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16

    Indirect leak of 48 byte(s) in 2 object(s) allocated from:
        #0 0x49bacd in __interceptor_malloc (./checkpolicy/test/dispol+0x49bacd)
        #1 0x5551e1 in ebitmap_set_bit ./libsepol/src/ebitmap.c:326:27
        #2 0x517873 in create_gap_ebitmap ./libsepol/src/policydb_validate.c:23:8
        #3 0x517873 in validate_init ./libsepol/src/policydb_validate.c:34:6
        #4 0x50fa47 in validate_array_init ./libsepol/src/policydb_validate.c:44:6
        #5 0x50fa47 in validate_policydb ./libsepol/src/policydb_validate.c:732:6
        #6 0x4f22df in policydb_read ./libsepol/src/policydb.c:4538:6
        #7 0x4cddb3 in main ./checkpolicy/test/dispol.c:437:8
        #8 0x7f5980e47e49 in __libc_start_main csu/../csu/libc-start.c:314:16

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
v3:
  - fix indentation
v2:
  - name function validate_array_destroy(), similar to the allocating
    validate_array_init()


 libsepol/src/policydb_validate.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/libsepol/src/policydb_validate.c b/libsepol/src/policydb_validate.c
index 246aa6e3..5804d247 100644
--- a/libsepol/src/policydb_validate.c
+++ b/libsepol/src/policydb_validate.c
@@ -722,12 +722,21 @@ bad:
 	return -1;
 }
 
+static void validate_array_destroy(validate_t flavors[])
+{
+	unsigned int i;
+
+	for (i = 0; i < SYM_NUM; i++) {
+		ebitmap_destroy(&flavors[i].gaps);
+	}
+}
+
 /*
  * Validate policydb
  */
 int validate_policydb(sepol_handle_t *handle, policydb_t *p)
 {
-	validate_t flavors[SYM_NUM];
+	validate_t flavors[SYM_NUM] = {};
 
 	if (validate_array_init(p, flavors))
 		goto bad;
@@ -756,9 +765,12 @@ int validate_policydb(sepol_handle_t *handle, policydb_t *p)
 	if (validate_datum_arrays(handle, p, flavors))
 		goto bad;
 
+	validate_array_destroy(flavors);
+
 	return 0;
 
 bad:
 	ERR(handle, "Invalid policydb");
+	validate_array_destroy(flavors);
 	return -1;
 }
-- 
2.33.0

Re: [PATCH 00/13] checkpolicy improvements

[James Carter]

On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
<cgzones@googlemail.com> wrote:
>
> Miscellaneous improvements to the checkpolicy subproject, affecting the
> traditional language compilers checkmodule(8) and checkpolicy(8).
>
> Avoid implicit conversions, free leaked memory, resolve several compiler
> warnings, use strict integer parsing.
>
> The last patch is an adoption of a patch proposed by liwugang [1], with
> the requested changes integrated.
>
>
> [1]: https://patchwork.kernel.org/project/selinux/patch/20210601151704.2688389-1-liwugang@163.com/
>
> Christian Göttsche (13):
>   libsepol: avoid implicit conversions
>   libsepol: free memory after policy validation
>   checkpolicy: enclose macro argument in parentheses
>   checkpolicy: misc checkmodule tweaks
>   checkpolicy: misc checkpolicy tweaks
>   checkpolicy: mark read-only parameters in module compiler const
>   checkpolicy: mark file local functions in policy_define static
>   checkpolicy: add missing function declarations
>   checkpolicy: resolve dismod memory leaks
>   checkpolicy: avoid implicit conversion
>   checkpolicy: error out on parsing too big integers
>   checkpolicy: print warning on source line overflow
>   checkpolicy: free extended permission memory
>
>  checkpolicy/checkmodule.c        | 18 +++----
>  checkpolicy/checkpolicy.c        | 26 +++++-----
>  checkpolicy/module_compiler.c    | 26 +++++-----
>  checkpolicy/module_compiler.h    |  4 +-
>  checkpolicy/policy_define.c      | 87 ++++++++++++++++----------------
>  checkpolicy/policy_parse.y       | 16 +++++-
>  checkpolicy/policy_scan.l        | 25 ++++++---
>  checkpolicy/test/dismod.c        |  7 ++-
>  libsepol/src/policydb_validate.c | 14 ++++-
>  libsepol/src/util.c              |  2 +-
>  10 files changed, 131 insertions(+), 94 deletions(-)
>
> --
> 2.33.0
>

For the whole series with the v2 and v3 patches:

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

Re: [PATCH 00/13] checkpolicy improvements

[James Carter]

On Wed, Sep 15, 2021 at 10:59 AM James Carter <jwcart2@gmail.com> wrote:
>
> On Tue, Sep 14, 2021 at 8:51 AM Christian Göttsche
> <cgzones@googlemail.com> wrote:
> >
> > Miscellaneous improvements to the checkpolicy subproject, affecting the
> > traditional language compilers checkmodule(8) and checkpolicy(8).
> >
> > Avoid implicit conversions, free leaked memory, resolve several compiler
> > warnings, use strict integer parsing.
> >
> > The last patch is an adoption of a patch proposed by liwugang [1], with
> > the requested changes integrated.
> >
> >
> > [1]: https://patchwork.kernel.org/project/selinux/patch/20210601151704.2688389-1-liwugang@163.com/
> >
> > Christian Göttsche (13):
> >   libsepol: avoid implicit conversions
> >   libsepol: free memory after policy validation
> >   checkpolicy: enclose macro argument in parentheses
> >   checkpolicy: misc checkmodule tweaks
> >   checkpolicy: misc checkpolicy tweaks
> >   checkpolicy: mark read-only parameters in module compiler const
> >   checkpolicy: mark file local functions in policy_define static
> >   checkpolicy: add missing function declarations
> >   checkpolicy: resolve dismod memory leaks
> >   checkpolicy: avoid implicit conversion
> >   checkpolicy: error out on parsing too big integers
> >   checkpolicy: print warning on source line overflow
> >   checkpolicy: free extended permission memory
> >
> >  checkpolicy/checkmodule.c        | 18 +++----
> >  checkpolicy/checkpolicy.c        | 26 +++++-----
> >  checkpolicy/module_compiler.c    | 26 +++++-----
> >  checkpolicy/module_compiler.h    |  4 +-
> >  checkpolicy/policy_define.c      | 87 ++++++++++++++++----------------
> >  checkpolicy/policy_parse.y       | 16 +++++-
> >  checkpolicy/policy_scan.l        | 25 ++++++---
> >  checkpolicy/test/dismod.c        |  7 ++-
> >  libsepol/src/policydb_validate.c | 14 ++++-
> >  libsepol/src/util.c              |  2 +-
> >  10 files changed, 131 insertions(+), 94 deletions(-)
> >
> > --
> > 2.33.0
> >
>
> For the whole series with the v2 and v3 patches:
>
> Acked-by: James Carter <jwcart2@gmail.com>

This series has been merged.
Thanks,
Jim