Skip to content

Commit

Permalink
fixup! fixup! fixup! fixup! fixup! fixup! fixup! Refactor code and fi…
Browse files Browse the repository at this point in the history 
…x a couple of missing DTLSv1.3 checks.
  • Loading branch information
@fwh-dc
fwh-dc committed May 21, 2024
1 parent 601919a commit 70b1946
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 74 deletions.
3 changes: 0 additions & 3 deletions include/openssl/tls1.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -355,9 +355,6 @@ __owur int SSL_check_chain(SSL *s, X509 *x, EVP_PKEY *pk, STACK_OF(X509) *chain)
# define SSL_CTX_get_tlsext_status_type(ssl) \
SSL_CTX_ctrl(ssl,SSL_CTRL_GET_TLSEXT_STATUS_REQ_TYPE,0,NULL)

# define SSL_CTX_IS_DTLS(ctx) \
(((ctx)->method->ssl3_enc->enc_flags & SSL_ENC_FLAG_DTLS) != 0)

# ifndef OPENSSL_NO_DEPRECATED_3_0
# define SSL_CTX_set_tlsext_ticket_key_cb(ssl, cb) \
SSL_CTX_callback_ctrl(ssl,SSL_CTRL_SET_TLSEXT_TICKET_KEY_CB,\
Expand Down
3 changes: 3 additions & 0 deletions ssl/ssl_local.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1245,6 +1245,9 @@ struct ssl_ctx_st {
# endif
};

# define SSL_CTX_IS_DTLS(ctx) \
(((ctx)->method->ssl3_enc->enc_flags & SSL_ENC_FLAG_DTLS) != 0)

typedef struct cert_pkey_st CERT_PKEY;

#define SSL_TYPE_SSL_CONNECTION 0
Expand Down
14 changes: 7 additions & 7 deletions ssl/statem/extensions.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -822,6 +822,8 @@ int tls_parse_all_extensions(SSL_CONNECTION *s, int context,
int should_add_extension(SSL_CONNECTION *s, unsigned int extctx,
unsigned int thisctx, int max_version)
{
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

/* Skip if not relevant for our context */
if ((extctx & thisctx) == 0)
return 0;
Expand All @@ -830,8 +832,7 @@ int should_add_extension(SSL_CONNECTION *s, unsigned int extctx,
if (!extension_is_relevant(s, extctx, thisctx)
|| ((extctx & SSL_EXT_TLS1_3_ONLY) != 0
&& (thisctx & SSL_EXT_CLIENT_HELLO) != 0
&& (SSL_CONNECTION_IS_DTLS(s) ? DTLS_VERSION_LT(max_version, DTLS1_3_VERSION)
: max_version < TLS1_3_VERSION)))
&& ssl_version_cmp(s, max_version, version1_3) < 0))
return 0;

return 1;
Expand Down Expand Up @@ -1446,18 +1447,17 @@ static int final_key_share(SSL_CONNECTION *s, unsigned int context, int sent)
* Find the first group we allow that is also in client's list
*/
for (i = 0; i < num_groups; i++) {
int version;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
: TLS1_3_VERSION;

group_id = pgroups[i];
version = SSL_CONNECTION_IS_DTLS(s) ?
DTLS1_3_VERSION : TLS1_3_VERSION;

if (check_in_list(s, group_id, clntgroups, clnt_num_groups,
1)
&& tls_group_allowed(s, group_id,
SSL_SECOP_CURVE_SUPPORTED)
&& tls_valid_group(s, group_id, version,
version, 0, NULL))
&& tls_valid_group(s, group_id, version1_3,
version1_3, 0, NULL))
break;
}

Expand Down
79 changes: 36 additions & 43 deletions ssl/statem/extensions_clnt.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ static int use_ecc(SSL_CONNECTION *s, int min_version, int max_version)
const uint16_t *pgroups = NULL;
size_t num_groups, j;
SSL *ssl = SSL_CONNECTION_GET_SSL(s);
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

/* See if we support any ECC ciphersuites */
if (s->version == SSL3_VERSION)
Expand All @@ -152,12 +153,14 @@ static int use_ecc(SSL_CONNECTION *s, int min_version, int max_version)
end = sk_SSL_CIPHER_num(cipher_stack);
for (i = 0; i < end; i++) {
const SSL_CIPHER *c = sk_SSL_CIPHER_value(cipher_stack, i);
const int cipher_minversion = SSL_CONNECTION_IS_DTLS(s) ? c->min_dtls
: c->min_tls;

alg_k = c->algorithm_mkey;
alg_a = c->algorithm_auth;
if ((alg_k & (SSL_kECDHE | SSL_kECDHEPSK))
|| (alg_a & SSL_aECDSA)
|| c->min_tls >= TLS1_3_VERSION) {
|| ssl_version_cmp(s, cipher_minversion, version1_3) >= 0) {
ret = 1;
break;
}
Expand Down Expand Up @@ -217,7 +220,7 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL_CONNECTION *s, WPACKET *pkt,
const uint16_t *pgroups = NULL;
size_t num_groups = 0, i, tls13added = 0, added = 0;
int min_version, max_version, reason;
const int isdtls = SSL_CONNECTION_IS_DTLS(s);
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

reason = ssl_get_min_max_version(s, &min_version, &max_version, NULL);
if (reason != 0) {
Expand All @@ -230,8 +233,7 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL_CONNECTION *s, WPACKET *pkt,
* if we don't have EC support then we don't send this extension.
*/
if (!use_ecc(s, min_version, max_version))
if ((!isdtls && max_version < TLS1_3_VERSION)
|| (isdtls && DTLS_VERSION_LT(max_version, DTLS1_3_VERSION)))
if (ssl_version_cmp(s, max_version, version1_3) < 0)
return EXT_RETURN_NOT_SENT;

/*
Expand All @@ -258,8 +260,7 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL_CONNECTION *s, WPACKET *pkt,
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
return EXT_RETURN_FAIL;
}
if ((okfortls13 && max_version == TLS1_3_VERSION)
|| (okfortls13 && max_version == DTLS1_3_VERSION))
if (okfortls13 && max_version == version1_3)
tls13added++;
added++;
}
Expand All @@ -273,8 +274,7 @@ EXT_RETURN tls_construct_ctos_supported_groups(SSL_CONNECTION *s, WPACKET *pkt,
return EXT_RETURN_FAIL;
}

if (tls13added == 0 && (max_version == TLS1_3_VERSION
|| max_version == DTLS1_3_VERSION)) {
if (tls13added == 0 && max_version == version1_3) {
SSLfatal_data(s, SSL_AD_INTERNAL_ERROR, SSL_R_NO_SUITABLE_GROUPS,
"No groups enabled for max supported SSL/TLS version");
return EXT_RETURN_FAIL;
Expand All @@ -288,14 +288,14 @@ EXT_RETURN tls_construct_ctos_session_ticket(SSL_CONNECTION *s, WPACKET *pkt,
size_t chainidx)
{
size_t ticklen;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

if (!tls_use_ticket(s))
return EXT_RETURN_NOT_SENT;

if (!s->new_session && s->session != NULL
&& s->session->ext.tick != NULL
&& s->session->ssl_version != TLS1_3_VERSION
&& s->session->ssl_version != DTLS1_3_VERSION) {
&& s->session->ssl_version != version1_3) {
ticklen = s->session->ext.ticklen;
} else if (s->session && s->ext.session_ticket != NULL
&& s->ext.session_ticket->data != NULL) {
Expand Down Expand Up @@ -565,7 +565,8 @@ EXT_RETURN tls_construct_ctos_supported_versions(SSL_CONNECTION *s, WPACKET *pkt
size_t chainidx)
{
int currv, min_version, max_version, reason;
int isdtls = SSL_CONNECTION_IS_DTLS(s);
const int isdtls = SSL_CONNECTION_IS_DTLS(s);
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

reason = ssl_get_min_max_version(s, &min_version, &max_version, NULL);
if (reason != 0) {
Expand All @@ -576,8 +577,7 @@ EXT_RETURN tls_construct_ctos_supported_versions(SSL_CONNECTION *s, WPACKET *pkt
/*
* Don't include this if we can't negotiate (D)TLSv1.3.
*/
if ((!isdtls && max_version < TLS1_3_VERSION)
|| (isdtls && DTLS_VERSION_LT(max_version, DTLS1_3_VERSION)))
if (ssl_version_cmp(s, max_version, version1_3) < 0)
return EXT_RETURN_NOT_SENT;

if (!WPACKET_put_bytes_u16(pkt, TLSEXT_TYPE_supported_versions)
Expand All @@ -587,19 +587,11 @@ EXT_RETURN tls_construct_ctos_supported_versions(SSL_CONNECTION *s, WPACKET *pkt
return EXT_RETURN_FAIL;
}

if (isdtls) {
for (currv = max_version; DTLS_VERSION_GE(currv, min_version); currv++) {
if (!WPACKET_put_bytes_u16(pkt, currv)) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
return EXT_RETURN_FAIL;
}
}
} else {
for (currv = max_version; currv >= min_version; currv--) {
if (!WPACKET_put_bytes_u16(pkt, currv)) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
return EXT_RETURN_FAIL;
}
for (currv = max_version; ssl_version_cmp(s, currv, min_version) >= 0;
isdtls ? currv++ : currv--) {
if (!WPACKET_put_bytes_u16(pkt, currv)) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
return EXT_RETURN_FAIL;
}
}
if (!WPACKET_close(pkt) || !WPACKET_close(pkt)) {
Expand Down Expand Up @@ -725,14 +717,13 @@ EXT_RETURN tls_construct_ctos_key_share(SSL_CONNECTION *s, WPACKET *pkt,
curve_id = s->s3.group_id;
} else {
for (i = 0; i < num_groups; i++) {
int version;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
: TLS1_3_VERSION;

if (!tls_group_allowed(s, pgroups[i], SSL_SECOP_CURVE_SUPPORTED))
continue;

version = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

if (!tls_valid_group(s, pgroups[i], version, version,
if (!tls_valid_group(s, pgroups[i], version1_3, version1_3,
0, NULL))
continue;

Expand Down Expand Up @@ -803,15 +794,15 @@ EXT_RETURN tls_construct_ctos_early_data(SSL_CONNECTION *s, WPACKET *pkt,
SSL_SESSION *edsess = NULL;
const EVP_MD *handmd = NULL;
SSL *ssl = SSL_CONNECTION_GET_SSL(s);
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;


if (s->hello_retry_request == SSL_HRR_PENDING)
handmd = ssl_handshake_md(s);

if (s->psk_use_session_cb != NULL
&& (!s->psk_use_session_cb(ssl, handmd, &id, &idlen, &psksess)
|| (psksess != NULL
&& psksess->ssl_version != TLS1_3_VERSION
&& psksess->ssl_version != DTLS1_3_VERSION))) {
|| (psksess != NULL && psksess->ssl_version != version1_3))) {
SSL_SESSION_free(psksess);
SSLfatal(s, SSL_AD_INTERNAL_ERROR, SSL_R_BAD_PSK);
return EXT_RETURN_FAIL;
Expand All @@ -833,7 +824,8 @@ EXT_RETURN tls_construct_ctos_early_data(SSL_CONNECTION *s, WPACKET *pkt,
} else if (psklen > 0) {
const unsigned char tls13_aes128gcmsha256_id[] = { 0x13, 0x01 };
const SSL_CIPHER *cipher;
int version;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
: TLS1_3_VERSION;

idlen = strlen(identity);
if (idlen > PSK_MAX_IDENTITY_LEN) {
Expand All @@ -853,12 +845,11 @@ EXT_RETURN tls_construct_ctos_early_data(SSL_CONNECTION *s, WPACKET *pkt,
}

psksess = SSL_SESSION_new();
version = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

if (psksess == NULL
|| !SSL_SESSION_set1_master_key(psksess, psk, psklen)
|| !SSL_SESSION_set_cipher(psksess, cipher)
|| !SSL_SESSION_set_protocol_version(psksess, version)) {
|| !SSL_SESSION_set_protocol_version(psksess, version1_3)) {
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
OPENSSL_cleanse(psk, psklen);
return EXT_RETURN_FAIL;
Expand Down Expand Up @@ -971,6 +962,7 @@ EXT_RETURN tls_construct_ctos_padding(SSL_CONNECTION *s, WPACKET *pkt,
{
unsigned char *padbytes;
size_t hlen;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

if ((s->options & SSL_OP_TLSEXT_PADDING) == 0)
return EXT_RETURN_NOT_SENT;
Expand All @@ -990,7 +982,7 @@ EXT_RETURN tls_construct_ctos_padding(SSL_CONNECTION *s, WPACKET *pkt,
* If we're going to send a PSK then that will be written out after this
* extension, so we need to calculate how long it is going to be.
*/
if ((s->session->ssl_version == TLS1_3_VERSION || s->session->ssl_version == DTLS1_3_VERSION)
if (s->session->ssl_version == version1_3
&& s->session->ext.ticklen != 0
&& s->session->cipher != NULL) {
const EVP_MD *md = ssl_md(SSL_CONNECTION_GET_CTX(s),
Expand Down Expand Up @@ -1047,6 +1039,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL_CONNECTION *s, WPACKET *pkt,
int dores = 0;
SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(s);
OSSL_TIME t;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

s->ext.tick_identity = 0;

Expand All @@ -1060,7 +1053,7 @@ EXT_RETURN tls_construct_ctos_psk(SSL_CONNECTION *s, WPACKET *pkt,
* If this is an incompatible or new session then we have nothing to resume
* so don't add this extension.
*/
if ((s->session->ssl_version != TLS1_3_VERSION && s->session->ssl_version != DTLS1_3_VERSION)
if (s->session->ssl_version != version1_3
|| (s->session->ext.ticklen == 0 && s->psksession == NULL))
return EXT_RETURN_NOT_SENT;

Expand Down Expand Up @@ -1782,6 +1775,7 @@ int tls_parse_stoc_supported_versions(SSL_CONNECTION *s, PACKET *pkt,
X509 *x, size_t chainidx)
{
unsigned int version;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

if (!PACKET_get_net_2(pkt, &version)
|| PACKET_remaining(pkt) != 0) {
Expand All @@ -1793,7 +1787,7 @@ int tls_parse_stoc_supported_versions(SSL_CONNECTION *s, PACKET *pkt,
* The only protocol version we support which is valid in this extension in
* a ServerHello is (D)TLSv1.3 therefore we shouldn't be getting anything else.
*/
if (version != TLS1_3_VERSION && version != DTLS1_3_VERSION) {
if (version != version1_3) {
SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
SSL_R_BAD_PROTOCOL_VERSION_NUMBER);
return 0;
Expand Down Expand Up @@ -1837,7 +1831,8 @@ int tls_parse_stoc_key_share(SSL_CONNECTION *s, PACKET *pkt,
if ((context & SSL_EXT_TLS1_3_HELLO_RETRY_REQUEST) != 0) {
const uint16_t *pgroups = NULL;
size_t i, num_groups;
int version;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
: TLS1_3_VERSION;

if (PACKET_remaining(pkt) != 0) {
SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_R_LENGTH_MISMATCH);
Expand All @@ -1860,11 +1855,9 @@ int tls_parse_stoc_key_share(SSL_CONNECTION *s, PACKET *pkt,
break;
}

version = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

if (i >= num_groups
|| !tls_group_allowed(s, group_id, SSL_SECOP_CURVE_SUPPORTED)
|| !tls_valid_group(s, group_id, version, version, 0, NULL)) {
|| !tls_valid_group(s, group_id, version1_3, version1_3, 0, NULL)) {
SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER, SSL_R_BAD_KEY_SHARE);
return 0;
}
Expand Down
16 changes: 8 additions & 8 deletions ssl/statem/extensions_srvr.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -647,8 +647,8 @@ int tls_parse_ctos_key_share(SSL_CONNECTION *s, PACKET *pkt,
}

while (PACKET_remaining(&key_share_list) > 0) {
const int version13 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
: TLS1_3_VERSION;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
: TLS1_3_VERSION;

if (!PACKET_get_net_2(&key_share_list, &group_id)
|| !PACKET_get_length_prefixed_2(&key_share_list, &encoded_pt)
Expand Down Expand Up @@ -688,7 +688,7 @@ int tls_parse_ctos_key_share(SSL_CONNECTION *s, PACKET *pkt,
* We tolerate but ignore a group id that we don't think is
* suitable for (D)TLSv1.3
*/
|| !tls_valid_group(s, group_id, version13, version13,
|| !tls_valid_group(s, group_id, version1_3, version1_3,
0, NULL)) {
/* Share not suitable */
continue;
Expand Down Expand Up @@ -734,6 +734,7 @@ int tls_parse_ctos_cookie(SSL_CONNECTION *s, PACKET *pkt, unsigned int context,
uint64_t tm, now;
SSL *ssl = SSL_CONNECTION_GET_SSL(s);
SSL_CTX *sctx = SSL_CONNECTION_GET_CTX(s);
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

/* Ignore any cookie if we're not set up to verify it */
if (sctx->verify_stateless_cookie_cb == NULL
Expand Down Expand Up @@ -806,7 +807,7 @@ int tls_parse_ctos_cookie(SSL_CONNECTION *s, PACKET *pkt, unsigned int context,
SSLfatal(s, SSL_AD_DECODE_ERROR, SSL_R_LENGTH_MISMATCH);
return 0;
}
if (version != TLS1_3_VERSION && version != DTLS1_3_VERSION) {
if (version != version1_3) {
SSLfatal(s, SSL_AD_ILLEGAL_PARAMETER,
SSL_R_BAD_PROTOCOL_VERSION_NUMBER);
return 0;
Expand Down Expand Up @@ -1084,7 +1085,8 @@ int tls_parse_ctos_psk(SSL_CONNECTION *s, PACKET *pkt, unsigned int context,
} else if (pskdatalen > 0) {
const SSL_CIPHER *cipher;
const unsigned char tls13_aes128gcmsha256_id[] = { 0x13, 0x01 };
int version;
const int version1_3 = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION
: TLS1_3_VERSION;

/*
* We found a PSK using an old style callback. We don't know
Expand All @@ -1098,14 +1100,12 @@ int tls_parse_ctos_psk(SSL_CONNECTION *s, PACKET *pkt, unsigned int context,
}

sess = SSL_SESSION_new();
version = SSL_CONNECTION_IS_DTLS(s) ? DTLS1_3_VERSION : TLS1_3_VERSION;

if (sess == NULL
|| !SSL_SESSION_set1_master_key(sess, pskdata,
pskdatalen)
|| !SSL_SESSION_set_cipher(sess, cipher)
|| !SSL_SESSION_set_protocol_version(sess,
version)) {
|| !SSL_SESSION_set_protocol_version(sess, version1_3)) {
OPENSSL_cleanse(pskdata, pskdatalen);
SSLfatal(s, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
goto err;
Expand Down
Loading

0 comments on commit 70b1946

Please sign in to comment.