Patchwork Fix p_secstodate overflow handling (bug 22463)

login
register
mail settings
Submitter Paul Eggert
Date Nov. 21, 2017, 9:54 p.m.
Message ID <0b913772-b3c4-f73f-0812-92fbba0577d0@cs.ucla.edu>
Download mbox | patch
Permalink /patch/24418/
State New
Headers show

Comments

Paul Eggert - Nov. 21, 2017, 9:54 p.m.
On 11/21/2017 05:33 AM, Joseph Myers wrote:
> RFC 4034 does not provide a specification of the
> function p_secstodate.

Although RFC 4034 does not specify the p_secstodate API, it does specify 
the string format and p_secstodate is supposed to generate that format.

I reread the RFC, and found that our patches both mishandled 
out-of-range timestamps. The RFC says that, for a timestamp greater than 
the 32-bit range, the string should be a decimal representation of the 
timestamp's low-order 32 bits. With this in mind we can simplify the fix 
since we need not worry about going past the year 2106 or about tm_year 
overflow or anything like that. We can also fix another bug, since the 
code is mishandling post-2106 timestamps now on 64-bit time_t platforms 
(just as it is mishandling post-2038 timestamps on 32-bit time_t platforms).

Attached is an (untested) patch with this simplification in mind. It 
uses your idea of a 'clock < 0' test along with a static assertion 
(though the assertion is merely that time_t is at least 32 bits wide, 
since that's all we need now) to avoid the need for complicated 
integer-overflow checking. It uses my idea to use strftime instead of 
sprintf to simplify and shorten the code and avoid having confusing 
gotos and pragmas merely to pacify GCC's false alarms. And it passes 
just an unsigned int to sprintf so that GCC doesn't spout off 
unnecessarily about format overflow.
Joseph Myers - Nov. 21, 2017, 10:26 p.m.
On Tue, 21 Nov 2017, Paul Eggert wrote:

> Attached is an (untested) patch with this simplification in mind. It uses your
> idea of a 'clock < 0' test along with a static assertion (though the assertion
> is merely that time_t is at least 32 bits wide, since that's all we need now)
> to avoid the need for complicated integer-overflow checking. It uses my idea
> to use strftime instead of sprintf to simplify and shorten the code and avoid
> having confusing gotos and pragmas merely to pacify GCC's false alarms. And it
> passes just an unsigned int to sprintf so that GCC doesn't spout off
> unnecessarily about format overflow.

The strftime use is incorrect - the %M%D needs to be %m%d.  This 
illustrates the need for including testcases, as in my second patch.
Joseph Myers - Nov. 21, 2017, 10:38 p.m.
On Tue, 21 Nov 2017, Paul Eggert wrote:

> I reread the RFC, and found that our patches both mishandled out-of-range
> timestamps. The RFC says that, for a timestamp greater than the 32-bit range,
> the string should be a decimal representation of the timestamp's low-order 32
> bits. With this in mind we can simplify the fix since we need not worry about

Where does it say that?  My reading is more that (a) the binary 
representation is the low-order 32 bits; (b) a decimal representation of 
the low-order 32-bits is valid as a textual representation, but (c) the 
timestamp is to be interpreted as whatever time with that low-order 
32-bits is within 68 years of the present, and so a YYYYMMDDHHmmSS 
representation corresponds to that time.
Joseph Myers - Nov. 21, 2017, 10:44 p.m.
On Tue, 21 Nov 2017, Joseph Myers wrote:

> On Tue, 21 Nov 2017, Paul Eggert wrote:
> 
> > I reread the RFC, and found that our patches both mishandled out-of-range
> > timestamps. The RFC says that, for a timestamp greater than the 32-bit range,
> > the string should be a decimal representation of the timestamp's low-order 32
> > bits. With this in mind we can simplify the fix since we need not worry about
> 
> Where does it say that?  My reading is more that (a) the binary 
> representation is the low-order 32 bits; (b) a decimal representation of 
> the low-order 32-bits is valid as a textual representation, but (c) the 
> timestamp is to be interpreted as whatever time with that low-order 
> 32-bits is within 68 years of the present, and so a YYYYMMDDHHmmSS 
> representation corresponds to that time.

And, incidentally, I don't see how any of this can say anything about what 
the API should do for values outside the 32-bit range, since the RFC is 
purely concerned with interpretation and representation of values *within* 
the range, as those are all that can be transmitted in the DNS protocol.  
So if a time value outside the range is passed, it seems to make logical 
sense to produce the most human-readable representation, i.e. 
YYYYMMDDHHmmSS when possible (while recognizing that this API is 
problematic post-2038, as then the value 0 on the network should no longer 
be interpreted as in the year 1970).
Paul Eggert - Nov. 21, 2017, 11 p.m.
On 11/21/2017 02:38 PM, Joseph Myers wrote:
> Where does it say that?

It's in RFC 4034 section 3.1.5, which says that the time values specify 
a "date and time in the form of a 32-bit unsigned number of seconds 
elapsed since 1 January 1970 00:00:00 UTC, ignoring leap seconds." 
Although section 3.2 says how these time values are represented, it does 
not give license to represent a date and time that is out of the 32-bit 
range. Section 3.1.5 goes on to say that the intent is to use serial 
number arithmetic a la RFC 1982, which boils down to using the low-order 
32-bits of the full timestamp.

> the RFC is
> purely concerned with interpretation and representation of values*within*  
> the range

By referring to RFC 1982, RFC 4034 is clearly concerned with how to deal 
with timestamp values outside the 32-bit window.

I'm still thinking that it would simplify things (and avoid similar 
confusion in the future) if p_secstodate always used sprintf ("%u", ...) 
for these timestamps.
Joseph Myers - Nov. 21, 2017, 11:35 p.m.
On Tue, 21 Nov 2017, Paul Eggert wrote:

> On 11/21/2017 02:38 PM, Joseph Myers wrote:
> > Where does it say that?
> 
> It's in RFC 4034 section 3.1.5, which says that the time values specify a
> "date and time in the form of a 32-bit unsigned number of seconds elapsed
> since 1 January 1970 00:00:00 UTC, ignoring leap seconds." Although section

That section is about the wire format (and clearly implies that *in the 
wire format* the low 32 bits are used).

> 3.2 says how these time values are represented, it does not give license to
> represent a date and time that is out of the 32-bit range. Section 3.1.5 goes
> on to say that the intent is to use serial number arithmetic a la RFC 1982,
> which boils down to using the low-order 32-bits of the full timestamp.

My reading of section 3.2 together with section 3.1.5 would be that *when 
giving a textual representation of a value from the wire format*, it's 
either YYYYMMDDHHmmSS for the *actual* time (within 2**31 seconds of the 
present time), as long as that involves a year from 0001 to 9999, or the 
decimal value of the 32 bits from the wire.

It says nothing about what the textual representation should be for an API 
that is given extra information beyond the 32 bits on the wire, such as 
p_secstodate on a 64-bit system.

> I'm still thinking that it would simplify things (and avoid similar confusion
> in the future) if p_secstodate always used sprintf ("%u", ...) for these
> timestamps.

I think that would be contrary to the point of this API for use in 
debugging, which means it should give a more human-readable output when 
possible.
Paul Eggert - Nov. 21, 2017, 11:54 p.m.
On 11/21/2017 03:35 PM, Joseph Myers wrote:
> My reading of section 3.2 together with section 3.1.5 would be that *when
> giving a textual representation of a value from the wire format*, it's
> either YYYYMMDDHHmmSS for the*actual*  time (within 2**31 seconds of the
> present time), as long as that involves a year from 0001 to 9999, or the
> decimal value of the 32 bits from the wire.

Sorry, I'm not quite following. Surely you don't mean that p_secstodate 
should have time-dependent behavior, i.e., that it should call 
clock_gettime or equivalent in order to determine whether the requested 
timestamp is in the 68-year window around the current time, so that it 
can generate the YYYYMMDDHHMMSS format.

> It says nothing about what the textual representation should be for an API
> that is given extra information beyond the 32 bits on the wire, such as
> p_secstodate on a 64-bit system.

Yes, we're agreed that the RFC does not specify an API. However, the API 
is intended for use in generating strings that follow the RFC-specified 
format, and that's all that the API has to do. I see little point to 
bikeshedding this API to try to support values outside the 32-bit range; 
doing that is more likely to cause trouble (because it will generate 
either nonconforming or numerically-incorrect strings) than to cure it.
Joseph Myers - Nov. 22, 2017, 12:05 a.m.
On Tue, 21 Nov 2017, Paul Eggert wrote:

> On 11/21/2017 03:35 PM, Joseph Myers wrote:
> > My reading of section 3.2 together with section 3.1.5 would be that *when
> > giving a textual representation of a value from the wire format*, it's
> > either YYYYMMDDHHmmSS for the*actual*  time (within 2**31 seconds of the
> > present time), as long as that involves a year from 0001 to 9999, or the
> > decimal value of the 32 bits from the wire.
> 
> Sorry, I'm not quite following. Surely you don't mean that p_secstodate should
> have time-dependent behavior, i.e., that it should call clock_gettime or

No.  My suggestion would be that we should say if anything it's the 
caller's responsibility to implement any such time-dependence and to call 
p_secstodate with a complete time value rather than just the low 32 bits.  
And that we should probably obsolete this interface before 2038, which is 
when such time-dependence might start to matter for the uses in glibc.  
(Any replacement would then explicitly take a full 64-bit time value on 
all platforms, possibly with a separate interface that implements the 
logic to convert a wire-format 32-bit value into a full 64-bit time value.  
But the replacement doesn't need designing now.)

(This code should of course move to using __time64_t and __gmtime64_r or 
similar once such internal interfaces exist that are explicitly 64-bit on 
32-bit platforms - generically, internal code in glibc using time_t etc. 
should move to explicitly 64-bit interfaces once available so that it 
continues to work properly on 32-bit platforms past 2038.)
Paul Eggert - Nov. 22, 2017, 3:32 a.m.
Joseph Myers wrote:
> we should probably obsolete this interface before 2038

This is the best idea I've heard so far. If we do it, it won't really matter 
what p_secstodate does with post-2038 (or especially post-2106) timestamps, so 
long as it doesn't do something stupid like overrun the array.
Florian Weimer - Nov. 22, 2017, 9:23 a.m.
On 11/22/2017 12:00 AM, Paul Eggert wrote:
> On 11/21/2017 02:38 PM, Joseph Myers wrote:
>> Where does it say that?
> 
> It's in RFC 4034 section 3.1.5, which says that the time values specify 
> a "date and time in the form of a 32-bit unsigned number of seconds 
> elapsed since 1 January 1970 00:00:00 UTC, ignoring leap seconds." 
> Although section 3.2 says how these time values are represented, it does 
> not give license to represent a date and time that is out of the 32-bit 
> range. Section 3.1.5 goes on to say that the intent is to use serial 
> number arithmetic a la RFC 1982, which boils down to using the low-order 
> 32-bits of the full timestamp.

p_secstodate was added with an import from BIND in 1996:

Tue Aug 13 18:26:45 1996  Ulrich Drepper  <drepper@cygnus.com>
…
	(__dn_count_labels, __p_secstodate): New functions.

At this time, the DNSSEC RFCs did not require modulo interpretation of 
the signer field.  It was truly restricted to 32 bits, as far as I can see:

   <https://tools.ietf.org/html/rfc2065>

I think we can change the function to always return NULL (or abort the 
process with an error message) and turn it into into a compat symbol.  I 
very much doubt that there are any users at all because the function 
does not implement the current DNSSEC specification.  (Anything 
DNSSEC-related still left in libresolv is equally useless because the 
protocol underwent several incompatible revisions.)

Thanks,
Florian
Joseph Myers - Nov. 22, 2017, 1:06 p.m.
On Wed, 22 Nov 2017, Florian Weimer wrote:

> I think we can change the function to always return NULL (or abort the process
> with an error message) and turn it into into a compat symbol.  I very much
> doubt that there are any users at all because the function does not implement

Well, there are two calls in ns_print.c, so you can't eliminate the 
function without doing something with those calls.
Florian Weimer - Nov. 22, 2017, 1:14 p.m.
On 11/22/2017 02:06 PM, Joseph Myers wrote:
> On Wed, 22 Nov 2017, Florian Weimer wrote:
> 
>> I think we can change the function to always return NULL (or abort the process
>> with an error message) and turn it into into a compat symbol.  I very much
>> doubt that there are any users at all because the function does not implement
> 
> Well, there are two calls in ns_print.c, so you can't eliminate the
> function without doing something with those calls.

Oh, right, but it has got these gems:

	/* KJD - need to complete this */
	/* XXX need to dump key, print otherdata length & other data */

So maybe we should just fix the generic/default case to use unknown RR 
type syntax and just dump those records in that format.  I'll send 
patches for that once my current round of docs/syscall wrapper work is 
done (too many patches in flight right now).

I don't think DNSSEC as a protocol (or TKEY, which is not part of 
DNSSEC) will survive contact with the year 2038 without changes, so 
doing any proactive Y2038 work here isn't worthwhile IMHO.

Thanks,
Florian
Joseph Myers - Nov. 22, 2017, 1:18 p.m.
On Wed, 22 Nov 2017, Florian Weimer wrote:

> So maybe we should just fix the generic/default case to use unknown RR type
> syntax and just dump those records in that format.  I'll send patches for that
> once my current round of docs/syscall wrapper work is done (too many patches
> in flight right now).

We still need to fix the build of p_secstodate so that regressions with 
mainline GCC / binutils can be reliably detected again, which is not 
currently the case and is meaning that new build failures keep appearing 
while old ones such as this one remain unfixed.

Patch

From c6e58222ed279191add7ec97d98b33ed6eb08a17 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 21 Nov 2017 13:01:14 -0800
Subject: [PATCH] [BZ #22463]

* resolv/res_debug.c (p_secstodate): If the timestamp is out of
time_t or 32-bit unsigned range, just generate an unsigned decimal
integer representing the low-order 32 bits, as required by RFC
4034.  Use strftime instead of sprintf to format timestamps, as
this is simpler and avoids the need to pacify GCC about overflows
that cannot occur.
---
 ChangeLog          | 10 ++++++++++
 resolv/res_debug.c | 19 ++++++++++---------
 2 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index c72be0c301..5bf75fd3c2 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@ 
+2017-11-21  Paul Eggert  <eggert@cs.ucla.edu>
+
+	[BZ #22463]
+	* resolv/res_debug.c (p_secstodate): If the timestamp is out of
+	time_t or 32-bit unsigned range, just generate an unsigned decimal
+	integer representing the low-order 32 bits, as required by RFC
+	4034.  Use strftime instead of sprintf to format timestamps, as
+	this is simpler and avoids the need to pacify GCC about overflows
+	that cannot occur.
+
 2017-11-21  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
 
 	* nptl/pthreadP.h (ASSERT_PTHREAD_INTERNAL_SIZE): Add workarond for
diff --git a/resolv/res_debug.c b/resolv/res_debug.c
index 4114c2db46..93fcded381 100644
--- a/resolv/res_debug.c
+++ b/resolv/res_debug.c
@@ -1052,23 +1052,24 @@  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 the string
+ * format expected by Internet RFC 4034.
  * 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 */
+	_Static_assert ((time_t) 0x80000000 != 0,
+			"time_t contains at least 32 bits");
+	unsigned int losecs = secs & 0xffffffff;
 	time_t clock = secs;
-	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 (losecs != secs || clock < 0
+	    || __gmtime_r (&clock, &timebuf) == NULL)
+	  sprintf (output, "%u", losecs);
+	else
+	  strftime (output, sizeof output, "%Y%M%D%H%M%S", &timebuf);
 	return (output);
 }
 libresolv_hidden_def (__p_secstodate)
-- 
2.14.3