| Message ID | 87o77ovybr.fsf@oldenburg.str.redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | time: Avoid memcmp overread in tzset (bug 31931) | expand |
On Jun 26 2024, Florian Weimer wrote: > diff --git a/time/tst-tzfile-fault.c b/time/tst-tzfile-fault.c > new file mode 100644 > index 0000000000..0b206ab1c3 > --- /dev/null > +++ b/time/tst-tzfile-fault.c > @@ -0,0 +1,44 @@ > +/* Attempt to trigger fault with very short TZ variable (bug 31931). > + Copyright (C) 2024 Free Software Foundation, Inc. > + This file is part of the GNU C Library. > + > + The GNU C Library is free software; you can redistribute it and/or > + modify it under the terms of the GNU Lesser General Public > + License as published by the Free Software Foundation; either > + version 2.1 of the License, or (at your option) any later version. > + > + The GNU C Library is distributed in the hope that it will be useful, > + but WITHOUT ANY WARRANTY; without even the implied warranty of > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU > + Lesser General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public > + License along with the GNU C Library; if not, see > + <https://www.gnu.org/licenses/>. */ > + > + > +#include <support/next_to_fault.h> > +#include <stdlib.h> > +#include <time.h> > +#include <string.h> > + > +static char tz[] = "TZ=/"; > + > +static int > +do_test (void) > +{ > + struct support_next_to_fault ntf > + = support_next_to_fault_allocate (sizeof (tz)); > + memcpy (ntf.buffer, tz, sizeof (tz)); > + putenv (tz); Did you mean putenv (ntf.buffer)? > diff --git a/time/tzfile.c b/time/tzfile.c > index 4147539964..2006f9a189 100644 > --- a/time/tzfile.c > +++ b/time/tzfile.c > @@ -134,8 +134,8 @@ __tzfile_read (const char *file, size_t extra, char **extrap) > and which is not the system wide default TZDEFAULT. */ > if (__libc_enable_secure > && ((*file == '/' > - && memcmp (file, TZDEFAULT, sizeof TZDEFAULT) > - && memcmp (file, default_tzdir, sizeof (default_tzdir) - 1)) > + && strcmp (file, TZDEFAULT) != 0 > + && strncmp (file, default_tzdir, sizeof (default_tzdir) - 1)) Please add != 0.
* Andreas Schwab: >> +static int >> +do_test (void) >> +{ >> + struct support_next_to_fault ntf >> + = support_next_to_fault_allocate (sizeof (tz)); >> + memcpy (ntf.buffer, tz, sizeof (tz)); >> + putenv (tz); > > Did you mean putenv (ntf.buffer)? Right. >> diff --git a/time/tzfile.c b/time/tzfile.c >> index 4147539964..2006f9a189 100644 >> --- a/time/tzfile.c >> +++ b/time/tzfile.c >> @@ -134,8 +134,8 @@ __tzfile_read (const char *file, size_t extra, char **extrap) >> and which is not the system wide default TZDEFAULT. */ >> if (__libc_enable_secure >> && ((*file == '/' >> - && memcmp (file, TZDEFAULT, sizeof TZDEFAULT) >> - && memcmp (file, default_tzdir, sizeof (default_tzdir) - 1)) >> + && strcmp (file, TZDEFAULT) != 0 >> + && strncmp (file, default_tzdir, sizeof (default_tzdir) - 1)) > > Please add != 0. I've made the change. I'm having second thoughts, though. Should we support this as an memcmp extension? See: git grep -E 'memcmp.*sizeof\s*\("' And glibc isn't the only project that does that: <https://codesearch.debian.net/search?q=memcmp.*sizeof%5Cs*%5C%28%22&literal=0> It seems to me our memcmp implementations already support that. Thanks, Florian
On Jun 26 2024, Florian Weimer wrote: > I'm having second thoughts, though. Should we support this as an > memcmp extension? See: git grep -E 'memcmp.*sizeof\s*\("' > And glibc isn't the only project that does that: > > <https://codesearch.debian.net/search?q=memcmp.*sizeof%5Cs*%5C%28%22&literal=0> Do any of those uses fail to check the size of the arguments? At least in glibc we are clean.
* Andreas Schwab: > On Jun 26 2024, Florian Weimer wrote: > >> I'm having second thoughts, though. Should we support this as an >> memcmp extension? See: git grep -E 'memcmp.*sizeof\s*\("' >> And glibc isn't the only project that does that: >> >> <https://codesearch.debian.net/search?q=memcmp.*sizeof%5Cs*%5C%28%22&literal=0> > > Do any of those uses fail to check the size of the arguments? At least > in glibc we are clean. Some do not, this one for example: “ /* coverity[-tainted_data_sink: arg-1] */ static int mod_extforward_hap_PROXY_v1 (connection * const con, union hap_PROXY_hdr * const hdr) { … char *s = hdr->v1.line + sizeof("PROXY")-1; /*checked in hap_PROXY_recv()*/ char *src_addr, *dst_addr, *src_port, *dst_port; int family; int src_lport, dst_lport; if (*s != ' ') return -1; ++s; if (s[0] == 'T' && s[1] == 'C' && s[2] == 'P' && s[4] == ' ') { if (s[3] == '4') { family = AF_INET; } else if (s[3] == '6') { family = AF_INET6; } else { return -1; } s += 5; } else if (0 == memcmp(s, "UNKNOWN", sizeof("UNKNOWN")-1) && (s[7] == '\0' || s[7] == ' ')) { return 0; /* keep local connection address */ … ” <https://sources.debian.org/src/lighttpd/1.4.76-1/src/mod_extforward.c/?hl=1449#L1449> This seems to be another example: <https://sources.debian.org/src/nvi/1.81.6-22/ipc/ip_run.c/?hl=73#L73> I get the impression the list is rather long. Thanks, Florian
* Florian Weimer: > * Andreas Schwab: > >> On Jun 26 2024, Florian Weimer wrote: >> >>> I'm having second thoughts, though. Should we support this as an >>> memcmp extension? See: git grep -E 'memcmp.*sizeof\s*\("' >>> And glibc isn't the only project that does that: >>> >>> <https://codesearch.debian.net/search?q=memcmp.*sizeof%5Cs*%5C%28%22&literal=0> >> >> Do any of those uses fail to check the size of the arguments? At least >> in glibc we are clean. > > Some do not, this one for example: > <https://sources.debian.org/src/lighttpd/1.4.76-1/src/mod_extforward.c/?hl=1449#L1449> > > This seems to be another example: > > <https://sources.debian.org/src/nvi/1.81.6-22/ipc/ip_run.c/?hl=73#L73> > > I get the impression the list is rather long. On the other hand, sysdeps/x86_64/multiarch/memcmp-avx2-movbe.S starts like this: “ ENTRY (MEMCMP) # ifdef USE_AS_WMEMCMP shl $2, %RDX_LP # elif defined __ILP32__ /* Clear the upper 32 bits. */ movl %edx, %edx # endif cmp $VEC_SIZE, %RDX_LP jb L(less_vec) /* From VEC to 2 * VEC. No branch when size == VEC_SIZE. */ vmovdqu (%rsi), %ymm1 VPCMPEQ (%rdi), %ymm1, %ymm1 vpmovmskb %ymm1, %eax ” If I'm reading this correctly (and a quick test confirms this), if there are only 16 accessible bytes, but we pass a total length of 32, we fault on the vmovdqu instruction. So we do not currently implement an extension which makes memcmp for non-array inputs defined. I assume we should add an access attribute to memcmp, although this will have quite a bit of a fallout. I think we should go ahead with the time/tzfile.c, too. Thanks, Florian
diff --git a/time/Makefile b/time/Makefile index 5b541fb9d3..f4c75b786d 100644 --- a/time/Makefile +++ b/time/Makefile @@ -50,7 +50,8 @@ tests := test_time clocktest tst-posixtz tst-strptime tst_wcsftime \ tst-clock tst-clock2 tst-clock_nanosleep tst-cpuclock1 \ tst-adjtime tst-ctime tst-difftime tst-mktime4 tst-clock_settime \ tst-settimeofday tst-itimer tst-gmtime tst-timegm \ - tst-timespec_get tst-timespec_getres tst-strftime4 + tst-timespec_get tst-timespec_getres tst-strftime4 \ + tst-tzfile-fault tests-time64 := \ tst-adjtime-time64 \ @@ -110,3 +111,5 @@ tst-tzname-ENV = TZDIR=${common-objpfx}timezone/testdata CPPFLAGS-tst-tzname.c += -DTZDEFRULES='"$(posixrules-file)"' bug-getdate1-ARGS = ${objpfx}bug-getdate1-fmt + +tst-tzfile-fault-ENV = GLIBC_TUNABLES=glibc.rtld.enable_secure=1 diff --git a/time/tst-tzfile-fault.c b/time/tst-tzfile-fault.c new file mode 100644 index 0000000000..0b206ab1c3 --- /dev/null +++ b/time/tst-tzfile-fault.c @@ -0,0 +1,44 @@ +/* Attempt to trigger fault with very short TZ variable (bug 31931). + Copyright (C) 2024 Free Software Foundation, Inc. + This file is part of the GNU C Library. + + The GNU C Library is free software; you can redistribute it and/or + modify it under the terms of the GNU Lesser General Public + License as published by the Free Software Foundation; either + version 2.1 of the License, or (at your option) any later version. + + The GNU C Library is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + Lesser General Public License for more details. + + You should have received a copy of the GNU Lesser General Public + License along with the GNU C Library; if not, see + <https://www.gnu.org/licenses/>. */ + + +#include <support/next_to_fault.h> +#include <stdlib.h> +#include <time.h> +#include <string.h> + +static char tz[] = "TZ=/"; + +static int +do_test (void) +{ + struct support_next_to_fault ntf + = support_next_to_fault_allocate (sizeof (tz)); + memcpy (ntf.buffer, tz, sizeof (tz)); + putenv (tz); + + tzset (); + + /* Avoid dangling pointer in environ. */ + putenv (tz); + support_next_to_fault_free (&ntf); + + return 0; +} + +#include <support/test-driver.c> diff --git a/time/tzfile.c b/time/tzfile.c index 4147539964..2006f9a189 100644 --- a/time/tzfile.c +++ b/time/tzfile.c @@ -134,8 +134,8 @@ __tzfile_read (const char *file, size_t extra, char **extrap) and which is not the system wide default TZDEFAULT. */ if (__libc_enable_secure && ((*file == '/' - && memcmp (file, TZDEFAULT, sizeof TZDEFAULT) - && memcmp (file, default_tzdir, sizeof (default_tzdir) - 1)) + && strcmp (file, TZDEFAULT) != 0 + && strncmp (file, default_tzdir, sizeof (default_tzdir) - 1)) || strstr (file, "../") != NULL)) /* This test is certainly a bit too restrictive but it should catch all critical cases. */