Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: ASN1_TIME_to_tm issues with localtime #6387

Closed
colmenero opened this issue May 8, 2023 · 3 comments · Fixed by #6403
Closed

[Bug]: ASN1_TIME_to_tm issues with localtime #6387

colmenero opened this issue May 8, 2023 · 3 comments · Fixed by #6403
Assignees
Labels

Comments

@colmenero
Copy link

colmenero commented May 8, 2023

Contact Details

colmenero@rti.com

Version

v5.5.1-stable

Description

wolfSSL_ASN1_TIME_to_tm converts the time in an WOLFSSL_ASN1_TIME structure to a struct tm.

There is no documentation for this function in wolfSSL's User Manual. Instead, if we read the documentation provided by OpenSSL as a reference:

ASN1_TIME_to_tm() converts the time s to the standard tm structure. If s is NULL, then the current time is converted. The output time is GMT. The tm_sec, tm_min, tm_hour, tm_mday, tm_wday, tm_yday, tm_mon and tm_year fields of tm structure are set to proper values, whereas all other fields are set to 0. [...]

However, wolfSSL_ASN1_TIME_to_tm sets some of the members that are not listed there to a value different than 0. For example, we can get a tm structure with a non-zero value of tm_gmtoff or with the tm_isdst member set to 1. This is caused by the line at the end of Asn1TimeToTm where wolfSSL calls mktime(tm). This is something OpenSSL doesn't do. Therefore, I think the difference should be documented.

mktime also expects and modifies an input time structure expressed as a localtime. This is problematic when trying to reverse the operation and convert the tm structure returned by wolfSSL_ASN1_TIME_to_tm back to WOLFSSL_ASN1_TIME. To do so the user could call:

wolfSSL_ASN1_TIME_adj(&asn1_time, timegm(&tm_time), 0, 0);

But this yields an offset depending on the local time because timegm doesn't consider timezones or tm_isdst. For more information, see the reproducer.

Note that it also impacts wolfSSL_ASN1_TIME_diff. This function calls wolfSSL_ASN1_TIME_to_tm and uses the return values without considering that tm_gmtoff or tm_isdst may be set. The number of seconds returned by wolfSSL_ASN1_TIME_diff should at least account for the time zones.

Reproduction steps

The following file is a reproducer of the problem: ASN1_TIME_to_tm.zip.

It starts with an unix epoch. It first converts it to an ASN1_time and then to an struct tm. At that point we can already see that the values have an offset with respect to GMT. If we then try to convert the tm structure to an ASN1_time with ASN1_TIME_adj and timegm, we get an offset error. This offset doesn't happen in OpenSSL because its ASN1_TIME_to_tm returns a value with no GMT offset.

The initial time_t epoch is 1683278245
The ASN1_TIME time is: May  5 09:17:25 2023 GMT
The tm struct time is: 2023-05-05 10:17:25
The tm struct values are: tm_sec: 25, tm_min: 17, tm_hour: 10, tm_mday: 5, tm_mon: 4, tm_year: 123, tm_isdst: 1, tm_gmtoff: 7200
The ASN1_TIME second time is: May  5 10:17:25 2023 GMT

Relevant log output

No response

@colmenero colmenero added the bug label May 8, 2023
@kareem-wolfssl kareem-wolfssl self-assigned this May 8, 2023
@kareem-wolfssl
Copy link
Contributor

Hi @colmenero,

Thanks for the report and reproducer. I was able to reproduce this on the current master.
It looks like we should be manually clearing tm_isdst and tm_gmtoff after calling mktime, I will look into this to confirm and update you once I have a fix out.

Thanks,
Kareem

@kareem-wolfssl
Copy link
Contributor

kareem-wolfssl commented May 11, 2023

Hi @colmenero ,

Please give this another try with #6403 applied. I see the issue fixed for me with your reproducer, but would like to confirm.

@colmenero
Copy link
Author

Hi @kareem-wolfssl,

I applied the changes in #6403 to the 5.5.1 wolfSSL that we are using and it fixes the issue that we were experiencing. It would be amazing if the fix made it to master.

Thanks a lot for the quick response!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants