On Mon, Sep 16, 2019 at 4:01 PM Stephen Smalley <sds@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
> > ---
> > 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.
--
Mike Palmiotto
https://crunchydata.com
