| Message ID | 20200211101438.403297-1-plautrba@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v5] libselinux: Eliminate use of security_compute_user() | expand |
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...
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@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 >>
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 >>
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); > >
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);
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(-)