| Message ID | 20190916203015.19474-1-mike.palmiotto@crunchydata.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] libselinux: fix string conversion of unknown perms | expand |
On 9/16/19 4:30 PM, Mike Palmiotto wrote: > Commit c19395d72295f5e69275d98df5db22dfdf214b6c fixed some handling of unknown > classes/permissions, but missed the case where an unknown permission is loaded > and then subsequently logged, either via denial or auditallow. If a permission > set has some valid values mixed with unknown values, say `{ read write foo }`, > a check on `{ read write foo }` would fail to log the entire set. > > To fix this, skip over the bad permissions/classes when expanding them to > strings. The unknowns should be logged during `selinux_set_mapping`, so > there is no need for further logging of the actual unknown permissions. It seems like we have a similar pattern in print_access_vector(), avc_dump_av(); if security_av_perm_to_string() returns NULL, we break out of the loops and skip the remaining bits. We do however log the hex value of the any remaining bits set in the av, so we don't lose all information. I think we can still take this patch as is but was wondering whether the other cases ought to be similarly handled? > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com> > --- > libselinux/src/stringrep.c | 24 ++++++++---------------- > 1 file changed, 8 insertions(+), 16 deletions(-) > > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c > index ad29f76d..b9bf3ee6 100644 > --- a/libselinux/src/stringrep.c > +++ b/libselinux/src/stringrep.c > @@ -268,7 +268,7 @@ const char *security_av_perm_to_string(security_class_t tclass, > > int security_av_string(security_class_t tclass, access_vector_t av, char **res) > { > - unsigned int i = 0; > + unsigned int i; > size_t len = 5; > access_vector_t tmp = av; > int rc = 0; > @@ -276,19 +276,12 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res) > char *ptr; > > /* first pass computes the required length */ > - while (tmp) { > + for (i = 0; tmp; tmp >>= 1, i++) { > if (tmp & 1) { > str = security_av_perm_to_string(tclass, av & (1<<i)); > if (str) > len += strlen(str) + 1; > - else { > - rc = -1; > - errno = EINVAL; > - goto out; > - } > } > - tmp >>= 1; > - i++; > } > > *res = malloc(len); > @@ -298,7 +291,6 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res) > } > > /* second pass constructs the string */ > - i = 0; > tmp = av; > ptr = *res; > > @@ -308,12 +300,12 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res) > } > > ptr += sprintf(ptr, "{ "); > - while (tmp) { > - if (tmp & 1) > - ptr += sprintf(ptr, "%s ", security_av_perm_to_string( > - tclass, av & (1<<i))); > - tmp >>= 1; > - i++; > + for (i = 0; tmp; tmp >>= 1, i++) { > + if (tmp & 1) { > + str = security_av_perm_to_string(tclass, av & (1<<i)); > + if (str) > + ptr += sprintf(ptr, "%s ", str); > + } > } > sprintf(ptr, "}"); > out: >
On 9/18/19 8:19 AM, Stephen Smalley wrote: > On 9/16/19 4:30 PM, Mike Palmiotto wrote: >> Commit c19395d72295f5e69275d98df5db22dfdf214b6c fixed some handling of >> unknown >> classes/permissions, but missed the case where an unknown permission >> is loaded >> and then subsequently logged, either via denial or auditallow. If a >> permission >> set has some valid values mixed with unknown values, say `{ read write >> foo }`, >> a check on `{ read write foo }` would fail to log the entire set. >> >> To fix this, skip over the bad permissions/classes when expanding them to >> strings. The unknowns should be logged during `selinux_set_mapping`, so >> there is no need for further logging of the actual unknown permissions. > > It seems like we have a similar pattern in print_access_vector(), > avc_dump_av(); if security_av_perm_to_string() returns NULL, we break > out of the loops and skip the remaining bits. We do however log the hex > value of the any remaining bits set in the av, so we don't lose all > information. I think we can still take this patch as is but was > wondering whether the other cases ought to be similarly handled? I have merged this patch now. > >> >> Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com> >> --- >> libselinux/src/stringrep.c | 24 ++++++++---------------- >> 1 file changed, 8 insertions(+), 16 deletions(-) >> >> diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c >> index ad29f76d..b9bf3ee6 100644 >> --- a/libselinux/src/stringrep.c >> +++ b/libselinux/src/stringrep.c >> @@ -268,7 +268,7 @@ const char >> *security_av_perm_to_string(security_class_t tclass, >> int security_av_string(security_class_t tclass, access_vector_t av, >> char **res) >> { >> - unsigned int i = 0; >> + unsigned int i; >> size_t len = 5; >> access_vector_t tmp = av; >> int rc = 0; >> @@ -276,19 +276,12 @@ int security_av_string(security_class_t tclass, >> access_vector_t av, char **res) >> char *ptr; >> /* first pass computes the required length */ >> - while (tmp) { >> + for (i = 0; tmp; tmp >>= 1, i++) { >> if (tmp & 1) { >> str = security_av_perm_to_string(tclass, av & (1<<i)); >> if (str) >> len += strlen(str) + 1; >> - else { >> - rc = -1; >> - errno = EINVAL; >> - goto out; >> - } >> } >> - tmp >>= 1; >> - i++; >> } >> *res = malloc(len); >> @@ -298,7 +291,6 @@ int security_av_string(security_class_t tclass, >> access_vector_t av, char **res) >> } >> /* second pass constructs the string */ >> - i = 0; >> tmp = av; >> ptr = *res; >> @@ -308,12 +300,12 @@ int security_av_string(security_class_t tclass, >> access_vector_t av, char **res) >> } >> ptr += sprintf(ptr, "{ "); >> - while (tmp) { >> - if (tmp & 1) >> - ptr += sprintf(ptr, "%s ", security_av_perm_to_string( >> - tclass, av & (1<<i))); >> - tmp >>= 1; >> - i++; >> + for (i = 0; tmp; tmp >>= 1, i++) { >> + if (tmp & 1) { >> + str = security_av_perm_to_string(tclass, av & (1<<i)); >> + if (str) >> + ptr += sprintf(ptr, "%s ", str); >> + } >> } >> sprintf(ptr, "}"); >> out: >> >
diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c index ad29f76d..b9bf3ee6 100644 --- a/libselinux/src/stringrep.c +++ b/libselinux/src/stringrep.c @@ -268,7 +268,7 @@ const char *security_av_perm_to_string(security_class_t tclass, int security_av_string(security_class_t tclass, access_vector_t av, char **res) { - unsigned int i = 0; + unsigned int i; size_t len = 5; access_vector_t tmp = av; int rc = 0; @@ -276,19 +276,12 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res) char *ptr; /* first pass computes the required length */ - while (tmp) { + for (i = 0; tmp; tmp >>= 1, i++) { if (tmp & 1) { str = security_av_perm_to_string(tclass, av & (1<<i)); if (str) len += strlen(str) + 1; - else { - rc = -1; - errno = EINVAL; - goto out; - } } - tmp >>= 1; - i++; } *res = malloc(len); @@ -298,7 +291,6 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res) } /* second pass constructs the string */ - i = 0; tmp = av; ptr = *res; @@ -308,12 +300,12 @@ int security_av_string(security_class_t tclass, access_vector_t av, char **res) } ptr += sprintf(ptr, "{ "); - while (tmp) { - if (tmp & 1) - ptr += sprintf(ptr, "%s ", security_av_perm_to_string( - tclass, av & (1<<i))); - tmp >>= 1; - i++; + for (i = 0; tmp; tmp >>= 1, i++) { + if (tmp & 1) { + str = security_av_perm_to_string(tclass, av & (1<<i)); + if (str) + ptr += sprintf(ptr, "%s ", str); + } } sprintf(ptr, "}"); out:
Commit c19395d72295f5e69275d98df5db22dfdf214b6c fixed some handling of unknown classes/permissions, but missed the case where an unknown permission is loaded and then subsequently logged, either via denial or auditallow. If a permission set has some valid values mixed with unknown values, say `{ read write foo }`, a check on `{ read write foo }` would fail to log the entire set. To fix this, skip over the bad permissions/classes when expanding them to strings. The unknowns should be logged during `selinux_set_mapping`, so there is no need for further logging of the actual unknown permissions. Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com> --- libselinux/src/stringrep.c | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-)