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 <hanno.becker@arm.com>
diff --git a/library/ssl_msg.c b/library/ssl_msg.c
index 72f09bb..bced3cd 100644
--- a/library/ssl_msg.c
+++ b/library/ssl_msg.c
@@ -5185,7 +5185,6 @@
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 @@
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;
}
