SELinux Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v5] libselinux: Eliminate use of security_compute_user()
@ 2020-02-11 10:14 Petr Lautrbach
  2020-02-11 15:04 ` Stephen Smalley
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Petr Lautrbach @ 2020-02-11 10:14 UTC (permalink / raw)
  To: selinux; +Cc: Petr Lautrbach

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>
---

v5 changes:

- context_free(usercon) when is_in_reachable() finds a duplicate

I see some value in reporting problem context during parsing a file and skipping this context
so I left the code after usercon = context_new(usercon_str) untouched.


 libselinux/src/get_context_list.c | 214 ++++++++++++++----------------
 1 file changed, 98 insertions(+), 116 deletions(-)

diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
index 689e46589f30..6078d980cde1 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,38 @@ int get_default_context(const char *user,
 	return 0;
 }
 
-static int find_partialcon(char ** list,
-			   unsigned int nreach, char *part)
+static int is_in_reachable(char **reachable, const char *usercon_str)
 {
-	const char *conrole, *contype;
-	char *partrole, *parttype, *ptr;
-	context_t con;
-	unsigned int i;
+	if (!reachable)
+		return 0;
 
-	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;
+	for (; *reachable != NULL; reachable++) {
+		if (strcmp(*reachable, usercon_str) == 0) {
+			return 1;
 		}
-		context_free(con);
 	}
-
-	return -1;
+	return 0;
 }
 
-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 +158,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 +222,84 @@ 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) + 2) {
+			fprintf(stderr, "%s: one of partial contexts is too big\n", __FUNCTION__);
+			errno = EINVAL;
+			rc = -1;
+			goto out;
+		}
+		usercon_len = user_len + strlen(start) + 2;
+		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, "%s:%s", user, start);
+		usercon = context_new(usercon_str);
+		if (!usercon) {
+			if (errno != EINVAL) {
+				free(usercon_str);
+				rc = -1;
+				goto out;
+			}
+			fprintf(stderr,
+				"%s: can't create a context from %s, skipping\n",
+				__FUNCTION__, usercon_str);
+			free(usercon_str);
 			start = end;
 			continue;
 		}
+		free(usercon_str);
+		if (context_range_set(usercon, fromlevel) != 0) {
+			context_free(usercon);
+			rc = -1;
+			goto out;
+		}
+		usercon_str = context_str(usercon);
+		if (!usercon_str) {
+			context_free(usercon);
+			rc = -1;
+			goto out;
+		}
 
-		/* 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)++;
+		/* check whether usercon is already in reachable */
+		if (is_in_reachable(*reachable, usercon_str)) {
+			context_free(usercon);
+			start = end;
+			continue;
+		}
+		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) {
+				context_free(usercon);
+				rc = -1;
+				goto out;
+			}
+			new_reachable[*nreachable + 1] = 0;
+			*reachable = new_reachable;
+			*nreachable += 1;
+		}
+		context_free(usercon);
 		start = end;
 	}
-
 	rc = 0;
 
       out:
@@ -313,21 +353,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 +420,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 +435,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 +445,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 +459,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 +467,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);
 
-- 
2.25.0


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

* Re: [PATCH v5] libselinux: Eliminate use of security_compute_user()
  2020-02-11 10:14 [PATCH v5] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
@ 2020-02-11 15:04 ` Stephen Smalley
  2020-02-11 21:19 ` Ondrej Mosnacek
  2020-02-12 16:01 ` Stephen Smalley
  2 siblings, 0 replies; 6+ messages in thread
From: Stephen Smalley @ 2020-02-11 15:04 UTC (permalink / raw)
  To: Petr Lautrbach, selinux; +Cc: Ondrej Mosnacek

On 2/11/20 5:14 AM, 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>

This looks fine to me; I'll wait to see if Ondrej has any further 
comments, but you can add my:

Acked-by: Stephen Smalley <sds@tycho.nsa.gov>

Then maybe in a decade we can actually remove /sys/fs/selinux/user...


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

* Re: [PATCH v5] libselinux: Eliminate use of security_compute_user()
  2020-02-11 10:14 [PATCH v5] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
  2020-02-11 15:04 ` Stephen Smalley
@ 2020-02-11 21:19 ` Ondrej Mosnacek
  2020-02-12 10:13   ` Petr Lautrbach
  2020-02-12 10:41   ` Petr Lautrbach
  2020-02-12 16:01 ` Stephen Smalley
  2 siblings, 2 replies; 6+ messages in thread
From: Ondrej Mosnacek @ 2020-02-11 21:19 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: SElinux list

I found one memory leak and one style nit, please see below. Otherwise
the patch looks good to me (but I only checked the code flow - I don't
understand the whole context well enough to fully judge its
correctness).

On Tue, Feb 11, 2020 at 11:14 AM Petr Lautrbach <plautrba@redhat.com> 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>
> ---
>
> v5 changes:
>
> - context_free(usercon) when is_in_reachable() finds a duplicate
>
> I see some value in reporting problem context during parsing a file and skipping this context
> so I left the code after usercon = context_new(usercon_str) untouched.
>
>
>  libselinux/src/get_context_list.c | 214 ++++++++++++++----------------
>  1 file changed, 98 insertions(+), 116 deletions(-)
>
> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 689e46589f30..6078d980cde1 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,38 @@ int get_default_context(const char *user,
>         return 0;
>  }
>
> -static int find_partialcon(char ** list,
> -                          unsigned int nreach, char *part)
> +static int is_in_reachable(char **reachable, const char *usercon_str)
>  {
> -       const char *conrole, *contype;
> -       char *partrole, *parttype, *ptr;
> -       context_t con;
> -       unsigned int i;
> +       if (!reachable)
> +               return 0;
>
> -       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;
> +       for (; *reachable != NULL; reachable++) {
> +               if (strcmp(*reachable, usercon_str) == 0) {
> +                       return 1;
>                 }
> -               context_free(con);
>         }
> -
> -       return -1;
> +       return 0;
>  }
>
> -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;

This is not your bug, but this should be just "errno = EINVAL". Should
probably be fixed in another patch...

> @@ -180,6 +158,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 +222,84 @@ 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) + 2) {
> +                       fprintf(stderr, "%s: one of partial contexts is too big\n", __FUNCTION__);
> +                       errno = EINVAL;
> +                       rc = -1;
> +                       goto out;
> +               }
> +               usercon_len = user_len + strlen(start) + 2;
> +               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, "%s:%s", user, start);
> +               usercon = context_new(usercon_str);
> +               if (!usercon) {
> +                       if (errno != EINVAL) {
> +                               free(usercon_str);
> +                               rc = -1;
> +                               goto out;
> +                       }
> +                       fprintf(stderr,
> +                               "%s: can't create a context from %s, skipping\n",
> +                               __FUNCTION__, usercon_str);
> +                       free(usercon_str);
>                         start = end;
>                         continue;
>                 }
> +               free(usercon_str);
> +               if (context_range_set(usercon, fromlevel) != 0) {
> +                       context_free(usercon);
> +                       rc = -1;
> +                       goto out;
> +               }
> +               usercon_str = context_str(usercon);

You can do context_free(usercon) right here (it's not used beyond this
line) and avoid doing it in all the paths after here.

> +               if (!usercon_str) {
> +                       context_free(usercon);
> +                       rc = -1;
> +                       goto out;
> +               }
>
> -               /* 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)++;
> +               /* check whether usercon is already in reachable */
> +               if (is_in_reachable(*reachable, usercon_str)) {
> +                       context_free(usercon);
> +                       start = end;
> +                       continue;
> +               }
> +               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) {

Unless I'm mistaken, you should free new_reachable here, otherwise it leaks.

> +                               context_free(usercon);
> +                               rc = -1;
> +                               goto out;
> +                       }
> +                       new_reachable[*nreachable + 1] = 0;
> +                       *reachable = new_reachable;
> +                       *nreachable += 1;
> +               }
> +               context_free(usercon);
>                 start = end;
>         }
> -
>         rc = 0;
>
>        out:
> @@ -313,21 +353,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 +420,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 +435,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 +445,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 +459,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 +467,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);
>
> --
> 2.25.0
>

-- 
Ondrej Mosnacek <omosnace at redhat dot com>
Software Engineer, Security Technologies
Red Hat, Inc.


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

* Re: [PATCH v5] libselinux: Eliminate use of security_compute_user()
  2020-02-11 21:19 ` Ondrej Mosnacek
@ 2020-02-12 10:13   ` Petr Lautrbach
  2020-02-12 10:41   ` Petr Lautrbach
  1 sibling, 0 replies; 6+ messages in thread
From: Petr Lautrbach @ 2020-02-12 10:13 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: Petr Lautrbach, SElinux list


Ondrej Mosnacek <omosnace@redhat.com> writes:

> I found one memory leak and one style nit, please see below. Otherwise
> the patch looks good to me (but I only checked the code flow - I don't
> understand the whole context well enough to fully judge its
> correctness).

Thanks! I'll apply your suggestions and resend the patch 

> On Tue, Feb 11, 2020 at 11:14 AM Petr Lautrbach <plautrba@redhat.com> 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>
>> ---
>>
>> v5 changes:
>>
>> - context_free(usercon) when is_in_reachable() finds a duplicate
>>
>> I see some value in reporting problem context during parsing a file and skipping this context
>> so I left the code after usercon = context_new(usercon_str) untouched.
>>
>>
>>  libselinux/src/get_context_list.c | 214 ++++++++++++++----------------
>>  1 file changed, 98 insertions(+), 116 deletions(-)
>>
>> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
>> index 689e46589f30..6078d980cde1 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,38 @@ int get_default_context(const char *user,
>>         return 0;
>>  }
>>
>> -static int find_partialcon(char ** list,
>> -                          unsigned int nreach, char *part)
>> +static int is_in_reachable(char **reachable, const char *usercon_str)
>>  {
>> -       const char *conrole, *contype;
>> -       char *partrole, *parttype, *ptr;
>> -       context_t con;
>> -       unsigned int i;
>> +       if (!reachable)
>> +               return 0;
>>
>> -       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;
>> +       for (; *reachable != NULL; reachable++) {
>> +               if (strcmp(*reachable, usercon_str) == 0) {
>> +                       return 1;
>>                 }
>> -               context_free(con);
>>         }
>> -
>> -       return -1;
>> +       return 0;
>>  }
>>
>> -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;
>
> This is not your bug, but this should be just "errno = EINVAL". Should
> probably be fixed in another patch...
>
>> @@ -180,6 +158,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 +222,84 @@ 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) + 2) {
>> +                       fprintf(stderr, "%s: one of partial contexts is too big\n", __FUNCTION__);
>> +                       errno = EINVAL;
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>> +               usercon_len = user_len + strlen(start) + 2;
>> +               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, "%s:%s", user, start);
>> +               usercon = context_new(usercon_str);
>> +               if (!usercon) {
>> +                       if (errno != EINVAL) {
>> +                               free(usercon_str);
>> +                               rc = -1;
>> +                               goto out;
>> +                       }
>> +                       fprintf(stderr,
>> +                               "%s: can't create a context from %s, skipping\n",
>> +                               __FUNCTION__, usercon_str);
>> +                       free(usercon_str);
>>                         start = end;
>>                         continue;
>>                 }
>> +               free(usercon_str);
>> +               if (context_range_set(usercon, fromlevel) != 0) {
>> +                       context_free(usercon);
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>> +               usercon_str = context_str(usercon);
>
> You can do context_free(usercon) right here (it's not used beyond this
> line) and avoid doing it in all the paths after here.
>
>> +               if (!usercon_str) {
>> +                       context_free(usercon);
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>>
>> -               /* 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)++;
>> +               /* check whether usercon is already in reachable */
>> +               if (is_in_reachable(*reachable, usercon_str)) {
>> +                       context_free(usercon);
>> +                       start = end;
>> +                       continue;
>> +               }
>> +               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) {
>
> Unless I'm mistaken, you should free new_reachable here, otherwise it leaks.
>
>> +                               context_free(usercon);
>> +                               rc = -1;
>> +                               goto out;
>> +                       }
>> +                       new_reachable[*nreachable + 1] = 0;
>> +                       *reachable = new_reachable;
>> +                       *nreachable += 1;
>> +               }
>> +               context_free(usercon);
>>                 start = end;
>>         }
>> -
>>         rc = 0;
>>
>>        out:
>> @@ -313,21 +353,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 +420,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 +435,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 +445,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 +459,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 +467,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);
>>
>> --
>> 2.25.0
>>


-- 
()  ascii ribbon campaign - against html e-mail 
/\  www.asciiribbon.org   - against proprietary attachments


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

* Re: [PATCH v5] libselinux: Eliminate use of security_compute_user()
  2020-02-11 21:19 ` Ondrej Mosnacek
  2020-02-12 10:13   ` Petr Lautrbach
@ 2020-02-12 10:41   ` Petr Lautrbach
  1 sibling, 0 replies; 6+ messages in thread
From: Petr Lautrbach @ 2020-02-12 10:41 UTC (permalink / raw)
  To: Ondrej Mosnacek; +Cc: Petr Lautrbach, SElinux list


Ondrej Mosnacek <omosnace@redhat.com> writes:

> I found one memory leak and one style nit, please see below. Otherwise
> the patch looks good to me (but I only checked the code flow - I don't
> understand the whole context well enough to fully judge its
> correctness).
>
> On Tue, Feb 11, 2020 at 11:14 AM Petr Lautrbach <plautrba@redhat.com> 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>
>> ---
>>
>> v5 changes:
>>
>> - context_free(usercon) when is_in_reachable() finds a duplicate
>>
>> I see some value in reporting problem context during parsing a file and skipping this context
>> so I left the code after usercon = context_new(usercon_str) untouched.
>>
>>
>>  libselinux/src/get_context_list.c | 214 ++++++++++++++----------------
>>  1 file changed, 98 insertions(+), 116 deletions(-)
>>
>> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
>> index 689e46589f30..6078d980cde1 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,38 @@ int get_default_context(const char *user,
>>         return 0;
>>  }
>>
>> -static int find_partialcon(char ** list,
>> -                          unsigned int nreach, char *part)
>> +static int is_in_reachable(char **reachable, const char *usercon_str)
>>  {
>> -       const char *conrole, *contype;
>> -       char *partrole, *parttype, *ptr;
>> -       context_t con;
>> -       unsigned int i;
>> +       if (!reachable)
>> +               return 0;
>>
>> -       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;
>> +       for (; *reachable != NULL; reachable++) {
>> +               if (strcmp(*reachable, usercon_str) == 0) {
>> +                       return 1;
>>                 }
>> -               context_free(con);
>>         }
>> -
>> -       return -1;
>> +       return 0;
>>  }
>>
>> -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;
>
> This is not your bug, but this should be just "errno = EINVAL". Should
> probably be fixed in another patch...
>
>> @@ -180,6 +158,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 +222,84 @@ 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) + 2) {
>> +                       fprintf(stderr, "%s: one of partial contexts is too big\n", __FUNCTION__);
>> +                       errno = EINVAL;
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>> +               usercon_len = user_len + strlen(start) + 2;
>> +               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, "%s:%s", user, start);
>> +               usercon = context_new(usercon_str);
>> +               if (!usercon) {
>> +                       if (errno != EINVAL) {
>> +                               free(usercon_str);
>> +                               rc = -1;
>> +                               goto out;
>> +                       }
>> +                       fprintf(stderr,
>> +                               "%s: can't create a context from %s, skipping\n",
>> +                               __FUNCTION__, usercon_str);
>> +                       free(usercon_str);
>>                         start = end;
>>                         continue;
>>                 }
>> +               free(usercon_str);
>> +               if (context_range_set(usercon, fromlevel) != 0) {
>> +                       context_free(usercon);
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>> +               usercon_str = context_str(usercon);
>
> You can do context_free(usercon) right here (it's not used beyond this
> line) and avoid doing it in all the paths after here.

It's used indirectly via usercon_str which points to
usercon->ptr->current_str which is destroyed by context_free(usercon)


>> +               if (!usercon_str) {
>> +                       context_free(usercon);
>> +                       rc = -1;
>> +                       goto out;
>> +               }
>>
>> -               /* 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)++;
>> +               /* check whether usercon is already in reachable */
>> +               if (is_in_reachable(*reachable, usercon_str)) {
>> +                       context_free(usercon);
>> +                       start = end;
>> +                       continue;
>> +               }
>> +               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) {
>
> Unless I'm mistaken, you should free new_reachable here, otherwise it
> leaks.

free new_reachable would destroy already found reachable's. But
*reachable should be updated to new_reachable at this point as the
original reachable is destroyed at this moment.


>
>> +                               context_free(usercon);
>> +                               rc = -1;
>> +                               goto out;
>> +                       }
>> +                       new_reachable[*nreachable + 1] = 0;
>> +                       *reachable = new_reachable;
>> +                       *nreachable += 1;
>> +               }
>> +               context_free(usercon);
>>                 start = end;
>>         }
>> -
>>         rc = 0;
>>
>>        out:
>> @@ -313,21 +353,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 +420,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 +435,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 +445,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 +459,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 +467,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);
>>
>> --
>> 2.25.0
>>


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

* Re: [PATCH v5] libselinux: Eliminate use of security_compute_user()
  2020-02-11 10:14 [PATCH v5] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
  2020-02-11 15:04 ` Stephen Smalley
  2020-02-11 21:19 ` Ondrej Mosnacek
@ 2020-02-12 16:01 ` Stephen Smalley
  2 siblings, 0 replies; 6+ messages in thread
From: Stephen Smalley @ 2020-02-12 16:01 UTC (permalink / raw)
  To: Petr Lautrbach, selinux

On 2/11/20 5:14 AM, 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>
> ---

> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 689e46589f30..6078d980cde1 100644
> --- a/libselinux/src/get_context_list.c
> +++ b/libselinux/src/get_context_list.c
> @@ -463,40 +467,18 @@ int get_ordered_context_list(const char *user,
>   				__FUNCTION__, selinux_default_context_path());
>   			/* Fall through */
>   		}
> -		rc = 0;
> +		rc = nreachable;

Shouldn't we do this outside the if (fp) statement?  Otherwise, if we 
got some reachable contexts from the per-user contexts file but the 
global default_contexts file was missing, we'll end up freeing the 
reachable contexts and returning 0 on the out path.

>   	}
>   
> -	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);
>   
> 


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

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-11 10:14 [PATCH v5] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
2020-02-11 15:04 ` Stephen Smalley
2020-02-11 21:19 ` Ondrej Mosnacek
2020-02-12 10:13   ` Petr Lautrbach
2020-02-12 10:41   ` Petr Lautrbach
2020-02-12 16:01 ` Stephen Smalley

SELinux Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/selinux/0 selinux/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 selinux selinux/ https://lore.kernel.org/selinux \
		selinux@vger.kernel.org
	public-inbox-index selinux

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.selinux


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git