[PATCH] time: Avoid memcmp overread in tzset (bug 31931)

[PATCH] time: Avoid memcmp overread in tzset (bug 31931)

[Florian Weimer]

The test does not necessarily trigger the crash, depending on memcmp
behavior.

---
 time/Makefile           |  5 ++++-
 time/tst-tzfile-fault.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 time/tzfile.c           |  4 ++--
 3 files changed, 50 insertions(+), 3 deletions(-)

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.  */

base-commit: a10b6ad471d7b528149f5ff32eef2f1c1dc1213c

Re: [PATCH] time: Avoid memcmp overread in tzset (bug 31931)

[Andreas Schwab]

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, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] time: Avoid memcmp overread in tzset (bug 31931)

[Florian Weimer]

* 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

Re: [PATCH] time: Avoid memcmp overread in tzset (bug 31931)

[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.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] time: Avoid memcmp overread in tzset (bug 31931)

[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:

“
/* 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

Re: [PATCH] time: Avoid memcmp overread in tzset (bug 31931)

[Florian Weimer]

* 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