From 0ed8e95c4b40558c2812b348f1b922ce7911b59b Mon Sep 17 00:00:00 2001 From: Matt Caswell Date: Sat, 26 Jul 2014 23:47:40 +0100 Subject: [PATCH] Fixed out-of-bounds read errors in ssl3_get_key_exchange. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit PR#3450 Conflicts: ssl/s3_clnt.c Reviewed-by: Emilia Käsper --- ssl/s3_clnt.c | 206 +++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 160 insertions(+), 46 deletions(-) diff --git a/ssl/s3_clnt.c b/ssl/s3_clnt.c index 2ceaa1eb24..844710dde4 100644 --- a/ssl/s3_clnt.c +++ b/ssl/s3_clnt.c @@ -1277,8 +1277,8 @@ int ssl3_get_key_exchange(SSL *s) #endif EVP_MD_CTX md_ctx; unsigned char *param,*p; - int al,i,j,param_len,ok; - long n,alg_k,alg_a; + int al,j,ok; + long i,param_len,n,alg_k,alg_a; EVP_PKEY *pkey=NULL; const EVP_MD *md = NULL; #ifndef OPENSSL_NO_RSA @@ -1354,36 +1354,48 @@ int ssl3_get_key_exchange(SSL *s) s->session->sess_cert=ssl_sess_cert_new(); } + /* Total length of the parameters including the length prefix */ param_len=0; + alg_k=s->s3->tmp.new_cipher->algorithm_mkey; alg_a=s->s3->tmp.new_cipher->algorithm_auth; EVP_MD_CTX_init(&md_ctx); + al=SSL_AD_DECODE_ERROR; + #ifndef OPENSSL_NO_PSK if (alg_k & SSL_kPSK) { char tmp_id_hint[PSK_MAX_IDENTITY_LEN+1]; - al=SSL_AD_HANDSHAKE_FAILURE; + param_len = 2; + if (param_len > n) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } n2s(p,i); - param_len=i+2; + /* Store PSK identity hint for later use, hint is used * in ssl3_send_client_key_exchange. Assume that the * maximum length of a PSK identity hint can be as * long as the maximum length of a PSK identity. */ if (i > PSK_MAX_IDENTITY_LEN) { + al=SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_DATA_LENGTH_TOO_LONG); goto f_err; } - if (param_len > n) + if (i > n - param_len) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_BAD_PSK_IDENTITY_HINT_LENGTH); goto f_err; } + param_len += i; + /* If received PSK identity hint contains NULL * characters, the hint is truncated from the first * NULL. p may not be ending with NULL, so create a @@ -1395,6 +1407,7 @@ int ssl3_get_key_exchange(SSL *s) s->ctx->psk_identity_hint = BUF_strdup(tmp_id_hint); if (s->ctx->psk_identity_hint == NULL) { + al=SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE); goto f_err; } @@ -1407,14 +1420,22 @@ int ssl3_get_key_exchange(SSL *s) #ifndef OPENSSL_NO_SRP if (alg_k & SSL_kSRP) { - n2s(p,i); - param_len=i+2; + param_len = 2; if (param_len > n) { - al=SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + n2s(p,i); + + if (i > n - param_len) + { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_SRP_N_LENGTH); goto f_err; } + param_len += i; + if (!(s->srp_ctx.N=BN_bin2bn(p,i,NULL))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1422,14 +1443,24 @@ int ssl3_get_key_exchange(SSL *s) } p+=i; + + if (2 > n - param_len) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + param_len += 2; + n2s(p,i); - param_len+=i+2; - if (param_len > n) + + if (i > n - param_len) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_SRP_G_LENGTH); goto f_err; } + param_len += i; + if (!(s->srp_ctx.g=BN_bin2bn(p,i,NULL))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1437,15 +1468,25 @@ int ssl3_get_key_exchange(SSL *s) } p+=i; + + if (1 > n - param_len) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + param_len += 1; + i = (unsigned int)(p[0]); p++; - param_len+=i+1; - if (param_len > n) + + if (i > n - param_len) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_SRP_S_LENGTH); goto f_err; } + param_len += i; + if (!(s->srp_ctx.s=BN_bin2bn(p,i,NULL))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1453,14 +1494,23 @@ int ssl3_get_key_exchange(SSL *s) } p+=i; + if (2 > n - param_len) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + param_len += 2; + n2s(p,i); - param_len+=i+2; - if (param_len > n) + + if (i > n - param_len) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_SRP_B_LENGTH); goto f_err; } + param_len += i; + if (!(s->srp_ctx.B=BN_bin2bn(p,i,NULL))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1498,14 +1548,23 @@ int ssl3_get_key_exchange(SSL *s) SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_MALLOC_FAILURE); goto err; } - n2s(p,i); - param_len=i+2; + + param_len = 2; if (param_len > n) { - al=SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + n2s(p,i); + + if (i > n - param_len) + { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_RSA_MODULUS_LENGTH); goto f_err; } + param_len += i; + if (!(rsa->n=BN_bin2bn(p,i,rsa->n))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1513,14 +1572,23 @@ int ssl3_get_key_exchange(SSL *s) } p+=i; + if (2 > n - param_len) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + param_len += 2; + n2s(p,i); - param_len+=i+2; - if (param_len > n) + + if (i > n - param_len) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_RSA_E_LENGTH); goto f_err; } + param_len += i; + if (!(rsa->e=BN_bin2bn(p,i,rsa->e))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1552,14 +1620,23 @@ int ssl3_get_key_exchange(SSL *s) SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_DH_LIB); goto err; } - n2s(p,i); - param_len=i+2; + + param_len = 2; if (param_len > n) { - al=SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + n2s(p,i); + + if (i > n - param_len) + { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_DH_P_LENGTH); goto f_err; } + param_len += i; + if (!(dh->p=BN_bin2bn(p,i,NULL))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1567,14 +1644,23 @@ int ssl3_get_key_exchange(SSL *s) } p+=i; + if (2 > n - param_len) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + param_len += 2; + n2s(p,i); - param_len+=i+2; - if (param_len > n) + + if (i > n - param_len) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_DH_G_LENGTH); goto f_err; } + param_len += i; + if (!(dh->g=BN_bin2bn(p,i,NULL))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1582,14 +1668,23 @@ int ssl3_get_key_exchange(SSL *s) } p+=i; + if (2 > n - param_len) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + param_len += 2; + n2s(p,i); - param_len+=i+2; - if (param_len > n) + + if (i > n - param_len) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_DH_PUB_KEY_LENGTH); goto f_err; } + param_len += i; + if (!(dh->pub_key=BN_bin2bn(p,i,NULL))) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,ERR_R_BN_LIB); @@ -1641,12 +1736,19 @@ int ssl3_get_key_exchange(SSL *s) */ /* XXX: For now we only support named (not generic) curves - * and the ECParameters in this case is just three bytes. + * and the ECParameters in this case is just three bytes. We + * also need one byte for the length of the encoded point */ - param_len=3; - if ((param_len > n) || - (*p != NAMED_CURVE_TYPE) || - ((curve_nid = tls1_ec_curve_id2nid(*(p + 2))) == 0)) + param_len=4; + if (param_len > n) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + + if ((*p != NAMED_CURVE_TYPE) || + ((curve_nid = tls1_ec_curve_id2nid(*(p + 2))) == 0)) { al=SSL_AD_INTERNAL_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_UNABLE_TO_FIND_ECDH_PARAMETERS); @@ -1688,15 +1790,15 @@ int ssl3_get_key_exchange(SSL *s) encoded_pt_len = *p; /* length of encoded point */ p+=1; - param_len += (1 + encoded_pt_len); - if ((param_len > n) || + + if ((encoded_pt_len > n - param_len) || (EC_POINT_oct2point(group, srvr_ecpoint, p, encoded_pt_len, bn_ctx) == 0)) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_BAD_ECPOINT); goto f_err; } + param_len += encoded_pt_len; n-=param_len; p+=encoded_pt_len; @@ -1739,7 +1841,15 @@ int ssl3_get_key_exchange(SSL *s) { if (TLS1_get_version(s) >= TLS1_2_VERSION) { - int sigalg = tls12_get_sigid(pkey); + int sigalg; + if (2 > n) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } + + sigalg = tls12_get_sigid(pkey); /* Should never happen */ if (sigalg == -1) { @@ -1757,7 +1867,6 @@ int ssl3_get_key_exchange(SSL *s) if (md == NULL) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_UNKNOWN_DIGEST); - al=SSL_AD_DECODE_ERROR; goto f_err; } #ifdef SSL_DEBUG @@ -1768,15 +1877,21 @@ fprintf(stderr, "USING TLSv1.2 HASH %s\n", EVP_MD_name(md)); } else md = EVP_sha1(); - + + if (2 > n) + { + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, + SSL_R_LENGTH_TOO_SHORT); + goto f_err; + } n2s(p,i); n-=2; j=EVP_PKEY_size(pkey); + /* Check signature length. If n is 0 then signature is empty */ if ((i != n) || (n > j) || (n <= 0)) { /* wrong packet length */ - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_WRONG_SIGNATURE_LENGTH); goto f_err; } @@ -1844,7 +1959,6 @@ fprintf(stderr, "USING TLSv1.2 HASH %s\n", EVP_MD_name(md)); /* still data left over */ if (n != 0) { - al=SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE,SSL_R_EXTRA_DATA_IN_MESSAGE); goto f_err; } -- 2.34.1