Message ID | 20190910195353.973-1-mike.palmiotto@crunchydata.com (mailing list archive) |
---|---|
State | Changes Requested |
Headers | show |
Series | libselinux: fix string conversion of unknown perms | expand |
On 9/10/19 3:53 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. > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com> > --- > libselinux/src/stringrep.c | 28 ++++++++++++---------------- > 1 file changed, 12 insertions(+), 16 deletions(-) > > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c > index ad29f76d..85579422 100644 > --- a/libselinux/src/stringrep.c > +++ b/libselinux/src/stringrep.c > @@ -276,19 +276,15 @@ 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++) { Remove the redundant initialization in the declaration now that you are doing it here (which is better, I agree). > 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; > + if (!str) { > + continue; > } No need to bracket it when it is a single statement. > + > + len += strlen(str) + 1; Might be clearer as: if (str) len += strlen(str) + 1; And just let it fall through to the end of the loop otherwise - no need for explicit continue here. > } > - tmp >>= 1; > - i++; > } > > *res = malloc(len); > @@ -298,7 +294,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 +303,13 @@ 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); > + } No need for { } around a single statement. > + } > } > sprintf(ptr, "}"); > out: >
On Mon, Sep 16, 2019 at 4:01 PM Stephen Smalley <sds@tycho.nsa.gov> wrote: > > On 9/10/19 3:53 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. > > > > Signed-off-by: Mike Palmiotto <mike.palmiotto@crunchydata.com> > > --- > > libselinux/src/stringrep.c | 28 ++++++++++++---------------- > > 1 file changed, 12 insertions(+), 16 deletions(-) > > > > diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c > > index ad29f76d..85579422 100644 > > --- a/libselinux/src/stringrep.c > > +++ b/libselinux/src/stringrep.c > > @@ -276,19 +276,15 @@ 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++) { > > Remove the redundant initialization in the declaration now that you are > doing it here (which is better, I agree). > > > 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; > > + if (!str) { > > + continue; > > } > > No need to bracket it when it is a single statement. > > > + > > + len += strlen(str) + 1; > > Might be clearer as: > if (str) > len += strlen(str) + 1; > And just let it fall through to the end of the loop otherwise - no need > for explicit continue here. > > > } > > - tmp >>= 1; > > - i++; > > } > > > > *res = malloc(len); > > @@ -298,7 +294,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 +303,13 @@ 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); > > + } > > No need for { } around a single statement. > > > + } > > } > > sprintf(ptr, "}"); > > out: > > > Thanks for the review. Fixed all of the above in v2.
diff --git a/libselinux/src/stringrep.c b/libselinux/src/stringrep.c index ad29f76d..85579422 100644 --- a/libselinux/src/stringrep.c +++ b/libselinux/src/stringrep.c @@ -276,19 +276,15 @@ 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; + if (!str) { + continue; } + + len += strlen(str) + 1; } - tmp >>= 1; - i++; } *res = malloc(len); @@ -298,7 +294,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 +303,13 @@ 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 | 28 ++++++++++++---------------- 1 file changed, 12 insertions(+), 16 deletions(-)