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

tmshort · 2022-08-31T19:11:34Z

This changes internal (libssl) times to use ossl_time.

Replacement public APIs (for backwards compatibility) are introduced to
support time_t in the interface (rather than long).

Checklist

documentation is added or updated
tests are added or updated

This changes internal (libssl) times to use ossl_time. Replacement public APIs (for backwards compatibility) are introduced to support time_t in the interface (rather than long).

paulidale · 2022-09-01T01:19:34Z

Seems like some overlap between this and #19082...

tmshort · 2022-09-01T13:32:44Z

Seems like some overlap between this and #19082...

Yes, I must've missed that one!

paulidale

Looks good and there are some changes here that aren't in #19082

paulidale · 2022-09-02T03:03:43Z

apps/s_time.c

 #define SECONDSSTR "30"

+/* not as accurate as in the library, but no worse than original code */
+OSSL_TIME ossl_time_now(void)


Why not include crypto/time.c and get the accurate version?

paulidale · 2022-09-02T03:06:18Z

doc/man3/OPENSSL_gmtime.pod

+OPENSSL_gmtime_diff() calculates the difference between the two B<struct tm>
+structures B<from> and B<to>. The difference in days is placed into B<pday>,
+the remaining seconds are placed to B<psec>. The value in B<psec> will be less
+than the number of seconds per day (3600). Leap seconds are not considered.


86400 seconds in a day not 3600.

Nah... I'm just going to restrict it to an hour... whoops.

paulidale · 2022-09-02T03:08:55Z

doc/man3/SSL_CTX_set_timeout.pod


 long SSL_CTX_set_timeout(SSL_CTX *ctx, long t);
 long SSL_CTX_get_timeout(SSL_CTX *ctx);
+ time_t SSL_CTX_set_timeout_ex(SSL_CTX *ctx, time_t t);


The timeout is a duration isn't it. As such, I don't see a need to accept a time_t. long is more than sufficient.

Duration and let call it "calandar time" are related but distinct concepts. Duration is not fixed in time, whereas a calendar time is. However, calendar time is nothing but a duration from a fixed point in time. So, they really could be the same type. Both are effectively bound by the same constraints, one of which is the size/type of storage. The OSSL_TIME code itself allows for subtraction (which results in a duration) and addition (one of which must be a duration). sleep(3) takes a duration in seconds, as an unsigned int. But I swear there was a type specific for durations, but I can't find it.

There are more than a few variations documented:

Call Type

alarm(2) unsigned int

clock(2) clock_t

clock_gettime(2) struct timespec

epoll_wait(2) int

getitimer(2)` struct timeval

poll(2) int

sleep(3) unsigned int

usleep(3) useconds_t

I can't locate a specific duration type either.

paulidale · 2022-09-02T03:09:43Z

doc/man3/SSL_SESSION_get_time.pod


+ time_t SSL_SESSION_get_time_ex(const SSL_SESSION *s);
+ time_t SSL_SESSION_set_time_ex(SSL_SESSION *s, time_t tm);
+ time_t SSL_SESSION_get_timeout_ex(const SSL_SESSION *s);


Again, not sure we need or want timeouts using time_t.

paulidale · 2022-09-02T03:10:18Z

doc/man3/SSL_get_default_timeout.pod

 #include <openssl/ssl.h>

 long SSL_get_default_timeout(const SSL *ssl);
+ time_t SSL_get_default_timeout_ex(const SSL *ssl);


tmshort · 2022-09-02T13:35:12Z

Looks good and there are some changes here that aren't in #19082

I figure, let's get #19082 in first, and then if there's anything left in here that's worthwhile, I can rebase.

kroeckx · 2022-09-03T17:11:44Z

Adding new functions with a time_t seems wrong. We should deprecate all public function with a time_t. If time_t is only used internally, we can fix Y2038 problems.

rsbeckerca · 2022-09-03T17:24:25Z

We should not use time_t internally as there are 32-bit builds that will not work in Epoch 1.

kroeckx · 2022-09-03T20:47:53Z

To clarify, we should only use time_t when needed to talk to the OS. Some OSs provide multiple interfaces, and might have one that doesn't have the Y2038 problem. That might be multiple interfaces that use time_t. Some OSs will not provide a Y2038 compatible interface, and I'm not sure if that's up to us to fix or not.

tmshort · 2022-09-06T13:56:53Z

To clarify, we should only use time_t when needed to talk to the OS. Some OSs provide multiple interfaces, and might have one that doesn't have the Y2038 problem. That might be multiple interfaces that use time_t. Some OSs will not provide a Y2038 compatible interface, and I'm not sure if that's up to us to fix or not.

Yes, but... the user/application talks to the OS and OpenSSL, and it's likely they already have a time_t value that they want to use with OpenSSL.

kroeckx · 2022-09-06T20:49:00Z

doc/man3/SSL_SESSION_get_time.pod


+SSL_SESSION_get_time_ex(), SSL_SESSION_set_time_ex(),
+SSL_SESSION_get_timeout_ex(), and SSL_SESSION_set_timeout_ex() functions are
+synonyms for the SSL_SESSION_*() (without the "_ex") counterparts.


I understand that as: #define SSL_SESSION_get_time_ex SSL_SESSION_get_time

kroeckx · 2022-09-06T20:51:55Z

It seems to me that most of the functions here actually need a time_t: - It's about a timeout, which is relative, not some specific time. - SSL_CTX_flush_sessions also seems to be used relative to the current time, and maybe it should just take a relative time instead of a time_t. The only exception is SSL_SESSION_get_time and related functions, which take a specific point in time. This can be changed to a type that is Y2038 compatible, it does not need to be a time_t.

rsbeckerca · 2022-09-06T21:55:36Z

My view is using something like an OSSL_time part of the API for all times in 3.1 instead of time_t. The issue is for platforms that are restricted to signed 32-bit time_t builds, which sadly still exist even on some 64-bit platforms. But I've said that before. time_t, in principle, is vulnerable to not knowing how the build is done unless there is a restriction put in place that would break the API anyway. I have seen applications fail that hit this for expiring self-signed certificates well into the future. A platform-specific hook, perhaps, to get around this would be wise. In my company's core app, we are temporarily reworking time_t interpretation so that anything prior to 1970 (time_t <0) is interpreted as next Epoch (>2038), since we have no possible artifacts before 1978. At least in this situation, the application will survive into 2100. The main system time goes beyond year 3000 in microseconds, but cannot be represented in the 32-bit time_t - a platform hook to allow me to get to that and use it in OpenSSL would be good. If customers want me to fix it beyond that point, some resurrection tech would be required ;)

tmshort · 2022-09-21T18:05:58Z

I'm abandoning this, but going to create a PR for the OPENSSL_gmtime functions.

Fix use of time_t internally and APIs

ff1ab8c

This changes internal (libssl) times to use ossl_time. Replacement public APIs (for backwards compatibility) are introduced to support time_t in the interface (rather than long).

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

tmshort mentioned this pull request Aug 31, 2022

Fix use of time_t internally and APIs #17786

Closed

2 tasks

fixup! Fix use of time_t internally and APIs

d01f2d6

tmshort mentioned this pull request Sep 1, 2022

Use OSSL_TIME more widely #19082

Closed

2 tasks

paulidale reviewed Sep 2, 2022

View reviewed changes

kroeckx reviewed Sep 6, 2022

View reviewed changes

tmshort closed this Sep 21, 2022

Call	Type
`alarm(2)`	`unsigned int`
`clock(2)`	`clock_t`
`clock_gettime(2)`	`struct timespec`
`epoll_wait(2)`	`int`
getitimer(2)`	`struct timeval`
`poll(2)`	`int`
`sleep(3)`	`unsigned int`
`usleep(3)`	`useconds_t`

Uh oh!

Comments

Conversation

tmshort commented Aug 31, 2022

Checklist

Uh oh!

paulidale commented Sep 1, 2022

Uh oh!

tmshort commented Sep 1, 2022

Uh oh!

paulidale left a comment

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!

tmshort commented Sep 2, 2022

Uh oh!

kroeckx commented Sep 3, 2022

Uh oh!

rsbeckerca commented Sep 3, 2022

Uh oh!

kroeckx commented Sep 3, 2022 via email

Uh oh!

tmshort commented Sep 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kroeckx commented Sep 6, 2022 via email

Uh oh!

rsbeckerca commented Sep 6, 2022

Uh oh!

tmshort commented Sep 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants