aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorJoseph Myers <joseph@codesourcery.com>2017-11-22 22:12:07 +0000
committerJoseph Myers <joseph@codesourcery.com>2017-11-22 22:12:07 +0000
commitf120cda6072d830df92656dad0c89967547b97dc (patch)
tree0b1291a340733649d974aa84172999c313f76aa1
parenta90d1ac2d2f7b20a9df676ac9bd0aa512ab5b708 (diff)
downloadglibc-f120cda6072d830df92656dad0c89967547b97dc.zip
glibc-f120cda6072d830df92656dad0c89967547b97dc.tar.gz
glibc-f120cda6072d830df92656dad0c89967547b97dc.tar.bz2
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. The function's interface is inherently problematic when dates beyond Y2038 might be involved, because of the ambiguity in how to interpret 32-bit timestamps as such dates (the RFC suggests interpreting times as being within 68 years of the present date, which would mean some kind of interface whose behavior depends on the present date). This patch works on the basis of making a minimal fix in preparation for obsoleting the function. The function is made to handle times in the interval [0, 0x7fffffff] only, on all platforms, with <overflow> used as the output string in other cases (and errno set to EOVERFLOW in such cases). This seems to be a reasonable state for the function to be in when made a compat symbol by a future patch, being compatible with any existing uses for existing timestamps without trying to work for later timestamps. Results independent of the range of time_t also simplify the testcase. 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 range check on the input, this patch then disables the -Wformat-overflow= warning for the sprintf call (I prefer that to the use of strftime, as being more transparently correct without knowing what each of %m and %M etc. is). 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 uses from within glibc are limited to 32-bit inputs. Tested with build-many-glibcs.py that this restores the build for arm with yesterday's mainline GCC. Also tested for x86_64 and x86. [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.
-rw-r--r--ChangeLog12
-rw-r--r--resolv/Makefile2
-rw-r--r--resolv/res_debug.c24
-rw-r--r--resolv/tst-p_secstodate.c67
4 files changed, 104 insertions, 1 deletions
diff --git a/ChangeLog b/ChangeLog
index 203141b..ba239a4 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,17 @@
2017-11-22 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.
+
* sysdeps/sparc/sparc64/soft-fp/s_frexpl.c: Remove file.
* sysdeps/sparc/sparc64/soft-fp/s_scalblnl.c: Likewise.
* sysdeps/sparc/sparc64/soft-fp/s_scalbnl.c: Likewise.
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..a4fbc56 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)
@@ -1054,6 +1055,8 @@ libresolv_hidden_def (__dn_count_labels)
/*
* Make dates expressed in seconds-since-Jan-1-1970 easy to read.
* SIG records are required to be printed like this, by the Secure DNS RFC.
+ * This is an obsolescent function and does not handle dates outside the
+ * signed 32-bit range.
*/
char *
p_secstodate (u_long secs) {
@@ -1063,12 +1066,31 @@ p_secstodate (u_long secs) {
struct tm *time;
struct tm timebuf;
- time = __gmtime_r(&clock, &timebuf);
+ /* The call to __gmtime_r can never produce a year overflowing
+ the range of int, given the check on SECS, but check for a
+ NULL return anyway to avoid a null pointer dereference in
+ case there are any other unspecified errors. */
+ if (secs > 0x7fffffff
+ || (time = __gmtime_r (&clock, &timebuf)) == NULL) {
+ strcpy (output, "<overflow>");
+ __set_errno (EOVERFLOW);
+ return output;
+ }
time->tm_year += 1900;
time->tm_mon += 1;
+ /* The struct tm fields, given the above range check,
+ 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..9dac1ad
--- /dev/null
+++ b/resolv/tst-p_secstodate.c
@@ -0,0 +1,67 @@
+/* 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;
+ /* Expected output. */
+ const char *out;
+};
+
+static const struct test tests[] =
+ {
+ { 0UL, "19700101000000" },
+ { 12345UL, "19700101032545" },
+ { 999999999UL, "20010909014639" },
+ { 2147483647UL, "20380119031407" },
+ { 2147483648UL, "<overflow>" },
+ { 4294967295UL, "<overflow>" },
+#if ULONG_MAX > 0xffffffffUL
+ { 4294967296UL, "<overflow>" },
+ { 9999999999UL, "<overflow>" },
+ { LONG_MAX, "<overflow>" },
+ { ULONG_MAX, "<overflow>" },
+#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);
+ if (strcmp (p, tests[i].out) != 0)
+ {
+ printf ("test %zu failed", i);
+ ret = 1;
+ }
+ }
+ return ret;
+}
+
+#include <support/test-driver.c>