Fix p_secstodate overflow handling (bug 22463)
Commit Message
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.
Comments
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.
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.
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(-)
@@ -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.
@@ -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