Fix use of time_t internally and APIs by tmshort · Pull Request #17786 · openssl/openssl

tmshort · 2022-03-01T16:13:30Z

WIP: to get initial feedback on how to handle time.

This WIP PR changes internal use of time to use time_t. Replacement public APIs (for backwards compatibility) are introduced to allow time_t through the system.

A new function OPENSSL_time_t_compare() is defined that accepts time_t values and offsets and can be used anywhere comparison of time is necessary.

There's still work to be done, but it's a proposal. Tests and documentation will also be updated.

Checklist

documentation is added or updated
tests are added or updated

tmshort · 2022-03-01T16:18:00Z

I'm expecting most of the checks to fail, since this is incomplete.

This changes internal times to use time_t. Replacement public APIs (for backwards compatibility) are introduced to allow time_t throughout the system. A new function OPENSSL_time_t_compare() is defined that accepts time_t values and offsets and can be used anywhere comparison of time is necessary.

tmshort · 2022-03-02T14:47:36Z

I still need to update util/missingcrypto.txt, but will wait on that if there are additional changes... as everything passes checks now. :D

tmshort · 2022-03-02T15:18:35Z

ssl/statem/extensions_srvr.c

    size_t rawlen, hmaclen, hrrlen, ciphlen;
-    unsigned long tm, now;
+    time_t now;
+    unsigned long tm;


PACKET_get_net_4() has an unsigned long * argument, which can be either 32-bit or 64-bit depending on the system; so a temporary variable is needed. That function (and related functions) should probably be changed to always use a sized-specified (e.g. 32-bit) type. A temporary is needed regardless as time_t could be 64-bit or 32-bit.

kroeckx · 2022-03-02T21:33:04Z

#17778 contains some of the same changes.

We now seem to have 3 PRs that want to do something about the Y2038 problem.

I'm currently not convinced that using time_t itself is the way to go ahead. My understanding is that at least glibc requires you to set a define to change from a 32 to 64 bit time_t on platforms that started as 32 bit. We probably want to change all platforms to 64 bit at some point. But if we use time_t in our public APIs, that's an ABI change. The only solution that I currently see is that we create our own type that's guaranteed to be 64 bit in the public APIs and deprecate the others.

tmshort · 2022-03-02T21:49:10Z

#17778 contains some of the same changes.

Yes, this includes the changes from that PR.

This is more comprehensive, adding new APIs to use time_t-types, so it maintains ABI compatibility. It would be easy to update the new APIs to move from time_t to e.g. openssl_time_t, defined as a 64-bit type.

The question becomes, does OpenSSL want to standardize on a time-type, or keep the APIs the current mish-mash that they are. The ASN.1 and X509 APIs are fairly consistent in their use of time_t

bernd-edlinger

I think this approach is way too complicated

bernd-edlinger · 2022-03-05T08:24:23Z

ssl/statem/extensions_srvr.c

-    if (tm > now || (now - tm) > 600) {
+    now = time(NULL);
+    if (OPENSSL_time_t_compare(now, 0, (time_t)tm, 0) < 0
+            || OPENSSL_time_t_compare(now, 0, (time_t)tm, 600) > 0) {


This can't work because this comparison needs to be done in 32 bit arithmetic.

No, it doesn't; it should be done in 64-bit arithmetic, regardless of time_t size.
Can you please explain why 32-bit arithmetic needs to be used?

The time stored in the cookie is currently a uint32_t. But we could easily change that...

The variable tm is a long, on 64-bit system, but stores only a 32-bit value, because of this:

PACKET_get_net_4(&cookie, &tm)

The PACKET_get_net_4() function really ought to only use an int32_t* or uint32_t*, but takes a unsigned long* which is 64-bits. So, the cookie time value is put into a 64-bit value on a 64-bit system.

Ok, the original code was likely also problematic, because on a 64-bit system only 4 bytes of the 8-byte unsigned long were filled in, and thus the 32-bit cookie time value was not sign-extended. Because it's not sign-extended, the code treats times pre-1970 as post-2038 (EDIT: on 32-bit systems).

The time stored in the cookie is currently a uint32_t. But we could easily change that...

Yeah, it probably should be a uint64_t, but that would make this much more complicated.

The time is put into the cookie as:

WPACKET_put_bytes_u32(pkt, (unsigned int)time(NULL))

Past 2038, for a 64-bit time_t, when it's extracted out, it will continue to be a treated properly, because it's not sign-extended when it's extracted. This effectively gives OpenSSL's cookie an 68 extra years (until 2106).

Past 2038, for a 32-bit time_t, there are problems, as we don't know what the behavior of time(NULL) is, it could wrap, it could give -1, it could give MAX_INT.

The value in tm is always going to be only 32-bits, regardless of sizeof(tm).
Common situations:
If sizeof(unsigned long) == sizeof(time_t) == 4 then the system can only safely handle time until 2038; as the time value put into the cookie is unknown.
If sizeof(unsigned long) == sizeof(time_t) == 8 then the code can only safely handle time until 2106. The unsigned tm is not sign-extended when converted to a time_t. The subsequent 64-bit arithmetic works.
Less common situations:
If sizeof(unsigned long) == 4 && sizeof(time_t) == 8 then the code can only safely handle time until 2106. The unsigned tm is not signed extended when converted to a time_t. The subsequent 64-bit arithmetic works.
if sizeof(unsigned long) == 8 && sizeof(time_t) == 4, then the system can only safely handle time until 2038; as the time value put into the cookie is unknown.

The sizeof(unsigned long) == 4 && sizeof(time_t) == 8 is actually common for "recent" 32 bit architecture.

Is there a reason we can't just change tm to an uint64_t?

tmshort · 2022-03-08T20:57:47Z

I think this approach is way too complicated

This approach provides a consistent mechanism for using time_t.
It makes all internal time calculations use the standard time_t.

I disagree with the complexity. Admittedly, it is "big", but OpenSSL ought to move to using proper and consistent types for time (e.g. time_t), rather than mixing different types.

rsbeckerca · 2022-03-08T21:52:51Z

I think this approach is way too complicated

This approach provides a consistent mechanism for using time_t. It makes all internal time calculations use the standard time_t.

I disagree with the complexity. Admittedly, it is "big", but OpenSSL ought to move to using proper and consistent types for time (e.g. time_t), rather than mixing different types.

FWIW: Some platforms use time64_t (z/OS, NonStop) for their 64-bit times for historical reasons. Some (z/OS) use time64_t time64(NULL) for fetching 64-bit time values with a _LARGE_TIME_API knob. There is no decided solution for getting 64-bit times yet on NonStop although one could be made to work with time64_t. AFIAK, these types are signed.

tmshort · 2022-03-08T22:22:20Z

FWIW: Some platforms use time64_t (z/OS, NonStop) for their 64-bit times for historical reasons. Some (z/OS) use time64_t time64(NULL) for fetching 64-bit time values with a _LARGE_TIME_API knob. There is no decided solution for getting 64-bit times yet on NonStop although one could be made to work with time64_t. AFIAK, these types are signed.

I think that if OpenSSL were to adopt it's own 64-bit time type and APIs, it would use time64_t where appropriate. Linux doesn't have a time64_t as an example, so this ends up being an abstraction layer. Now... that would be complicated.

rsbeckerca · 2022-03-08T22:25:02Z

FWIW: Some platforms use time64_t (z/OS, NonStop) for their 64-bit times for historical reasons. Some (z/OS) use time64_t time64(NULL) for fetching 64-bit time values with a _LARGE_TIME_API knob. There is no decided solution for getting 64-bit times yet on NonStop although one could be made to work with time64_t. AFIAK, these types are signed.

I think that if OpenSSL were to adopt it's own 64-bit time type and APIs, it would use time64_t where appropriate. Linux doesn't have a time64_t as an example, so this ends up being an abstraction layer. Now... that would be complicated.

It's worse. On some 64-bit platforms, sizeof(unsigned long) == 4 and sizeof(unsigned long long) == 8.

tmshort · 2022-03-08T22:34:02Z

It's worse. On some 64-bit platforms, sizeof(unsigned long) == 4 and sizeof(unsigned long long) == 8.

You mean... Windows? (LLP64)

In this case, we aren't dealing with unsigned long long... I did evaluate the sizes of the types in question for one of the code comments above.

rsbeckerca · 2022-03-08T22:52:12Z

It's worse. On some 64-bit platforms, sizeof(unsigned long) == 4 and sizeof(unsigned long long) == 8.

You mean... Windows? (LLP64)

In this case, we aren't dealing with unsigned long long... I did evaluate the sizes of the types in question for one of the code comments above.

From my testing, the defaults on z/OS and NonStop also are like that, with long being 4 bytes. So more than just Windows.

kroeckx · 2022-03-08T23:33:48Z

Linux doesn't have a time64_t as an example

glibc has a __time64_t. But you should always use time_t with the APIs that use a time_t.

glibc sets __TIMESIZE == 32 for architectures that have both 32 and 64 bit time_t, and __TIMESIZE == 64 for those that always have a 64 bit time_t.

You can change time_t to be 64 bit by using _TIME_BITS=64 (also requires _FILE_OFFSET_BITS=64). This is supported since glibc 2.34.

That means that if internally we want to use a 64 bit time_t, we would need to build with -D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64. But if we also have time_t in our public API, applications need to have the same defines set, or things will break. We probably want to use a fixed 64 bit type for public APIs.

rsbeckerca · 2022-03-08T23:43:56Z

Linux doesn't have a time64_t as an example

glibc has a __time64_t. But you should always use time_t with the APIs that use a time_t.

glibc sets __TIMESIZE == 32 for architectures that have both 32 and 64 bit time_t, and __TIMESIZE == 64 for those that always have a 64 bit time_t.

You can change time_t to be 64 bit by using _TIME_BITS=64 (also requires _FILE_OFFSET_BITS=64). This is supported since glibc 2.34.

That means that if internally we want to use a 64 bit time_t, we would need to build with -D_TIME_BITS=64 -D_FILE_OFFSET_BITS=64. But if we also have time_t in our public API, applications need to have the same defines set, or things will break. We probably want to use a fixed 64 bit type for public APIs.

Not all platforms have glibc 2.34 - mine certainly does not. I think we are limited to 2.12 or something near there. There are incompatible dependencies that were added forcing gcc that are not available (as in gcc does not port).

tmshort · 2022-03-09T14:57:28Z

We probably want to use a fixed 64 bit type for public APIs.

That is what I was thinking. An openssl-specific time type (e.g. openssl_time_t).

tmshort · 2022-03-09T14:58:59Z

From my testing, the defaults on z/OS and NonStop also are like that, with long being 4 bytes. So more than just Windows.

Yes, but Windows has just a few more deployments than those OS's.

rsbeckerca · 2022-03-09T15:07:14Z

From my testing, the defaults on z/OS and NonStop also are like that, with long being 4 bytes. So more than just Windows.

Yes, but Windows has just a few more deployments than those OS's.

Only a few orders of magnitude ;)

kaduk

Without getting too much into the details of the implementation, it seems like the key question for us to answer is whether we want to use time_t in our public APIs.
From a standardization perspective, time_t is clearly the right time to use, but the glibc behavior of making its ABI dependent on a preprocessor symbol, and the decision of some platforms to fix time_t at 32-bits and only offer a 64-bit time64_t muddies the waters quite a bit. On the whole, like Kurt, I'm leaning towards using an int64_t for public APIs.

It does seem clear that we should use time_t internally and convert right at the public-API boundary, and the possibility for a 64-bit time_t drives the choice of int64_t as the type used for the public API.

rsbeckerca · 2022-03-14T16:50:08Z

News: I just had a conversation with the glibc team. They appear to be moving to exclusively Linux support. Other platforms appear to be are excluded for the future. I have an email I can share privately about it. Depending on glibc is not a good idea unless OpenSSL is going down the same path.

kaduk · 2022-03-14T16:57:02Z

@rsbeckerca that mail sounds juicy :)
I mention glibc only in that it's the default libc on most major linux distributions, i.e., something we cannot ignore.
I was not proposing that we rely on it as an intrinsic dependency of openssl.

kaduk · 2022-03-14T16:58:16Z

It does seem clear that we should use time_t internally and convert right at the public-API boundary, and the possibility for a 64-bit time_t drives the choice of int64_t as the type used for the public API.

Well, hmm, that's not actually quite right. We want to use time64_t internally if available, so I should have said "a custom internal typedef that is usually time_t" here.

tmshort · 2022-03-18T12:57:49Z

Where do we want to go with time then? The conversation has sputtered out.

rsbeckerca · 2022-03-18T13:13:07Z

Where do we want to go with time then? The conversation has sputtered out.

My preference is to have something like OPENSSL_time as an explicit type, something like a standard C unsigned long long (64 bits) in some units (nanoseconds, microseconds, or seconds?) from some epoch starting point or signed long long from 1 Jan 1970. Platforms can map to/from that with a wrapper, say OPENSSL_time OPENSSL_from_time_t(time_t val) and time_t OPENSSL_to_time_t(OPENSSL_time), and OPENSSL_time OPENSSL_now() potentially controlled in the Configuration infrastructure to select the time_t style or attributes instead of having knobs for each platform. Depending on time_t being something useful from a portable standpoint does not look viable.

tmshort · 2022-03-18T13:33:24Z

I think that a common time reference (i.e. POSIX use of seconds since Jan 1, 1970 0:00:00) is at least a good starting point, as it makes translation and API use easier. That would allow SSL_function(time(NULL)) to still be applicable on most (if not all) platforms.

rsbeckerca · 2022-03-18T13:49:20Z

I think that a common time reference (i.e. POSIX use of seconds since Jan 1, 1970 0:00:00) is at least a good starting point, as it makes translation and API use easier. That would allow SSL_function(time(NULL)) to still be applicable on most (if not all) platforms.

I agree with that. That usage should still up-convert to any current platform representation without change. Where I am going on our platform is trying to implement a 64-bit time_t variant that will work long-term, at least until HPE comes up with their own solution in the future. I can base that off of seconds since Jan 1, 1970 0:00:00 also but run until 3000 at least - they can try to resurrect me to fix it at that point.

rsbeckerca · 2022-03-18T22:57:26Z

I checked with the NonStop dev team and time_t is 64 bits if the build explicitly specifies 64-bit builds, which covers 4 of our 15 standard platform builds across 1.1.1x and 3.0.x. We still have an issue with 4 of the 8 3.0.x 32-bit builds that are required for historical reasons and are not going away any time soon and will continue into the 3.1.x series. The 32-bit builds represent the bulk of what is being used on the platform and cannot be change due to a few required 32-bit system legacy libraries that are in use. We can live with the 1.1.1x series being subject to Y2038, so it really is the 4 modern 32-bit builds that are in question.

bernd-edlinger · 2022-03-19T06:45:23Z

For our internal use (observing timeouts and so) we only need a projection from the time_t to a 32-bit value.
given two times t0 and t1 we define:

t0 < t1 := (int)((uint32_) t0 - (uint32_t) t1) < 0
t0 > t1 := (int)((uint32_t) t0 - (uint32_t) t1) > 0
t0 == t1 := (int)((uint32_t) t0 - (uint32_t) t1) == 0

that covers 68 years in the future and in the past.
Using these definitions You end up with what I did in #17701

openssl/ssl/statem/extensions_srvr.c

Lines 805 to 806 in 64eb07e

    
           now = (unsigned long)time(NULL); 
        
           if ((uint32_t)(now - tm) > 600) {

since (uint32_t)(now - tm) > 600
is equivalent to: now < tm || now > tm + 600
when < and > is defined in terms of the definitions above.

mspncp · 2022-03-25T07:42:16Z

For sake of completeness: on Windows, we can't rely on time_t to be 64bits throughout, as long as we support 32bit platforms.

https://docs.microsoft.com/en-us/cpp/c-runtime-library/time-management

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/time-time32-time64#remarks

rsbeckerca · 2022-03-25T11:46:31Z

For sake of completeness: on Windows, we can't rely on time_t to be 64bits throughout, as long as we support 32bit platforms.

https://docs.microsoft.com/en-us/cpp/c-runtime-library/time-management

https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/time-time32-time64#remarks

Same conclusion as on NonStop and possibly z/OS.

openssl-machine · 2022-04-25T00:07:17Z

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

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

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

openssl-machine · 2022-06-26T00:07:33Z

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

openssl-machine · 2022-07-27T00:07:45Z

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

openssl-machine · 2022-08-27T00:08:44Z

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

rsbeckerca · 2022-08-31T19:12:36Z

Was this intended to be closed without resolution or did I miss something?

tmshort · 2022-08-31T19:16:12Z

I closed it as I put up a newer version that uses OSSL_TIME internally, #19107

This was referenced Mar 1, 2022

Improve post-2038 compatibility of time_t usage #17778

Closed

Fix time_t arithmetic after 2038 #17701

Closed

Fix time_t arithmetic after 2038 [1.1.1] #17703

Closed

Update the SSL time APIs #17658

Open

github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Mar 1, 2022

tmshort force-pushed the fix-time_t-apis branch from 3f154ee to f23d700 Compare March 2, 2022 00:30

tmshort changed the title ~~WIP: Fix use of time_t internally and APIs~~ Fix use of time_t internally and APIs Mar 2, 2022

tmshort and others added 4 commits March 1, 2022 22:21

fixup! Fix use of time_t internally and APIs

0dcb338

fixup! Fix use of time_t internally and APIs

dd270d3

fixup! Fix use of time_t internally and APIs

720cf4f

fixup! Fix use of time_t internally and APIs

1b55e6c

tmshort requested review from bernd-edlinger, kaduk and paulidale March 2, 2022 14:46

tmshort commented Mar 2, 2022

View reviewed changes

tmshort added approval: otc review pending approval: review pending This pull request needs review by a committer branch: master Applies to master branch labels Mar 4, 2022

bernd-edlinger requested changes Mar 5, 2022

View reviewed changes

kaduk reviewed Mar 14, 2022

View reviewed changes

t8m added triaged: feature The issue/pr requests/adds a feature triaged: refactor The issue/pr requests/implements refactoring labels Mar 14, 2022

tmshort closed this Aug 31, 2022

Uh oh!

Comments

Conversation

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

Uh oh!

Checklist

Uh oh!

tmshort commented Mar 1, 2022

Uh oh!

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

Uh oh!

Uh oh!

tmshort Mar 2, 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!

kroeckx commented Mar 2, 2022

Uh oh!

tmshort commented Mar 2, 2022

Uh oh!

bernd-edlinger left a comment

Choose a reason for hiding this comment

Uh oh!

bernd-edlinger Mar 5, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Mar 8, 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!

kaduk Mar 8, 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!

tmshort Mar 8, 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!

tmshort Mar 8, 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!

tmshort Mar 8, 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!

tmshort Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

kroeckx Mar 8, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort commented Mar 8, 2022

Uh oh!

rsbeckerca commented Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmshort commented Mar 8, 2022

Uh oh!

rsbeckerca commented Mar 8, 2022

Uh oh!

tmshort commented Mar 8, 2022

Uh oh!

rsbeckerca commented Mar 8, 2022

Uh oh!

kroeckx commented Mar 8, 2022

Uh oh!

rsbeckerca commented Mar 8, 2022

Uh oh!

tmshort commented Mar 9, 2022

Uh oh!

tmshort commented Mar 9, 2022

Uh oh!

rsbeckerca commented Mar 9, 2022

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

tmshort commented Mar 1, 2022 •

edited

Loading

tmshort commented Mar 2, 2022 •

edited

Loading

tmshort Mar 2, 2022 •

edited

Loading

tmshort Mar 8, 2022 •

edited

Loading

kaduk Mar 8, 2022 •

edited

Loading

tmshort Mar 8, 2022 •

edited

Loading

tmshort Mar 8, 2022 •

edited

Loading

tmshort Mar 8, 2022 •

edited

Loading

rsbeckerca commented Mar 8, 2022 •

edited

Loading

rsbeckerca commented Mar 18, 2022 •

edited

Loading