Fix miscalculations of maximum record expansion in mbedtls_ssl_get_record_expansion() #1915
Conversation
`mbedtls_ssl_get_record_expansion()` is supposed to return the maximum
difference between the size of a protected record and the size of the
encapsulated plaintext.
It had the following two bugs:
(1) It did not consider the new ChaChaPoly ciphersuites, returning
the error code #MBEDTLS_ERR_SSL_INTERNAL_ERROR in this case.
(2) It did not correctly estimate the maximum record expansion in case
of CBC ciphersuites in (D)TLS versions 1.1 and higher, in which
case the ciphertext is prefixed by an explicit IV.
This commit fixes both bugs.
hanno-becker
added
the
needs-review
library/ssl_tls.c
Outdated
| * we never use more than block_size) | ||
| * - explicit IV | ||
| */ | ||
| transform_expansion = transform->maclen + 2 * block_size; |
There was a problem hiding this comment.
This would be clearer with successive additions, with a structure like this:
transform_expansion = transform->maclen;
transform_expansion += block_size; // max padding size
if (TLS version >= 1.1) transform_expansion += block_size; // max IV size for explicit padding
Please use this structure in backports. But for the development branch, please de-duplicate the calculation.
There was a problem hiding this comment.
Functionally, this looks fine.
- I see one other place where
MBEDTLS_MODE_CHACHAPOLY(Allow ChaChaPoly for TLS session tickets #1917) but this is only a limitation, not a bug. - The new size calculation for CBC looks correct.
- I can't see any other missing cases here.
Ideally I'd like a non-regression test but this looks difficult with the current TLS stack.
However, this code duplicates some existing logic. If we managed to get it wrong, we shouldn't have duplicated it. Please create an auxiliary function to de-duplicate the calculation.
library/ssl_tls.c
Outdated
| #if defined(MBEDTLS_SSL_PROTO_TLS1_1) || defined(MBEDTLS_SSL_PROTO_TLS1_2) | ||
| if( ssl->minor_ver >= MBEDTLS_SSL_MINOR_VERSION_2 ) | ||
| { | ||
| /* Expansion due to addition of |
There was a problem hiding this comment.
This logic is largely duplicated in mbedtls_ssl_derive_keys with the calculation of transform->minlen. We have a mostly-duplicated calculation, and we got it wrong in one place, so we really should de-duplicate it.
transform->minlen is the size of a record with a payload of size 0, i.e. it's the record overhead for a payload of size 0. get_record_expansion is the maximum record overhead. The difference between the two is that transform->minlen calculates the exact padding length (which is not necessarily the block size: it's true in most cases, but not for mac-then-encrypt with a MAC size that isn't a multiple of the block size) whereas get_record_expansion uses the maximum padding size.
We could either have a function that takes a flag to indicate “padding for empty record” vs “maximum padding size”, or a pair of functions, one that returns the constant part of the overhead and one that returns the part that gets padded.
There was a problem hiding this comment.
I suggest the following: We introduce a function mbedtls_ssl_transform_get_expansion which takes an SSL transform and an optional plaintext length, and returns pre-expansion and post-expansion values for the transform. If the plaintext length is specified, both must be accurate; if not, the postexpansion must be an overapproximation for all plaintext lengths. Pre-expansions never vary, so it is always accurate.
With that, minlen can be defined as pre_exp(0) + post_exp(0), and mbedtls_ssl_get_record_expansion() can be defined as mbedtls_ssl_hdr_len + pre_exp(unspecified) + post_exp(unspecified).
Since transform->minlen is redundant, we might remove it from mbedtls_ssl_transform on that occasion.
The pre-expansion determines the offset of ssl->out_msg from ssl->out_iv, so we could also rewrite updating these pointers in terms of mbedtls_ssl_transform_get_expansion.
The above proposal can not yet be implemented as stated since not all information determining a transform are actually contained in mbedtls_ssl_transform, for example the Encrypt-then-MAC flag. PR #1633 fixes this, so if you think the above sounds reasonable, I suggest we come back to it as soon as #1633 is merged, and stay with the current version of this PR until then.
WDYT?
There was a problem hiding this comment.
Ok, but why not add the missing information to the transform, or pass it separately?
There was a problem hiding this comment.
@gilles-peskine-arm I don't want to mix up this PR with #1633, which does add the missing fields.
There was a problem hiding this comment.
Ok, then let's stick with the duplicated calculation for now.
There was a problem hiding this comment.
I'm ok to keep the duplicated calculation until #1633 is merged. But please make the calculation clearer as per https://github.com/ARMmbed/mbedtls/pull/1915/files#r207555016
There was a problem hiding this comment.
@gilles-peskine-arm If you insist I will change it, but if we leave a thorough cleanup for post #1633 anyway, would you accept leaving this PR as is for now? I think the documentation is quite clear on the source of the arithmetic, and the PR has already seen two approvals and @mpg's work on #1939 builds on it.
There was a problem hiding this comment.
I'd really prefer the clear code. It doesn't conflict with any of the changes in #1939, does it?
gilles-peskine-arm
added
needs-work
and removed
needs-review
hanno-becker
added
needs-review
library/ssl_tls.c
Outdated
| transform_expansion = transform->maclen + 2 * block_size; | ||
| } | ||
| else | ||
| #endif /* MBEDTLS_SSL_PROTO_TLS1_1 || MBEDTLS_SSL_PROTO_TLS1_2 */ |
There was a problem hiding this comment.
Note: I'm not too happy about this structure, because it doesn't automatically detect when new versions are added. I'd much prefer "if( 1.1 or 1.2 ) then ... else if( 1.0 or 3.0 ) then ... else return error" as elsewhere. For now it's OK, but when you rework this during deduplication, please use a safer pattern.
|
@hanno-arm Note: #1939 now includes this (by merging it). In order to avoid unnecessary complexity, please refrain from rewriting history in this PR from now on. If changes are needed here, please only add new commits. Thanks! |
hanno-becker
removed
the
needs-backports
|
@mpg @k-stachowiak @gilles-peskine-arm Please re-review. |
gilles-peskine-arm
removed
the
needs-review
|
Note: as noted by Gilles, this is difficult to test properly with the current stack. However, #1939 includes tests that rely on |
mpg
added
the
approved
simonbutcher
added
needs-ci
|
@sbutcher-arm This PR is only partially included in #1939 (#1939 doesn't include a readability-improving change that @gilles-peskine-arm requested), so it should be merged even if #1939 gets in. |
simonbutcher
added
approved for design
approved
Summary: This PR fixes miscalculations of the maximum record expansion in
mbedtls_ssl_get_record_expansion()in the case of ChachaPoly ciphersuites, as well as CBC ciphersuites in (D)TLS versions 1.1 or higher. Fixes #1913 and #1914.This PR has high relevance for the ongoing fragmentation work #1879, which uses
mbedtls_ssl_get_record_expansion()to deduce from the configured MTU the maximum record payload.