[openssl-commits] [openssl] master update
Matt Caswell
matt at openssl.org
Thu Sep 29 14:11:05 UTC 2016
The branch master has been updated
via 0023baffb8f648c22d397bfa5e1cc8749749bd29 (commit)
via ff8194774ca2d8e30223c6f8e2583112514e9fb7 (commit)
via 4a424545c4f3148bfbf54270422e05177b4c392f (commit)
via c13d2a5be720a8ab8f0cb67fc2750ed27eee3d9e (commit)
via 1ff8434040b35f35c27f77ef064481622490bba9 (commit)
from ac8cc3efb26fa91c4f29463044cfe9e7070ebc14 (commit)
- Log -----------------------------------------------------------------
commit 0023baffb8f648c22d397bfa5e1cc8749749bd29
Author: Matt Caswell <matt at openssl.org>
Date: Thu Sep 29 14:45:49 2016 +0100
Add an example of usage to the WPACKET_reserve_bytes() documentation
Reviewed-by: Rich Salz <rsalz at openssl.org>
commit ff8194774ca2d8e30223c6f8e2583112514e9fb7
Author: Matt Caswell <matt at openssl.org>
Date: Thu Sep 29 14:39:47 2016 +0100
Address style feedback comments
Reviewed-by: Rich Salz <rsalz at openssl.org>
commit 4a424545c4f3148bfbf54270422e05177b4c392f
Author: Matt Caswell <matt at openssl.org>
Date: Thu Sep 29 12:04:08 2016 +0100
Fix a bug in CKE construction for PSK
In plain PSK we don't need to do anymore construction after the preamble.
We weren't detecting this case and treating it as an unknown cipher.
Reviewed-by: Rich Salz <rsalz at openssl.org>
commit c13d2a5be720a8ab8f0cb67fc2750ed27eee3d9e
Author: Matt Caswell <matt at openssl.org>
Date: Thu Sep 29 11:46:08 2016 +0100
Convert ServerKeyExchange construction to WPACKET
Reviewed-by: Rich Salz <rsalz at openssl.org>
commit 1ff8434040b35f35c27f77ef064481622490bba9
Author: Matt Caswell <matt at openssl.org>
Date: Thu Sep 29 11:43:37 2016 +0100
Add the WPACKET_reserve_bytes() function
WPACKET_allocate_bytes() requires you to know the size of the data you
are allocating for, before you create it. Sometimes this isn't the case,
for example we know the maximum size that a signature will be before we
create it, but not the actual size. WPACKET_reserve_bytes() enables us to
reserve bytes in the WPACKET, but not count them as written yet. We then
subsequently need to acall WPACKET_allocate_bytes to actually count them as
written.
Reviewed-by: Rich Salz <rsalz at openssl.org>
-----------------------------------------------------------------------
Summary of changes:
ssl/packet.c | 33 +++++--
ssl/packet_locl.h | 40 ++++++++-
ssl/statem/statem_clnt.c | 2 +-
ssl/statem/statem_srvr.c | 218 +++++++++++++++++++++--------------------------
4 files changed, 164 insertions(+), 129 deletions(-)
diff --git a/ssl/packet.c b/ssl/packet.c
index 4077de5..2a8fe25 100644
--- a/ssl/packet.c
+++ b/ssl/packet.c
@@ -14,6 +14,27 @@
int WPACKET_allocate_bytes(WPACKET *pkt, size_t len, unsigned char **allocbytes)
{
+ if (!WPACKET_reserve_bytes(pkt, len, allocbytes))
+ return 0;
+
+ pkt->written += len;
+ pkt->curr += len;
+ return 1;
+}
+
+int WPACKET_sub_allocate_bytes__(WPACKET *pkt, size_t len,
+ unsigned char **allocbytes, size_t lenbytes)
+{
+ if (!WPACKET_start_sub_packet_len__(pkt, lenbytes)
+ || !WPACKET_allocate_bytes(pkt, len, allocbytes)
+ || !WPACKET_close(pkt))
+ return 0;
+
+ return 1;
+}
+
+int WPACKET_reserve_bytes(WPACKET *pkt, size_t len, unsigned char **allocbytes)
+{
/* Internal API, so should not fail */
assert(pkt->subs != NULL && len != 0);
if (pkt->subs == NULL || len == 0)
@@ -39,20 +60,18 @@ int WPACKET_allocate_bytes(WPACKET *pkt, size_t len, unsigned char **allocbytes)
return 0;
}
*allocbytes = (unsigned char *)pkt->buf->data + pkt->curr;
- pkt->written += len;
- pkt->curr += len;
return 1;
}
-int WPACKET_sub_allocate_bytes__(WPACKET *pkt, size_t len,
- unsigned char **allocbytes, size_t lenbytes)
+int WPACKET_sub_reserve_bytes__(WPACKET *pkt, size_t len,
+ unsigned char **allocbytes, size_t lenbytes)
{
- if (!WPACKET_start_sub_packet_len__(pkt, lenbytes)
- || !WPACKET_allocate_bytes(pkt, len, allocbytes)
- || !WPACKET_close(pkt))
+ if (!WPACKET_reserve_bytes(pkt, lenbytes + len, allocbytes))
return 0;
+ *allocbytes += lenbytes;
+
return 1;
}
diff --git a/ssl/packet_locl.h b/ssl/packet_locl.h
index 44a8f82..8d3fd37 100644
--- a/ssl/packet_locl.h
+++ b/ssl/packet_locl.h
@@ -675,7 +675,7 @@ int WPACKET_start_sub_packet(WPACKET *pkt);
* WPACKET_* calls. If not then the underlying buffer may be realloc'd and
* change its location.
*/
-int WPACKET_allocate_bytes(WPACKET *pkt, size_t bytes,
+int WPACKET_allocate_bytes(WPACKET *pkt, size_t len,
unsigned char **allocbytes);
/*
@@ -701,6 +701,44 @@ int WPACKET_sub_allocate_bytes__(WPACKET *pkt, size_t len,
WPACKET_sub_allocate_bytes__((pkt), (len), (bytes), 4)
/*
+ * The same as WPACKET_allocate_bytes() except the reserved bytes are not
+ * actually counted as written. Typically this will be for when we don't know
+ * how big arbitrary data is going to be up front, but we do know what the
+ * maximum size will be. If this function is used, then it should be immediately
+ * followed by a WPACKET_allocate_bytes() call before any other WPACKET
+ * functions are called (unless the write to the allocated bytes is abandoned).
+ *
+ * For example: If we are generating a signature, then the size of that
+ * signature may not be known in advance. We can use WPACKET_reserve_bytes() to
+ * handle this:
+ *
+ * if (!WPACKET_sub_reserve_bytes_u16(&pkt, EVP_PKEY_size(pkey), &sigbytes1)
+ * || EVP_SignFinal(md_ctx, sigbytes1, &siglen, pkey) <= 0
+ * || !WPACKET_sub_allocate_bytes_u16(&pkt, siglen, &sigbytes2)
+ * || sigbytes1 != sigbytes2)
+ * goto err;
+ */
+int WPACKET_reserve_bytes(WPACKET *pkt, size_t len, unsigned char **allocbytes);
+
+/*
+ * The "reserve_bytes" equivalent of WPACKET_sub_allocate_bytes__()
+ */
+int WPACKET_sub_reserve_bytes__(WPACKET *pkt, size_t len,
+ unsigned char **allocbytes, size_t lenbytes);
+
+/*
+ * Convenience macros for WPACKET_sub_reserve_bytes with different lengths
+ */
+#define WPACKET_sub_reserve_bytes_u8(pkt, len, bytes) \
+ WPACKET_reserve_bytes__((pkt), (len), (bytes), 1)
+#define WPACKET_sub_reserve_bytes_u16(pkt, len, bytes) \
+ WPACKET_sub_reserve_bytes__((pkt), (len), (bytes), 2)
+#define WPACKET_sub_reserve_bytes_u24(pkt, len, bytes) \
+ WPACKET_sub_reserve_bytes__((pkt), (len), (bytes), 3)
+#define WPACKET_sub_reserve_bytes_u32(pkt, len, bytes) \
+ WPACKET_sub_reserve_bytes__((pkt), (len), (bytes), 4)
+
+/*
* Write the value stored in |val| into the WPACKET. The value will consume
* |bytes| amount of storage. An error will occur if |val| cannot be
* accommodated in |bytes| storage, e.g. attempting to write the value 256 into
diff --git a/ssl/statem/statem_clnt.c b/ssl/statem/statem_clnt.c
index 67f4ac4..e4b2219 100644
--- a/ssl/statem/statem_clnt.c
+++ b/ssl/statem/statem_clnt.c
@@ -2496,7 +2496,7 @@ int tls_construct_client_key_exchange(SSL *s)
} else if (alg_k & SSL_kSRP) {
if (!tls_construct_cke_srp(s, &pkt, &al))
goto err;
- } else {
+ } else if (!(alg_k & SSL_kPSK)) {
ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE);
SSLerr(SSL_F_TLS_CONSTRUCT_CLIENT_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
goto err;
diff --git a/ssl/statem/statem_srvr.c b/ssl/statem/statem_srvr.c
index 799700b..3c3544c 100644
--- a/ssl/statem/statem_srvr.c
+++ b/ssl/statem/statem_srvr.c
@@ -1591,7 +1591,6 @@ int tls_construct_server_key_exchange(SSL *s)
{
#ifndef OPENSSL_NO_DH
EVP_PKEY *pkdh = NULL;
- int j;
#endif
#ifndef OPENSSL_NO_EC
unsigned char *encodedPoint = NULL;
@@ -1600,36 +1599,30 @@ int tls_construct_server_key_exchange(SSL *s)
#endif
EVP_PKEY *pkey;
const EVP_MD *md = NULL;
- unsigned char *p, *d;
- int al, i;
+ int al = SSL_AD_INTERNAL_ERROR, i;
unsigned long type;
- int n;
const BIGNUM *r[4];
- int nr[4], kn;
- BUF_MEM *buf;
EVP_MD_CTX *md_ctx = EVP_MD_CTX_new();
+ WPACKET pkt;
+ size_t paramlen, paramoffset;
+
+ if (!WPACKET_init(&pkt, s->init_buf)
+ || !ssl_set_handshake_header2(s, &pkt,
+ SSL3_MT_SERVER_KEY_EXCHANGE)
+ || !WPACKET_get_total_written(&pkt, ¶moffset)) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_HELLO, ERR_R_INTERNAL_ERROR);
+ goto f_err;
+ }
if (md_ctx == NULL) {
SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_MALLOC_FAILURE);
- al = SSL_AD_INTERNAL_ERROR;
goto f_err;
}
type = s->s3->tmp.new_cipher->algorithm_mkey;
- buf = s->init_buf;
-
r[0] = r[1] = r[2] = r[3] = NULL;
- n = 0;
#ifndef OPENSSL_NO_PSK
- if (type & SSL_PSK) {
- /*
- * reserve size for record length and PSK identity hint
- */
- n += 2;
- if (s->cert->psk_identity_hint)
- n += strlen(s->cert->psk_identity_hint);
- }
/* Plain PSK or RSAPSK nothing to do */
if (type & (SSL_kPSK | SSL_kRSAPSK)) {
} else
@@ -1646,7 +1639,6 @@ int tls_construct_server_key_exchange(SSL *s)
pkdh = EVP_PKEY_new();
if (pkdh == NULL || dhp == NULL) {
DH_free(dhp);
- al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
ERR_R_INTERNAL_ERROR);
goto f_err;
@@ -1660,7 +1652,6 @@ int tls_construct_server_key_exchange(SSL *s)
DH *dhp = s->cert->dh_tmp_cb(s, 0, 1024);
pkdh = ssl_dh_to_pkey(dhp);
if (pkdh == NULL) {
- al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
ERR_R_INTERNAL_ERROR);
goto f_err;
@@ -1723,7 +1714,6 @@ int tls_construct_server_key_exchange(SSL *s)
s->s3->tmp.pkey = ssl_generate_pkey_curve(curve_id);
/* Generate a new key for this curve */
if (s->s3->tmp.pkey == NULL) {
- al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_EVP_LIB);
goto f_err;
}
@@ -1737,13 +1727,6 @@ int tls_construct_server_key_exchange(SSL *s)
}
/*
- * We only support named (not generic) curves in ECDH ephemeral key
- * exchanges. In this situation, we need four additional bytes to
- * encode the entire ServerECDHParams structure.
- */
- n += 4 + encodedlen;
-
- /*
* We'll generate the serverKeyExchange message explicitly so we
* can set these to NULLs
*/
@@ -1774,25 +1757,6 @@ int tls_construct_server_key_exchange(SSL *s)
SSL_R_UNKNOWN_KEY_EXCHANGE_TYPE);
goto f_err;
}
- for (i = 0; i < 4 && r[i] != NULL; i++) {
- nr[i] = BN_num_bytes(r[i]);
-#ifndef OPENSSL_NO_SRP
- if ((i == 2) && (type & SSL_kSRP))
- n += 1 + nr[i];
- else
-#endif
-#ifndef OPENSSL_NO_DH
- /*-
- * for interoperability with some versions of the Microsoft TLS
- * stack, we need to zero pad the DHE pub key to the same length
- * as the prime, so use the length of the prime here
- */
- if ((i == 2) && (type & (SSL_kDHE | SSL_kDHEPSK)))
- n += 2 + nr[0];
- else
-#endif
- n += 2 + nr[i];
- }
if (!(s->s3->tmp.new_cipher->algorithm_auth & (SSL_aNULL | SSL_aSRP))
&& !(s->s3->tmp.new_cipher->algorithm_mkey & SSL_PSK)) {
@@ -1801,53 +1765,46 @@ int tls_construct_server_key_exchange(SSL *s)
al = SSL_AD_DECODE_ERROR;
goto f_err;
}
- kn = EVP_PKEY_size(pkey);
- /* Allow space for signature algorithm */
- if (SSL_USE_SIGALGS(s))
- kn += 2;
- /* Allow space for signature length */
- kn += 2;
} else {
pkey = NULL;
- kn = 0;
}
- if (!BUF_MEM_grow_clean(buf, n + SSL_HM_HEADER_LENGTH(s) + kn)) {
- SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_LIB_BUF);
- goto err;
- }
- d = p = ssl_handshake_start(s);
-
#ifndef OPENSSL_NO_PSK
if (type & SSL_PSK) {
- /* copy PSK identity hint */
- if (s->cert->psk_identity_hint) {
- size_t len = strlen(s->cert->psk_identity_hint);
- if (len > PSK_MAX_IDENTITY_LEN) {
- /*
- * Should not happen - we already checked this when we set
- * the identity hint
- */
- SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
- ERR_R_INTERNAL_ERROR);
- goto err;
- }
- s2n(len, p);
- memcpy(p, s->cert->psk_identity_hint, len);
- p += len;
- } else {
- s2n(0, p);
+ size_t len = (s->cert->psk_identity_hint == NULL)
+ ? 0 : strlen(s->cert->psk_identity_hint);
+
+ /*
+ * It should not happen that len > PSK_MAX_IDENTITY_LEN - we already
+ * checked this when we set the identity hint - but just in case
+ */
+ if (len > PSK_MAX_IDENTITY_LEN
+ || !WPACKET_sub_memcpy_u16(&pkt, s->cert->psk_identity_hint,
+ len)) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
+ ERR_R_INTERNAL_ERROR);
+ goto f_err;
}
}
#endif
for (i = 0; i < 4 && r[i] != NULL; i++) {
+ unsigned char *binval;
+ int res;
+
#ifndef OPENSSL_NO_SRP
if ((i == 2) && (type & SSL_kSRP)) {
- *p = nr[i];
- p++;
+ res = WPACKET_start_sub_packet_u8(&pkt);
} else
#endif
+ res = WPACKET_start_sub_packet_u16(&pkt);
+
+ if (!res) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
+ ERR_R_INTERNAL_ERROR);
+ goto f_err;
+ }
+
#ifndef OPENSSL_NO_DH
/*-
* for interoperability with some versions of the Microsoft TLS
@@ -1855,38 +1812,46 @@ int tls_construct_server_key_exchange(SSL *s)
* as the prime
*/
if ((i == 2) && (type & (SSL_kDHE | SSL_kDHEPSK))) {
- s2n(nr[0], p);
- for (j = 0; j < (nr[0] - nr[2]); ++j) {
- *p = 0;
- ++p;
+ size_t len = BN_num_bytes(r[0]) - BN_num_bytes(r[2]);
+
+ if (len > 0) {
+ if (!WPACKET_allocate_bytes(&pkt, len, &binval)) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
+ ERR_R_INTERNAL_ERROR);
+ goto f_err;
+ }
+ memset(binval, 0, len);
}
- } else
+ }
#endif
- s2n(nr[i], p);
- BN_bn2bin(r[i], p);
- p += nr[i];
+ if (!WPACKET_allocate_bytes(&pkt, BN_num_bytes(r[i]), &binval)
+ || !WPACKET_close(&pkt)) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
+ ERR_R_INTERNAL_ERROR);
+ goto f_err;
+ }
+
+ BN_bn2bin(r[i], binval);
}
#ifndef OPENSSL_NO_EC
if (type & (SSL_kECDHE | SSL_kECDHEPSK)) {
/*
- * XXX: For now, we only support named (not generic) curves. In
- * this situation, the serverKeyExchange message has: [1 byte
- * CurveType], [2 byte CurveName] [1 byte length of encoded
- * point], followed by the actual encoded point itself
+ * We only support named (not generic) curves. In this situation, the
+ * ServerKeyExchange message has: [1 byte CurveType], [2 byte CurveName]
+ * [1 byte length of encoded point], followed by the actual encoded
+ * point itself
*/
- *p = NAMED_CURVE_TYPE;
- p += 1;
- *p = 0;
- p += 1;
- *p = curve_id;
- p += 1;
- *p = encodedlen;
- p += 1;
- memcpy(p, encodedPoint, encodedlen);
+ if (!WPACKET_put_bytes_u8(&pkt, NAMED_CURVE_TYPE)
+ || !WPACKET_put_bytes_u8(&pkt, 0)
+ || !WPACKET_put_bytes_u8(&pkt, curve_id)
+ || !WPACKET_sub_memcpy_u8(&pkt, encodedPoint, encodedlen)) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
+ ERR_R_INTERNAL_ERROR);
+ goto f_err;
+ }
OPENSSL_free(encodedPoint);
encodedPoint = NULL;
- p += encodedlen;
}
#endif
@@ -1897,36 +1862,49 @@ int tls_construct_server_key_exchange(SSL *s)
* points to the space at the end.
*/
if (md) {
+ unsigned char *sigbytes1, *sigbytes2;
+ unsigned int siglen;
+
+ /* Get length of the parameters we have written above */
+ if (!WPACKET_get_length(&pkt, ¶mlen)) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
+ ERR_R_INTERNAL_ERROR);
+ goto f_err;
+ }
/* send signature algorithm */
if (SSL_USE_SIGALGS(s)) {
- if (!tls12_get_sigandhash_old(p, pkey, md)) {
+ if (!tls12_get_sigandhash(&pkt, pkey, md)) {
/* Should never happen */
- al = SSL_AD_INTERNAL_ERROR;
SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
ERR_R_INTERNAL_ERROR);
goto f_err;
}
- p += 2;
}
#ifdef SSL_DEBUG
fprintf(stderr, "Using hash %s\n", EVP_MD_name(md));
#endif
- if (EVP_SignInit_ex(md_ctx, md, NULL) <= 0
- || EVP_SignUpdate(md_ctx, &(s->s3->client_random[0]),
- SSL3_RANDOM_SIZE) <= 0
- || EVP_SignUpdate(md_ctx, &(s->s3->server_random[0]),
- SSL3_RANDOM_SIZE) <= 0
- || EVP_SignUpdate(md_ctx, d, n) <= 0
- || EVP_SignFinal(md_ctx, &(p[2]),
- (unsigned int *)&i, pkey) <= 0) {
- SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_LIB_EVP);
- al = SSL_AD_INTERNAL_ERROR;
+ /*
+ * Create the signature. We don't know the actual length of the sig
+ * until after we've created it, so we reserve enough bytes for it
+ * up front, and then properly allocate them in the WPACKET
+ * afterwards.
+ */
+ if (!WPACKET_sub_reserve_bytes_u16(&pkt, EVP_PKEY_size(pkey),
+ &sigbytes1)
+ || EVP_SignInit_ex(md_ctx, md, NULL) <= 0
+ || EVP_SignUpdate(md_ctx, &(s->s3->client_random[0]),
+ SSL3_RANDOM_SIZE) <= 0
+ || EVP_SignUpdate(md_ctx, &(s->s3->server_random[0]),
+ SSL3_RANDOM_SIZE) <= 0
+ || EVP_SignUpdate(md_ctx, s->init_buf->data + paramoffset,
+ paramlen) <= 0
+ || EVP_SignFinal(md_ctx, sigbytes1, &siglen, pkey) <= 0
+ || !WPACKET_sub_allocate_bytes_u16(&pkt, siglen, &sigbytes2)
+ || sigbytes1 != sigbytes2) {
+ SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE,
+ ERR_R_INTERNAL_ERROR);
goto f_err;
}
- s2n(i, p);
- n += i + 2;
- if (SSL_USE_SIGALGS(s))
- n += 2;
} else {
/* Is this error check actually needed? */
al = SSL_AD_HANDSHAKE_FAILURE;
@@ -1936,8 +1914,7 @@ int tls_construct_server_key_exchange(SSL *s)
}
}
- if (!ssl_set_handshake_header(s, SSL3_MT_SERVER_KEY_EXCHANGE, n)) {
- al = SSL_AD_HANDSHAKE_FAILURE;
+ if (!ssl_close_construct_packet(s, &pkt)) {
SSLerr(SSL_F_TLS_CONSTRUCT_SERVER_KEY_EXCHANGE, ERR_R_INTERNAL_ERROR);
goto f_err;
}
@@ -1955,6 +1932,7 @@ int tls_construct_server_key_exchange(SSL *s)
#endif
EVP_MD_CTX_free(md_ctx);
ossl_statem_set_error(s);
+ WPACKET_cleanup(&pkt);
return 0;
}
More information about the openssl-commits
mailing list