Patchwork Fix p_secstodate overflow handling (bug 22463)

login
register
mail settings
Submitter Paul Eggert
Date Nov. 21, 2017, 5:15 a.m.
Message ID <ff85009d-d959-e4be-3345-55b65606a39d@cs.ucla.edu>
Download mbox | patch
Permalink /patch/24396/
State New
Headers show

Comments

Paul Eggert - Nov. 21, 2017, 5:15 a.m.
Joseph Myers wrote:
> If __gmtime_r returns NULL (because the year overflows the range of
> int), this will dereference a null pointer.

'clock' must be in the range 0 .. 2**32-1, so the year cannot overflow the range 
of int. This range is enforced by all callers of p_secstodate, and is required 
by Internet RFC 4034 section 3.2.

> With current GCC
> mainline, there is a compilation failure because of the possible
> buffer overrun.

That's a false alarm since the buffer cannot overrun. In Gnulib I work around 
such alarms by using the 'assume' macro defined by verify.h. Or one can use 
__builtin_unreachable () directly, as is done in the attached patch. (The 
'assume' macro is more readable, but is one more thing to add....)

> I couldn't find a specification for how this function is meant to
> behave.  This patch makes the overflow cases use <overflow> as the
> output string

The RFC says that if gmtime_r fails, one can just output the time_t value as an 
unsigned decimal integer.

Although all the GCC diagnostics for p_secstodate are false alarms, there is a 
bug here that GCC does not report: on platforms with 32-bit signed time_t, the 
code mishandles timestamps after 2038. Proposed patches attached. Although I 
haven't tested them, the first patch just adds a Gnulib include file that is 
well-tested so it should be OK.

The second patch fixes the abovementioned bug. Also, to pacify GCC it uses a 
call to __builtin_unreachable; we could simply make the static buffer larger, or 
use snprintf, but I hate surrendering to false alarms.

If you'd rather not add intprops.h to glibc, the TIME_T_MAX macros from 
elsewhere in glibc could be duped here. However, I think it's time to 
incorporate intprops.h, as glibc is running into several overflow-related 
problems for which intprops.h has well-tested solutions.
Joseph Myers - Nov. 21, 2017, 1:33 p.m.
On Tue, 21 Nov 2017, Paul Eggert wrote:

> Joseph Myers wrote:
> > If __gmtime_r returns NULL (because the year overflows the range of
> > int), this will dereference a null pointer.
> 
> 'clock' must be in the range 0 .. 2**32-1, so the year cannot overflow the
> range of int. This range is enforced by all callers of p_secstodate, and is
> required by Internet RFC 4034 section 3.2.

p_secstodate is a public interface, exported from libresolv.so as a 
non-compat symbol (under the name __p_secstodate, with a #define of 
p_secstodate in the public resolv.h header).  Thus, arguments outside that 
range need to be handled in some safe way, absent a specification saying 
they are undefined, and as far as I can see they can produce buffer 
overruns at present.  RFC 4034 does not provide a specification of the 
function p_secstodate.

> That's a false alarm since the buffer cannot overrun. In Gnulib I work around
> such alarms by using the 'assume' macro defined by verify.h. Or one can use
> __builtin_unreachable () directly, as is done in the attached patch. (The
> 'assume' macro is more readable, but is one more thing to add....)

I tried using __builtin_unreachable.  One version of the patch had

+       if (time->tm_mon < 0 || time->tm_mon > 11
+           || time->tm_mday < 1 || time->tm_mday > 31
+           || time->tm_hour < 0 || time->tm_hour > 23
+           || time->tm_min < 0 || time->tm_min > 59
+           || time->tm_sec < 0 || time->tm_sec > 60)
+               __builtin_unreachable ();

but it didn't help to avoid the errors (I don't know why VRP didn't 
operate effectively in that case).

> code mishandles timestamps after 2038. Proposed patches attached. Although I
> haven't tested them, the first patch just adds a Gnulib include file that is
> well-tested so it should be OK.

It adds a gnulib include file that there have been multiple objections to 
including in glibc in the past.
Joseph Myers - Nov. 21, 2017, 2:07 p.m.
On Tue, 21 Nov 2017, Paul Eggert wrote:

> If you'd rather not add intprops.h to glibc, the TIME_T_MAX macros from
> elsewhere in glibc could be duped here. However, I think it's time to

Both intprops.h and TIME_T_MAX seem massively over-engineered for the 
present bug; neither the buffer overrun, not the build failure, nor 
handling of out-of-range u_long values should need a 500-line patch.  
Since time_t is always at least as wide as long in glibc, "clock < 0" is 
entirely sufficient to detect the case of a u_long value that does not fit 
in time_t.

Patch

From fa6f9fb3fd4d9faeaf83d80c31b7d8891e9410b7 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 20 Nov 2017 20:58:44 -0800
Subject: [PATCH 2/2] [BZ #22463]

* resolv/res_debug.c: Include <intprops.h>.
(libresolv_hidden_def): If the timestamp is out of time_t range,
just generate an unsigned decimal integer.
Use strftime instead of sprintf to format timestamps.
Use __builtin_unreachable to pacify GCC.
---
 ChangeLog          |  7 +++++++
 resolv/res_debug.c | 32 +++++++++++++++++++++++---------
 2 files changed, 30 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index eece2d8..e47525d 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,12 @@ 
 2017-11-20  Paul Eggert  <eggert@cs.ucla.edu>
 
+	[BZ #22463]
+	* resolv/res_debug.c: Include <intprops.h>.
+	(libresolv_hidden_def): If the timestamp is out of time_t range,
+	just generate an unsigned decimal integer.
+	Use strftime instead of sprintf to format timestamps.
+	Use __builtin_unreachable to pacify GCC.
+
 	New internal include file <intprops.h>
 	* include/intprops.h: New file, taken from Gnulib.
 
diff --git a/resolv/res_debug.c b/resolv/res_debug.c
index 4114c2d..01285b7 100644
--- a/resolv/res_debug.c
+++ b/resolv/res_debug.c
@@ -99,6 +99,7 @@ 
 
 #include <ctype.h>
 #include <errno.h>
+#include <intprops.h>
 #include <math.h>
 #include <netdb.h>
 #include <resolv/resolv-internal.h>
@@ -1052,23 +1053,36 @@  libresolv_hidden_def (__dn_count_labels)
 
 
 /*
- * Make dates expressed in seconds-since-Jan-1-1970 easy to read.
+ * Convert SECS, a count of seconds since 1970-01-01, to string format.
+ * SECS must be less than 2**32.
  * SIG records are required to be printed like this, by the Secure DNS RFC.
  */
 char *
 p_secstodate (u_long secs) {
 	/* XXX nonreentrant */
-	static char output[15];		/* YYYYMMDDHHMMSS and null */
-	time_t clock = secs;
+
+	/* A null-terminated string containing either YYYYMMDDHHMMSS
+	   or an unsigned decimal integer with at most 10 digits.
+	   See Internet RFC 4034 section 3.2.  */
+	static char output[sizeof "YYYYMMDDHHMMSS"];
+
 	struct tm *time;
 
 	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 (secs <= TYPE_MAXIMUM (time_t)) {
+	  time_t clock = secs;
+	  time = __gmtime_r (&clock, &timebuf);
+	} else if (secs <= 0xffffffff)
+	  time = NULL;
+	else
+	  __builtin_unreachable ();
+
+	if (time == NULL)
+	  sprintf (output, "%lu", secs);
+	else
+	  strftime (output, sizeof output, "%Y%M%D%H%M%S", &timebuf);
+
 	return (output);
 }
 libresolv_hidden_def (__p_secstodate)
-- 
2.7.4