* [PATCH] libselinux: fix string conversion of unknown perms
@ 2019-09-10 19:53 Mike Palmiotto
2019-09-16 20:01 ` Stephen Smalley
0 siblings, 1 reply; 3+ messages in thread
From: Mike Palmiotto @ 2019-09-10 19:53 UTC (permalink / raw)
To: selinux; +Cc: Mike Palmiotto
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++) {
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:
--
2.21.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] libselinux: fix string conversion of unknown perms
2019-09-10 19:53 [PATCH] libselinux: fix string conversion of unknown perms Mike Palmiotto
@ 2019-09-16 20:01 ` Stephen Smalley
2019-09-17 13:20 ` Mike Palmiotto
0 siblings, 1 reply; 3+ messages in thread
From: Stephen Smalley @ 2019-09-16 20:01 UTC (permalink / raw)
To: Mike Palmiotto, selinux
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:
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] libselinux: fix string conversion of unknown perms
2019-09-16 20:01 ` Stephen Smalley
@ 2019-09-17 13:20 ` Mike Palmiotto
0 siblings, 0 replies; 3+ messages in thread
From: Mike Palmiotto @ 2019-09-17 13:20 UTC (permalink / raw)
To: Stephen Smalley; +Cc: selinux
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.
--
Mike Palmiotto
https://crunchydata.com
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-09-17 13:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-10 19:53 [PATCH] libselinux: fix string conversion of unknown perms Mike Palmiotto
2019-09-16 20:01 ` Stephen Smalley
2019-09-17 13:20 ` Mike Palmiotto
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).