diff options
author | Miod Vallat <miod@cvs.openbsd.org> | 2014-07-11 22:57:26 +0000 |
---|---|---|
committer | Miod Vallat <miod@cvs.openbsd.org> | 2014-07-11 22:57:26 +0000 |
commit | 95e961976a467f2d2e570f8ad265579204d61f61 (patch) | |
tree | d90e249fb176cfa71f840dd4ba8f712925f57a54 /lib/libssl | |
parent | b0c253ff72a0e8913218a63ea7af6e28248a3e1d (diff) |
As reported by David Ramos, most consumer of ssl_get_message() perform late
bounds check, after reading the 2-, 3- or 4-byte size of the next chunk to
process. But the size fields themselves are not checked for being entirely
contained in the buffer.
Since reading past your bounds is bad practice, and may not possible if you
are using a secure memory allocator, we need to add the necessary bounds check,
at the expense of some readability.
As a bonus, a wrong size GOST session key will now trigger an error instead of
a printf to stderr and it being handled as if it had the correct size.
Creating this diff made my eyes bleed (in the real sense); reviewing it
made guenther@'s and beck@'s eyes bleed too (in the literal sense).
ok guenther@ beck@
Diffstat (limited to 'lib/libssl')
-rw-r--r-- | lib/libssl/src/ssl/d1_clnt.c | 10 | ||||
-rw-r--r-- | lib/libssl/src/ssl/s3_clnt.c | 85 | ||||
-rw-r--r-- | lib/libssl/src/ssl/s3_srvr.c | 106 |
3 files changed, 148 insertions, 53 deletions
diff --git a/lib/libssl/src/ssl/d1_clnt.c b/lib/libssl/src/ssl/d1_clnt.c index 3f47a3854b2..b85908c7330 100644 --- a/lib/libssl/src/ssl/d1_clnt.c +++ b/lib/libssl/src/ssl/d1_clnt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: d1_clnt.c,v 1.28 2014/07/11 09:24:44 beck Exp $ */ +/* $OpenBSD: d1_clnt.c,v 1.29 2014/07/11 22:57:25 miod Exp $ */ /* * DTLS implementation written by Nagendra Modadugu * (nagendra@cs.stanford.edu) for the OpenSSL project 2005. @@ -879,6 +879,8 @@ dtls1_get_hello_verify(SSL *s) return (1); } + if (2 > n) + goto truncated; data = (unsigned char *)s->init_msg; if ((data[0] != (s->version >> 8)) || (data[1] != (s->version&0xff))) { @@ -889,7 +891,11 @@ dtls1_get_hello_verify(SSL *s) } data += 2; + if (2 + 1 > n) + goto truncated; cookie_len = *(data++); + if (2 + 1 + cookie_len > n) + goto truncated; if (cookie_len > sizeof(s->d1->cookie)) { al = SSL_AD_ILLEGAL_PARAMETER; goto f_err; @@ -901,6 +907,8 @@ dtls1_get_hello_verify(SSL *s) s->d1->send_cookie = 1; return 1; +truncated: + al = SSL_AD_DECODE_ERROR; f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); return -1; diff --git a/lib/libssl/src/ssl/s3_clnt.c b/lib/libssl/src/ssl/s3_clnt.c index 3596acf1de5..884b9f1efb8 100644 --- a/lib/libssl/src/ssl/s3_clnt.c +++ b/lib/libssl/src/ssl/s3_clnt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: s3_clnt.c,v 1.77 2014/07/11 15:44:53 miod Exp $ */ +/* $OpenBSD: s3_clnt.c,v 1.78 2014/07/11 22:57:25 miod Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -814,6 +814,8 @@ ssl3_get_server_hello(SSL *s) d = p = (unsigned char *)s->init_msg; + if (2 > n) + goto truncated; if ((p[0] != (s->version >> 8)) || (p[1] != (s->version & 0xff))) { SSLerr(SSL_F_SSL3_GET_SERVER_HELLO, SSL_R_WRONG_SSL_VERSION); s->version = (s->version&0xff00) | p[1]; @@ -823,6 +825,10 @@ ssl3_get_server_hello(SSL *s) p += 2; /* load the server hello data */ + + if (p + SSL3_RANDOM_SIZE + 1 - d > n) + goto truncated; + /* load the server random */ memcpy(s->s3->server_random, p, SSL3_RANDOM_SIZE); p += SSL3_RANDOM_SIZE; @@ -838,6 +844,9 @@ ssl3_get_server_hello(SSL *s) goto f_err; } + if (p + j + 2 - d > n) + goto truncated; + /* * Check if we want to resume the session based on external * pre-shared secret @@ -935,6 +944,8 @@ ssl3_get_server_hello(SSL *s) } /* lets get the compression algorithm */ /* COMPRESSION */ + if (p + 1 - d > n) + goto truncated; if (*(p++) != 0) { al = SSL_AD_ILLEGAL_PARAMETER; SSLerr(SSL_F_SSL3_GET_SERVER_HELLO, @@ -958,15 +969,15 @@ ssl3_get_server_hello(SSL *s) } } - if (p != (d + n)) { - /* wrong packet length */ - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_SERVER_HELLO, - SSL_R_BAD_PACKET_LENGTH); - goto f_err; - } + if (p != d + n) + goto truncated; return (1); + +truncated: + /* wrong packet length */ + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_SERVER_HELLO, SSL_R_BAD_PACKET_LENGTH); f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); err: @@ -1015,6 +1026,8 @@ ssl3_get_server_certificate(SSL *s) goto err; } + if (p + 3 - d > n) + goto truncated; n2l3(p, llen); if (llen + 3 != n) { al = SSL_AD_DECODE_ERROR; @@ -1023,6 +1036,8 @@ ssl3_get_server_certificate(SSL *s) goto f_err; } for (nc = 0; nc < llen; ) { + if (p + 3 - d > n) + goto truncated; n2l3(p, l); if ((l + nc + 3) > llen) { al = SSL_AD_DECODE_ERROR; @@ -1094,7 +1109,7 @@ ssl3_get_server_certificate(SSL *s) x = NULL; al = SSL3_AL_FATAL; SSLerr(SSL_F_SSL3_GET_SERVER_CERTIFICATE, - SSL_R_UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS); + SSL_R_UNABLE_TO_FIND_PUBLIC_KEY_PARAMETERS); goto f_err; } @@ -1103,7 +1118,7 @@ ssl3_get_server_certificate(SSL *s) x = NULL; al = SSL3_AL_FATAL; SSLerr(SSL_F_SSL3_GET_SERVER_CERTIFICATE, - SSL_R_UNKNOWN_CERTIFICATE_TYPE); + SSL_R_UNKNOWN_CERTIFICATE_TYPE); goto f_err; } @@ -1137,6 +1152,11 @@ ssl3_get_server_certificate(SSL *s) ret = 1; if (0) { +truncated: + /* wrong packet length */ + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_SERVER_CERTIFICATE, + SSL_R_BAD_PACKET_LENGTH); f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); } @@ -1206,6 +1226,8 @@ ssl3_get_key_exchange(SSL *s) ERR_R_MALLOC_FAILURE); goto err; } + if (2 > n) + goto truncated; n2s(p, i); param_len = i + 2; if (param_len > n) { @@ -1221,6 +1243,8 @@ ssl3_get_key_exchange(SSL *s) } p += i; + if (param_len + 2 > n) + goto truncated; n2s(p, i); param_len += i + 2; if (param_len > n) { @@ -1258,6 +1282,8 @@ ssl3_get_key_exchange(SSL *s) ERR_R_DH_LIB); goto err; } + if (2 > n) + goto truncated; n2s(p, i); param_len = i + 2; if (param_len > n) { @@ -1273,6 +1299,8 @@ ssl3_get_key_exchange(SSL *s) } p += i; + if (param_len + 2 > n) + goto truncated; n2s(p, i); param_len += i + 2; if (param_len > n) { @@ -1288,6 +1316,8 @@ ssl3_get_key_exchange(SSL *s) } p += i; + if (param_len + 2 > n) + goto truncated; n2s(p, i); param_len += i + 2; if (param_len > n) { @@ -1376,6 +1406,8 @@ ssl3_get_key_exchange(SSL *s) goto err; } + if (param_len + 1 > n) + goto truncated; encoded_pt_len = *p; /* length of encoded point */ p += 1; @@ -1435,6 +1467,8 @@ ssl3_get_key_exchange(SSL *s) * Check key type is consistent * with signature */ + if (2 > n) + goto truncated; if (sigalg != (int)p[1]) { SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_WRONG_SIGNATURE_TYPE); @@ -1453,11 +1487,13 @@ ssl3_get_key_exchange(SSL *s) } else md = EVP_sha1(); + if (2 > n) + goto truncated; n2s(p, i); n -= 2; j = EVP_PKEY_size(pkey); - if ((i != n) || (n > j) || (n <= 0)) { + if (i != n || n > j) { /* wrong packet length */ al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, @@ -1534,6 +1570,10 @@ ssl3_get_key_exchange(SSL *s) EVP_PKEY_free(pkey); EVP_MD_CTX_cleanup(&md_ctx); return (1); +truncated: + /* wrong packet length */ + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_KEY_EXCHANGE, SSL_R_BAD_PACKET_LENGTH); f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); err: @@ -1606,13 +1646,26 @@ ssl3_get_certificate_request(SSL *s) } /* get the certificate types */ + if (1 > n) + goto truncated; ctype_num= *(p++); if (ctype_num > SSL3_CT_NUMBER) ctype_num = SSL3_CT_NUMBER; + if (p + ctype_num - d > n) { + SSLerr(SSL_F_SSL3_GET_CERTIFICATE_REQUEST, + SSL_R_DATA_LENGTH_TOO_LONG); + goto err; + } + for (i = 0; i < ctype_num; i++) s->s3->tmp.ctype[i] = p[i]; p += ctype_num; if (SSL_USE_SIGALGS(s)) { + if (p + 2 - d > n) { + SSLerr(SSL_F_SSL3_GET_CERTIFICATE_REQUEST, + SSL_R_DATA_LENGTH_TOO_LONG); + goto err; + } n2s(p, llen); /* Check we have enough room for signature algorithms and * following length value. @@ -1633,6 +1686,11 @@ ssl3_get_certificate_request(SSL *s) } /* get the CA RDNs */ + if (p + 2 - d > n) { + SSLerr(SSL_F_SSL3_GET_CERTIFICATE_REQUEST, + SSL_R_DATA_LENGTH_TOO_LONG); + goto err; + } n2s(p, llen); if ((unsigned long)(p - d + llen) != n) { @@ -1698,6 +1756,11 @@ cont: ca_sk = NULL; ret = 1; + if (0) { +truncated: + SSLerr(SSL_F_SSL3_GET_CERTIFICATE_REQUEST, + SSL_R_BAD_PACKET_LENGTH); + } err: if (ca_sk != NULL) sk_X509_NAME_pop_free(ca_sk, X509_NAME_free); diff --git a/lib/libssl/src/ssl/s3_srvr.c b/lib/libssl/src/ssl/s3_srvr.c index 66a4552237d..89325b7be90 100644 --- a/lib/libssl/src/ssl/s3_srvr.c +++ b/lib/libssl/src/ssl/s3_srvr.c @@ -1,4 +1,4 @@ -/* $OpenBSD: s3_srvr.c,v 1.74 2014/07/11 15:18:52 miod Exp $ */ +/* $OpenBSD: s3_srvr.c,v 1.75 2014/07/11 22:57:25 miod Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -894,18 +894,17 @@ ssl3_get_client_hello(SSL *s) s->state = SSL3_ST_SR_CLNT_HELLO_B; } s->first_packet = 1; - n = s->method->ssl_get_message(s, - SSL3_ST_SR_CLNT_HELLO_B, - SSL3_ST_SR_CLNT_HELLO_C, - SSL3_MT_CLIENT_HELLO, - SSL3_RT_MAX_PLAIN_LENGTH, - &ok); + n = s->method->ssl_get_message(s, SSL3_ST_SR_CLNT_HELLO_B, + SSL3_ST_SR_CLNT_HELLO_C, SSL3_MT_CLIENT_HELLO, + SSL3_RT_MAX_PLAIN_LENGTH, &ok); if (!ok) return ((int)n); s->first_packet = 0; - d = p=(unsigned char *)s->init_msg; + d = p = (unsigned char *)s->init_msg; + if (2 > n) + goto truncated; /* * Use version from inside client hello, not from record header. * (may differ: see RFC 2246, Appendix E, second paragraph) @@ -944,12 +943,17 @@ ssl3_get_client_hello(SSL *s) return (1); } + if (p + SSL3_RANDOM_SIZE + 1 - d > n) + goto truncated; + /* load the client random */ memcpy(s->s3->client_random, p, SSL3_RANDOM_SIZE); p += SSL3_RANDOM_SIZE; /* get the session-id */ j= *(p++); + if (p + j - d > n) + goto truncated; s->hit = 0; /* @@ -988,6 +992,8 @@ ssl3_get_client_hello(SSL *s) if (SSL_IS_DTLS(s)) { /* cookie stuff */ + if (p + 1 - d > n) + goto truncated; cookie_len = *(p++); /* @@ -1003,6 +1009,9 @@ ssl3_get_client_hello(SSL *s) goto f_err; } + if (p + cookie_len - d > n) + goto truncated; + /* verify the cookie if appropriate option is set. */ if ((SSL_get_options(s) & SSL_OP_COOKIE_EXCHANGE) && cookie_len > 0) { @@ -1032,6 +1041,8 @@ ssl3_get_client_hello(SSL *s) p += cookie_len; } + if (p + 2 - d > n) + goto truncated; n2s(p, i); if ((i == 0) && (j != 0)) { /* we need a cipher if we are not resuming a session */ @@ -1040,13 +1051,8 @@ ssl3_get_client_hello(SSL *s) SSL_R_NO_CIPHERS_SPECIFIED); goto f_err; } - if ((p + i) >= (d + n)) { - /* not enough data */ - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, - SSL_R_LENGTH_MISMATCH); - goto f_err; - } + if (p + i - d > n) + goto truncated; if ((i > 0) && (ssl_bytes_to_cipher_list(s, p, i, &(ciphers)) == NULL)) { goto err; @@ -1078,14 +1084,11 @@ ssl3_get_client_hello(SSL *s) } /* compression */ + if (p + 1 - d > n) + goto truncated; i= *(p++); - if ((p + i) > (d + n)) { - /* not enough data */ - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, - SSL_R_LENGTH_MISMATCH); - goto f_err; - } + if (p + i - d > n) + goto truncated; for (j = 0; j < i; j++) { if (p[j] == 0) break; @@ -1247,6 +1250,9 @@ ssl3_get_client_hello(SSL *s) if (ret < 0) ret = 1; if (0) { +truncated: + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_HELLO, SSL_R_BAD_PACKET_LENGTH); f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); } @@ -1847,7 +1853,7 @@ ssl3_get_client_key_exchange(SSL *s) int i, al, ok; long n; unsigned long alg_k; - unsigned char *p; + unsigned char *d, *p; RSA *rsa = NULL; EVP_PKEY *pkey = NULL; BIGNUM *pub = NULL; @@ -1863,7 +1869,7 @@ ssl3_get_client_key_exchange(SSL *s) SSL3_ST_SR_KEY_EXCH_B, SSL3_MT_CLIENT_KEY_EXCHANGE, 2048, &ok); if (!ok) return ((int)n); - p = (unsigned char *)s->init_msg; + d = p = (unsigned char *)s->init_msg; alg_k = s->s3->tmp.new_cipher->algorithm_mkey; @@ -1897,6 +1903,8 @@ ssl3_get_client_key_exchange(SSL *s) /* TLS and [incidentally] DTLS{0xFEFF} */ if (s->version > SSL3_VERSION && s->version != DTLS1_BAD_VER) { + if (2 > n) + goto truncated; n2s(p, i); if (n != i + 2) { if (!(s->options & SSL_OP_TLS_D5_BUG)) { @@ -1919,6 +1927,8 @@ ssl3_get_client_key_exchange(SSL *s) /* SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE,SSL_R_BAD_RSA_DECRYPT); */ } + if (p + 2 - d > n) /* needed in the SSL3 case */ + goto truncated; if ((al == -1) && !((p[0] == (s->client_version >> 8)) && (p[1] == (s->client_version & 0xff)))) { /* @@ -1975,6 +1985,8 @@ ssl3_get_client_key_exchange(SSL *s) OPENSSL_cleanse(p, i); } else if (alg_k & (SSL_kEDH|SSL_kDHr|SSL_kDHd)) { + if (2 > n) + goto truncated; n2s(p, i); if (n != i + 2) { if (!(s->options & SSL_OP_SSLEAY_080_CLIENT_DH_BUG)) { @@ -2206,6 +2218,8 @@ ssl3_get_client_key_exchange(SSL *s) client_pub_pkey) <= 0) ERR_clear_error(); } + if (2 > n) + goto truncated; /* Decrypt session key */ if (ASN1_get_object((const unsigned char **)&p, &Tlen, &Ttag, &Tclass, n) != V_ASN1_CONSTRUCTED || @@ -2242,11 +2256,14 @@ gerr: } else { al = SSL_AD_HANDSHAKE_FAILURE; SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, - SSL_R_UNKNOWN_CIPHER_TYPE); + SSL_R_UNKNOWN_CIPHER_TYPE); goto f_err; } return (1); +truncated: + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_KEY_EXCHANGE, SSL_R_BAD_PACKET_LENGTH); f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); err: @@ -2338,6 +2355,8 @@ ssl3_get_cert_verify(SSL *s) al = SSL_AD_INTERNAL_ERROR; goto f_err; } + if (2 > n) + goto truncated; /* Check key type is consistent with signature */ if (sigalg != (int)p[1]) { SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, @@ -2355,14 +2374,12 @@ ssl3_get_cert_verify(SSL *s) p += 2; n -= 2; } + if (2 > n) + goto truncated; n2s(p, i); n -= 2; - if (i > n) { - SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, - SSL_R_LENGTH_MISMATCH); - al = SSL_AD_DECODE_ERROR; - goto f_err; - } + if (i > n) + goto truncated; } j = EVP_PKEY_size(pkey); if ((i > j) || (n > j) || (n <= 0)) { @@ -2445,7 +2462,10 @@ ssl3_get_cert_verify(SSL *s) EVP_PKEY_CTX *pctx = EVP_PKEY_CTX_new(pkey, NULL); EVP_PKEY_verify_init(pctx); if (i != 64) { - fprintf(stderr, "GOST signature length is %d", i); + SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, + SSL_R_WRONG_SIGNATURE_SIZE); + al = SSL_AD_DECODE_ERROR; + goto f_err; } for (idx = 0; idx < 64; idx++) { signature[63 - idx] = p[idx]; @@ -2469,6 +2489,9 @@ ssl3_get_cert_verify(SSL *s) ret = 1; if (0) { +truncated: + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_CERT_VERIFY, SSL_R_BAD_PACKET_LENGTH); f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); } @@ -2490,7 +2513,6 @@ ssl3_get_client_certificate(SSL *s) X509 *x = NULL; unsigned long l, nc, llen, n; const unsigned char *p, *q; - unsigned char *d; STACK_OF(X509) *sk = NULL; n = s->method->ssl_get_message(s, SSL3_ST_SR_CERT_A, SSL3_ST_SR_CERT_B, @@ -2528,7 +2550,7 @@ ssl3_get_client_certificate(SSL *s) SSL_R_WRONG_MESSAGE_TYPE); goto f_err; } - p = d = (unsigned char *)s->init_msg; + p = (const unsigned char *)s->init_msg; if ((sk = sk_X509_new_null()) == NULL) { SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE, @@ -2536,16 +2558,14 @@ ssl3_get_client_certificate(SSL *s) goto err; } + if (3 > n) + goto truncated; n2l3(p, llen); - if (llen + 3 != n) { - al = SSL_AD_DECODE_ERROR; - SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE, - SSL_R_LENGTH_MISMATCH); - goto f_err; - } + if (llen + 3 != n) + goto truncated; for (nc = 0; nc < llen;) { n2l3(p, l); - if ((l + nc + 3) > llen) { + if (l + nc + 3 > llen) { al = SSL_AD_DECODE_ERROR; SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE, SSL_R_CERT_LENGTH_MISMATCH); @@ -2635,6 +2655,10 @@ ssl3_get_client_certificate(SSL *s) ret = 1; if (0) { +truncated: + al = SSL_AD_DECODE_ERROR; + SSLerr(SSL_F_SSL3_GET_CLIENT_CERTIFICATE, + SSL_R_BAD_PACKET_LENGTH); f_err: ssl3_send_alert(s, SSL3_AL_FATAL, al); } |