Skip to content

Conversation

@gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Mar 3, 2025

Backport of #10011. Differences:

  • size_t ssl->in_hsfraglen becomes unsigned ssl->badmac_seen_or_in_hsfraglen: name change and printf specifier changes.

PR checklist

@gilles-peskine-arm gilles-peskine-arm added component-tls needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Mar 3, 2025
@gilles-peskine-arm gilles-peskine-arm force-pushed the tls-defragment-incremental-3.6 branch 2 times, most recently from dd3e3f5 to 7988f71 Compare March 4, 2025 09:37
Pacify `clang -Wdocumentation`.

Signed-off-by: Gilles Peskine <[email protected]>
In preparation for reworking mbedtls_ssl_prepare_handshake_record(), tweak
the "waiting for more handshake fragments" log message in
ssl_consume_current_message(), and add a similar one in
mbedtls_ssl_prepare_handshake_record(). Assert both.

Signed-off-by: Gilles Peskine <[email protected]>
In preparation for reworking mbedtls_ssl_prepare_handshake_record(),
tweak the "handshake fragment:" log message.

This changes what information is displayed when a record contains data
beyond the expected end of the handshake message. This case is currently
untested and its handling will change in a subsequent commit.

Signed-off-by: Gilles Peskine <[email protected]>
Minor refactoring of the initial checks and preparation when receiving the
first fragment. Use `ssl->in_hsfraglen` to determine whether there is a
pending handshake fragment, for consistency, and possibly for more
robustness in case handshake fragments are mixed with non-handshake
records (although this is not currently supported anyway).

Signed-off-by: Gilles Peskine <[email protected]>
Reassemble handshake fragments incrementally instead of all at the end. That
is, every time we receive a non-initial handshake fragment, append it to the
initial fragment. Since we only have to deal with at most two handshake
fragments at the same time, this simplifies the code (no re-parsing of a
record) and is a little more memory-efficient (no need to store one record
header per record).

This commit also fixes a bug. The previous code did not calculate offsets
correctly when records use an explicit IV, which is the case in TLS 1.2 with
CBC (encrypt-then-MAC or not), GCM and CCM encryption (i.e. all but null and
ChachaPoly). This led to the wrong data when an encrypted handshake message
was fragmented (Finished or renegotiation). The new code handles this
correctly.

Signed-off-by: Gilles Peskine <[email protected]>
Pacify `clang -Wformat-pedantic`.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the tls-defragment-incremental-3.6 branch from 7988f71 to e6eed84 Compare March 5, 2025 16:03
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, and removed needs-ci Needs to pass CI tests needs-preceding-pr Requires another PR to be merged first labels Mar 5, 2025
minosgalanakis
minosgalanakis previously approved these changes Mar 5, 2025
Copy link
Contributor

@minosgalanakis minosgalanakis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Failthfull backport which contains a single MSCV fix not included in the development PR.

The pointer to the framework may need to be updated before merging, since we are failing component_test_tls13_only_psk because all tests are being skipped.

Changed log messages and added more tests in
`tests/opt-testcases/handshake-generated.sh`.

Signed-off-by: Gilles Peskine <[email protected]>
@gilles-peskine-arm gilles-peskine-arm force-pushed the tls-defragment-incremental-3.6 branch from baaccb3 to 7719169 Compare March 6, 2025 08:36
Signed-off-by: Gilles Peskine <[email protected]>
A handshake record may contain multiple handshake messages, or multiple
fragments (there can be the final fragment of a pending message, then zero
or more whole messages, and an initial fragment of an incomplete message).
This was previously untested, but supported, so don't break it.

Signed-off-by: Gilles Peskine <[email protected]>
There is no longer any different processing at this point, just
near-identical log messages.

Signed-off-by: Gilles Peskine <[email protected]>
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm satisfied this is a faithful backport with the name and type of in_hsfraglen changed.

git range-diff wasn't as useful as it usually is, so this time I just took the whole diff for dev and 3.6, did s/badmac_seen_or_in_hsfraglen/in_hsfraglen/ in the 3.6 diff (and s/^@@ [^ ]* [^ ]* @@/@@/ in both to reduce noise), before diffing the diffs. All that was left was differences related to the type change, which all looked OK.

Signed-off-by: Gilles Peskine <[email protected]>
Copy link
Contributor

@mpg mpg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@mpg mpg added approved Design and code approved - may be waiting for CI or backports and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 7, 2025
@minosgalanakis
Copy link
Contributor

minosgalanakis commented Mar 7, 2025

The CI is still failing at full_without_ecdhe_ecdsa with all tests skipped?

@gilles-peskine-arm
Copy link
Contributor Author

gilles-peskine-arm commented Mar 7, 2025

@minosgalanakis Not all tests skipped, there's one test case that's failing, "Connection ID, 3D+MTU: Cli+Srv enabled, CID on renegotiation", and that's a known intermittent failure. I'll start another job.

After discussion on Slack, let's just go ahead and merge. Note that we had a pass before c22e315, and an almost-pass after.

@gilles-peskine-arm gilles-peskine-arm merged commit 26c378c into Mbed-TLS:features/tls-defragmentation/3.6 Mar 7, 2025
3 of 6 checks passed
@github-project-automation github-project-automation bot moved this from In Development to Done in Roadmap pull requests (new board) Mar 7, 2025
@mpg mpg mentioned this pull request Mar 7, 2025
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Design and code approved - may be waiting for CI or backports component-tls priority-high High priority - will be reviewed soon size-s Estimated task size: small (~2d)

Development

Successfully merging this pull request may close these issues.

3 participants