From: jsing Date: Tue, 11 Jan 2022 18:28:41 +0000 (+0000) Subject: Plumb decode errors through key share parsing code. X-Git-Url: http://artulab.com/gitweb/?a=commitdiff_plain;h=7dd422f86e2f79d586d541189cce858f1a896d33;p=openbsd Plumb decode errors through key share parsing code. Distinguish between decode errors and other errors, so that we can send a SSL_AD_DECODE_ERROR alert when appropriate. Fixes a tlsfuzzer failure, due to it expecting a decode error alert and not receiving one. Prompted by anton@ ok tb@ --- diff --git a/lib/libssl/ssl_clnt.c b/lib/libssl/ssl_clnt.c index 19d83653c9d..981161290f6 100644 --- a/lib/libssl/ssl_clnt.c +++ b/lib/libssl/ssl_clnt.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_clnt.c,v 1.134 2022/01/09 15:55:37 jsing Exp $ */ +/* $OpenBSD: ssl_clnt.c,v 1.135 2022/01/11 18:28:41 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1214,7 +1214,7 @@ ssl3_get_server_certificate(SSL *s) static int ssl3_get_server_kex_dhe(SSL *s, CBS *cbs) { - int invalid_params, invalid_key; + int decode_error, invalid_params, invalid_key; int nid = NID_dhKeyAgreement; tls_key_share_free(S3I(s)->hs.key_share); @@ -1222,29 +1222,35 @@ ssl3_get_server_kex_dhe(SSL *s, CBS *cbs) goto err; if (!tls_key_share_peer_params(S3I(s)->hs.key_share, cbs, - &invalid_params)) - goto decode_err; + &decode_error, &invalid_params)) { + if (decode_error) { + SSLerror(s, SSL_R_BAD_PACKET_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + } + goto err; + } if (!tls_key_share_peer_public(S3I(s)->hs.key_share, cbs, - &invalid_key)) - goto decode_err; + &decode_error, &invalid_key)) { + if (decode_error) { + SSLerror(s, SSL_R_BAD_PACKET_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + } + goto err; + } if (invalid_params) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); SSLerror(s, SSL_R_BAD_DH_P_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); goto err; } if (invalid_key) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); SSLerror(s, SSL_R_BAD_DH_PUB_KEY_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); goto err; } return 1; - decode_err: - SSLerror(s, SSL_R_BAD_PACKET_LENGTH); - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); - err: return 0; } @@ -1254,6 +1260,7 @@ ssl3_get_server_kex_ecdhe(SSL *s, CBS *cbs) { uint8_t curve_type; uint16_t curve_id; + int decode_error; CBS public; if (!CBS_get_u8(cbs, &curve_type)) @@ -1285,14 +1292,18 @@ ssl3_get_server_kex_ecdhe(SSL *s, CBS *cbs) if ((S3I(s)->hs.key_share = tls_key_share_new(curve_id)) == NULL) goto err; - if (!tls_key_share_peer_public(S3I(s)->hs.key_share, &public, NULL)) + if (!tls_key_share_peer_public(S3I(s)->hs.key_share, &public, + &decode_error, NULL)) { + if (decode_error) + goto decode_err; goto err; + } return 1; decode_err: - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); SSLerror(s, SSL_R_BAD_PACKET_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); err: return 0; } diff --git a/lib/libssl/ssl_kex.c b/lib/libssl/ssl_kex.c index 78b528b1684..cd6713b8b23 100644 --- a/lib/libssl/ssl_kex.c +++ b/lib/libssl/ssl_kex.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_kex.c,v 1.8 2021/12/04 14:03:22 jsing Exp $ */ +/* $OpenBSD: ssl_kex.c,v 1.9 2022/01/11 18:28:41 jsing Exp $ */ /* * Copyright (c) 2020, 2021 Joel Sing * @@ -156,18 +156,24 @@ ssl_kex_public_dhe(DH *dh, CBB *cbb) } int -ssl_kex_peer_params_dhe(DH *dh, CBS *cbs, int *invalid_params) +ssl_kex_peer_params_dhe(DH *dh, CBS *cbs, int *decode_error, + int *invalid_params) { BIGNUM *p = NULL, *g = NULL; CBS dh_p, dh_g; int ret = 0; + *decode_error = 0; *invalid_params = 0; - if (!CBS_get_u16_length_prefixed(cbs, &dh_p)) + if (!CBS_get_u16_length_prefixed(cbs, &dh_p)) { + *decode_error = 1; goto err; - if (!CBS_get_u16_length_prefixed(cbs, &dh_g)) + } + if (!CBS_get_u16_length_prefixed(cbs, &dh_g)) { + *decode_error = 1; goto err; + } if ((p = BN_bin2bn(CBS_data(&dh_p), CBS_len(&dh_p), NULL)) == NULL) goto err; @@ -194,17 +200,21 @@ ssl_kex_peer_params_dhe(DH *dh, CBS *cbs, int *invalid_params) } int -ssl_kex_peer_public_dhe(DH *dh, CBS *cbs, int *invalid_key) +ssl_kex_peer_public_dhe(DH *dh, CBS *cbs, int *decode_error, + int *invalid_key) { BIGNUM *pub_key = NULL; int check_flags; CBS dh_y; int ret = 0; + *decode_error = 0; *invalid_key = 0; - if (!CBS_get_u16_length_prefixed(cbs, &dh_y)) + if (!CBS_get_u16_length_prefixed(cbs, &dh_y)) { + *decode_error = 1; goto err; + } if ((pub_key = BN_bin2bn(CBS_data(&dh_y), CBS_len(&dh_y), NULL)) == NULL) diff --git a/lib/libssl/ssl_locl.h b/lib/libssl/ssl_locl.h index fcb369405c9..0eca4e673d3 100644 --- a/lib/libssl/ssl_locl.h +++ b/lib/libssl/ssl_locl.h @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_locl.h,v 1.380 2022/01/09 15:53:52 jsing Exp $ */ +/* $OpenBSD: ssl_locl.h,v 1.381 2022/01/11 18:28:41 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1424,8 +1424,10 @@ int ssl_kex_generate_dhe(DH *dh, DH *dh_params); int ssl_kex_generate_dhe_params_auto(DH *dh, size_t key_len); int ssl_kex_params_dhe(DH *dh, CBB *cbb); int ssl_kex_public_dhe(DH *dh, CBB *cbb); -int ssl_kex_peer_params_dhe(DH *dh, CBS *cbs, int *invalid_params); -int ssl_kex_peer_public_dhe(DH *dh, CBS *cbs, int *invalid_key); +int ssl_kex_peer_params_dhe(DH *dh, CBS *cbs, int *decode_error, + int *invalid_params); +int ssl_kex_peer_public_dhe(DH *dh, CBS *cbs, int *decode_error, + int *invalid_key); int ssl_kex_derive_dhe(DH *dh, DH *dh_peer, uint8_t **shared_key, size_t *shared_key_len); diff --git a/lib/libssl/ssl_srvr.c b/lib/libssl/ssl_srvr.c index 0979750e222..dd622c28319 100644 --- a/lib/libssl/ssl_srvr.c +++ b/lib/libssl/ssl_srvr.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_srvr.c,v 1.137 2022/01/09 15:40:13 jsing Exp $ */ +/* $OpenBSD: ssl_srvr.c,v 1.138 2022/01/11 18:28:41 jsing Exp $ */ /* Copyright (C) 1995-1998 Eric Young (eay@cryptsoft.com) * All rights reserved. * @@ -1701,21 +1701,26 @@ ssl3_get_client_kex_dhe(SSL *s, CBS *cbs) { uint8_t *key = NULL; size_t key_len = 0; - int invalid_key; + int decode_error, invalid_key; int ret = 0; if (S3I(s)->hs.key_share == NULL) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); SSLerror(s, SSL_R_MISSING_TMP_DH_KEY); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_HANDSHAKE_FAILURE); goto err; } if (!tls_key_share_peer_public(S3I(s)->hs.key_share, cbs, - &invalid_key)) + &decode_error, &invalid_key)) { + if (decode_error) { + SSLerror(s, SSL_R_BAD_PACKET_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + } goto err; + } if (invalid_key) { - ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); SSLerror(s, SSL_R_BAD_DH_PUB_KEY_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_ILLEGAL_PARAMETER); goto err; } @@ -1738,6 +1743,7 @@ ssl3_get_client_kex_ecdhe(SSL *s, CBS *cbs) { uint8_t *key = NULL; size_t key_len = 0; + int decode_error; CBS public; int ret = 0; @@ -1747,10 +1753,19 @@ ssl3_get_client_kex_ecdhe(SSL *s, CBS *cbs) goto err; } - if (!CBS_get_u8_length_prefixed(cbs, &public)) + if (!CBS_get_u8_length_prefixed(cbs, &public)) { + SSLerror(s, SSL_R_BAD_PACKET_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); goto err; - if (!tls_key_share_peer_public(S3I(s)->hs.key_share, &public, NULL)) + } + if (!tls_key_share_peer_public(S3I(s)->hs.key_share, &public, + &decode_error, NULL)) { + if (decode_error) { + SSLerror(s, SSL_R_BAD_PACKET_LENGTH); + ssl3_send_alert(s, SSL3_AL_FATAL, SSL_AD_DECODE_ERROR); + } goto err; + } if (!tls_key_share_derive(S3I(s)->hs.key_share, &key, &key_len)) goto err; diff --git a/lib/libssl/ssl_tlsext.c b/lib/libssl/ssl_tlsext.c index 7538efdc8c6..69f8ddbc40a 100644 --- a/lib/libssl/ssl_tlsext.c +++ b/lib/libssl/ssl_tlsext.c @@ -1,4 +1,4 @@ -/* $OpenBSD: ssl_tlsext.c,v 1.107 2022/01/11 18:24:03 jsing Exp $ */ +/* $OpenBSD: ssl_tlsext.c,v 1.108 2022/01/11 18:28:41 jsing Exp $ */ /* * Copyright (c) 2016, 2017, 2019 Joel Sing * Copyright (c) 2017 Doug Hogan @@ -1478,6 +1478,7 @@ int tlsext_keyshare_server_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) { CBS client_shares, key_exchange; + int decode_error; uint16_t group; if (!CBS_get_u16_length_prefixed(cbs, &client_shares)) @@ -1515,8 +1516,11 @@ tlsext_keyshare_server_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) return 0; } if (!tls_key_share_peer_public(S3I(s)->hs.key_share, - &key_exchange, NULL)) + &key_exchange, &decode_error, NULL)) { + if (!decode_error) + *alert = SSL_AD_INTERNAL_ERROR; return 0; + } } return 1; @@ -1561,6 +1565,7 @@ int tlsext_keyshare_client_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) { CBS key_exchange; + int decode_error; uint16_t group; /* Unpack server share. */ @@ -1588,8 +1593,11 @@ tlsext_keyshare_client_parse(SSL *s, uint16_t msg_type, CBS *cbs, int *alert) return 0; } if (!tls_key_share_peer_public(S3I(s)->hs.key_share, - &key_exchange, NULL)) + &key_exchange, &decode_error, NULL)) { + if (!decode_error) + *alert = SSL_AD_INTERNAL_ERROR; return 0; + } return 1; } diff --git a/lib/libssl/tls_internal.h b/lib/libssl/tls_internal.h index f7f939215ad..a009635a05b 100644 --- a/lib/libssl/tls_internal.h +++ b/lib/libssl/tls_internal.h @@ -1,4 +1,4 @@ -/* $OpenBSD: tls_internal.h,v 1.4 2022/01/07 15:46:30 jsing Exp $ */ +/* $OpenBSD: tls_internal.h,v 1.5 2022/01/11 18:28:41 jsing Exp $ */ /* * Copyright (c) 2018, 2019, 2021 Joel Sing * @@ -72,9 +72,9 @@ int tls_key_share_generate(struct tls_key_share *ks); int tls_key_share_params(struct tls_key_share *ks, CBB *cbb); int tls_key_share_public(struct tls_key_share *ks, CBB *cbb); int tls_key_share_peer_params(struct tls_key_share *ks, CBS *cbs, - int *invalid_params); + int *decode_error, int *invalid_params); int tls_key_share_peer_public(struct tls_key_share *ks, CBS *cbs, - int *invalid_key); + int *decode_error, int *invalid_key); int tls_key_share_derive(struct tls_key_share *ks, uint8_t **shared_key, size_t *shared_key_len); diff --git a/lib/libssl/tls_key_share.c b/lib/libssl/tls_key_share.c index eb30a0ea694..e5e6c304b68 100644 --- a/lib/libssl/tls_key_share.c +++ b/lib/libssl/tls_key_share.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls_key_share.c,v 1.3 2022/01/07 15:46:30 jsing Exp $ */ +/* $OpenBSD: tls_key_share.c,v 1.4 2022/01/11 18:28:41 jsing Exp $ */ /* * Copyright (c) 2020, 2021 Joel Sing * @@ -301,14 +301,15 @@ tls_key_share_public(struct tls_key_share *ks, CBB *cbb) static int tls_key_share_peer_params_dhe(struct tls_key_share *ks, CBS *cbs, - int *invalid_params) + int *decode_error, int *invalid_params) { if (ks->dhe != NULL || ks->dhe_peer != NULL) return 0; if ((ks->dhe_peer = DH_new()) == NULL) return 0; - if (!ssl_kex_peer_params_dhe(ks->dhe_peer, cbs, invalid_params)) + if (!ssl_kex_peer_params_dhe(ks->dhe_peer, cbs, decode_error, + invalid_params)) return 0; if ((ks->dhe = DHparams_dup(ks->dhe_peer)) == NULL) return 0; @@ -318,22 +319,24 @@ tls_key_share_peer_params_dhe(struct tls_key_share *ks, CBS *cbs, int tls_key_share_peer_params(struct tls_key_share *ks, CBS *cbs, - int *invalid_params) + int *decode_error, int *invalid_params) { if (ks->nid != NID_dhKeyAgreement) return 0; - return tls_key_share_peer_params_dhe(ks, cbs, invalid_params); + return tls_key_share_peer_params_dhe(ks, cbs, decode_error, + invalid_params); } static int tls_key_share_peer_public_dhe(struct tls_key_share *ks, CBS *cbs, - int *invalid_key) + int *decode_error, int *invalid_key) { if (ks->dhe_peer == NULL) return 0; - return ssl_kex_peer_public_dhe(ks->dhe_peer, cbs, invalid_key); + return ssl_kex_peer_public_dhe(ks->dhe_peer, cbs, decode_error, + invalid_key); } static int @@ -362,30 +365,39 @@ tls_key_share_peer_public_ecdhe_ecp(struct tls_key_share *ks, CBS *cbs) } static int -tls_key_share_peer_public_x25519(struct tls_key_share *ks, CBS *cbs) +tls_key_share_peer_public_x25519(struct tls_key_share *ks, CBS *cbs, + int *decode_error) { size_t out_len; + *decode_error = 0; + if (ks->x25519_peer_public != NULL) return 0; - if (CBS_len(cbs) != X25519_KEY_LENGTH) + if (CBS_len(cbs) != X25519_KEY_LENGTH) { + *decode_error = 1; return 0; + } return CBS_stow(cbs, &ks->x25519_peer_public, &out_len); } int -tls_key_share_peer_public(struct tls_key_share *ks, CBS *cbs, int *invalid_key) +tls_key_share_peer_public(struct tls_key_share *ks, CBS *cbs, int *decode_error, + int *invalid_key) { + *decode_error = 0; + if (invalid_key != NULL) *invalid_key = 0; if (ks->nid == NID_dhKeyAgreement) - return tls_key_share_peer_public_dhe(ks, cbs, invalid_key); + return tls_key_share_peer_public_dhe(ks, cbs, decode_error, + invalid_key); if (ks->nid == NID_X25519) - return tls_key_share_peer_public_x25519(ks, cbs); + return tls_key_share_peer_public_x25519(ks, cbs, decode_error); return tls_key_share_peer_public_ecdhe_ecp(ks, cbs); }