[RFC,v2,22/22] selinux: restrict policy strings

Commit Message

Christian Göttsche Dec. 16, 2024, 4:40 p.m. UTC

From: Christian Göttsche <[e-mail]>

Validate the characters and the lengths of strings parsed from binary
policies.

  * Disallow control characters
  * Limit characters of identifiers to alphanumeric, underscore, dash,
    and dot
  * Limit identifiers in length to 128, expect types to 1024 and
    categories to 32, characters (excluding NUL-terminator)

Signed-off-by: Christian Göttsche <[e-mail]>
---
v2:
  - add wrappers for str_read() to minimize the usage of magic numbers
  - limit sensitivities to a length of 32, to match categories,
    suggested by Daniel
---
 security/selinux/ss/conditional.c |  2 +-
 security/selinux/ss/policydb.c    | 60 ++++++++++++++++++++-----------
 security/selinux/ss/policydb.h    | 45 ++++++++++++++++++++++-
 3 files changed, 84 insertions(+), 23 deletions(-)

Patch

diff --git a/security/selinux/ss/conditional.c b/security/selinux/ss/conditional.c
index 481aa17a6f26..7688615a3934 100644
--- a/security/selinux/ss/conditional.c
+++ b/security/selinux/ss/conditional.c
@@ -280,7 +280,7 @@  int cond_read_bool(struct policydb *p, struct symtab *s, struct policy_file *fp)
 
 	len = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_bool(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto err;
 
diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
index 34db40753654..c72182b91f49 100644
--- a/security/selinux/ss/policydb.c
+++ b/security/selinux/ss/policydb.c
@@ -1226,8 +1226,9 @@  static int context_read_and_validate(struct context *c, struct policydb *p,
  * binary representation file.
  */
 
-int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len, int kind, u32 max_len)
 {
+	u32 i;
 	int rc;
 	char *str;
 
@@ -1237,19 +1238,35 @@  int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
 	if (oom_check(sizeof(char), len, fp))
 		return -EINVAL;
 
+	if (max_len != 0 && len > max_len)
+		return -EINVAL;
+
 	str = kmalloc(len + 1, flags | __GFP_NOWARN);
 	if (!str)
 		return -ENOMEM;
 
 	rc = next_entry(str, fp, len);
-	if (rc) {
-		kfree(str);
-		return rc;
+	if (rc)
+		goto bad_str;
+
+	rc = -EINVAL;
+	for (i = 0; i < len; i++) {
+		if (iscntrl(str[i]))
+			goto bad_str;
+
+		if (kind == STR_IDENTIFIER &&
+		    !(isalnum(str[i]) || str[i] == '_' || str[i] == '-' || str[i] == '.'))
+			goto bad_str;
+
 	}
 
 	str[len] = '\0';
 	*strp = str;
 	return 0;
+
+bad_str:
+	kfree(str);
+	return rc;
 }
 
 static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *fp)
@@ -1274,7 +1291,7 @@  static int perm_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	if (perdatum->value < 1 || perdatum->value > 32)
 		goto bad;
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_perm(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1320,7 +1337,7 @@  static int common_read(struct policydb *p, struct symtab *s, struct policy_file
 		goto bad;
 	comdatum->permissions.nprim = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_class(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1557,12 +1574,12 @@  static int class_read(struct policydb *p, struct symtab *s, struct policy_file *
 
 	ncons = le32_to_cpu(buf[5]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_class(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
 	if (len2) {
-		rc = str_read(&cladatum->comkey, GFP_KERNEL, fp, len2);
+		rc = str_read_class(&cladatum->comkey, GFP_KERNEL, fp, len2);
 		if (rc)
 			goto bad;
 
@@ -1696,7 +1713,7 @@  static int role_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
 		role->bounds = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_role(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1763,7 +1780,7 @@  static int type_read(struct policydb *p, struct symtab *s, struct policy_file *f
 		typdatum->primary = le32_to_cpu(buf[2]);
 	}
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_type(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1827,7 +1844,7 @@  static int user_read(struct policydb *p, struct symtab *s, struct policy_file *f
 	if (p->policyvers >= POLICYDB_VERSION_BOUNDARY)
 		usrdatum->bounds = le32_to_cpu(buf[2]);
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_user(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1876,7 +1893,7 @@  static int sens_read(struct policydb *p, struct symtab *s, struct policy_file *f
 		goto bad;
 	levdatum->isalias = val;
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_sens(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -1919,7 +1936,7 @@  static int cat_read(struct policydb *p, struct symtab *s, struct policy_file *fp
 		goto bad;
 	catdatum->isalias = val;
 
-	rc = str_read(&key, GFP_KERNEL, fp, len);
+	rc = str_read_cat(&key, GFP_KERNEL, fp, len);
 	if (rc)
 		goto bad;
 
@@ -2225,7 +2242,7 @@  static int filename_trans_read_helper_compat(struct policydb *p, struct policy_f
 	len = le32_to_cpu(buf[0]);
 
 	/* path component string */
-	rc = str_read(&name, GFP_KERNEL, fp, len);
+	rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, 0);
 	if (rc)
 		return rc;
 
@@ -2324,7 +2341,7 @@  static int filename_trans_read_helper(struct policydb *p, struct policy_file *fp
 	len = le32_to_cpu(buf[0]);
 
 	/* path component string */
-	rc = str_read(&name, GFP_KERNEL, fp, len);
+	rc = str_read(&name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, 0);
 	if (rc)
 		return rc;
 
@@ -2478,7 +2495,7 @@  static int genfs_read(struct policydb *p, struct policy_file *fp)
 		if (!newgenfs)
 			goto out;
 
-		rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len);
+		rc = str_read(&newgenfs->fstype, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 		if (rc)
 			goto out;
 
@@ -2517,7 +2534,7 @@  static int genfs_read(struct policydb *p, struct policy_file *fp)
 			if (!newc)
 				goto out;
 
-			rc = str_read(&newc->u.name, GFP_KERNEL, fp, len);
+			rc = str_read(&newc->u.name, GFP_KERNEL, fp, len, STR_UNCONSTRAINT, 0);
 			if (rc)
 				goto out;
 
@@ -2620,7 +2637,7 @@  static int ocontext_read(struct policydb *p,
 					goto out;
 				len = le32_to_cpu(buf[0]);
 
-				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+				rc = str_read(&c->u.name, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 				if (rc)
 					goto out;
 
@@ -2688,7 +2705,7 @@  static int ocontext_read(struct policydb *p,
 					goto out;
 
 				len = le32_to_cpu(buf[1]);
-				rc = str_read(&c->u.name, GFP_KERNEL, fp, len);
+				rc = str_read(&c->u.name, GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 				if (rc)
 					goto out;
 
@@ -2754,7 +2771,7 @@  static int ocontext_read(struct policydb *p,
 				len = le32_to_cpu(buf[0]);
 
 				rc = str_read(&c->u.ibendport.dev_name,
-					      GFP_KERNEL, fp, len);
+					      GFP_KERNEL, fp, len, STR_IDENTIFIER, 128);
 				if (rc)
 					goto out;
 
@@ -2822,7 +2839,8 @@  int policydb_read(struct policydb *p, struct policy_file *fp)
 		goto bad;
 	}
 
-	rc = str_read(&policydb_str, GFP_KERNEL, fp, len);
+	rc = str_read(&policydb_str, GFP_KERNEL, fp, len,
+		      STR_UNCONSTRAINT, strlen(POLICYDB_STRING));
 	if (rc) {
 		if (rc == -ENOMEM) {
 			pr_err("SELinux:  unable to allocate memory for policydb string of length %d\n",
diff --git a/security/selinux/ss/policydb.h b/security/selinux/ss/policydb.h
index d979965ef939..22e5e5adf73c 100644
--- a/security/selinux/ss/policydb.h
+++ b/security/selinux/ss/policydb.h
@@ -402,7 +402,50 @@  static inline const char *sym_name(const struct policydb *p, unsigned int sym_nu
 	return p->sym_val_to_name[sym_num][element_nr];
 }
 
-extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len);
+#define STR_UNCONSTRAINT 0
+#define STR_IDENTIFIER 1
+extern int str_read(char **strp, gfp_t flags, struct policy_file *fp, u32 len,
+		    int kind, u32 max_len);
+
+static inline int str_read_bool(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_cat(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 32);
+}
+
+static inline int str_read_class(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_perm(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_role(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
+
+static inline int str_read_sens(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 32);
+}
+
+static inline int str_read_type(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 1024);
+}
+
+static inline int str_read_user(char **strp, gfp_t flags, struct policy_file *fp, u32 len)
+{
+	return str_read(strp, flags, fp, len, STR_IDENTIFIER, 128);
+}
 
 extern u16 string_to_security_class(struct policydb *p, const char *name);
 extern u32 string_to_av_perm(struct policydb *p, u16 tclass, const char *name);