Fix #74341: openssl_x509_parse fails to parse ASN.1 UTCTime without seconds

[maurice2k]

Added support for ASN.1 UTCTime without seconds part (being 11 characters long instead of 13)

[krakjoe]

This should target a development branch, and should come with tests.

[maurice2k]

@krakjoe, just added a test for the bugfix.
What else can I do to have the fix merged as soon as possible? Is 7.0.18 still possible (RC1 released today)?

[nikic]

Looks like the new test is failing on AppVeyor.

[maurice2k]

I don't know why but the Windows AppVeyor build system seems to have other defines set for the built or the system time is not correct. There's always a difference in the resulting timestamp +/- 1 hour, probably a DST issue. I tried the test with an official PHP build for windows and there's no problem with the timestamps.

There are other tests with date/time output that are deliberately skipped on windows which I find is rather strange. I've adjusted my test again with expected result using %d and hope it'll work now.

[nikic]

@weltling Do you know what might be the problem here?

[maurice2k]

Ok, I've found the problem: openssl_x509_parse uses mktime directly and applies DST itself and thus is completely date_default_timezone_set agnostic. Sounds like a problem that should be fixed because magic is happening here.

[weltling]

@nikic waiting for the latest run yet, gonna check when it failed.

Cheers.

[weltling]

Ahh, was too slow in respond. @maurice2k yeah, if the php.ini option is ignored, so we probably good to check with a placeholder.

Thanks.

[bukka]

As it was said, this is now pointless so shouldn't be in the test...

[maurice2k]

Merge into 7.0.18?

[weltling]

@maurice2k as @krakjoe mentioned, this should've been targeting the dev branch, thus PHP-7.0. Please do so next time, here's more https://wiki.php.net/vcs/gitworkflow. If there are no other issues, can indeed go with 7.0 so it'll land in 7.0.19 and up.

Thanks.

[nikic]

I'm not sure I fully understand the timezone issue. Unless I'm missing something, the time (at least in UTCTime Z format) is given in UTC, and the result is a Unix timestamp, so also in UTC. Shouldn't the result always be the same, regardless of timezone settings or DST?

[nikic]

This code looks pretty fishy to me: https://github.com/php/php-src/pull/2444/files#diff-69bad938d17f4283faa5f7fea17fa627R812 Why do we need to "overcorrect" by 3600 seconds if there is no DST?

[maurice2k]

Yes, exactly...

The "overcorrection" part is not in use as HAVE_TM_GMTOFF is defined as 1, at least on Linux.
But I believe it's still broken because with is_dst == 1 it subtracts 3600 seconds and otherwise adds them. I think there should be only subtraction...., should it?

I haven't touched that code and it's not part of the bugfix. But I'll have a look at that later on.

[maurice2k]

The reason for this code is that mktime is using the locale timezone settings but should be using UTC when interpreting the broken-down time struct as timestamp.

[maurice2k]

Let's see if my assumption was correct, otherwise I'll revert the last commit.

[nikic]

Looks like it worked, remaining failure is unrelated.

[nikic]

Merged as 46d2865 into 7.0+, so this will be in 7.0.19 and 7.1.5. Thanks!

[weltling]

Hmm, it looks like i could have done a wrong test fix in 5136048 then. Not sure why the offset was calculated that way, maybe it was necessary on older OS. I'll recheck this part, it might be better we don't rely on timezone at all, but calculate the offset on demand by getting difference between utc and local time.

Thanks.

[bukka]

So this was actually not correct and seems like https://www.obj-sys.com/asn1tutorial/node15.html (linked in the bug report) is not exactly valid reference. This was actually addressed in OpenSSL itself in openssl/openssl#23483 . Above is also linked PHP bug report shortly after that: #13343 .

I have just created the linked PR above to address this: #14439

[bukka]

The fix was merged to master so PHP 8.4 will no longer allow this.