Patchwork Fix p_secstodate overflow handling (bug 22463)

login
register
mail settings
Submitter Joseph Myers
Date Nov. 21, 2017, 5:16 p.m.
Message ID <alpine.DEB.2.20.1711211713230.30987@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/24414/
State New
Headers show

Comments

Joseph Myers - Nov. 21, 2017, 5:16 p.m.
Here is a revised patch version.  This one detects the case where the 
conversion to time_t produces a negative result (with a static assertion 
to ensure this is the only case where the conversion is problematic); 
produces output in integer seconds since the epoch if __gmtime_r 
overflowed or the time_t value was negative, but the output would be less 
than 14 characters; still uses <overflow> in cases where the integer 
output would be 14 or more characters; adds a testcase.


Fix p_secstodate overflow handling (bug 22463).

The resolv/res_debug.c function p_secstodate (which is a public
function exported from libresolv, taking an unsigned long argument)
does:

        struct tm timebuf;
        time = __gmtime_r(&clock, &timebuf);
        time->tm_year += 1900;
        time->tm_mon += 1;
        sprintf(output, "%04d%02d%02d%02d%02d%02d",
                time->tm_year, time->tm_mon, time->tm_mday,
                time->tm_hour, time->tm_min, time->tm_sec);

If __gmtime_r returns NULL (because the year overflows the range of
int), this will dereference a null pointer.  Otherwise, if the
computed year does not fit in four characters, this will cause a
buffer overrun of the fixed-size 15-byte buffer.  With current GCC
mainline, there is a compilation failure because of the possible
buffer overrun.

I couldn't find a specification for how this function is meant to
behave, but Paul pointed to RFC 4034 as relevant to the cases where
this function is called from within glibc.  This patch makes the
overflow cases use integer seconds since the epoch unless that would
require 14 or more characters (and so be potentially ambiguous or
overflow the buffer), and <overflow> as the output string in other
overflow cases, in which case the patch also makes it set errno to
EOVERFLOW (__gmtime_r will already have done so in the case of NULL
return).  I couldn't persuade GCC to recognize the ranges of the
struct tm fields by adding explicit range checks with a call to
__builtin_unreachable if outside the range (this looks similar to
<https://gcc.gnu.org/bugzilla/show_bug.cgi?id=80776>), so having added
a check for the year, this patch then disables the -Wformat-overflow=
warning for the sprintf call.

I do not know why this build failure should be new with mainline GCC
(that is, I don't know what GCC change might have introduced it, when
the basic functionality for such warnings was already in GCC 7).

I do not know if this is a security issue (that is, if there are
plausible ways in which a date before -999 or after 9999 from an
untrusted source might end up in this function).  The system clock is
arguably an untrusted source (in that e.g. NTP is insecure), but
probably not to that extent (NTP can't communicate such wild
timestamps), and Paul says uses from within glibc are limited to
32-bit inputs.

Tested with build-many-glibcs.py that this restores the build for arm
with mainline GCC.  Also tested for x86_64 and x86.

2017-11-21  Joseph Myers  <joseph@codesourcery.com>

	[BZ #22463]
	* resolv/res_debug.c: Include <libc-diag.h>.
	(p_secstodate): Use "<overflow>" as output and set errno to
	EOVERFLOW if year would not fit in four characters.
	(p_secstodate) [__GNUC_PREREQ (7, 0)]: Disable -Wformat-overflow=
	for sprintf call.
Joseph Myers - Nov. 21, 2017, 5:26 p.m.
Here's the updated ChangeLog entry for this revised patch version.

2017-11-21  Joseph Myers  <joseph@codesourcery.com>

	[BZ #22463]
	* resolv/res_debug.c: Include <libc-diag.h>.
	(p_secstodate): Assert time_t at least as wide as u_long.  On
	overflow, use integer seconds since the epoch as output, or use
	"<overflow>" as output and set errno to EOVERFLOW if integer
	seconds since the epoch would be 14 or more characters.
	(p_secstodate) [__GNUC_PREREQ (7, 0)]: Disable -Wformat-overflow=
	for sprintf call.
	* resolv/tst-p_secstodate.c: New file.
	* resolv/Makefile (tests): Add tst-p_secstodate.
	($(objpfx)tst-p_secstodate): Depend on $(objpfx)libresolv.so.

Patch

diff --git a/resolv/Makefile b/resolv/Makefile
index 0323496..aff6710 100644
--- a/resolv/Makefile
+++ b/resolv/Makefile
@@ -55,6 +55,7 @@  tests += \
   tst-resolv-network \
   tst-resolv-res_init-multi \
   tst-resolv-search \
+  tst-p_secstodate \
 
 # These tests need libdl.
 ifeq (yes,$(build-shared))
@@ -176,6 +177,7 @@  $(objpfx)tst-ns_name.out: tst-ns_name.data
 $(objpfx)tst-ns_name_compress: $(objpfx)libresolv.so
 $(objpfx)tst-ns_name_pton: $(objpfx)libresolv.so
 $(objpfx)tst-res_hnok: $(objpfx)libresolv.so
+$(objpfx)tst-p_secstodate: $(objpfx)libresolv.so
 
 
 # This test case uses the deprecated RES_USE_INET6 resolver option.
diff --git a/resolv/res_debug.c b/resolv/res_debug.c
index 4114c2d..131afc1 100644
--- a/resolv/res_debug.c
+++ b/resolv/res_debug.c
@@ -107,6 +107,7 @@ 
 #include <string.h>
 #include <time.h>
 #include <shlib-compat.h>
+#include <libc-diag.h>
 
 #ifdef SPRINTF_CHAR
 # define SPRINTF(x) strlen(sprintf/**/x)
@@ -1059,16 +1060,44 @@  char *
 p_secstodate (u_long secs) {
 	/* XXX nonreentrant */
 	static char output[15];		/* YYYYMMDDHHMMSS and null */
+	_Static_assert (sizeof (time_t) >= sizeof (u_long),
+			"time_t at least as wide as u_long");
 	time_t clock = secs;
+	if (clock < 0)
+		goto overflow;
 	struct tm *time;
 
 	struct tm timebuf;
 	time = __gmtime_r(&clock, &timebuf);
+	if (time == NULL
+	    || time->tm_year > 9999 - 1900) {
+	overflow:
+		if (sizeof (u_long) > 4 && secs >= 10000000000000UL) {
+			/* Cannot represent time in this buffer
+			   without ambiguity.  */
+			strcpy (output, "<overflow>");
+			__set_errno (EOVERFLOW);
+		} else {
+			/* Fall back to integer seconds as per RFC 4034.  */
+			sprintf (output, "%lu", secs);
+		}
+		return output;
+	}
 	time->tm_year += 1900;
 	time->tm_mon += 1;
+	/* The struct tm fields, given the above check of tm_year,
+	   must have values that mean this sprintf exactly fills the
+	   buffer.  But as of GCC 8 of 2017-11-21, GCC cannot tell
+	   that, even given range checks on all fields with
+	   __builtin_unreachable called for out-of-range values.  */
+	DIAG_PUSH_NEEDS_COMMENT;
+#if __GNUC_PREREQ (7, 0)
+	DIAG_IGNORE_NEEDS_COMMENT (8, "-Wformat-overflow=");
+#endif
 	sprintf(output, "%04d%02d%02d%02d%02d%02d",
 		time->tm_year, time->tm_mon, time->tm_mday,
 		time->tm_hour, time->tm_min, time->tm_sec);
+	DIAG_POP_NEEDS_COMMENT;
 	return (output);
 }
 libresolv_hidden_def (__p_secstodate)
diff --git a/resolv/tst-p_secstodate.c b/resolv/tst-p_secstodate.c
new file mode 100644
index 0000000..349cf2b
--- /dev/null
+++ b/resolv/tst-p_secstodate.c
@@ -0,0 +1,86 @@ 
+/* Test p_secstodate.
+   Copyright (C) 2017 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
+   <http://www.gnu.org/licenses/>.  */
+
+#include <array_length.h>
+#include <limits.h>
+#include <resolv.h>
+#include <stdbool.h>
+#include <stdio.h>
+#include <string.h>
+
+struct test
+{
+  /* Argument to p_secstodate.  */
+  unsigned long int in;
+  /* Output in YYYYMMDDHHMMSS format.  */
+  const char *out_date;
+  /* Output in integer seconds since the epoch.  If both outputs are
+     NULL, <overflow> is expected instead.  */
+  const char *out_secs;
+};
+
+static const struct test tests[] =
+  {
+    { 0UL, "19700101000000", "0" },
+    { 12345UL, "19700101032545", "12345" },
+    { 999999999UL, "20010909014639", "999999999" },
+    { 2147483647UL, "20380119031407", "2147483647" },
+    { 2147483648UL, "20380119031408", "2147483648" },
+    { 4294967295UL, "21060207062815", "4294967295" },
+#if ULONG_MAX > 0xffffffffUL
+    { 4294967296UL, "21060207062816", "4294967296" },
+    { 9999999999UL, "22861120174639", "9999999999" },
+    { 253402300799UL, "99991231235959", "253402300799" },
+    { 253402300800UL, NULL, "253402300800" },
+    { 9999999999999UL, NULL, "9999999999999" },
+    /* Once the decimal number of seconds since the epoch reaches 14
+       digits, do not consider it a valid output to avoid ambiguity
+       with YYYYMMDDHHMMSS output.  */
+    { 10000000000000UL, NULL, NULL },
+    { 9223372036854775807UL, NULL, NULL },
+    { 9223372036854775808UL, NULL, NULL },
+    { ULONG_MAX, NULL, NULL },
+#endif
+  };
+
+static int
+do_test (void)
+{
+  int ret = 0;
+  for (size_t i = 0; i < array_length (tests); i++)
+    {
+      char *p = p_secstodate (tests[i].in);
+      printf ("Test %zu: %lu -> %s\n", i, tests[i].in, p);
+      const char *exp1 = tests[i].out_date, *exp2 = tests[i].out_secs;
+      if (exp1 == NULL && exp2 == NULL)
+	exp1 = "<overflow>";
+      bool ok = false;
+      if (exp1 != NULL && strcmp (p, exp1) == 0)
+	ok = true;
+      if (exp2 != NULL && strcmp (p, exp2) == 0)
+	ok = true;
+      if (ok == false)
+	{
+	  printf ("test %zu failed", i);
+	  ret = 1;
+	}
+    }
+  return ret;
+}
+
+#include <support/test-driver.c>