Fix time_t arithmetic after 2038 by bernd-edlinger · Pull Request #17701 · openssl/openssl

bernd-edlinger · 2022-02-14T16:13:55Z

Some arithmetics using long when this type is 32
bit and time_t is 64 bit might break after 2038.

Some arithmetics using long when this type is 32 bit and time_t is 64 bit might break after 2038.

paulidale · 2022-02-15T01:44:20Z

Would it make sense to implement our own non-floating point version of difftime(3)?

bernd-edlinger · 2022-02-16T11:35:23Z

normally I would simply use time_t, but that does not work because of the frozen ABI.

paulidale · 2022-02-17T03:03:29Z

I'm not sure I understand.

kaduk · 2022-02-17T23:43:04Z

apps/s_time.c


    bytes_read = 0;
-    finishtime = (long)time(NULL) + maxtime;
+    finishtime = (long)((unsigned long)time(NULL) + maxtime);


AFAICT the local variables finishtime and maxtime are not part of the API or ABI. Can't we change their type (and other internal variables' type) to be native time_t and only cast at the boundaries where we have to conform to existing API contracts?

Yes, that makes sense, I am also tempted to reverse the computation and store a starttime instead of the finishtime, that needs also lots of type casts, but would also be robust if time is set back.

I would advise using a separate PR for the "store starttime instead of finishtime" change.

bernd-edlinger · 2022-02-19T08:38:25Z

I'm not sure I understand.

consider #17658 where the internal type was changed from long to time_t in 3.0
while the public API kept long. That is just plain broken.
On the other hand 1.1.1 has long in the API and in the internal representation,
that is at least consistent, and can be fixed by using a cyclic time model.
See #17703 where the time-out computation avoids the undefined long overflow
and uses a cyclic time, instead of a linear time, with an end date.

t8m · 2022-02-28T11:51:09Z

Needs second approval.

kaduk · 2022-02-28T18:20:20Z

ssl/statem/extensions_srvr.c

    /* We tolerate a cookie age of up to 10 minutes (= 60 * 10 seconds) */
    now = (unsigned long)time(NULL);
-    if (tm > now || (now - tm) > 600) {
+    if ((uint32_t)(now - tm) > 600) {


Why is this cast needed? now - tm is already evaluated in unsigned long, and I don't see that casting the result of the subtraction is needed in order to compare against 600.

Internally, we should really be using time_t and not casting to unsigned long or uint32_t unless it's being returned from a public API.

consider #17658 where the internal type was changed from long to time_t in 3.0 while the public API kept long. That is just plain broken.

Yes and no. The public API is certainly broken; the internal use of time_t isn't. I don't think there was a will to change public APIs to use time_t in the 3.0 release, due to backwards compatibility concerns. Although we did change the options to use uint64_t...

Examples such as #8687 updated the types for SSL_SESSION to be time_t; as an example of how to deal with this.

Why is this cast needed? now - tm is already evaluated in unsigned long, and I don't see that casting the result of the subtraction is needed in order to compare against 600.

although tm is of type unsigned long it is only a 4 byte value, see here:

openssl/ssl/statem/extensions_srvr.c

Line 796 in d2d2401

|| !PACKET_get_net_4(&cookie, &tm)

and here:

openssl/include/internal/packet.h

Lines 217 to 228 in d2d2401

__owur static ossl_inline int PACKET_peek_net_4(const PACKET *pkt,

unsigned long *data)

{

if (PACKET_remaining(pkt) < 4)

return 0;

*data = ((unsigned long)(*pkt->curr)) << 24;

*data |= ((unsigned long)(*(pkt->curr + 1))) << 16;

*data |= ((unsigned long)(*(pkt->curr + 2))) << 8;

*data |= *(pkt->curr + 3);

return 1;

Well, it will not break in 2038, but in 2106, but it will eventually break.

Examples such as #8687 updated the types for SSL_SESSION to be time_t; as an example of how to deal with this.

This would only be okay if the API changes as well. If the API cannot change, this simply breaks the API.
When the API cannot change, we should make the internal time representation a cyclic time model.

although tm is of type unsigned long it is only a 4 byte value, see here:

Yes, I saw that before I commented.

Please spell out a plausible scenario where the result of evaluating (uint32_t)(now - tm) > 600 and the result of evaluating (now - tm) > 600 will be different. We're measuring the delay between when we issued the cookie and when we're processing the copy sent back to us. If our clock went backwards, both expressions will produce a large value for the LHS since the nominally negative value "wraps around" in unsigned arithmetic, and it seems physically implausible for us to wait more than 2^32 seconds between issuing and receiving the cookie.

I don't see a part of this particular codeline that is using absolute time and thus subject to 2038 or 2106 issues -- I only see unsigned arithmetic that's comparing time differences against a duration.

Oh, I really thought that would be obvious:
let unsigned long be 64 byte long, and time_t be 64 byte long as well,
and the year after 2106.
Then (now - tm) will always be > 2^32.
QED.

Well, it will not break in 2038, but in 2106, but it will eventually break.

This is a flawed assumption. 32-bit time_t is signed, it does not support any date beyond 2038, nor before 1902. you cannot cast a signed time_t to an unsigned type and assume it will work beyond 2038 (as you are ignoring dates before 1970). Most software in a 32-bit time_t system will be completely broken in 2038.

No, I am of course talking about systems using 64 bit time_t, and this silly code will break, because long is a compiler dependent thing.

Oh, I really thought that would be obvious:
let unsigned long be 64 byte long, and time_t be 64 byte long as well,
and the year after 2106.

I suppose it probably would have been obvious if I had been primed to think about 2106. But the PR title and the commit's commit message only mentioned 2038...

kaduk · 2022-02-28T18:42:44Z

I put an alternative up in #17778 that uses time_t internally for s_time and switches BIO_SSL to using time_t as well.
@bernd-edlinger I'd be interested to hear why you prefer this approach over that one.

paulidale · 2022-03-01T00:30:07Z

I think the version in #17778 is clearer but I'm okay approving either.
Just so long as both aren't merged :)

tmshort · 2022-03-01T13:29:12Z

Would it make sense to implement our own non-floating point version of difftime(3)?

Possibly a function to compare two time_ts with an offset. e.g.

/* 
 * Returns:
 * -1 if a+offset_a < b+offset_b
 * 0 if a+offset_a == b+offset_b
 * 1 if a+offset_a > b+offset_b
 */
OSSL_time_compare(time_t a, time_t offset_a, time_t b, time_t offset_b)

I'd suspect that one of the offsets would always be 0, as in the case of checking for a finish time:

if (OSSL_time_compare(starttime, duration, time(NULL), 0) <= 0)

Use this (or similar) for all time calculations, and a lot of issues go away. I did something similar in #8687

tmshort · 2022-03-01T16:15:08Z

Alternative proposal: #17786 (it's a WIP, and could subsume this work).

openssl-machine · 2022-03-02T01:00:13Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

paulidale · 2022-03-02T01:01:39Z

Let's see what the alternative ends up, it looks most promising IMO.

kaduk · 2022-03-02T04:13:50Z

Perhaps we should fork off an issue for discussion of what we are actually trying to fix.
We might want to handle any/all of:

the system time_t is fine but our code uses narrower types that only hold 32 bits (signed and unsigned to be handled separately, though we might care about both or just signed)
the system time_t is signed 32 bit and we want to avoid undefined or implementation defined behavior in our code for times after 2038 (or 2106)
the system time_t is 32-bit but the system also provides a time64_t that is 64 bit
our code uses non-fixed-width types (e.g., unsigned long) but puts values in them that are constrained to a fixed-width representation (as for the TLS 1.3 Cookie's timestamp)
...

tmshort · 2022-03-02T14:45:04Z

our code uses non-fixed-width types (e.g., unsigned long) but puts values in them that are constrained to a fixed-width representation (as for the TLS 1.3 Cookie's timestamp)

I tried to fix one of these types of situations; but because of 64-bit longs, it didn't work. The function extracted 4 bytes, so one would think uint32_t* would be an appropriate type, but instead, the argument is unsigned long *. In many of my previous projects, for networking code, we would use explicit uintXX_t types where appropriate. This is generally missing within OpenSSL.

tmshort · 2022-03-02T14:51:33Z

the system time_t is 32-bit but the system also provides a time64_t that is 64 bit

That thought had come to me too. Possibly use an openssl_time_t type (which could be public), but it then it complicates the APIs. It would make sense to always use 64-bit time regardless of the system, and do appropriate translation. But again, APIs.

t8m · 2022-10-24T11:17:43Z

The approval is stale, this needs to be rebased or abandoned.

t8m · 2025-04-30T10:53:05Z

@bernd-edlinger please rebase this or close if you are no longer interested in this PR.

openssl-machine · 2025-05-31T00:09:05Z

This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2025-07-01T00:09:23Z

This PR is waiting for the creator to make requested changes but it has not been updated for 61 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2025-07-30T00:09:59Z

This PR has been closed. It was waiting for the creator to make requested changes but it has not been updated for 90 days.

Fix time_t arithmetic after 2038

64eb07e

Some arithmetics using long when this type is 32 bit and time_t is 64 bit might break after 2038.

bernd-edlinger added branch: master Applies to master branch approval: review pending This pull request needs review by a committer branch: 3.0 Applies to openssl-3.0 branch labels Feb 14, 2022

t8m added the triaged: bug The issue/pr is/fixes a bug label Feb 14, 2022

bernd-edlinger mentioned this pull request Feb 14, 2022

Fix time_t arithmetic after 2038 [1.1.1] #17703

Closed

kaduk reviewed Feb 17, 2022

View reviewed changes

t8m added the approval: otc review pending label Feb 28, 2022

t8m approved these changes Feb 28, 2022

View reviewed changes

t8m removed the approval: otc review pending label Feb 28, 2022

kaduk reviewed Feb 28, 2022

View reviewed changes

kaduk mentioned this pull request Feb 28, 2022

Improve post-2038 compatibility of time_t usage #17778

Closed

paulidale approved these changes Mar 1, 2022

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Mar 1, 2022

bernd-edlinger mentioned this pull request Mar 19, 2022

Fix use of time_t internally and APIs #17786

Closed

2 tasks

t8m removed the approval: done This pull request has the required number of approvals label Oct 24, 2022

t8m added the branch: 3.1 Applies to openssl-3.1 (EOL) label Oct 24, 2022

t8m added the branch: 3.2 Applies to openssl-3.2 (EOL) label Oct 26, 2023

t8m added waiting-for: contributor response This pull request is awaiting a response by the contributor branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 branch: 3.5 Applies to openssl-3.5 and removed branch: 3.1 Applies to openssl-3.1 (EOL) labels Apr 30, 2025

openssl-machine closed this Jul 30, 2025

	__owur static ossl_inline int PACKET_peek_net_4(const PACKET *pkt,
	unsigned long *data)
	{
	if (PACKET_remaining(pkt) < 4)
	return 0;

	data = ((unsigned long)(pkt->curr)) << 24;
	data \|= ((unsigned long)((pkt->curr + 1))) << 16;
	data \|= ((unsigned long)((pkt->curr + 2))) << 8;
	data \|= (pkt->curr + 3);

	return 1;

Uh oh!

Comments

Conversation

bernd-edlinger commented Feb 14, 2022

Uh oh!

paulidale commented Feb 15, 2022

Uh oh!

bernd-edlinger commented Feb 16, 2022

Uh oh!

paulidale commented Feb 17, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger commented Feb 19, 2022

Uh oh!

t8m commented Feb 28, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmshort Feb 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaduk commented Feb 28, 2022

Uh oh!

paulidale commented Mar 1, 2022

Uh oh!

tmshort commented Mar 1, 2022

Uh oh!

tmshort commented Mar 1, 2022

Uh oh!

openssl-machine commented Mar 2, 2022

Uh oh!

paulidale commented Mar 2, 2022

Uh oh!

kaduk commented Mar 2, 2022

Uh oh!

tmshort commented Mar 2, 2022

Uh oh!

tmshort commented Mar 2, 2022

Uh oh!

t8m commented Oct 24, 2022

Uh oh!

t8m commented Apr 30, 2025

Uh oh!

openssl-machine commented May 31, 2025

Uh oh!

openssl-machine commented Jul 1, 2025

Uh oh!

openssl-machine commented Jul 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tmshort Feb 28, 2022 •

edited

Loading