diff mbox series

[v2] libselinux: Eliminate use of security_compute_user()

Message ID 20200204194603.525561-1-plautrba@redhat.com (mailing list archive)
State Changes Requested
Headers show
Series [v2] libselinux: Eliminate use of security_compute_user() | expand

Commit Message

Petr Lautrbach Feb. 4, 2020, 7:46 p.m. UTC
get_ordered_context_list() code used to ask the kernel to compute the complete
set of reachable contexts using /sys/fs/selinux/user aka
security_compute_user(). This set can be so huge so that it doesn't fit into a
kernel page and security_compute_user() fails. Even if it doesn't fail,
get_ordered_context_list() throws away the vast majority of the returned
contexts because they don't match anything in
/etc/selinux/targeted/contexts/default_contexts or
/etc/selinux/targeted/contexts/users/

get_ordered_context_list() is rewritten to compute set of contexts based on
/etc/selinux/targeted/contexts/users/ and
/etc/selinux/targeted/contexts/default_contexts files and to return only valid
contexts, using security_check_context(), from this set.

Fixes: https://github.com/SELinuxProject/selinux/issues/28

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---

This is second version of https://patchwork.kernel.org/patch/10936883/ updated
based on comments in the thread:

- fixed indentation
- error when the size of partial context is too big
- dropped two free(usercon_str)
- parsing continues only if the partial context can't be parsed

Tested on Fedora using python3 and atd:

Before:

# python3
>>> import selinux
>>> selinux.get_ordered_context_list("staff_u", "system_u:system_r:crond_t:s0-s0:c0.c1023")
['staff_u:unconfined_r:unconfined_t:s0']

[staff@localhost ~]$ at now + 1 minutes <<EOF
id -Z > /tmp/id-Z
EOF
[staff@localhost ~]$ cat /tmp/id-Z
staff_u:unconfined_r:unconfined_t:s0


After:
>>> selinux.get_ordered_context_list("staff_u", "system_u:system_r:crond_t:s0-s0:c0.c1023")
['staff_u:staff_r:staff_t:s0-s0:c0.c1023', 'staff_u:staff_r:cronjob_t:s0-s0:c0.c1023', 'staff_u:staff_r:staff_t:s0-s0:c0.c1023', 'staff_u:sysadm_r:sysadm_t:s0-s0:c0.c1023', 'staff_u:unconfined_r:unconfined_t:s0-s0:c0.c1023', 'staff_u:staff_r:cronjob_t:s0-s0:c0.c1023', 'staff_u:sysadm_r:cronjob_t:s0-s0:c0.c1023', 'staff_u:system_r:system_cronjob_t:s0-s0:c0.c1023', 'staff_u:unconfined_r:unconfined_cronjob_t:s0-s0:c0.c1023']

[staff@localhost ~]$ cat /tmp/id-Z 
staff_u:staff_r:staff_t:s0-s0:c0.c1023

 libselinux/src/get_context_list.c | 197 +++++++++++-------------------
 1 file changed, 74 insertions(+), 123 deletions(-)

Comments

Stephen Smalley Feb. 5, 2020, 3:38 p.m. UTC | #1
On 2/4/20 2:46 PM, Petr Lautrbach wrote:
> get_ordered_context_list() code used to ask the kernel to compute the complete
> set of reachable contexts using /sys/fs/selinux/user aka
> security_compute_user(). This set can be so huge so that it doesn't fit into a
> kernel page and security_compute_user() fails. Even if it doesn't fail,
> get_ordered_context_list() throws away the vast majority of the returned
> contexts because they don't match anything in
> /etc/selinux/targeted/contexts/default_contexts or
> /etc/selinux/targeted/contexts/users/
> 
> get_ordered_context_list() is rewritten to compute set of contexts based on
> /etc/selinux/targeted/contexts/users/ and
> /etc/selinux/targeted/contexts/default_contexts files and to return only valid
> contexts, using security_check_context(), from this set.
> 
> Fixes: https://github.com/SELinuxProject/selinux/issues/28
> 
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
<snip>
> After:
>>>> selinux.get_ordered_context_list("staff_u", "system_u:system_r:crond_t:s0-s0:c0.c1023")
> ['staff_u:staff_r:staff_t:s0-s0:c0.c1023', 'staff_u:staff_r:cronjob_t:s0-s0:c0.c1023', 'staff_u:staff_r:staff_t:s0-s0:c0.c1023', 'staff_u:sysadm_r:sysadm_t:s0-s0:c0.c1023', 'staff_u:unconfined_r:unconfined_t:s0-s0:c0.c1023', 'staff_u:staff_r:cronjob_t:s0-s0:c0.c1023', 'staff_u:sysadm_r:cronjob_t:s0-s0:c0.c1023', 'staff_u:system_r:system_cronjob_t:s0-s0:c0.c1023', 'staff_u:unconfined_r:unconfined_cronjob_t:s0-s0:c0.c1023']

We should likely de-duplicate the list; I think that was being handled 
previously by virtue of using the reachable contexts as our baseline and 
just re-ordering them.  Here we just need to check whether we already 
have a context in the list before inserting a new one.

> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 689e46589f30..a3dcaea2ffc4 100644
> --- a/libselinux/src/get_context_list.c
> +++ b/libselinux/src/get_context_list.c
> @@ -114,61 +115,25 @@ int get_default_context(const char *user,
<snip>
> @@ -243,23 +209,66 @@ static int get_context_order(FILE * fp,
<snip>
> +		context_range_set(usercon, fromlevel);
> +		usercon_str = context_str(usercon);

Both context_range_set() and context_str() could fail on an allocation 
failure, returning 1 or NULL respectively.
diff mbox series

Patch

diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
index 689e46589f30..a3dcaea2ffc4 100644
--- a/libselinux/src/get_context_list.c
+++ b/libselinux/src/get_context_list.c
@@ -2,6 +2,7 @@ 
 #include <errno.h>
 #include <stdio.h>
 #include <stdio_ext.h>
+#include <stdint.h>
 #include <stdlib.h>
 #include <string.h>
 #include <ctype.h>
@@ -114,61 +115,25 @@  int get_default_context(const char *user,
 	return 0;
 }
 
-static int find_partialcon(char ** list,
-			   unsigned int nreach, char *part)
-{
-	const char *conrole, *contype;
-	char *partrole, *parttype, *ptr;
-	context_t con;
-	unsigned int i;
-
-	partrole = part;
-	ptr = part;
-	while (*ptr && !isspace(*ptr) && *ptr != ':')
-		ptr++;
-	if (*ptr != ':')
-		return -1;
-	*ptr++ = 0;
-	parttype = ptr;
-	while (*ptr && !isspace(*ptr) && *ptr != ':')
-		ptr++;
-	*ptr = 0;
-
-	for (i = 0; i < nreach; i++) {
-		con = context_new(list[i]);
-		if (!con)
-			return -1;
-		conrole = context_role_get(con);
-		contype = context_type_get(con);
-		if (!conrole || !contype) {
-			context_free(con);
-			return -1;
-		}
-		if (!strcmp(conrole, partrole) && !strcmp(contype, parttype)) {
-			context_free(con);
-			return i;
-		}
-		context_free(con);
-	}
-
-	return -1;
-}
-
-static int get_context_order(FILE * fp,
+static int get_context_user(FILE * fp,
 			     char * fromcon,
-			     char ** reachable,
-			     unsigned int nreach,
-			     unsigned int *ordering, unsigned int *nordered)
+			     const char * user,
+			     char ***reachable,
+			     unsigned int *nreachable)
 {
 	char *start, *end = NULL;
 	char *line = NULL;
-	size_t line_len = 0;
+	size_t line_len = 0, usercon_len;
+	size_t user_len = strlen(user);
 	ssize_t len;
 	int found = 0;
-	const char *fromrole, *fromtype;
+	const char *fromrole, *fromtype, *fromlevel;
 	char *linerole, *linetype;
-	unsigned int i;
+	char **new_reachable = NULL;
+	char *usercon_str;
 	context_t con;
+	context_t usercon;
+
 	int rc;
 
 	errno = -EINVAL;
@@ -180,6 +145,7 @@  static int get_context_order(FILE * fp,
 		return -1;
 	fromrole = context_role_get(con);
 	fromtype = context_type_get(con);
+	fromlevel = context_range_get(con);
 	if (!fromrole || !fromtype) {
 		context_free(con);
 		return -1;
@@ -243,23 +209,66 @@  static int get_context_order(FILE * fp,
 		if (*end)
 			*end++ = 0;
 
-		/* Check for a match in the reachable list. */
-		rc = find_partialcon(reachable, nreach, start);
-		if (rc < 0) {
-			/* No match, skip it. */
+		/* Check whether a new context is valid */
+		if (SIZE_MAX - user_len < strlen(start) + 1) {
+			fprintf(stderr, "%s: one of partial contexts is too big\n", __FUNCTION__);
+			errno = EINVAL;
+			rc = -1;
+			goto out;
+		}
+		usercon_len = user_len + strlen(start) + 1;
+		usercon_str = malloc(usercon_len);
+		if (!usercon_str) {
+			rc = -1;
+			goto out;
+		}
+
+		/* set range from fromcon in the new usercon */
+		snprintf(usercon_str, usercon_len - 1, "%s:%s", user, start);
+		usercon = context_new(usercon_str);
+		free(usercon_str);
+		if (!usercon) {
+			if (errno != EINVAL) {
+				rc = -1;
+				goto out;
+			}
+			fprintf(stderr,
+				"%s: can't create a context from %s, skipping\n",
+				__FUNCTION__, usercon_str);
 			start = end;
 			continue;
 		}
-
-		/* If a match is found and the entry is not already ordered
-		   (e.g. due to prior match in prior config file), then set
-		   the ordering for it. */
-		i = rc;
-		if (ordering[i] == nreach)
-			ordering[i] = (*nordered)++;
+		context_range_set(usercon, fromlevel);
+		usercon_str = context_str(usercon);
+
+		if (security_check_context(usercon_str) == 0) {
+			if (*nreachable == 0) {
+				new_reachable = malloc(2 * sizeof(char *));
+				if (!new_reachable) {
+					context_free(usercon);
+					rc = -1;
+					goto out;
+				}
+			} else {
+				new_reachable = realloc(*reachable, (*nreachable + 2) * sizeof(char *));
+				if (!new_reachable) {
+					context_free(usercon);
+					rc = -1;
+					goto out;
+				}
+			}
+			new_reachable[*nreachable] = strdup(usercon_str);
+			if (new_reachable[*nreachable] == NULL) {
+				rc = -1;
+				goto out;
+			}
+			new_reachable[*nreachable + 1] = 0;
+			*reachable = new_reachable;
+			*nreachable += 1;
+		}
+		context_free(usercon);
 		start = end;
 	}
-
 	rc = 0;
 
       out:
@@ -313,21 +322,6 @@  static int get_failsafe_context(const char *user, char ** newcon)
 	return 0;
 }
 
-struct context_order {
-	char * con;
-	unsigned int order;
-};
-
-static int order_compare(const void *A, const void *B)
-{
-	const struct context_order *c1 = A, *c2 = B;
-	if (c1->order < c2->order)
-		return -1;
-	else if (c1->order > c2->order)
-		return 1;
-	return strcmp(c1->con, c2->con);
-}
-
 int get_ordered_context_list_with_level(const char *user,
 					const char *level,
 					char * fromcon,
@@ -395,11 +389,8 @@  int get_ordered_context_list(const char *user,
 			     char *** list)
 {
 	char **reachable = NULL;
-	unsigned int *ordering = NULL;
-	struct context_order *co = NULL;
-	char **ptr;
 	int rc = 0;
-	unsigned int nreach = 0, nordered = 0, freefrom = 0, i;
+	unsigned nreachable = 0, freefrom = 0;
 	FILE *fp;
 	char *fname = NULL;
 	size_t fname_len;
@@ -413,23 +404,6 @@  int get_ordered_context_list(const char *user,
 		freefrom = 1;
 	}
 
-	/* Determine the set of reachable contexts for the user. */
-	rc = security_compute_user(fromcon, user, &reachable);
-	if (rc < 0)
-		goto failsafe;
-	nreach = 0;
-	for (ptr = reachable; *ptr; ptr++)
-		nreach++;
-	if (!nreach)
-		goto failsafe;
-
-	/* Initialize ordering array. */
-	ordering = malloc(nreach * sizeof(unsigned int));
-	if (!ordering)
-		goto failsafe;
-	for (i = 0; i < nreach; i++)
-		ordering[i] = nreach;
-
 	/* Determine the ordering to apply from the optional per-user config
 	   and from the global config. */
 	fname_len = strlen(user_contexts_path) + strlen(user) + 2;
@@ -440,8 +414,8 @@  int get_ordered_context_list(const char *user,
 	fp = fopen(fname, "re");
 	if (fp) {
 		__fsetlocking(fp, FSETLOCKING_BYCALLER);
-		rc = get_context_order(fp, fromcon, reachable, nreach, ordering,
-				       &nordered);
+		rc = get_context_user(fp, fromcon, user, &reachable, &nreachable);
+
 		fclose(fp);
 		if (rc < 0 && errno != ENOENT) {
 			fprintf(stderr,
@@ -454,8 +428,7 @@  int get_ordered_context_list(const char *user,
 	fp = fopen(selinux_default_context_path(), "re");
 	if (fp) {
 		__fsetlocking(fp, FSETLOCKING_BYCALLER);
-		rc = get_context_order(fp, fromcon, reachable, nreach, ordering,
-				       &nordered);
+		rc = get_context_user(fp, fromcon, user, &reachable, &nreachable);
 		fclose(fp);
 		if (rc < 0 && errno != ENOENT) {
 			fprintf(stderr,
@@ -463,40 +436,18 @@  int get_ordered_context_list(const char *user,
 				__FUNCTION__, selinux_default_context_path());
 			/* Fall through */
 		}
-		rc = 0;
+		rc = nreachable;
 	}
 
-	if (!nordered)
+	if (!nreachable)
 		goto failsafe;
 
-	/* Apply the ordering. */
-	co = malloc(nreach * sizeof(struct context_order));
-	if (!co)
-		goto failsafe;
-	for (i = 0; i < nreach; i++) {
-		co[i].con = reachable[i];
-		co[i].order = ordering[i];
-	}
-	qsort(co, nreach, sizeof(struct context_order), order_compare);
-	for (i = 0; i < nreach; i++)
-		reachable[i] = co[i].con;
-	free(co);
-
-	/* Only report the ordered entries to the caller. */
-	if (nordered <= nreach) {
-		for (i = nordered; i < nreach; i++)
-			free(reachable[i]);
-		reachable[nordered] = NULL;
-		rc = nordered;
-	}
-
       out:
 	if (rc > 0)
 		*list = reachable;
 	else
 		freeconary(reachable);
 
-	free(ordering);
 	if (freefrom)
 		freecon(fromcon);