Improve post-2038 compatibility of time_t usage by kaduk · Pull Request #17778 · openssl/openssl

kaduk · 2022-02-28T18:40:47Z

The Unix epoch will overflow a signed 32-bit integer in the year 2038,
and many systems are updating time_t to be 64-bits or to treat the time
as an unsigned integer as a result. To protect openssl against such
issues:

update s_time's internal logic to consistently use time_t for tracking
timeouts
update BIO_SSL to use a time_t to track the last renegotiation time,
rather than the ad hoc 'unsigned long'. The structure is private to
bio_ssl.c and so there is no external ABI breakage

The Unix epoch will overflow a signed 32-bit integer in the year 2038, and many systems are updating time_t to be 64-bits or to treat the time as an unsigned integer as a result. To protect openssl against such issues: - update s_time's internal logic to consistently use time_t for tracking timeouts - update BIO_SSL to use a time_t to track the last renegotiation time, rather than the ad hoc 'unsigned long'. The structure is private to bio_ssl.c and so there is no external ABI breakage

kaduk · 2022-02-28T18:41:11Z

Alternative to #17701

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

apps/s_time.c

    }
    totalTime += tm_Time_F(STOP); /* Add the time for this iteration */

-    i = (int)((long)time(NULL) - finishtime + maxtime);


I couldn't find anywhere that this value was used.
It seems plausible that at one point it was used in the printf call a couple lines later, but that the expression got duplicated as part of a cleanup change; I did not look into the history to try to confirm that.

kaduk · 2022-02-28T18:46:04Z

I'll also note that I only looked at places that #17701 was already touching, and did not do an independent survey of other time(3) usage

paulidale · 2022-03-01T00:28:50Z

This version seems clearer to me.

rsbeckerca · 2022-03-01T03:59:32Z

Some systems are keeping time_t as 32-bit and moving to time64_t. I know of three. This is not a permanent fix, however (permanent being a relative term). I think having an OpenSSL-specific time type may be the only rational way to go - OpenSSL_time_t and have the platform implement whatever it must to deal with 2038. Given how fast people are moving off of 1.0.1m (really, it's still in production), this is somewhat urgent.

bernd-edlinger · 2022-03-01T06:55:55Z

ssl/bio_ssl.c

-            tm = (unsigned long)time(NULL);
-            if (tm > sb->last_time + sb->renegotiate_timeout) {
+            tm = time(NULL);
+            if (tm - sb->last_time > (time_t)sb->renegotiate_timeout) {


my approach had the nice added value that it is robust if the system time is set back,
that's why I rewrote the condition to tm - bs->last_time > bs->renegotiate_timeout
I thought that's obvious.

tmshort · 2022-03-01T16:14:50Z

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

tmshort · 2022-02-28T18:43:48Z

apps/s_time.c


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


Overflow for 32-bit time_t systems in 2038.

#8687 includes timeoutcmp() for SSL_SESSION structures, that looks at overflow... It basically gives an extra bit of time (so on 32-bit systems, it's an extra 68 years...) Does that kick the can down the road? Yes, will 32-bit systems still exists after 2038? Probably? Will they have been updated with a 64-bit time_t? Who knows.

tmshort · 2022-02-28T18:52:08Z

apps/s_time.c

    totalTime = 0.0;

-    finishtime = (long)time(NULL) + maxtime;
+    finishtime = time(NULL) + maxtime;


Again overflow.

In general, I believe that finishtimes should no longer be used. Better to subtract the timeout value from "now", and compare to the starttime. In other words, keep two values around (start and duration) rather than finishtime.

I agree with this as a better approach

openssl-machine · 2022-04-01T00:07:12Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

openssl-machine · 2022-05-02T00:07:16Z

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

rsbeckerca · 2022-05-02T01:38:22Z

Where are we with this particular PR?

openssl-machine · 2022-06-02T00:07:31Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

openssl-machine · 2022-07-03T00:07:41Z

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

openssl-machine · 2022-08-03T00:07:55Z

This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago

openssl-machine · 2022-09-03T00:08:41Z

This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago

openssl-machine · 2022-10-04T00:08:52Z

This PR is in a state where it requires action by @openssl/committers but the last update was 154 days ago

levitte · 2022-10-04T02:47:27Z

We have OSSL_TIME nowadays. That might otherwise be useful here.
It's internal, but almost entirely made of static in-line functions implemented in a header file, so...

openssl-machine · 2022-11-24T00:09:25Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

openssl-machine · 2022-12-25T00:09:00Z

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

rsbeckerca · 2024-05-30T15:29:30Z

@rsbeckerca isn't this a system level issue then? I can't imagine that nonstop will just require all applications to implement their own version of time_t to address the 2038 issue, will they?

I think any solution will have to take into account that there is no consistency between 32-bit and 64-bit time implementations on different platforms. It is consistent in gcc and some 64-bit builds, but not universal. The time_t on S/390 when I was last on it was 32-bit even when building for 64-bit, which made me a bit wonky. The application had to adapt to use time64_t when on S/390 no matter what.

nhorman · 2024-05-30T16:36:03Z

@rsbeckerca ok, thats reasonable, but thats not what this PR does. My goal here was to motivate movement on this issue in some way, given that it has been idle for a year, be that to make arguments for its inclusion, or its rejection (either in favor of something else, or to close it entirely if there is not current interest to pursue it). So I suppose in my mind the first question is to @kaduk - do you want to address the comments made in this PR and continue to pursue its inclusion, or should we close this, and address the problem in a future issue?

openssl-machine · 2024-06-30T00:08:38Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

openssl-machine · 2024-07-31T00:08:23Z

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

openssl-machine · 2024-08-31T00:08:42Z

This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago

openssl-machine · 2024-10-01T00:08:40Z

This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago

kaduk · 2024-10-01T03:08:10Z

@rsbeckerca ok, thats reasonable, but thats not what this PR does. My goal here was to motivate movement on this issue in some way, given that it has been idle for a year, be that to make arguments for its inclusion, or its rejection (either in favor of something else, or to close it entirely if there is not current interest to pursue it). So I suppose in my mind the first question is to @kaduk - do you want to address the comments made in this PR and continue to pursue its inclusion, or should we close this, and address the problem in a future issue?

The discussion of what we want to do "in this space" (itself a vaguely defined matter) got split across quite a few PRs but after a brief survey it does seem like I should pick this one up again and we can get some small incremental improvements in even if we end up wanting to do a bigger cleanup pass later.

nhorman · 2024-10-01T07:23:37Z

ack, assigning to you, @openssl/committers can we pick up reviewing this please

hlandau

LGTM

t8m · 2024-10-04T15:22:00Z

Please note that we explicitly added new public APIs with time_t arguments as the type for holding time values instead of long. I do not think we are going to fix issues on platforms where time_t cannot hold time after 2038.

openssl-machine · 2024-11-04T00:08:26Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

openssl-machine · 2024-12-05T08:23:14Z

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

t8m · 2024-12-06T16:11:57Z

There are some unresolved comments from @tmshort and @bernd-edlinger .

@kaduk could you please respond to them?

openssl-machine · 2025-01-06T00:09:02Z

This PR is in a state where it requires action by @openssl/committers but the last update was 30 days ago

openssl-machine · 2025-02-06T00:08:40Z

This PR is in a state where it requires action by @openssl/committers but the last update was 61 days ago

openssl-machine · 2025-03-09T00:08:40Z

This PR is in a state where it requires action by @openssl/committers but the last update was 92 days ago

openssl-machine · 2025-04-09T00:09:04Z

This PR is in a state where it requires action by @openssl/committers but the last update was 123 days ago

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

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:57Z

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

kaduk mentioned this pull request Feb 28, 2022

Fix time_t arithmetic after 2038 #17701

Closed

kaduk commented Feb 28, 2022

View reviewed changes

paulidale approved these changes Mar 1, 2022

View reviewed changes

paulidale added approval: review pending This pull request needs review by a committer branch: 3.0 Applies to openssl-3.0 branch branch: master Applies to master branch triaged: bug The issue/pr is/fixes a bug labels Mar 1, 2022

bernd-edlinger reviewed Mar 1, 2022

View reviewed changes

tmshort reviewed Mar 1, 2022

View reviewed changes

kroeckx mentioned this pull request Mar 2, 2022

Fix use of time_t internally and APIs #17786

Closed

2 tasks

t8m closed this Jul 7, 2022

t8m reopened this Jul 7, 2022

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

nhorman requested a review from a team October 1, 2024 07:23

nhorman assigned kaduk Oct 1, 2024

nhorman added the help wanted label Oct 1, 2024

hlandau approved these changes Oct 4, 2024

View reviewed changes

t8m added branch: 3.3 Applies to openssl-3.3 branch: 3.4 Applies to openssl-3.4 labels Oct 4, 2024

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

openssl-machine closed this Jul 30, 2025

Uh oh!

Comments

Conversation

kaduk commented Feb 28, 2022

Uh oh!

kaduk commented Feb 28, 2022

Uh oh!

kaduk Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

kaduk commented Feb 28, 2022

Uh oh!

paulidale commented Mar 1, 2022

Uh oh!

rsbeckerca commented Mar 1, 2022

Uh oh!

bernd-edlinger Mar 1, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort commented Mar 1, 2022

Uh oh!

tmshort Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Feb 28, 2022

Choose a reason for hiding this comment

Uh oh!

tom-cosgrove-arm Jun 29, 2023

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Apr 1, 2022

Uh oh!

openssl-machine commented May 2, 2022

Uh oh!

rsbeckerca commented May 2, 2022

Uh oh!

openssl-machine commented Jun 2, 2022

Uh oh!

openssl-machine commented Jul 3, 2022

Uh oh!

openssl-machine commented Aug 3, 2022

Uh oh!

openssl-machine commented Sep 3, 2022

Uh oh!

openssl-machine commented Oct 4, 2022

Uh oh!

levitte commented Oct 4, 2022

Uh oh!

openssl-machine commented Nov 24, 2022

Uh oh!

openssl-machine commented Dec 25, 2022

Uh oh!

rsbeckerca commented May 30, 2024

Uh oh!

nhorman commented May 30, 2024

Uh oh!

openssl-machine commented Jun 30, 2024

Uh oh!

openssl-machine commented Jul 31, 2024

Uh oh!

openssl-machine commented Aug 31, 2024

Uh oh!

openssl-machine commented Oct 1, 2024

Uh oh!

kaduk commented Oct 1, 2024

Uh oh!

nhorman commented Oct 1, 2024

Uh oh!

hlandau left a comment

Choose a reason for hiding this comment

Uh oh!

t8m commented Oct 4, 2024

Uh oh!

openssl-machine commented Nov 4, 2024

Uh oh!

openssl-machine commented Dec 5, 2024

Uh oh!