From 12078f4c22411c3270d346a93e02b8a6a66c8f9f Mon Sep 17 00:00:00 2001 From: Hanno Becker Date: Tue, 2 Mar 2021 15:28:41 +0000 Subject: [PATCH] Keep pointer to TLS record sequence number static The field `cur_out_ctr` in the SSL context keeps track of the record sequence number for the next outgoing record. For TLS, this sequence number is implicit and not transmitted on the wire, while for DTLS, it's part of of the record header. For DTLS, the position of the record sequence number of the next outgoing record in that record's header is tracked in the pointer `out_ctr`. This pointer moves forward along with other pointers such as `out_hdr` or `out_msg` within the outgoing data buffer `out_buf` as multiple records are written in the same datagram. For TLS, the `out_ctr` pointer is logically superfluous, but for some reason, we're still maintaining it by having it point to the 8 Bytes prior to the header of the next outgoing record, and always copying `cur_out_ctr` to this position prior to encrypting an outgoing record. After a record has been prepared for writing in `ssl_write_record()`, the `out_xxx` pointers (except for `out_buf`, which is static), are shifted forward so that they point to the header and content of the next outgoing record. This is used only in DTLS in order to stack multiple records into a single datagram, but the shifting is happening for TLS as well. However, it has little effect in TLS because we're always flushing immediately after writing, and afterwards reset the `out_xxx` pointers. While the present code works as-is, it is wrong to shift `out_ctr` in the case of TLS, because it makes `out_ctr` point to the last 8 Bytes of the ciphertext of the last outgoing record. Should we ever aim to prepare more than one protected record in `out_buf` before dispatching it to the underlying transport, the superfluous copying of `cur_out_ctr` to `out_buf` will corrupt the last 8 bytes of the last record. This commit aims to fix this problem in the minimal possible way, by simply not shifting `out_ctr` after a record has been written. It does deliberately not attempt to remove `out_ctr` for TLS altogether, because any change in the messaging layer is hard to review, and we're going to replace it soon anyhow. The shifting happens in the helper routine mbedtls_ssl_update_out_pointers, which assumed correctness of `out_hdr` for the beginning of the record header of the next outgoing record, and derives the other `out_xxx` variables. We remove the update of `out_ctr` from this function in the case of TLS, and instead move the proper initialization of `out_ctr` to `out_buf == initial_out_hdr - 8` to the function mbedtls_ssl_reset_in_out_pointers(). Signed-off-by: Hanno Becker --- library/ssl_msg.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/library/ssl_msg.c b/library/ssl_msg.c index 72f09bb42..bced3cd19 100644 --- a/library/ssl_msg.c +++ b/library/ssl_msg.c @@ -5185,7 +5185,6 @@ void mbedtls_ssl_update_out_pointers( mbedtls_ssl_context *ssl, else #endif { - ssl->out_ctr = ssl->out_hdr - 8; ssl->out_len = ssl->out_hdr + 3; #if defined(MBEDTLS_SSL_DTLS_CONNECTION_ID) ssl->out_cid = ssl->out_len; @@ -5266,6 +5265,7 @@ void mbedtls_ssl_reset_in_out_pointers( mbedtls_ssl_context *ssl ) else #endif /* MBEDTLS_SSL_PROTO_DTLS */ { + ssl->out_ctr = ssl->out_buf; ssl->out_hdr = ssl->out_buf + 8; ssl->in_hdr = ssl->in_buf + 8; }