Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Check for EVP_MD being NULL inside ssl. #18784

Closed
wants to merge 1 commit into from
Closed

Check for EVP_MD being NULL inside ssl. #18784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 3 additions & 2 deletions ssl/s3_lib.c
View file
Open in desktop
Expand Up @@ -4312,9 +4312,10 @@ const SSL_CIPHER *ssl3_choose_cipher(SSL *s, STACK_OF(SSL_CIPHER) *clnt,

if (prefer_sha256) {
const SSL_CIPHER *tmp = sk_SSL_CIPHER_value(allow, ii);
const EVP_MD *md = ssl_md(s->ctx, tmp->algorithm2);

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)) {
Copy link
Contributor

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.

ret = tmp;
break;
}
Expand Down
7 changes: 5 additions & 2 deletions ssl/ssl_ciph.c
View file
Open in desktop
Expand Up @@ -555,11 +555,14 @@ int ssl_cipher_get_evp(SSL_CTX *ctx, const SSL_SESSION *s,
if (c->algorithm_mac == SSL_AEAD)
mac_pkey_type = NULL;
} else {
if (!ssl_evp_md_up_ref(ctx->ssl_digest_methods[i])) {
const EVP_MD *digest = ctx->ssl_digest_methods[i];
beldmit marked this conversation as resolved.
Show resolved Hide resolved

if (digest == NULL
|| !ssl_evp_md_up_ref(digest)) {
ssl_evp_cipher_free(*enc);
return 0;
}
*md = ctx->ssl_digest_methods[i];
*md = digest;
if (mac_pkey_type != NULL)
*mac_pkey_type = ctx->ssl_mac_pkey_id[i];
if (mac_secret_size != NULL)
Expand Down
4 changes: 4 additions & 0 deletions ssl/statem/extensions_srvr.c
View file
Open in desktop
Expand Up @@ -1156,6 +1156,10 @@ int tls_parse_ctos_psk(SSL *s, PACKET *pkt, unsigned int context, X509 *x,
}

md = ssl_md(s->ctx, sess->cipher->algorithm2);
if (md == NULL) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
goto err;
}
if (!EVP_MD_is_a(md,
EVP_MD_get0_name(ssl_md(s->ctx,
s->s3.tmp.new_cipher->algorithm2)))) {
Expand Down
6 changes: 4 additions & 2 deletions ssl/statem/statem_clnt.c
View file
Open in desktop
Expand Up @@ -1346,12 +1346,14 @@ static int set_client_ciphersuite(SSL *s, const unsigned char *cipherchars)
s->session->cipher_id = s->session->cipher->id;
if (s->hit && (s->session->cipher_id != c->id)) {
if (SSL_IS_TLS13(s)) {
const EVP_MD *md = ssl_md(s->ctx, c->algorithm2);

/*
* In TLSv1.3 it is valid for the server to select a different
* ciphersuite as long as the hash is the same.
*/
if (ssl_md(s->ctx, c->algorithm2)
!= ssl_md(s->ctx, s->session->cipher->algorithm2)) {
if (md == NULL
|| md != ssl_md(s->ctx, s->session->cipher->algorithm2)) {
SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
SSL_R_CIPHERSUITE_DIGEST_HAS_CHANGED);
return 0;
Expand Down
15 changes: 10 additions & 5 deletions ssl/tls13_enc.c
View file
Open in desktop
Expand Up @@ -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);
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor

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.

unsigned char hash[EVP_MAX_MD_SIZE];
unsigned char finsecret[EVP_MAX_MD_SIZE];
unsigned char *key = NULL;
size_t len = 0, hashlen;
OSSL_PARAM params[2], *p = params;

if (md == NULL)
return 0;

/* Safe to cast away const here since we're not "getting" any data */
if (s->ctx->propq != NULL)
*p++ = OSSL_PARAM_construct_utf8_string(OSSL_ALG_PARAM_PROPERTIES,
Expand All @@ -281,7 +285,7 @@ size_t tls13_final_finish_mac(SSL *s, const char *str, size_t slen,
} else if (SSL_IS_FIRST_HANDSHAKE(s)) {
key = s->client_finished_secret;
} else {
if (!tls13_derive_finishedkey(s, ssl_handshake_md(s),
if (!tls13_derive_finishedkey(s, md,
s->client_app_traffic_secret,
finsecret, hashlen))
goto err;
Expand Down Expand Up @@ -781,7 +785,7 @@ int tls13_update_key(SSL *s, int sending)
RECORD_LAYER_reset_read_sequence(&s->rlayer);
}

if (!derive_secret_key_and_iv(s, sending, ssl_handshake_md(s),
if (!derive_secret_key_and_iv(s, sending, md,
s->s3.tmp.new_sym_enc, insecret, NULL,
application_traffic,
sizeof(application_traffic) - 1, secret, key,
Expand Down Expand Up @@ -826,7 +830,7 @@ int tls13_export_keying_material(SSL *s, unsigned char *out, size_t olen,
unsigned int hashsize, datalen;
int ret = 0;

if (ctx == NULL || !ossl_statem_export_allowed(s))
if (ctx == NULL || md == NULL || !ossl_statem_export_allowed(s))
goto err;

if (!use_context)
Expand Down Expand Up @@ -895,7 +899,8 @@ int tls13_export_keying_material_early(SSL *s, unsigned char *out, size_t olen,
*
* Here Transcript-Hash is the cipher suite hash algorithm.
*/
if (EVP_DigestInit_ex(ctx, md, NULL) <= 0
if (md == NULL
|| EVP_DigestInit_ex(ctx, md, NULL) <= 0
|| EVP_DigestUpdate(ctx, context, contextlen) <= 0
|| EVP_DigestFinal_ex(ctx, hash, &hashsize) <= 0
|| EVP_DigestInit_ex(ctx, md, NULL) <= 0
Expand Down