Fix potential NULL pointer dereference before calling EVP_SignInit

[icy17]

No description provided.

[staabm]

Same problem here?

php-src/ext/openssl/openssl.c

Line 7381 in 7db7c42

md_ctx = EVP_MD_CTX_create();

Maybe in more places...?

[icy17]

Same problem here?

php-src/ext/openssl/openssl.c

Line 7381 in 7db7c42

md_ctx = EVP_MD_CTX_create();

Maybe in more places...?

Yes, this is a potential NULL dereference.

[nielsdos]

Thanks for this patch.
One thing to improve: I think you need to emit an error, like is done here:

php-src/ext/phar/util.c

Line 1593 in 1e4bb03

spprintf(error, 0, "openssl signature could not be verified");

[icy17]

openssl signature could not be verified

got it, but I'm not sure about the content of error, should be "openssl signature could not be verified"?

[nielsdos]

openssl signature could not be verified

got it, but I'm not sure about the content of error, should be "openssl signature could not be verified"?

Works for me

[nielsdos]

Seems right, thanks! I'll merge this into PHP-8.2 and above.
Note that for next time you create a PR for a bugfix, please target not the master branch, but the lowest supported branch for bugfixes.

[nielsdos]

I adapted the patch a bit in the merge to also free key like the error block below did too, while at it I just copied the error message too, such that the error block is identical except for freeing sigbuf.
Thanks!

[icy17]

Hi! Thanks for merging my pull requests!
I noticed these PR addresse potential security issues. I'm wondering if it would be appropriate to apply for CVEs for these bugs? Could you let me know if that's possible and what steps I should take?

[nielsdos]

I noticed these PR addresse potential security issues. I'm wondering if it would be appropriate to apply for CVEs for these bugs? Could you let me know if that's possible and what steps I should take?

I've looked in the OpenSSL code, and the only way the function can fail is if the memory allocation fails. On modern systems the allocator normally doesn't fail, but even if it does, this doesn't seem attacker controllable. If the context is NULL, then the request will crash, but it would've aborted anyway because we're out of memory at this point, it's just that the abort of the request happens more gracefully now.
I don't really see how this fits in PHP's security policy: https://wiki.php.net/security