From 42db3438e170653a0dea8617f6b9a9f5f25fd2be Mon Sep 17 00:00:00 2001 From: jsing <> Date: Wed, 7 Oct 2020 07:46:18 +0000 Subject: Include a TLS record header when switching to the legacy stack. When switching to the legacy TLS stack we previously copied any remaining handshake messages into the receive buffer, but do not include any TLS record header (largely due to the fact that we've already processed part of the TLS record that we actually received - that part is placed into the init_buf). This worked fine with the old record layer implementation, however the new record layer expects to find the TLS record header. This means that if we switch from the new stack to the legacy stack (i.e. the remote side does not support TLSv1.3) and there is more than one handshake message in the TLS plaintext record (which Microsoft's TLS stack is known to do), we now read a TLS record of zero bytes instead of getting the correct length. Fix this by generating a pseudo-TLS record header when switching from the new TLS stack to the legacy stack. Found the hard way by guenther@. Thanks to tb@ for coming up with a reproducible test case and doing much of the debugging. ok inoguchi@ tb@ --- src/lib/libssl/tls13_legacy.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/lib/libssl/tls13_legacy.c b/src/lib/libssl/tls13_legacy.c index 317a1cb0f5..a26afeeeb9 100644 --- a/src/lib/libssl/tls13_legacy.c +++ b/src/lib/libssl/tls13_legacy.c @@ -1,4 +1,4 @@ -/* $OpenBSD: tls13_legacy.c,v 1.13 2020/09/13 15:04:35 jsing Exp $ */ +/* $OpenBSD: tls13_legacy.c,v 1.14 2020/10/07 07:46:18 jsing Exp $ */ /* * Copyright (c) 2018, 2019 Joel Sing * @@ -297,22 +297,35 @@ static int tls13_use_legacy_stack(struct tls13_ctx *ctx) { SSL *s = ctx->ssl; + CBB cbb, fragment; CBS cbs; + memset(&cbb, 0, sizeof(cbb)); + if (!ssl3_setup_init_buffer(s)) - return 0; + goto err; if (!ssl3_setup_buffers(s)) - return 0; + goto err; if (!ssl_init_wbio_buffer(s, 1)) - return 0; + goto err; /* Stash any unprocessed data from the last record. */ tls13_record_layer_rbuf(ctx->rl, &cbs); if (CBS_len(&cbs) > 0) { - if (!CBS_write_bytes(&cbs, - S3I(s)->rbuf.buf + SSL3_RT_HEADER_LENGTH, - S3I(s)->rbuf.len - SSL3_RT_HEADER_LENGTH, NULL)) - return 0; + if (!CBB_init_fixed(&cbb, S3I(s)->rbuf.buf, + S3I(s)->rbuf.len)) + goto err; + if (!CBB_add_u8(&cbb, SSL3_RT_HANDSHAKE)) + goto err; + if (!CBB_add_u16(&cbb, TLS1_2_VERSION)) + goto err; + if (!CBB_add_u16_length_prefixed(&cbb, &fragment)) + goto err; + if (!CBB_add_bytes(&fragment, CBS_data(&cbs), + CBS_len(&cbs))) + goto err; + if (!CBB_finish(&cbb, NULL, NULL)) + goto err; S3I(s)->rbuf.offset = SSL3_RT_HEADER_LENGTH; S3I(s)->rbuf.left = CBS_len(&cbs); @@ -328,13 +341,18 @@ tls13_use_legacy_stack(struct tls13_ctx *ctx) tls13_handshake_msg_data(ctx->hs_msg, &cbs); if (!CBS_write_bytes(&cbs, s->internal->init_buf->data, s->internal->init_buf->length, NULL)) - return 0; + goto err; S3I(s)->tmp.reuse_message = 1; S3I(s)->tmp.message_type = tls13_handshake_msg_type(ctx->hs_msg); S3I(s)->tmp.message_size = CBS_len(&cbs); return 1; + + err: + CBB_cleanup(&cbb); + + return 0; } int -- cgit v1.2.3-55-g6feb