Fix UB and error propagation when X509_gmtime_adj() fails#21046
Closed
ndossche wants to merge 1 commit intophp:PHP-8.4from
Closed
Fix UB and error propagation when X509_gmtime_adj() fails#21046ndossche wants to merge 1 commit intophp:PHP-8.4from
ndossche wants to merge 1 commit intophp:PHP-8.4from
Conversation
This causes UB later on when the certificate is passed to another
function:
```
/work/php-src/Zend/zend_string.h:191:2: runtime error: null pointer passed as argument 2, which is declared to never be null
#0 0x55cfb9407d94 in zend_string_init /work/php-src/Zend/zend_string.h:191
#1 0x55cfb941ceb6 in add_assoc_stringl_ex /work/php-src/Zend/zend_API.c:1986
#2 0x55cfb7f4c16d in add_assoc_stringl /work/php-src/Zend/zend_API.h:579
#3 0x55cfb7f4cccd in php_openssl_add_assoc_asn1_string /work/php-src/ext/openssl/openssl_backend_common.c:113
#4 0x55cfb7f2eb98 in zif_openssl_x509_parse /work/php-src/ext/openssl/openssl.c:1074
#5 0x55cfb9160993 in zend_test_execute_internal /work/php-src/ext/zend_test/observer.c:306
#6 0x55cfb958ee2d in ZEND_DO_FCALL_SPEC_RETVAL_USED_HANDLER /work/php-src/Zend/zend_vm_execute.h:2154
#7 0x55cfb97854bd in execute_ex /work/php-src/Zend/zend_vm_execute.h:116519
#8 0x55cfb9795c96 in zend_execute /work/php-src/Zend/zend_vm_execute.h:121962
#9 0x55cfb99666c6 in zend_execute_script /work/php-src/Zend/zend.c:1980
#10 0x55cfb919583e in php_execute_script_ex /work/php-src/main/main.c:2645
#11 0x55cfb9195b48 in php_execute_script /work/php-src/main/main.c:2685
#12 0x55cfb996bf48 in do_cli /work/php-src/sapi/cli/php_cli.c:951
#13 0x55cfb996e6a1 in main /work/php-src/sapi/cli/php_cli.c:1362
#14 0x7fb0b68301c9 (/lib/x86_64-linux-gnu/libc.so.6+0x2a1c9) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
#15 0x7fb0b683028a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2a28a) (BuildId: 274eec488d230825a136fa9c4d85370fed7a0a5e)
#16 0x55cfb7e097d4 in _start (/work/php-src/build-dbg-ubsan/sapi/cli/php+0x14097d4) (BuildId: b2b405964cc047ab6da19abaf92a8899a99e4a47)
```
Furthermore, it also simply does not propagate the error to userland.
bukka
reviewed
Apr 3, 2026
Member
bukka
left a comment
bukka
left a comment
There was a problem hiding this comment.
looks good except small CS issue.
| X509_gmtime_adj(X509_getm_notBefore(new_cert), 0); | ||
| X509_gmtime_adj(X509_getm_notAfter(new_cert), 60*60*24*num_days); | ||
| if (!X509_gmtime_adj(X509_getm_notBefore(new_cert), 0) | ||
| || !X509_gmtime_adj(X509_getm_notAfter(new_cert), 60*60*24*num_days)) { |
Member
There was a problem hiding this comment.
Suggested change
| || !X509_gmtime_adj(X509_getm_notAfter(new_cert), 60*60*24*num_days)) { | |
| || !X509_gmtime_adj(X509_getm_notAfter(new_cert), 60*60*24*num_days)) { |
Member
Author
There was a problem hiding this comment.
I actually went with the style that Zend/ uses, but I'm okay with adjusting this.
Member
There was a problem hiding this comment.
think in exts we usually use one or two tabs for this. At least that's what I have been using. 1 space is a bit weird...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This causes UB later on when the certificate is passed to another function:
Furthermore, it also simply does not propagate the error to userland.
This was found by a hybrid static-dynamic analyser that looks for inconsistent handling of error checks in bindings.