Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Backport 3.6] Defragment incoming TLS handshake messages (reuse badmac_seen) #9981

Conversation

gilles-peskine-arm
Copy link
Contributor

@gilles-peskine-arm gilles-peskine-arm commented Feb 13, 2025

This is the 3.6 backport of #9872, with extra commits to preserve ABI compatibility in 3.6.

A simpler alternative to #9949, which wasn't possible when #9949 was made. Originally we needed room for a message size and a pointer in the SSL context. Now, after an improvement to the original PR, we only need room for a message size, and that's easier to find.

PR checklist

Sorry, something went wrong.

@gilles-peskine-arm gilles-peskine-arm added component-tls needs-ci Needs to pass CI tests size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Feb 13, 2025
rojer and others added 11 commits February 17, 2025 15:59
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Co-authored-by: minosgalanakis <30719586+minosgalanakis@users.noreply.github.com>
Signed-off-by: Deomid Ryabkov <rojer@rojer.me>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Except the first

Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
The first fragment of a fragmented handshake message always starts at the beginning of the buffer so there's no need to store it.

Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
No behavior change.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>

Verified

This commit was created on github.com and signed with GitHub’s verified signature. The key has expired.
Prepare to unify two fields of the `mbedtls_ssl_context` structure:
`badmac_seen` (always present but only used in DTLS) and
`in_hsfraglen` (always present but only used in non-DTLS TLS).

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In the `mbedtls_ssl_context` structure, change the type of `in_hsfraglen`
from `size_t` to `unsigned`. This is in preparation for merging
`in_hsfraglen` into `badmac_seen_or_in_hsfraglen`, which has the type
`unsigned` and cannot change since we do not want to change the ABI.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
In the `mbedtls_ssl_context` structure, merge the field `in_hsfraglen` into
`badmac_seen_or_in_hsfraglen`. This restores the ABI of `libmbedtls` as it
was in Mbed TLS 3.6.0 through 3.6.2.

The field `badmac_seen_or_in_hsfraglen` (formerly `badmac_seen`) was only
used for DTLS (despite being present in non-DTLS builds), and the field
`in_hsfraglen` was only used in non-DTLS TLS. Therefore the two values can
be stored in the same field.

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm changed the base branch from mbedtls-3.6 to features/tls-defragmentation/3.6 February 17, 2025 15:31
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm force-pushed the tls_hs_defrag_in-3.6-badmac_seen branch from da8f72e to cb72cd2 Compare February 17, 2025 15:36
Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
@gilles-peskine-arm gilles-peskine-arm added needs-review Every commit must be reviewed by at least two team members, needs-reviewer This PR needs someone to pick it up for review and removed needs-ci Needs to pass CI tests labels Feb 17, 2025
@minosgalanakis minosgalanakis self-requested a review February 18, 2025 11:41
@minosgalanakis
Copy link
Contributor

Thanks for raising this backport PR and the elegant sollution.

Looks good overall, but it would be good to update the programs, using badmac_seen namely ssl_context_info

Signed-off-by: Gilles Peskine <Gilles.Peskine@arm.com>
minosgalanakis
minosgalanakis previously approved these changes Feb 18, 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

@mpg mpg self-requested a review February 19, 2025 08:58
@mpg
Copy link
Contributor

mpg commented Feb 19, 2025

@gilles-peskine-arm According to git range-diff, 3 commits from the original PR are missing from the backport:

 7:  afa11db62010 <  -:  ------------ Remove obselete checks due to the introduction of handhsake defragmen...
 8:  eb77e5b1c778 <  -:  ------------ Update the changelog message
 9:  cf4e6a18e664 <  -:  ------------ Remove unused variable in ssl_server.c

Can you explain why?

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 except for the 3 missing commits.

As usual, thanks for the nice commit structure of the ABI-fixing refactoring, always appreciated as a reviewer.

rojer and others added 3 commits February 19, 2025 22:03
tation. h/t @waleed-elmelegy-arm

Mbed-TLS@909e716

Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
Signed-off-by: Waleed Elmelegy <waleed.elmelegy@arm.com>
Signed-off-by: Deomid rojer Ryabkov <rojer@rojer.me>
@gilles-peskine-arm gilles-peskine-arm removed the needs-reviewer This PR needs someone to pick it up for review label Feb 19, 2025
@gilles-peskine-arm
Copy link
Contributor Author

My bad, I had started from #9949 then removed the tests and hadn't noticed the non-ssl-opt commits in the middle of the ssl-opt commits. I've cherry-picked the extra commits now.

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 now, thanks!

@mpg mpg requested a review from minosgalanakis February 20, 2025 09:06
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

@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 Feb 21, 2025
@mpg
Copy link
Contributor

mpg commented Feb 21, 2025

For the record, I've double-checked the report from the API/ABI compat checked, only two issues are found, both of which are about the renamed field:

  • "Renaming of a field in data type may indicate a change in the semantic meaning of the field." yes absolutely, but applications will be blissfully unaware because the field's private;
  • "Recompilation of a client program may be broken with the error message: @type_name has no member named @target." again not an issue for a private field.

@mpg mpg added this pull request to the merge queue Feb 21, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to no response for status checks Feb 21, 2025
@mpg mpg merged commit cca140b into Mbed-TLS:features/tls-defragmentation/3.6 Feb 24, 2025
4 of 6 checks passed
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.

None yet

5 participants