On Mon, Feb 21, 2022 at 8:04 AM Vit Mojzis <vmojzis@xxxxxxxxxx> wrote:
>
> On 17. 02. 22 15:16, Christian Göttsche wrote:
> > On Thu, 17 Feb 2022 at 14:14, Vit Mojzis <vmojzis@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >>
> >> 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
> >>
>
