[PATCH v2] libsepol: ensure that decls hold consistent symbols when loading a binary policy

* [PATCH v2] libsepol: ensure that decls hold consistent symbols when loading a binary policy
@ 2021-01-06  8:08 Nicolas Iooss
  2021-01-08 18:00 ` James Carter
  0 siblings, 1 reply; 2+ messages in thread
From: Nicolas Iooss @ 2021-01-06  8:08 UTC (permalink / raw)
  To: selinux; +Cc: James Carter

While fuzzing /usr/libexec/hll/pp, a policy module was generated which
made "key" be NULL in required_scopes_to_cil() when doing:

    key = pdb->sym_val_to_name[sym][i];

This was caused by using a decl->required.scope[sym] bitmap which was
not consistent with the policy symbols.

Ensure this consistency when loading a binary policy by introducing a
new function which is called after policydb_index_others(), so that
p->sym_val_to_name[sym] can be used to check whether a symbol exists, in
a performent way (instead of searching the hash table
p->symtab[sym].table).

This issue has been found while fuzzing hll/pp with the American Fuzzy
Lop.

Signed-off-by: Nicolas Iooss <nicolas.iooss@m4x.org>
---
This replaces patch "libsepol: ensure that hashtab_search is not called
with a NULL key"
(https://lore.kernel.org/selinux/CAP+JOzQcM03WUJ-Fg2LuLxzCGiagJnuyJozv7ed6f34vnKEKXA@mail.gmail.com/T/#m059ac9bc2bdb9e4c436ebe3cef03124de25f1f06)

 libsepol/src/policydb.c | 48 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/libsepol/src/policydb.c b/libsepol/src/policydb.c
index 6faaaa5895d0..f43d5c67463e 100644
--- a/libsepol/src/policydb.c
+++ b/libsepol/src/policydb.c
@@ -1209,6 +1209,51 @@ int policydb_index_decls(sepol_handle_t * handle, policydb_t * p)
 	return 0;
 }
 
+/*
+ * Verify the consistency of declarations, after the symbols were indexed
+ */
+int policydb_verify_decl_symbols(sepol_handle_t * handle, policydb_t * p)
+{
+	avrule_block_t *curblock;
+	avrule_decl_t *decl;
+	struct ebitmap_node *node;
+	unsigned int i, sym;
+
+	for (curblock = p->global; curblock != NULL; curblock = curblock->next) {
+		for (decl = curblock->branch_list; decl != NULL;
+		     decl = decl->next) {
+			for (sym = 0; sym < SYM_NUM; sym++) {
+				ebitmap_for_each_positive_bit(&decl->declared.scope[sym], node, i) {
+					if (i >= p->symtab[sym].nprim) {
+						ERR(handle, "too large value for symbol in declared scope %u: %u >= %u",
+						    decl->decl_id, i, p->symtab[sym].nprim);
+						return -1;
+					}
+					if (p->sym_val_to_name[sym][i] == NULL) {
+						ERR(handle, "invalid symbol %u in declared scope %u",
+						    i, decl->decl_id);
+						return -1;
+					}
+				}
+				ebitmap_for_each_positive_bit(&decl->required.scope[sym], node, i) {
+					if (i >= p->symtab[sym].nprim) {
+						ERR(handle, "too large value for symbol in required scope %u: %u >= %u",
+						    decl->decl_id, i, p->symtab[sym].nprim);
+						return -1;
+					}
+					if (p->sym_val_to_name[sym][i] == NULL) {
+						ERR(handle, "invalid symbol %u in required scope %u",
+						    i, decl->decl_id);
+						return -1;
+					}
+				}
+			}
+		}
+	}
+
+	return 0;
+}
+
 /*
  * Define the other val_to_name and val_to_struct arrays
  * in a policy database structure.  
@@ -4501,6 +4546,9 @@ int policydb_read(policydb_t * p, struct policy_file *fp, unsigned verbose)
 	if (policydb_index_others(fp->handle, p, verbose))
 		goto bad;
 
+	if (policydb_verify_decl_symbols(fp->handle, p))
+		goto bad;
+
 	if (ocontext_read(info, p, fp) == -1) {
 		goto bad;
 	}
-- 
2.30.0


^ permalink raw reply related	[flat|nested] 2+ messages in thread