openssl_x509_parse should not allow omitted seconds in UTCTimes

[botovq]

Description

PR #2444, "openssl_x509_parse fails to parse ASN.1 UTCTime without seconds" added support for UTCTimes in certificates omitting seconds. This seems incorrect. While it is true that seconds are optional in the ASN.1 specification of UTCTime in ITU T-REC X.680 section 47.3, the same is not true for its DER encoding, in particular for certificates or CRLs. RFC 5280, 4.1.2.5.1:

   For the purposes of this profile, UTCTime values MUST be expressed in
   Greenwich Mean Time (Zulu) and MUST include seconds (i.e., times are
   YYMMDDHHMMSSZ), even where the number of seconds is zero.

ITU-TREC X.690, section 11.8.2 also states explicitly: "the seconds shall always be present".

Similar statements hold true for GeneralizedTime. (For some reason no exception was made despite the fact that the ASN.1 spec allows omitting minutes or seconds).

As an aside, it is a bit dangerous to use strlen() on the data in an ASN1_STRING. These have historically been NUL terminated in OpenSSL, but this is an implementation detail, not a documented API contract. There is no real reason for them to be NUL-terminated: ASN.1 strings are length-prefixed strings.

PHP Version

PHP 8.3.2

Operating System

No response

[bukka]

Apology for completely missing this issue. I just stumbled on it when I investigate break in that test which happened due to changes in OpenSSL 3.3 correctly prohibiting missing seconds and breaking that test. That PR really seems incorrect to me and we should prohibit such certs as well.

[bukka]

Just created #14439 to address this which is slightly modified revert of the previous PR. I'm not sure yet if we should target 8.2...

[botovq]

Thanks for looking into it.

I'm not sure yet if we should target 8.2...

Do you expect people to report test failures when they're using 8.2 linked against OpenSSL >= 3.3?

[bukka]

The thing is that OpenSSL did not consider this as a bug for stable versions but it went to minor version. So if we followed their reasoning, it would probably just make sense to apply this to master only which means PHP 8.4. Also this change is just for versions below OpenSSL 3.3 as 3.3 won't even load the cert.

[bukka]

Also I plan to change that strlen to strnlen (we have got a wrap around that in master called zend_strnlen).

[bukka]

I decided to merge the change only to master as it doesn't seem worth it to break it in stable versions and we should update UPGRADING notes for this as it is in some way a BC break. So it will be part of 8.4.

In addition I also created a PR for suggested strlen usage: #14522

[bukka]

It's also worth to link openssl/openssl#23483 (comment) which correctly points out that even OpenSSL 3.3 is still not strictly implementing DER. I think it's best to do those kind of changes in master only though.

[botovq]

I'm perfectly happy with your reasoning and the approach you're taken. Thank you!