[PATCH 2/5] checkpolicy: check identifier before copying

[PATCH 2/5] checkpolicy: check identifier before copying

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Avoid calling strdup(3) with a NULL pointer, which can happen with an
invalid policy context, e.g.:

    class C
    sid S
    class C { P }
    ;
    user U roles j;
    sid S s:l:q:q:q

Fixes: 6f2b689f ("checkpolicy: Fix MLS users in optional blocks")
Reported-by: oss-fuzz (issue 390004173)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/policy_define.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 2f811b67..96a481f7 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -4437,6 +4437,7 @@ static int parse_semantic_categories(char *id, level_datum_t * levdatum __attrib
 
 int define_user(void)
 {
+	const char *username;
 	char *id;
 	user_datum_t *usrdatum, *usr_global;
 	level_datum_t *levdatum;
@@ -4463,7 +4464,13 @@ int define_user(void)
 		return 0;
 	}
 
-	id = strdup(queue_head(id_queue));
+	username = queue_head(id_queue);
+	if (!username) {
+		yyerror("no user name");
+		return -1;
+	}
+
+	id = strdup(username);
 
 	if ((usrdatum = declare_user()) == NULL) {
 		free(id);
-- 
2.47.1

[PATCH 3/5] checkpolicy: remove unneeded queue_head()

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Just check the value of the subsequent queue_remove() call.

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

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 96a481f7..275ef5fe 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -4686,14 +4686,13 @@ static int parse_security_context(context_struct_t * c)
 
 	if (mlspol) {
 		/* extract the low sensitivity */
-		id = (char *)queue_head(id_queue);
+		id = (char *)queue_remove(id_queue);
 		if (!id) {
 			yyerror("no sensitivity name for sid context"
 				" definition?");
 			return -1;
 		}
 
-		id = (char *)queue_remove(id_queue);
 		for (l = 0; l < 2; l++) {
 			levdatum = (level_datum_t *)
 			    hashtab_search(policydbp->p_levels.table,
-- 
2.47.1

[PATCH 4/5] checkpolicy: do not consume unmatched identifiers

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Avoid consuming identifiers during pass 1 in functions that do not parse
them during pass 2. This currently works due to the subsequent
parse_security_context(NULL) call.

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

diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index 275ef5fe..a056be67 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -4850,7 +4850,6 @@ int define_fs_context(unsigned int major, unsigned int minor)
 int define_pirq_context(unsigned int pirq)
 {
 	ocontext_t *newc, *c, *l, *head;
-	char *id;
 
 	if (policydbp->target_platform != SEPOL_TARGET_XEN) {
 		yyerror("pirqcon not supported for target");
@@ -4858,8 +4857,6 @@ int define_pirq_context(unsigned int pirq)
 	}
 
 	if (pass == 1) {
-		id = (char *) queue_remove(id_queue);
-		free(id);
 		parse_security_context(NULL);
 		return 0;
 	}
@@ -4904,7 +4901,6 @@ bad:
 int define_iomem_context(uint64_t low, uint64_t high)
 {
 	ocontext_t *newc, *c, *l, *head;
-	char *id;
 
 	if (policydbp->target_platform != SEPOL_TARGET_XEN) {
 		yyerror("iomemcon not supported for target");
@@ -4912,8 +4908,6 @@ int define_iomem_context(uint64_t low, uint64_t high)
 	}
 
 	if (pass == 1) {
-		id = (char *)queue_remove(id_queue);
-		free(id);
 		parse_security_context(NULL);
 		return 0;
 	}
@@ -4968,7 +4962,6 @@ bad:
 int define_ioport_context(unsigned long low, unsigned long high)
 {
 	ocontext_t *newc, *c, *l, *head;
-	char *id;
 
 	if (policydbp->target_platform != SEPOL_TARGET_XEN) {
 		yyerror("ioportcon not supported for target");
@@ -4976,8 +4969,6 @@ int define_ioport_context(unsigned long low, unsigned long high)
 	}
 
 	if (pass == 1) {
-		id = (char *)queue_remove(id_queue);
-		free(id);
 		parse_security_context(NULL);
 		return 0;
 	}
@@ -5032,7 +5023,6 @@ bad:
 int define_pcidevice_context(unsigned long device)
 {
 	ocontext_t *newc, *c, *l, *head;
-	char *id;
 
 	if (policydbp->target_platform != SEPOL_TARGET_XEN) {
 		yyerror("pcidevicecon not supported for target");
@@ -5040,8 +5030,6 @@ int define_pcidevice_context(unsigned long device)
 	}
 
 	if (pass == 1) {
-		id = (char *) queue_remove(id_queue);
-		free(id);
 		parse_security_context(NULL);
 		return 0;
 	}
@@ -5845,7 +5833,6 @@ int define_ipv6_cidr_node_context(void)
 	}
 
 	if (pass == 1) {
-		free(queue_remove(id_queue));
 		free(queue_remove(id_queue));
 		parse_security_context(NULL);
 		return 0;
-- 
2.47.1

[PATCH 5/5] checkpolicy: clear queue between parser passes

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

Clear the identifier queue after pass 1 to void unhandled identifiers
from pass 1 leaking into pass 2 and leading to confusing error messages.
For example for the following policy the error changes from
'no user name' to 'unknown role j':

    class C
    sid S
    class C { P }
    ;
    user U roles j;
    sid S s:l:q:q:q

While on it call set_source_file() from init_parser().

Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 checkpolicy/fuzz/checkpolicy-fuzzer.c | 10 +++-------
 checkpolicy/parse_util.c              |  9 +++------
 checkpolicy/policy_define.c           |  7 +++++--
 checkpolicy/queue.c                   | 18 ++++++++++++++++++
 checkpolicy/queue.h                   |  1 +
 5 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/checkpolicy/fuzz/checkpolicy-fuzzer.c b/checkpolicy/fuzz/checkpolicy-fuzzer.c
index 331201c0..c99ef412 100644
--- a/checkpolicy/fuzz/checkpolicy-fuzzer.c
+++ b/checkpolicy/fuzz/checkpolicy-fuzzer.c
@@ -25,11 +25,10 @@ extern unsigned int policydb_errors;
 
 extern int yynerrs;
 extern FILE *yyin;
-extern void init_parser(int);
+extern void init_parser(int pass, const char *input_name);
 extern int yyparse(void);
 extern void yyrestart(FILE *);
 extern int yylex_destroy(void);
-extern void set_source_file(const char *name);
 
 jmp_buf fuzzing_pre_parse_stack_state;
 
@@ -87,8 +86,6 @@ static int read_source_policy(policydb_t *p, const uint8_t *data, size_t size)
 
 	rewind(yyin);
 
-	set_source_file("fuzz-input");
-
 	id_queue = queue_create();
 	if (id_queue == NULL) {
 		fclose(yyin);
@@ -99,7 +96,7 @@ static int read_source_policy(policydb_t *p, const uint8_t *data, size_t size)
 	policydbp = p;
 	mlspol = p->mls;
 
-	init_parser(1);
+	init_parser(1, "fuzz-input-1");
 
 	if (setjmp(fuzzing_pre_parse_stack_state) != 0) {
 		queue_destroy(id_queue);
@@ -119,8 +116,7 @@ static int read_source_policy(policydb_t *p, const uint8_t *data, size_t size)
 	}
 
 	rewind(yyin);
-	init_parser(2);
-	set_source_file("fuzz-input");
+	init_parser(2, "fuzz-input-2");
 	yyrestart(yyin);
 
 	rc = yyparse();
diff --git a/checkpolicy/parse_util.c b/checkpolicy/parse_util.c
index eda814e1..389c9ff3 100644
--- a/checkpolicy/parse_util.c
+++ b/checkpolicy/parse_util.c
@@ -23,7 +23,7 @@
 
 /* these are defined in policy_parse.y and are needed for read_source_policy */
 extern FILE *yyin;
-extern void init_parser(int);
+extern void init_parser(int pass, const char *input_name);
 extern int yyparse(void);
 extern void yyrestart(FILE *);
 extern int yylex_destroy(void);
@@ -31,7 +31,6 @@ extern queue_t id_queue;
 extern unsigned int policydb_errors;
 extern policydb_t *policydbp;
 extern int mlspol;
-extern void set_source_file(const char *name);
 
 int read_source_policy(policydb_t * p, const char *file, const char *progname)
 {
@@ -42,7 +41,6 @@ int read_source_policy(policydb_t * p, const char *file, const char *progname)
 		fprintf(stderr, "%s:  unable to open %s:  %s\n", progname, file, strerror(errno));
 		return -1;
 	}
-	set_source_file(file);
 
 	id_queue = queue_create();
 	if (id_queue == NULL) {
@@ -58,7 +56,7 @@ int read_source_policy(policydb_t * p, const char *file, const char *progname)
 		goto cleanup;
 	}
 
-	init_parser(1);
+	init_parser(1, file);
 	if (yyparse() || policydb_errors) {
 		fprintf(stderr,
 			"%s:  error(s) encountered while parsing configuration\n",
@@ -66,8 +64,7 @@ int read_source_policy(policydb_t * p, const char *file, const char *progname)
 		goto cleanup;
 	}
 	rewind(yyin);
-	init_parser(2);
-	set_source_file(file);
+	init_parser(2, file);
 	yyrestart(yyin);
 	if (yyparse() || policydb_errors) {
 		fprintf(stderr,
diff --git a/checkpolicy/policy_define.c b/checkpolicy/policy_define.c
index a056be67..f19e9f6d 100644
--- a/checkpolicy/policy_define.c
+++ b/checkpolicy/policy_define.c
@@ -58,7 +58,7 @@
 #include "module_compiler.h"
 #include "policy_define.h"
 
-extern void init_parser(int pass_number);
+extern void init_parser(int pass_number, const char *input_name);
 __attribute__ ((format(printf, 1, 2)))
 extern void yyerror2(const char *fmt, ...);
 
@@ -71,17 +71,20 @@ extern unsigned long policydb_lineno;
 extern unsigned long source_lineno;
 extern unsigned int policydb_errors;
 extern char source_file[PATH_MAX];
+extern void set_source_file(const char *name);
 
 extern int yywarn(const char *msg);
 extern int yyerror(const char *msg);
 
 /* initialize all of the state variables for the scanner/parser */
-void init_parser(int pass_number)
+void init_parser(int pass_number, const char *input_name)
 {
 	policydb_lineno = 1;
 	source_lineno = 1;
 	policydb_errors = 0;
 	pass = pass_number;
+	set_source_file(input_name);
+	queue_clear(id_queue);
 }
 
 void yyerror2(const char *fmt, ...)
diff --git a/checkpolicy/queue.c b/checkpolicy/queue.c
index 9f4c651a..5eee2871 100644
--- a/checkpolicy/queue.c
+++ b/checkpolicy/queue.c
@@ -104,6 +104,24 @@ queue_element_t queue_head(queue_t q)
 	return q->head->element;
 }
 
+void queue_clear(queue_t q)
+{
+	queue_node_ptr_t p, temp;
+
+	if (!q)
+		return;
+
+	p = q->head;
+	while (p != NULL) {
+		free(p->element);
+		temp = p;
+		p = p->next;
+		free(temp);
+	}
+
+	q->head = q->tail = NULL;
+}
+
 void queue_destroy(queue_t q)
 {
 	queue_node_ptr_t p, temp;
diff --git a/checkpolicy/queue.h b/checkpolicy/queue.h
index 45116dee..3ce2e5bd 100644
--- a/checkpolicy/queue.h
+++ b/checkpolicy/queue.h
@@ -33,6 +33,7 @@ int queue_insert(queue_t, queue_element_t);
 int queue_push(queue_t, queue_element_t);
 queue_element_t queue_remove(queue_t);
 queue_element_t queue_head(queue_t);
+void queue_clear(queue_t);
 void queue_destroy(queue_t);
 
 /* 
-- 
2.47.1

[PATCH 1/5] libselinux: set errno in failure case

[Christian Göttsche]

From: Christian Göttsche <cgzones@googlemail.com>

In case an entry read from a textual fcontext definition is too long set
errno and the error string accordingly.

Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
Reported-by: oss-fuzz (issue 389974971)
Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
---
 libselinux/src/label_support.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/libselinux/src/label_support.c b/libselinux/src/label_support.c
index 978ba828..57e191c8 100644
--- a/libselinux/src/label_support.c
+++ b/libselinux/src/label_support.c
@@ -45,8 +45,11 @@ static inline int read_spec_entry(char **entry, const char **ptr, size_t *len, c
 	}
 
 	if (*len) {
-		if (*len >= UINT16_MAX)
+		if (*len >= UINT16_MAX) {
+			errno = EINVAL;
+			*errbuf = "Spec entry too long";
 			return -1;
+		}
 
 		*entry = strndup(tmp_buf, *len);
 		if (!*entry)
-- 
2.47.1

Re: [PATCH 1/5] libselinux: set errno in failure case

[James Carter]

On Wed, Jan 15, 2025 at 8:13 AM Christian Göttsche
<cgoettsche@seltendoof.de> wrote:
>
> From: Christian Göttsche <cgzones@googlemail.com>
>
> In case an entry read from a textual fcontext definition is too long set
> errno and the error string accordingly.
>
> Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
> Reported-by: oss-fuzz (issue 389974971)
> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>

For these five patches:
Acked-by: James Carter <jwcart2@gmail.com>

> ---
>  libselinux/src/label_support.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/libselinux/src/label_support.c b/libselinux/src/label_support.c
> index 978ba828..57e191c8 100644
> --- a/libselinux/src/label_support.c
> +++ b/libselinux/src/label_support.c
> @@ -45,8 +45,11 @@ static inline int read_spec_entry(char **entry, const char **ptr, size_t *len, c
>         }
>
>         if (*len) {
> -               if (*len >= UINT16_MAX)
> +               if (*len >= UINT16_MAX) {
> +                       errno = EINVAL;
> +                       *errbuf = "Spec entry too long";
>                         return -1;
> +               }
>
>                 *entry = strndup(tmp_buf, *len);
>                 if (!*entry)
> --
> 2.47.1
>
>

Re: [PATCH 1/5] libselinux: set errno in failure case

[Petr Lautrbach]

James Carter <jwcart2@gmail.com> writes:

> On Wed, Jan 15, 2025 at 8:13 AM Christian Göttsche
> <cgoettsche@seltendoof.de> wrote:
>>
>> From: Christian Göttsche <cgzones@googlemail.com>
>>
>> In case an entry read from a textual fcontext definition is too long set
>> errno and the error string accordingly.
>>
>> Fixes: 92306daf ("libselinux: rework selabel_file(5) database")
>> Reported-by: oss-fuzz (issue 389974971)
>> Signed-off-by: Christian Göttsche <cgzones@googlemail.com>
>
> For these five patches:
> Acked-by: James Carter <jwcart2@gmail.com>

Merged. Thanks!


>> ---
>>  libselinux/src/label_support.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/libselinux/src/label_support.c b/libselinux/src/label_support.c
>> index 978ba828..57e191c8 100644
>> --- a/libselinux/src/label_support.c
>> +++ b/libselinux/src/label_support.c
>> @@ -45,8 +45,11 @@ static inline int read_spec_entry(char **entry, const char **ptr, size_t *len, c
>>         }
>>
>>         if (*len) {
>> -               if (*len >= UINT16_MAX)
>> +               if (*len >= UINT16_MAX) {
>> +                       errno = EINVAL;
>> +                       *errbuf = "Spec entry too long";
>>                         return -1;
>> +               }
>>
>>                 *entry = strndup(tmp_buf, *len);
>>                 if (!*entry)
>> --
>> 2.47.1
>>
>>