Repository navigation
openssl_x509_parse should not allow omitted seconds in UTCTimes #13343
Description
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
Activity
bukka commentedon Jun 2, 2024
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 commentedon Jun 2, 2024
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 commentedon Jun 2, 2024
bukka commentedon Jun 2, 2024
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 commentedon Jun 2, 2024
Also I plan to change that strlen to strnlen (we have got a wrap around that in master called zend_strnlen).
Fix phpGH-13343: openssl_x509_parse should not allow omitted seconds …
bukka commentedon Jun 9, 2024
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 commentedon Jun 9, 2024
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 commentedon Jun 10, 2024
Fixed ASN.1 UTCTime parsing