selinux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] libselinux: Eliminate use of security_compute_user()
@ 2019-05-09  8:42 Petr Lautrbach
  2019-05-10  1:44 ` William Roberts
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Petr Lautrbach @ 2019-05-09  8:42 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>
---
 libselinux/src/get_context_list.c | 185 ++++++++++--------------------
 1 file changed, 60 insertions(+), 125 deletions(-)

diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
index 689e4658..a36c6253 100644
--- a/libselinux/src/get_context_list.c
+++ b/libselinux/src/get_context_list.c
@@ -114,61 +114,24 @@ 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;
-	ssize_t len;
+	ssize_t len, ulen;
 	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,7 +143,8 @@ static int get_context_order(FILE * fp,
 		return -1;
 	fromrole = context_role_get(con);
 	fromtype = context_type_get(con);
-	if (!fromrole || !fromtype) {
+	fromlevel = context_range_get(con);
+	if (!fromrole || !fromtype || !fromlevel) {
 		context_free(con);
 		return -1;
 	}
@@ -243,23 +207,52 @@ 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. */
-			start = end;
-			continue;
+		/* Check whether a new context is valid */
+	        ulen = strlen(user) + strlen(start) + 1;
+		usercon_str = malloc(ulen);
+		if (!usercon_str) {
+			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)++;
+		/* set range from fromcon in the new usercon */
+		snprintf(usercon_str, ulen - 1, "%s:%s", user, start);
+		if (!(usercon = context_new(usercon_str))) {
+			fprintf(stderr,
+				"%s: can't create a context from %s\n",
+				__FUNCTION__, usercon_str);
+			free(usercon_str);
+
+			continue;
+		}
+		free(usercon_str);
+		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);
+			new_reachable[*nreachable + 1] = 0;
+			*reachable = new_reachable;
+			*nreachable += 1;
+		}
+		context_free(usercon);
 		start = end;
 	}
-
 	rc = 0;
 
       out:
@@ -313,21 +306,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 +373,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 +388,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 +398,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 +412,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 +420,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.21.0


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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2019-05-09  8:42 [PATCH] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
@ 2019-05-10  1:44 ` William Roberts
  2019-05-10  1:45   ` William Roberts
  2019-05-10 14:20   ` Stephen Smalley
  2019-05-10 14:11 ` Stephen Smalley
  2020-01-22 13:56 ` Stephen Smalley
  2 siblings, 2 replies; 9+ messages in thread
From: William Roberts @ 2019-05-10  1:44 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Thu, May 9, 2019 at 1:43 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.

Whats the best way to test this?

>
> Fixes: https://github.com/SELinuxProject/selinux/issues/28
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> ---
>  libselinux/src/get_context_list.c | 185 ++++++++++--------------------
>  1 file changed, 60 insertions(+), 125 deletions(-)
>
> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 689e4658..a36c6253 100644
> --- a/libselinux/src/get_context_list.c
> +++ b/libselinux/src/get_context_list.c
> @@ -114,61 +114,24 @@ 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;
> -       ssize_t len;
> +       ssize_t len, ulen;
>         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,7 +143,8 @@ static int get_context_order(FILE * fp,
>                 return -1;
>         fromrole = context_role_get(con);
>         fromtype = context_type_get(con);
> -       if (!fromrole || !fromtype) {
> +       fromlevel = context_range_get(con);
> +       if (!fromrole || !fromtype || !fromlevel) {
>                 context_free(con);
>                 return -1;
>         }
> @@ -243,23 +207,52 @@ 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. */
> -                       start = end;
> -                       continue;
> +               /* Check whether a new context is valid */
> +               ulen = strlen(user) + strlen(start) + 1;

is their anything guaranteeing this doesn't overflow and result in
malloc(0) which is undefined?
also ulen is signed, why? All uses are where a size_t is valid, I
think it can change to a size_t.

> +               usercon_str = malloc(ulen);

spaces after tab, see:
https://github.com/SELinuxProject/selinux/pull/146/files#diff-55a16252233f48b841c06ffbe22bb66dR211

> +               if (!usercon_str) {
> +                       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)++;
> +               /* set range from fromcon in the new usercon */
> +               snprintf(usercon_str, ulen - 1, "%s:%s", user, start);
> +               if (!(usercon = context_new(usercon_str))) {

You can drop the too free(usercon_str) if you rework this logic:
usercon = context_new(usercon_str))
free(usercon_str);
if (!usercon) {
    // error
    continue;
}

> +                       fprintf(stderr,
> +                               "%s: can't create a context from %s\n",
> +                               __FUNCTION__, usercon_str);
> +                       free(usercon_str);
> +
> +                       continue;

Why is it OK to continue?

> +               }
> +               free(usercon_str);
> +               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);

oom error

> +                       new_reachable[*nreachable + 1] = 0;
> +                       *reachable = new_reachable;
> +                       *nreachable += 1;
> +               }
> +               context_free(usercon);
>                 start = end;
>         }
> -
>         rc = 0;
>
>        out:
> @@ -313,21 +306,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 +373,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 +388,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 +398,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 +412,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 +420,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.21.0
>

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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2019-05-10  1:44 ` William Roberts
@ 2019-05-10  1:45   ` William Roberts
  2019-05-10 14:20   ` Stephen Smalley
  1 sibling, 0 replies; 9+ messages in thread
From: William Roberts @ 2019-05-10  1:45 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux

On Thu, May 9, 2019 at 6:44 PM William Roberts <bill.c.roberts@gmail.com> wrote:
>
> On Thu, May 9, 2019 at 1:43 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.
>
> Whats the best way to test this?
>
> >
> > Fixes: https://github.com/SELinuxProject/selinux/issues/28
> >
> > Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
> > ---
> >  libselinux/src/get_context_list.c | 185 ++++++++++--------------------
> >  1 file changed, 60 insertions(+), 125 deletions(-)
> >
> > diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> > index 689e4658..a36c6253 100644
> > --- a/libselinux/src/get_context_list.c
> > +++ b/libselinux/src/get_context_list.c
> > @@ -114,61 +114,24 @@ 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;
> > -       ssize_t len;
> > +       ssize_t len, ulen;
> >         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,7 +143,8 @@ static int get_context_order(FILE * fp,
> >                 return -1;
> >         fromrole = context_role_get(con);
> >         fromtype = context_type_get(con);
> > -       if (!fromrole || !fromtype) {
> > +       fromlevel = context_range_get(con);
> > +       if (!fromrole || !fromtype || !fromlevel) {
> >                 context_free(con);
> >                 return -1;
> >         }
> > @@ -243,23 +207,52 @@ 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. */
> > -                       start = end;
> > -                       continue;
> > +               /* Check whether a new context is valid */
> > +               ulen = strlen(user) + strlen(start) + 1;
>
> is their anything guaranteeing this doesn't overflow and result in
> malloc(0) which is undefined?
> also ulen is signed, why? All uses are where a size_t is valid, I
> think it can change to a size_t.
>
> > +               usercon_str = malloc(ulen);
>
> spaces after tab, see:
> https://github.com/SELinuxProject/selinux/pull/146/files#diff-55a16252233f48b841c06ffbe22bb66dR211
>
> > +               if (!usercon_str) {
> > +                       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)++;
> > +               /* set range from fromcon in the new usercon */
> > +               snprintf(usercon_str, ulen - 1, "%s:%s", user, start);
> > +               if (!(usercon = context_new(usercon_str))) {
>
> You can drop the too free(usercon_str) if you rework this logic:
(sic) two not too
> usercon = context_new(usercon_str))
> free(usercon_str);
> if (!usercon) {
>     // error
>     continue;
> }
>
> > +                       fprintf(stderr,
> > +                               "%s: can't create a context from %s\n",
> > +                               __FUNCTION__, usercon_str);
> > +                       free(usercon_str);
> > +
> > +                       continue;
>
> Why is it OK to continue?
>
> > +               }
> > +               free(usercon_str);
> > +               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);
>
> oom error
>
> > +                       new_reachable[*nreachable + 1] = 0;
> > +                       *reachable = new_reachable;
> > +                       *nreachable += 1;
> > +               }
> > +               context_free(usercon);
> >                 start = end;
> >         }
> > -
> >         rc = 0;
> >
> >        out:
> > @@ -313,21 +306,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 +373,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 +388,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 +398,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 +412,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 +420,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.21.0
> >

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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2019-05-09  8:42 [PATCH] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
  2019-05-10  1:44 ` William Roberts
@ 2019-05-10 14:11 ` Stephen Smalley
  2019-05-16 15:07   ` Petr Lautrbach
  2020-01-22 13:56 ` Stephen Smalley
  2 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2019-05-10 14:11 UTC (permalink / raw)
  To: Petr Lautrbach, selinux, William Roberts, Christopher J. PeBenito

On 5/9/19 4:42 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>
> ---
>   libselinux/src/get_context_list.c | 185 ++++++++++--------------------
>   1 file changed, 60 insertions(+), 125 deletions(-)
> 
> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 689e4658..a36c6253 100644
> --- a/libselinux/src/get_context_list.c
> +++ b/libselinux/src/get_context_list.c
> @@ -114,61 +114,24 @@ 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;
> -	ssize_t len;
> +	ssize_t len, ulen;
>   	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,7 +143,8 @@ static int get_context_order(FILE * fp,
>   		return -1;
>   	fromrole = context_role_get(con);
>   	fromtype = context_type_get(con);
> -	if (!fromrole || !fromtype) {
> +	fromlevel = context_range_get(con);
> +	if (!fromrole || !fromtype || !fromlevel) {
>   		context_free(con);
>   		return -1;
>   	}
> @@ -243,23 +207,52 @@ 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. */
> -			start = end;
> -			continue;
> +		/* Check whether a new context is valid */
> +	        ulen = strlen(user) + strlen(start) + 1;
> +		usercon_str = malloc(ulen);
> +		if (!usercon_str) {
> +			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)++;
> +		/* set range from fromcon in the new usercon */
> +		snprintf(usercon_str, ulen - 1, "%s:%s", user, start);
> +		if (!(usercon = context_new(usercon_str))) {
> +			fprintf(stderr,
> +				"%s: can't create a context from %s\n",
> +				__FUNCTION__, usercon_str);
> +			free(usercon_str);
> +
> +			continue;
> +		}
> +		free(usercon_str);
> +		context_range_set(usercon, fromlevel);

I think the main potential stumbling block here is the MLS range 
component. The kernel policy defines the default level and allowed range 
for the (SELinux) user, and uses this information in the kernel function 
mls_setup_user_range(), 
https://elixir.bootlin.com/linux/latest/source/security/selinux/ss/mls.c#L402, 
to determine the most suitable MLS range for the user session, based on 
both the from-context and the user default and range from the kernel 
policy.  Just using the level from the from-context could fail if the 
user isn't authorized to operate at that level, and even if the user is 
authorized to operate at that level, it could introduce a change in the 
default behavior if the user's default level differs. I think when we 
have discussed this in the past on the list, we were going to either 
export the user's default level and range information from the kernel 
via selinuxfs and replicate the mls_setup_user_ranges() logic in 
userspace, or have it automatically extracted from the kernel policy 
during policy build into a userspace configuration file that could be 
used directly by userspace.  Or something like that.  This gets a bit 
tricky though in that the logic involves comparing MLS levels, which is 
intrinsically policy-specific logic, and thus if we wanted to truly 
replicate it in userspace, we'd probably need to use libsepol.  Ugh. 
Maybe the kernel could just provide a simple selinuxfs interface for 
computing the result of mls_setup_user_range() and return that piece.

That said, I don't know to what extent anyone is relying on this logic 
and to what extent it is obsoleted by the use of the level/range from 
seusers.  It looks like today we are replacing the level/range in the 
original from-context with the one from seusers before calling this 
code, in which case the fromlevel is in fact the one we ultimately want 
to use.  So perhaps this doesn't matter and we can just go with your 
approach.

> +		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);
> +			new_reachable[*nreachable + 1] = 0;
> +			*reachable = new_reachable;
> +			*nreachable += 1;
> +		}
> +		context_free(usercon);
>   		start = end;
>   	}
> -
>   	rc = 0;
>   
>         out:
> @@ -313,21 +306,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 +373,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 +388,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 +398,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 +412,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 +420,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);
>   
> 


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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2019-05-10  1:44 ` William Roberts
  2019-05-10  1:45   ` William Roberts
@ 2019-05-10 14:20   ` Stephen Smalley
  1 sibling, 0 replies; 9+ messages in thread
From: Stephen Smalley @ 2019-05-10 14:20 UTC (permalink / raw)
  To: William Roberts, Petr Lautrbach; +Cc: selinux

On 5/9/19 9:44 PM, William Roberts wrote:
> On Thu, May 9, 2019 at 1:43 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.
> 
> Whats the best way to test this?

libselinux has some test utilities to exercise these interfaces 
directly, e.g. getconlist, getdefaultcon, getseuser.  Fedora re-packages 
some of them under a more unique name, e.g. selinuxconlist, 
selinuxdefcon, as part of the libselinux-utils package.

For operational testing, you'd test that user logins, cron jobs, etc are 
correctly assigned contexts.  Preferably for users assigned different 
roles and ranges in seusers and/or kernel policy.  So adding some Linux 
users mapped to different SELinux users and ranges via semanage login -a 
and then logging in as them, creating cron jobs that report the context 
in which they are run, etc.

> 
>>
>> Fixes: https://github.com/SELinuxProject/selinux/issues/28
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>> ---
>>   libselinux/src/get_context_list.c | 185 ++++++++++--------------------
>>   1 file changed, 60 insertions(+), 125 deletions(-)
>>
>> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
>> index 689e4658..a36c6253 100644
>> --- a/libselinux/src/get_context_list.c
>> +++ b/libselinux/src/get_context_list.c
>> @@ -114,61 +114,24 @@ 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;
>> -       ssize_t len;
>> +       ssize_t len, ulen;
>>          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,7 +143,8 @@ static int get_context_order(FILE * fp,
>>                  return -1;
>>          fromrole = context_role_get(con);
>>          fromtype = context_type_get(con);
>> -       if (!fromrole || !fromtype) {
>> +       fromlevel = context_range_get(con);
>> +       if (!fromrole || !fromtype || !fromlevel) {
>>                  context_free(con);
>>                  return -1;
>>          }
>> @@ -243,23 +207,52 @@ 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. */
>> -                       start = end;
>> -                       continue;
>> +               /* Check whether a new context is valid */
>> +               ulen = strlen(user) + strlen(start) + 1;
> 
> is their anything guaranteeing this doesn't overflow and result in
> malloc(0) which is undefined?
> also ulen is signed, why? All uses are where a size_t is valid, I
> think it can change to a size_t.
> 
>> +               usercon_str = malloc(ulen);
> 
> spaces after tab, see:
> https://github.com/SELinuxProject/selinux/pull/146/files#diff-55a16252233f48b841c06ffbe22bb66dR211
> 
>> +               if (!usercon_str) {
>> +                       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)++;
>> +               /* set range from fromcon in the new usercon */
>> +               snprintf(usercon_str, ulen - 1, "%s:%s", user, start);
>> +               if (!(usercon = context_new(usercon_str))) {
> 
> You can drop the too free(usercon_str) if you rework this logic:
> usercon = context_new(usercon_str))
> free(usercon_str);
> if (!usercon) {
>      // error
>      continue;
> }
> 
>> +                       fprintf(stderr,
>> +                               "%s: can't create a context from %s\n",
>> +                               __FUNCTION__, usercon_str);
>> +                       free(usercon_str);
>> +
>> +                       continue;
> 
> Why is it OK to continue?
> 
>> +               }
>> +               free(usercon_str);
>> +               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);
> 
> oom error
> 
>> +                       new_reachable[*nreachable + 1] = 0;
>> +                       *reachable = new_reachable;
>> +                       *nreachable += 1;
>> +               }
>> +               context_free(usercon);
>>                  start = end;
>>          }
>> -
>>          rc = 0;
>>
>>         out:
>> @@ -313,21 +306,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 +373,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 +388,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 +398,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 +412,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 +420,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.21.0
>>


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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2019-05-10 14:11 ` Stephen Smalley
@ 2019-05-16 15:07   ` Petr Lautrbach
  2020-01-22 13:35     ` Stephen Smalley
  0 siblings, 1 reply; 9+ messages in thread
From: Petr Lautrbach @ 2019-05-16 15:07 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Petr Lautrbach, selinux, William Roberts, Christopher J. PeBenito


Stephen Smalley <sds@tycho.nsa.gov> writes:

> On 5/9/19 4:42 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>
>> ---
>>   libselinux/src/get_context_list.c | 185 
>>   ++++++++++--------------------
>>   1 file changed, 60 insertions(+), 125 deletions(-)
>>
>> diff --git a/libselinux/src/get_context_list.c 
>> b/libselinux/src/get_context_list.c
>> index 689e4658..a36c6253 100644
>> --- a/libselinux/src/get_context_list.c
>> +++ b/libselinux/src/get_context_list.c
>> @@ -114,61 +114,24 @@ 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;
>> -	ssize_t len;
>> +	ssize_t len, ulen;
>>   	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,7 +143,8 @@ static int get_context_order(FILE * fp,
>>   		return -1;
>>   	fromrole = context_role_get(con);
>>   	fromtype = context_type_get(con);
>> -	if (!fromrole || !fromtype) {
>> +	fromlevel = context_range_get(con);
>> +	if (!fromrole || !fromtype || !fromlevel) {
>>   		context_free(con);
>>   		return -1;
>>   	}
>> @@ -243,23 +207,52 @@ 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. */
>> -			start = end;
>> -			continue;
>> +		/* Check whether a new context is valid */
>> +	        ulen = strlen(user) + strlen(start) + 1;
>> +		usercon_str = malloc(ulen);
>> +		if (!usercon_str) {
>> +			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)++;
>> +		/* set range from fromcon in the new usercon */
>> +		snprintf(usercon_str, ulen - 1, "%s:%s", user, 
>> start);
>> +		if (!(usercon = context_new(usercon_str))) {
>> +			fprintf(stderr,
>> +				"%s: can't create a context from 
>> %s\n",
>> +				__FUNCTION__, usercon_str);
>> +			free(usercon_str);
>> +
>> +			continue;
>> +		}
>> +		free(usercon_str);
>> +		context_range_set(usercon, fromlevel);
>
> I think the main potential stumbling block here is the MLS range 
> component. The
> kernel policy defines the default level and allowed range for 
> the (SELinux)
> user, and uses this information in the kernel function 
> mls_setup_user_range(),
> https://elixir.bootlin.com/linux/latest/source/security/selinux/ss/mls.c#L402,
> to determine the most suitable MLS range for the user session, 
> based on both the
> from-context and the user default and range from the kernel 
> policy.  Just using
> the level from the from-context could fail if the user isn't 
> authorized to
> operate at that level, and even if the user is authorized to 
> operate at that
> level, it could introduce a change in the default behavior if 
> the user's default
> level differs. I think when we have discussed this in the past 
> on the list, we
> were going to either export the user's default level and range 
> information from
> the kernel via selinuxfs and replicate the 
> mls_setup_user_ranges() logic in
> userspace, or have it automatically extracted from the kernel 
> policy during
> policy build into a userspace configuration file that could be 
> used directly by
> userspace.  Or something like that.  This gets a bit tricky 
> though in that the
> logic involves comparing MLS levels, which is intrinsically 
> policy-specific
> logic, and thus if we wanted to truly replicate it in userspace, 
> we'd probably
> need to use libsepol.  Ugh. Maybe the kernel could just provide 
> a simple
> selinuxfs interface for computing the result of 
> mls_setup_user_range() and
> return that piece.
>
> That said, I don't know to what extent anyone is relying on this 
> logic and to
> what extent it is obsoleted by the use of the level/range from 
> seusers.  It
> looks like today we are replacing the level/range in the 
> original from-context
> with the one from seusers before calling this code, in which 
> case the fromlevel
> is in fact the one we ultimately want to use.  So perhaps this 
> doesn't matter
> and we can just go with your approach.

The problem is much complicated than I originally thought and this
patch changes the behavior of get_ordered_context_list what is 
probably
not acceptable.

I'll do more tests and think about it the light of new (for me)
information.

Thanks all for reviews and inputs.

Petr


>> +		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);
>> +			new_reachable[*nreachable + 1] = 0;
>> +			*reachable = new_reachable;
>> +			*nreachable += 1;
>> +		}
>> +		context_free(usercon);
>>   		start = end;
>>   	}
>> -
>>   	rc = 0;
>>           out:
>> @@ -313,21 +306,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 +373,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 +388,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 +398,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 +412,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 +420,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);
>>   
>>


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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2019-05-16 15:07   ` Petr Lautrbach
@ 2020-01-22 13:35     ` Stephen Smalley
  2020-01-23  8:44       ` Petr Lautrbach
  0 siblings, 1 reply; 9+ messages in thread
From: Stephen Smalley @ 2020-01-22 13:35 UTC (permalink / raw)
  To: Petr Lautrbach; +Cc: selinux, William Roberts, Christopher J. PeBenito

On 5/16/19 11:07 AM, Petr Lautrbach wrote:
> 
> Stephen Smalley <sds@tycho.nsa.gov> writes:
> 
>> On 5/9/19 4:42 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>
>>> ---
<snip>
>> I think the main potential stumbling block here is the MLS range 
>> component. The
>> kernel policy defines the default level and allowed range for the 
>> (SELinux)
>> user, and uses this information in the kernel function 
>> mls_setup_user_range(),
>> https://elixir.bootlin.com/linux/latest/source/security/selinux/ss/mls.c#L402, 
>>
>> to determine the most suitable MLS range for the user session, based 
>> on both the
>> from-context and the user default and range from the kernel policy.  
>> Just using
>> the level from the from-context could fail if the user isn't 
>> authorized to
>> operate at that level, and even if the user is authorized to operate 
>> at that
>> level, it could introduce a change in the default behavior if the 
>> user's default
>> level differs. I think when we have discussed this in the past on the 
>> list, we
>> were going to either export the user's default level and range 
>> information from
>> the kernel via selinuxfs and replicate the mls_setup_user_ranges() 
>> logic in
>> userspace, or have it automatically extracted from the kernel policy 
>> during
>> policy build into a userspace configuration file that could be used 
>> directly by
>> userspace.  Or something like that.  This gets a bit tricky though in 
>> that the
>> logic involves comparing MLS levels, which is intrinsically 
>> policy-specific
>> logic, and thus if we wanted to truly replicate it in userspace, we'd 
>> probably
>> need to use libsepol.  Ugh. Maybe the kernel could just provide a simple
>> selinuxfs interface for computing the result of mls_setup_user_range() 
>> and
>> return that piece.
>>
>> That said, I don't know to what extent anyone is relying on this logic 
>> and to
>> what extent it is obsoleted by the use of the level/range from 
>> seusers.  It
>> looks like today we are replacing the level/range in the original 
>> from-context
>> with the one from seusers before calling this code, in which case the 
>> fromlevel
>> is in fact the one we ultimately want to use.  So perhaps this doesn't 
>> matter
>> and we can just go with your approach.
> 
> The problem is much complicated than I originally thought and this
> patch changes the behavior of get_ordered_context_list what is probably
> not acceptable.
> 
> I'll do more tests and think about it the light of new (for me)
> information.
> 
> Thanks all for reviews and inputs.

I would like to re-visit this patch again.  I did some looking at how 
get_ordered_context_list() and its variant interfaces are currently 
being used by callers, and at the internal logic of 
get_ordered_context_list() in userspace and mls_setup_user_ranges() in 
the kernel.  Since we are already substituting the range/level from 
seusers into the from-context before calling security_compute_user(), 
and since the only sensible configuration of seusers would be to use a 
range/level that falls within (or is identical to) the SELinux user's 
authorized range, I don't think your patch is likely to break anything. 
There are corner cases where it could yield a different result but I 
would be surprised if such corner cases are in real use and arguably 
they would be configuration errors.  Consequently, I think we should 
refresh your patch, address any comments made on it previously, and 
submit it for merging and try it out.  If we encounter any real world 
breakage from it, we can consider adding a new selinuxfs node that 
exports the kernel's mls_setup_user_ranges() logic and rework 
get_ordered_context_list() to use that to obtain the MLS portion of the 
context, but I don't think it is worth doing that without a real example 
where simply applying your patch breaks something.  Thoughts?

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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2019-05-09  8:42 [PATCH] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
  2019-05-10  1:44 ` William Roberts
  2019-05-10 14:11 ` Stephen Smalley
@ 2020-01-22 13:56 ` Stephen Smalley
  2 siblings, 0 replies; 9+ messages in thread
From: Stephen Smalley @ 2020-01-22 13:56 UTC (permalink / raw)
  To: Petr Lautrbach, selinux, William Roberts

On 5/9/19 4:42 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>
> ---
>   libselinux/src/get_context_list.c | 185 ++++++++++--------------------
>   1 file changed, 60 insertions(+), 125 deletions(-)
> 
> diff --git a/libselinux/src/get_context_list.c b/libselinux/src/get_context_list.c
> index 689e4658..a36c6253 100644
> --- a/libselinux/src/get_context_list.c
> +++ b/libselinux/src/get_context_list.c
> @@ -180,7 +143,8 @@ static int get_context_order(FILE * fp,
>   		return -1;
>   	fromrole = context_role_get(con);
>   	fromtype = context_type_get(con);
> -	if (!fromrole || !fromtype) {
> +	fromlevel = context_range_get(con);
> +	if (!fromrole || !fromtype || !fromlevel) {
>   		context_free(con);
>   		return -1;
>   	}

One caveat here: MLS is still optional for SELinux and IIRC Gentoo 
doesn't enable it, so the from-context may not have any level and 
context_range_get() can legitimately return NULL in that case. 
context_range_set(con, NULL) is also legitimate and won't cause any 
errors.  So you don't need to check that fromlevel is non-NULL as long 
as you are only using it later in context_range_set().



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

* Re: [PATCH] libselinux: Eliminate use of security_compute_user()
  2020-01-22 13:35     ` Stephen Smalley
@ 2020-01-23  8:44       ` Petr Lautrbach
  0 siblings, 0 replies; 9+ messages in thread
From: Petr Lautrbach @ 2020-01-23  8:44 UTC (permalink / raw)
  To: Stephen Smalley
  Cc: Petr Lautrbach, selinux, William Roberts, Christopher J. PeBenito


Stephen Smalley <sds@tycho.nsa.gov> writes:

> On 5/16/19 11:07 AM, Petr Lautrbach wrote:
>>
>> Stephen Smalley <sds@tycho.nsa.gov> writes:
>>
>>> On 5/9/19 4:42 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>
>>>> ---
> <snip>
>>> I think the main potential stumbling block here is the MLS range component.
>>> The
>>> kernel policy defines the default level and allowed range for the (SELinux)
>>> user, and uses this information in the kernel function
>>> mls_setup_user_range(),
>>> https://elixir.bootlin.com/linux/latest/source/security/selinux/ss/mls.c#L402, 
>>>
>>> to determine the most suitable MLS range for the user session, based on both
>>> the
>>> from-context and the user default and range from the kernel policy.  Just
>>> using
>>> the level from the from-context could fail if the user isn't authorized to
>>> operate at that level, and even if the user is authorized to operate at that
>>> level, it could introduce a change in the default behavior if the user's
>>> default
>>> level differs. I think when we have discussed this in the past on the list,
>>> we
>>> were going to either export the user's default level and range information
>>> from
>>> the kernel via selinuxfs and replicate the mls_setup_user_ranges() logic in
>>> userspace, or have it automatically extracted from the kernel policy during
>>> policy build into a userspace configuration file that could be used directly
>>> by
>>> userspace.  Or something like that.  This gets a bit tricky though in that
>>> the
>>> logic involves comparing MLS levels, which is intrinsically policy-specific
>>> logic, and thus if we wanted to truly replicate it in userspace, we'd
>>> probably
>>> need to use libsepol.  Ugh. Maybe the kernel could just provide a simple
>>> selinuxfs interface for computing the result of mls_setup_user_range() and
>>> return that piece.
>>>
>>> That said, I don't know to what extent anyone is relying on this logic and to
>>> what extent it is obsoleted by the use of the level/range from seusers.  It
>>> looks like today we are replacing the level/range in the original
>>> from-context
>>> with the one from seusers before calling this code, in which case the
>>> fromlevel
>>> is in fact the one we ultimately want to use.  So perhaps this doesn't matter
>>> and we can just go with your approach.
>>
>> The problem is much complicated than I originally thought and this
>> patch changes the behavior of get_ordered_context_list what is probably
>> not acceptable.
>>
>> I'll do more tests and think about it the light of new (for me)
>> information.
>>
>> Thanks all for reviews and inputs.
>
> I would like to re-visit this patch again.  I did some looking at how
> get_ordered_context_list() and its variant interfaces are currently being used
> by callers, and at the internal logic of get_ordered_context_list() in userspace
> and mls_setup_user_ranges() in the kernel.  Since we are already substituting
> the range/level from seusers into the from-context before calling
> security_compute_user(), and since the only sensible configuration of seusers
> would be to use a range/level that falls within (or is identical to) the SELinux
> user's authorized range, I don't think your patch is likely to break anything.
> There are corner cases where it could yield a different result but I would be
> surprised if such corner cases are in real use and arguably they would be
> configuration errors.  Consequently, I think we should refresh your patch,
> address any comments made on it previously, and submit it for merging and try it
> out.  If we encounter any real world breakage from it, we can consider adding a
> new selinuxfs node that exports the kernel's mls_setup_user_ranges() logic and
> rework get_ordered_context_list() to use that to obtain the MLS portion of the
> context, but I don't think it is worth doing that without a real example where
> simply applying your patch breaks something.  Thoughts?


No objection at the moment. But it'll take me few days, we're kind of busy
when it's about https://www.devconf.info/cz/


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

end of thread, other threads:[~2020-01-23  8:44 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-09  8:42 [PATCH] libselinux: Eliminate use of security_compute_user() Petr Lautrbach
2019-05-10  1:44 ` William Roberts
2019-05-10  1:45   ` William Roberts
2019-05-10 14:20   ` Stephen Smalley
2019-05-10 14:11 ` Stephen Smalley
2019-05-16 15:07   ` Petr Lautrbach
2020-01-22 13:35     ` Stephen Smalley
2020-01-23  8:44       ` Petr Lautrbach
2020-01-22 13:56 ` Stephen Smalley

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).