New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check for EVP_MD being NULL inside ssl. #18784
Conversation
Fix multiple places that could potentially segfault if memory allocations fail. e.g. ssl_load_ciphers() could fail while calling ssl_evp_md_fetch(). Found by openssl#18355
if (EVP_MD_is_a(ssl_md(s->ctx, tmp->algorithm2), | ||
OSSL_DIGEST_NAME_SHA2_256)) { | ||
if (md != NULL | ||
&& EVP_MD_is_a(md, OSSL_DIGEST_NAME_SHA2_256)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if EVP_MD_is_a and friends should return false when passed a NULL pointer? Some functions do, some don't.
@@ -257,13 +257,17 @@ int tls13_generate_master_secret(SSL *s, unsigned char *out, | |||
size_t tls13_final_finish_mac(SSL *s, const char *str, size_t slen, | |||
unsigned char *out) | |||
{ | |||
const char *mdname = EVP_MD_get0_name(ssl_handshake_md(s)); | |||
const EVP_MD *md = ssl_handshake_md(s); | |||
const char *mdname = EVP_MD_get0_name(md); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we need a NULL check here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like EVP_MD_get0_name returns NULL if its argument is NULL, so this should be OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safer to check it nonetheless. There is no telling what changes will be made in the future that break this assumption.
For example, EVP_MD_CTX_get_size()
never used to return a failure -- it could as of 3.0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This pull request is ready to merge |
Merged to master and 3.0. Thank you. |
Fix multiple places that could potentially segfault if memory allocations fail. e.g. ssl_load_ciphers() could fail while calling ssl_evp_md_fetch(). Found by #18355 Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #18784) (cherry picked from commit b740012)
Fix multiple places that could potentially segfault if memory allocations fail. e.g. ssl_load_ciphers() could fail while calling ssl_evp_md_fetch(). Found by #18355 Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from #18784)
Fix multiple places that could potentially segfault if memory allocations fail. e.g. ssl_load_ciphers() could fail while calling ssl_evp_md_fetch(). Found by openssl#18355 Reviewed-by: Paul Dale <pauli@openssl.org> Reviewed-by: Hugo Landau <hlandau@openssl.org> Reviewed-by: Dmitry Belyavskiy <beldmit@gmail.com> (Merged from openssl#18784)
Fix multiple places that could potentially segfault if memory
allocations fail. e.g. ssl_load_ciphers() could fail while calling
ssl_evp_md_fetch().
Found by #18355
Checklist