| Message ID | 20220217131415.1195383-1-vmojzis@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4bab3ecc37e1 |
| Headers | show |
| Series | [v2] libselinux: Strip spaces before values in config | expand |
On Thu, 17 Feb 2022 at 14:14, Vit Mojzis <vmojzis@redhat.com> wrote: > > Spaces before values in /etc/selinux/config should be ignored just as > spaces after them are. > > E.g. "SELINUXTYPE= targeted" should be a valid value. > > Fixes: > # sed -i 's/^SELINUXTYPE=/SELINUXTYPE= /g' /etc/selinux/config > # dnf install <any_package> > ... > RPM: error: selabel_open: (/etc/selinux/ targeted/contexts/files/file_contexts) No such file or directory > RPM: error: Plugin selinux: hook tsm_pre failed > ... > Error: Could not run transaction. > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > --- > > Sorry for the delay. I sent the fixed patch to a wrong mailing list. > > libselinux/src/selinux_config.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c > index 97f81a8b..d2e49ee1 100644 > --- a/libselinux/src/selinux_config.c > +++ b/libselinux/src/selinux_config.c > @@ -92,6 +92,7 @@ int selinux_getenforcemode(int *enforce) > FILE *cfg = fopen(SELINUXCONFIG, "re"); > if (cfg) { > char *buf; > + char *tag; > int len = sizeof(SELINUXTAG) - 1; > buf = malloc(selinux_page_size); > if (!buf) { > @@ -101,21 +102,24 @@ int selinux_getenforcemode(int *enforce) > while (fgets_unlocked(buf, selinux_page_size, cfg)) { > if (strncmp(buf, SELINUXTAG, len)) > continue; > + tag = buf+len; > + while (isspace(*tag)) > + tag++; > if (!strncasecmp > - (buf + len, "enforcing", sizeof("enforcing") - 1)) { > + (tag, "enforcing", sizeof("enforcing") - 1)) { > *enforce = 1; > ret = 0; > break; > } else > if (!strncasecmp > - (buf + len, "permissive", > + (tag, "permissive", > sizeof("permissive") - 1)) { > *enforce = 0; > ret = 0; > break; > } else > if (!strncasecmp > - (buf + len, "disabled", > + (tag, "disabled", > sizeof("disabled") - 1)) { > *enforce = -1; > ret = 0; > @@ -176,7 +180,10 @@ static void init_selinux_config(void) > > if (!strncasecmp(buf_p, SELINUXTYPETAG, > sizeof(SELINUXTYPETAG) - 1)) { > - type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1); > + buf_p += sizeof(SELINUXTYPETAG) - 1; > + while (isspace(*buf_p)) Strictly speaking one should cast to unsigned char to avoid UB, see [1], but that seems to be not done in SElinux userspace as grep -REw "(isalnum|isalpha|isascii|isblank|iscntrl|isdigit|isgraph|islower|isprint|ispunct|isspace|isupper|isxdigit|toascii|toupper|tolower)" shows 87 usages. [1]: https://wiki.sei.cmu.edu/confluence/display/c/STR37-C.+Arguments+to+character-handling+functions+must+be+representable+as+an+unsigned+char > + buf_p++; > + type = strdup(buf_p); > if (!type) { > free(line_buf); > fclose(fp); > @@ -199,6 +206,8 @@ static void init_selinux_config(void) > } else if (!strncmp(buf_p, REQUIRESEUSERS, > sizeof(REQUIRESEUSERS) - 1)) { > value = buf_p + sizeof(REQUIRESEUSERS) - 1; > + while (isspace(*value)) > + value++; > intptr = &require_seusers; > } else { > continue; > -- > 2.30.2 >
On 17. 02. 22 15:16, Christian Göttsche wrote: > On Thu, 17 Feb 2022 at 14:14, Vit Mojzis <vmojzis@redhat.com> wrote: >> >> Spaces before values in /etc/selinux/config should be ignored just as >> spaces after them are. >> >> E.g. "SELINUXTYPE= targeted" should be a valid value. >> >> Fixes: >> # sed -i 's/^SELINUXTYPE=/SELINUXTYPE= /g' /etc/selinux/config >> # dnf install <any_package> >> ... >> RPM: error: selabel_open: (/etc/selinux/ targeted/contexts/files/file_contexts) No such file or directory >> RPM: error: Plugin selinux: hook tsm_pre failed >> ... >> Error: Could not run transaction. >> >> Signed-off-by: Vit Mojzis <vmojzis@redhat.com> >> --- >> >> Sorry for the delay. I sent the fixed patch to a wrong mailing list. >> >> libselinux/src/selinux_config.c | 17 +++++++++++++---- >> 1 file changed, 13 insertions(+), 4 deletions(-) >> >> diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c >> index 97f81a8b..d2e49ee1 100644 >> --- a/libselinux/src/selinux_config.c >> +++ b/libselinux/src/selinux_config.c >> @@ -92,6 +92,7 @@ int selinux_getenforcemode(int *enforce) >> FILE *cfg = fopen(SELINUXCONFIG, "re"); >> if (cfg) { >> char *buf; >> + char *tag; >> int len = sizeof(SELINUXTAG) - 1; >> buf = malloc(selinux_page_size); >> if (!buf) { >> @@ -101,21 +102,24 @@ int selinux_getenforcemode(int *enforce) >> while (fgets_unlocked(buf, selinux_page_size, cfg)) { >> if (strncmp(buf, SELINUXTAG, len)) >> continue; >> + tag = buf+len; >> + while (isspace(*tag)) >> + tag++; >> if (!strncasecmp >> - (buf + len, "enforcing", sizeof("enforcing") - 1)) { >> + (tag, "enforcing", sizeof("enforcing") - 1)) { >> *enforce = 1; >> ret = 0; >> break; >> } else >> if (!strncasecmp >> - (buf + len, "permissive", >> + (tag, "permissive", >> sizeof("permissive") - 1)) { >> *enforce = 0; >> ret = 0; >> break; >> } else >> if (!strncasecmp >> - (buf + len, "disabled", >> + (tag, "disabled", >> sizeof("disabled") - 1)) { >> *enforce = -1; >> ret = 0; >> @@ -176,7 +180,10 @@ static void init_selinux_config(void) >> >> if (!strncasecmp(buf_p, SELINUXTYPETAG, >> sizeof(SELINUXTYPETAG) - 1)) { >> - type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1); >> + buf_p += sizeof(SELINUXTYPETAG) - 1; >> + while (isspace(*buf_p)) > > Strictly speaking one should cast to unsigned char to avoid UB, see > [1], but that > seems to be not done in SElinux userspace as > > grep -REw "(isalnum|isalpha|isascii|isblank|iscntrl|isdigit|isgraph|islower|isprint|ispunct|isspace|isupper|isxdigit|toascii|toupper|tolower)" > > shows 87 usages. > > [1]: https://wiki.sei.cmu.edu/confluence/display/c/STR37-C.+Arguments+to+character-handling+functions+must+be+representable+as+an+unsigned+char > Right, thank you. So, should I fix the patch (which would make the use of the cast inconsistent in selinux_config.c), or leave it as is? If this is something worth fixing I can make a new patch adding the cast to all the calls (except for cases where EOF can be expected? -- not sure if we need to watch for that). Vit >> + buf_p++; >> + type = strdup(buf_p); >> if (!type) { >> free(line_buf); >> fclose(fp); >> @@ -199,6 +206,8 @@ static void init_selinux_config(void) >> } else if (!strncmp(buf_p, REQUIRESEUSERS, >> sizeof(REQUIRESEUSERS) - 1)) { >> value = buf_p + sizeof(REQUIRESEUSERS) - 1; >> + while (isspace(*value)) >> + value++; >> intptr = &require_seusers; >> } else { >> continue; >> -- >> 2.30.2 >>
On Thu, Feb 17, 2022 at 1:24 PM Vit Mojzis <vmojzis@redhat.com> wrote: > > Spaces before values in /etc/selinux/config should be ignored just as > spaces after them are. > > E.g. "SELINUXTYPE= targeted" should be a valid value. > > Fixes: > # sed -i 's/^SELINUXTYPE=/SELINUXTYPE= /g' /etc/selinux/config > # dnf install <any_package> > ... > RPM: error: selabel_open: (/etc/selinux/ targeted/contexts/files/file_contexts) No such file or directory > RPM: error: Plugin selinux: hook tsm_pre failed > ... > Error: Could not run transaction. > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> Acked-by: James Carter <jwcart2@gmail.com> > --- > > Sorry for the delay. I sent the fixed patch to a wrong mailing list. > > libselinux/src/selinux_config.c | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c > index 97f81a8b..d2e49ee1 100644 > --- a/libselinux/src/selinux_config.c > +++ b/libselinux/src/selinux_config.c > @@ -92,6 +92,7 @@ int selinux_getenforcemode(int *enforce) > FILE *cfg = fopen(SELINUXCONFIG, "re"); > if (cfg) { > char *buf; > + char *tag; > int len = sizeof(SELINUXTAG) - 1; > buf = malloc(selinux_page_size); > if (!buf) { > @@ -101,21 +102,24 @@ int selinux_getenforcemode(int *enforce) > while (fgets_unlocked(buf, selinux_page_size, cfg)) { > if (strncmp(buf, SELINUXTAG, len)) > continue; > + tag = buf+len; > + while (isspace(*tag)) > + tag++; > if (!strncasecmp > - (buf + len, "enforcing", sizeof("enforcing") - 1)) { > + (tag, "enforcing", sizeof("enforcing") - 1)) { > *enforce = 1; > ret = 0; > break; > } else > if (!strncasecmp > - (buf + len, "permissive", > + (tag, "permissive", > sizeof("permissive") - 1)) { > *enforce = 0; > ret = 0; > break; > } else > if (!strncasecmp > - (buf + len, "disabled", > + (tag, "disabled", > sizeof("disabled") - 1)) { > *enforce = -1; > ret = 0; > @@ -176,7 +180,10 @@ static void init_selinux_config(void) > > if (!strncasecmp(buf_p, SELINUXTYPETAG, > sizeof(SELINUXTYPETAG) - 1)) { > - type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1); > + buf_p += sizeof(SELINUXTYPETAG) - 1; > + while (isspace(*buf_p)) > + buf_p++; > + type = strdup(buf_p); > if (!type) { > free(line_buf); > fclose(fp); > @@ -199,6 +206,8 @@ static void init_selinux_config(void) > } else if (!strncmp(buf_p, REQUIRESEUSERS, > sizeof(REQUIRESEUSERS) - 1)) { > value = buf_p + sizeof(REQUIRESEUSERS) - 1; > + while (isspace(*value)) > + value++; > intptr = &require_seusers; > } else { > continue; > -- > 2.30.2 >
On Mon, Feb 21, 2022 at 8:04 AM Vit Mojzis <vmojzis@redhat.com> wrote: > > On 17. 02. 22 15:16, Christian Göttsche wrote: > > On Thu, 17 Feb 2022 at 14:14, Vit Mojzis <vmojzis@redhat.com> wrote: > >> > >> Spaces before values in /etc/selinux/config should be ignored just as > >> spaces after them are. > >> > >> E.g. "SELINUXTYPE= targeted" should be a valid value. > >> > >> Fixes: > >> # sed -i 's/^SELINUXTYPE=/SELINUXTYPE= /g' /etc/selinux/config > >> # dnf install <any_package> > >> ... > >> RPM: error: selabel_open: (/etc/selinux/ targeted/contexts/files/file_contexts) No such file or directory > >> RPM: error: Plugin selinux: hook tsm_pre failed > >> ... > >> Error: Could not run transaction. > >> > >> Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > >> --- > >> > >> Sorry for the delay. I sent the fixed patch to a wrong mailing list. > >> > >> libselinux/src/selinux_config.c | 17 +++++++++++++---- > >> 1 file changed, 13 insertions(+), 4 deletions(-) > >> > >> diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c > >> index 97f81a8b..d2e49ee1 100644 > >> --- a/libselinux/src/selinux_config.c > >> +++ b/libselinux/src/selinux_config.c > >> @@ -92,6 +92,7 @@ int selinux_getenforcemode(int *enforce) > >> FILE *cfg = fopen(SELINUXCONFIG, "re"); > >> if (cfg) { > >> char *buf; > >> + char *tag; > >> int len = sizeof(SELINUXTAG) - 1; > >> buf = malloc(selinux_page_size); > >> if (!buf) { > >> @@ -101,21 +102,24 @@ int selinux_getenforcemode(int *enforce) > >> while (fgets_unlocked(buf, selinux_page_size, cfg)) { > >> if (strncmp(buf, SELINUXTAG, len)) > >> continue; > >> + tag = buf+len; > >> + while (isspace(*tag)) > >> + tag++; > >> if (!strncasecmp > >> - (buf + len, "enforcing", sizeof("enforcing") - 1)) { > >> + (tag, "enforcing", sizeof("enforcing") - 1)) { > >> *enforce = 1; > >> ret = 0; > >> break; > >> } else > >> if (!strncasecmp > >> - (buf + len, "permissive", > >> + (tag, "permissive", > >> sizeof("permissive") - 1)) { > >> *enforce = 0; > >> ret = 0; > >> break; > >> } else > >> if (!strncasecmp > >> - (buf + len, "disabled", > >> + (tag, "disabled", > >> sizeof("disabled") - 1)) { > >> *enforce = -1; > >> ret = 0; > >> @@ -176,7 +180,10 @@ static void init_selinux_config(void) > >> > >> if (!strncasecmp(buf_p, SELINUXTYPETAG, > >> sizeof(SELINUXTYPETAG) - 1)) { > >> - type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1); > >> + buf_p += sizeof(SELINUXTYPETAG) - 1; > >> + while (isspace(*buf_p)) > > > > Strictly speaking one should cast to unsigned char to avoid UB, see > > [1], but that > > seems to be not done in SElinux userspace as > > > > grep -REw "(isalnum|isalpha|isascii|isblank|iscntrl|isdigit|isgraph|islower|isprint|ispunct|isspace|isupper|isxdigit|toascii|toupper|tolower)" > > > > shows 87 usages. > > > > [1]: https://wiki.sei.cmu.edu/confluence/display/c/STR37-C.+Arguments+to+character-handling+functions+must+be+representable+as+an+unsigned+char > > > > > Right, thank you. So, should I fix the patch (which would make the use > of the cast inconsistent in selinux_config.c), or leave it as is? > If this is something worth fixing I can make a new patch adding the cast > to all the calls (except for cases where EOF can be expected? -- not > sure if we need to watch for that). > > Vit > Since, we aren't casting it to unsigned char anywhere else, I think the patch is fine as it is. If we feel the need to fix up the other 87 usages someday, then we can fix this one then. It seems unlikely to cause any problems. Jim > > >> + buf_p++; > >> + type = strdup(buf_p); > >> if (!type) { > >> free(line_buf); > >> fclose(fp); > >> @@ -199,6 +206,8 @@ static void init_selinux_config(void) > >> } else if (!strncmp(buf_p, REQUIRESEUSERS, > >> sizeof(REQUIRESEUSERS) - 1)) { > >> value = buf_p + sizeof(REQUIRESEUSERS) - 1; > >> + while (isspace(*value)) > >> + value++; > >> intptr = &require_seusers; > >> } else { > >> continue; > >> -- > >> 2.30.2 > >> >
On Mon, Feb 28, 2022 at 3:22 PM James Carter <jwcart2@gmail.com> wrote: > > On Thu, Feb 17, 2022 at 1:24 PM Vit Mojzis <vmojzis@redhat.com> wrote: > > > > Spaces before values in /etc/selinux/config should be ignored just as > > spaces after them are. > > > > E.g. "SELINUXTYPE= targeted" should be a valid value. > > > > Fixes: > > # sed -i 's/^SELINUXTYPE=/SELINUXTYPE= /g' /etc/selinux/config > > # dnf install <any_package> > > ... > > RPM: error: selabel_open: (/etc/selinux/ targeted/contexts/files/file_contexts) No such file or directory > > RPM: error: Plugin selinux: hook tsm_pre failed > > ... > > Error: Could not run transaction. > > > > Signed-off-by: Vit Mojzis <vmojzis@redhat.com> > > Acked-by: James Carter <jwcart2@gmail.com> > Merged. Thanks, Jim > > --- > > > > Sorry for the delay. I sent the fixed patch to a wrong mailing list. > > > > libselinux/src/selinux_config.c | 17 +++++++++++++---- > > 1 file changed, 13 insertions(+), 4 deletions(-) > > > > diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c > > index 97f81a8b..d2e49ee1 100644 > > --- a/libselinux/src/selinux_config.c > > +++ b/libselinux/src/selinux_config.c > > @@ -92,6 +92,7 @@ int selinux_getenforcemode(int *enforce) > > FILE *cfg = fopen(SELINUXCONFIG, "re"); > > if (cfg) { > > char *buf; > > + char *tag; > > int len = sizeof(SELINUXTAG) - 1; > > buf = malloc(selinux_page_size); > > if (!buf) { > > @@ -101,21 +102,24 @@ int selinux_getenforcemode(int *enforce) > > while (fgets_unlocked(buf, selinux_page_size, cfg)) { > > if (strncmp(buf, SELINUXTAG, len)) > > continue; > > + tag = buf+len; > > + while (isspace(*tag)) > > + tag++; > > if (!strncasecmp > > - (buf + len, "enforcing", sizeof("enforcing") - 1)) { > > + (tag, "enforcing", sizeof("enforcing") - 1)) { > > *enforce = 1; > > ret = 0; > > break; > > } else > > if (!strncasecmp > > - (buf + len, "permissive", > > + (tag, "permissive", > > sizeof("permissive") - 1)) { > > *enforce = 0; > > ret = 0; > > break; > > } else > > if (!strncasecmp > > - (buf + len, "disabled", > > + (tag, "disabled", > > sizeof("disabled") - 1)) { > > *enforce = -1; > > ret = 0; > > @@ -176,7 +180,10 @@ static void init_selinux_config(void) > > > > if (!strncasecmp(buf_p, SELINUXTYPETAG, > > sizeof(SELINUXTYPETAG) - 1)) { > > - type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1); > > + buf_p += sizeof(SELINUXTYPETAG) - 1; > > + while (isspace(*buf_p)) > > + buf_p++; > > + type = strdup(buf_p); > > if (!type) { > > free(line_buf); > > fclose(fp); > > @@ -199,6 +206,8 @@ static void init_selinux_config(void) > > } else if (!strncmp(buf_p, REQUIRESEUSERS, > > sizeof(REQUIRESEUSERS) - 1)) { > > value = buf_p + sizeof(REQUIRESEUSERS) - 1; > > + while (isspace(*value)) > > + value++; > > intptr = &require_seusers; > > } else { > > continue; > > -- > > 2.30.2 > >
diff --git a/libselinux/src/selinux_config.c b/libselinux/src/selinux_config.c index 97f81a8b..d2e49ee1 100644 --- a/libselinux/src/selinux_config.c +++ b/libselinux/src/selinux_config.c @@ -92,6 +92,7 @@ int selinux_getenforcemode(int *enforce) FILE *cfg = fopen(SELINUXCONFIG, "re"); if (cfg) { char *buf; + char *tag; int len = sizeof(SELINUXTAG) - 1; buf = malloc(selinux_page_size); if (!buf) { @@ -101,21 +102,24 @@ int selinux_getenforcemode(int *enforce) while (fgets_unlocked(buf, selinux_page_size, cfg)) { if (strncmp(buf, SELINUXTAG, len)) continue; + tag = buf+len; + while (isspace(*tag)) + tag++; if (!strncasecmp - (buf + len, "enforcing", sizeof("enforcing") - 1)) { + (tag, "enforcing", sizeof("enforcing") - 1)) { *enforce = 1; ret = 0; break; } else if (!strncasecmp - (buf + len, "permissive", + (tag, "permissive", sizeof("permissive") - 1)) { *enforce = 0; ret = 0; break; } else if (!strncasecmp - (buf + len, "disabled", + (tag, "disabled", sizeof("disabled") - 1)) { *enforce = -1; ret = 0; @@ -176,7 +180,10 @@ static void init_selinux_config(void) if (!strncasecmp(buf_p, SELINUXTYPETAG, sizeof(SELINUXTYPETAG) - 1)) { - type = strdup(buf_p + sizeof(SELINUXTYPETAG) - 1); + buf_p += sizeof(SELINUXTYPETAG) - 1; + while (isspace(*buf_p)) + buf_p++; + type = strdup(buf_p); if (!type) { free(line_buf); fclose(fp); @@ -199,6 +206,8 @@ static void init_selinux_config(void) } else if (!strncmp(buf_p, REQUIRESEUSERS, sizeof(REQUIRESEUSERS) - 1)) { value = buf_p + sizeof(REQUIRESEUSERS) - 1; + while (isspace(*value)) + value++; intptr = &require_seusers; } else { continue;
Spaces before values in /etc/selinux/config should be ignored just as spaces after them are. E.g. "SELINUXTYPE= targeted" should be a valid value. Fixes: # sed -i 's/^SELINUXTYPE=/SELINUXTYPE= /g' /etc/selinux/config # dnf install <any_package> ... RPM: error: selabel_open: (/etc/selinux/ targeted/contexts/files/file_contexts) No such file or directory RPM: error: Plugin selinux: hook tsm_pre failed ... Error: Could not run transaction. Signed-off-by: Vit Mojzis <vmojzis@redhat.com> --- Sorry for the delay. I sent the fixed patch to a wrong mailing list. libselinux/src/selinux_config.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)