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.

Sign up for GitHub

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

Jump to bottom

Add TLS Handshake defragmentation tests #9928

Open
wants to merge 30 commits into
base: development
Choose a base branch
from
Open

Add TLS Handshake defragmentation tests #9928

wants to merge 30 commits into from
+484 −41

Conversation

waleed-elmelegy-arm
Copy link
Contributor

@waleed-elmelegy-arm waleed-elmelegy-arm commented Jan 24, 2025 •
edited by mpg
Loading

Description

Fixes #9887
Add TLS Handshake defragmentation tests based on implementation done in #9872 .

PR checklist

  • changelog provided
  • development PR provided
  • framework PR not required
  • 3.6 PR provided #TODO
  • 2.28 PR not required because: New feature
  • tests provided

rojer and others added 3 commits December 25, 2024 14:34
@rojer
Defragment incoming TLS handshake messages
ac2cf1f 
Signed-off-by: Deomid rojer Ryabkov <[email protected]>
@rojer @minosgalanakis
Update ChangeLog.d/tls-hs-defrag-in.txt
5f7c2c2 
Co-authored-by: minosgalanakis <[email protected]>
Signed-off-by: Deomid Ryabkov <[email protected]>
@rojer
Review comments
cad11ad 
Signed-off-by: Deomid rojer Ryabkov <[email protected]>
@waleed-elmelegy-arm waleed-elmelegy-arm added component-tls size-s Estimated task size: small (~2d) labels Jan 24, 2025
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the defrag-tls-handshake-tests branch 2 times, most recently from bfec8af to fc9f04e Compare January 24, 2025 17:53
@waleed-elmelegy-arm waleed-elmelegy-arm added the needs-preceding-pr Requires another PR to be merged first label Jan 24, 2025
@waleed-elmelegy-arm waleed-elmelegy-arm mentioned this pull request Jan 24, 2025
Open
6 tasks
rojer added 2 commits January 26, 2025 11:12
@rojer
Remove mbedtls_ssl_reset_in_out_pointers
3dfe75e 
Signed-off-by: Deomid rojer Ryabkov <[email protected]>
@rojer
Allow fragments less HS msg header size (4 bytes)
aaa152e 
Except the first

Signed-off-by: Deomid rojer Ryabkov <[email protected]>
@minosgalanakis minosgalanakis self-requested a review January 27, 2025 10:05
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the defrag-tls-handshake-tests branch 2 times, most recently from 4a74b6c to 9fae2db Compare January 27, 2025 11:32
@waleed-elmelegy-arm waleed-elmelegy-arm marked this pull request as ready for review January 27, 2025 11:33
@waleed-elmelegy-arm waleed-elmelegy-arm added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests labels Jan 27, 2025
mpg
mpg reviewed Jan 27, 2025
View reviewed changes
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.

Looking pretty good, I like the selection of fragment sizes tested. Just two things:

  • missing TLS 1.3 as pointed out in a comment;
  • we're only testing this when we're the client, should we also have some tests where we're the server (we'll probably want them to use client authentication so that the client has a large message to fragment: its certificate)?

@rojer
Add a safety check for in_hsfraglen
b70e76a 
Signed-off-by: Deomid rojer Ryabkov <[email protected]>
mpg
mpg reviewed Jan 29, 2025
View reviewed changes
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.

Thanks for addressing my feedback. This looks good to me, but the CI disagrees. Can you look into it? We probably need some require lines...

mpg
mpg reviewed Jan 29, 2025
View reviewed changes
mpg
mpg reviewed Jan 29, 2025
View reviewed changes
@mpg
Copy link
Contributor

mpg commented Jan 29, 2025

Several tests are failing with:

ssl_tls12_server.c:1077: |1| 0x7fffb0dde280: bad client hello message: 5 != 4 + 284

(or obviously a variant with different sizes). We might need to adapt this check in the original PR - Cc @rojer

minosgalanakis
minosgalanakis reviewed Jan 29, 2025
View reviewed changes
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.

Looks ok, just have some questions about the conditions we are testing.

mpg
mpg reviewed Jan 30, 2025
View reviewed changes
@waleed-elmelegy-arm waleed-elmelegy-arm force-pushed the defrag-tls-handshake-tests branch 3 times, most recently from c8993e0 to 909e716 Compare January 30, 2025 18:51
mpg
mpg reviewed Jan 31, 2025
View reviewed changes
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.

Thanks for addressing our comments. Looks good to me on code inspection, but the CI doesn't seem fully happy yet, I'll have a look.

@minosgalanakis minosgalanakis force-pushed the defrag-tls-handshake-tests branch 2 times, most recently from a11d40b to d9bf91d Compare February 13, 2025 00:10
@rojer
Remove in_hshdr
dd14c0a 
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 <[email protected]>
@minosgalanakis minosgalanakis force-pushed the defrag-tls-handshake-tests branch from d9bf91d to 0e0c80a Compare February 13, 2025 10:57
@gilles-peskine-arm gilles-peskine-arm mentioned this pull request Feb 13, 2025
Open
6 tasks
@minosgalanakis minosgalanakis force-pushed the defrag-tls-handshake-tests branch 2 times, most recently from d37f592 to 253d6c9 Compare February 13, 2025 20:44
waleed-elmelegy-arm and others added 20 commits February 16, 2025 18:42
@waleed-elmelegy-arm @minosgalanakis
Add TLS Hanshake defragmentation tests
3538de1 
Tests uses openssl s_server with a mix of max_send_frag
and split_send_frag options.

Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Improve TLS handshake defragmentation tests
60b9fb6 
* Add tests for the server side.
* Remove restriction for TLS 1.2 so that we can test TLS 1.2 & 1.3.
* Use latest version of openSSL to make sure -max_send_frag &
  -split_send_frag flags are supported.

Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Fix typo in TLS Handshake defrafmentation tests
960e20f 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Remove unnecessary string check in handshake defragmentation tests
9e89ce9 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Require openssl to support TLS 1.3 in handshake defragmentation tests
85b23fa 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Add client authentication to handshake defragmentation tests
deb4484 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Remove unneeded mtu option from handshake fragmentation tests
12447e6 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Enforce client authentication in handshake fragmentation tests
603c821 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Add a comment to elaborate using split_send_frag in handshake defragm…
d58957f 
…entation tests

Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Add guard to handshake defragmentation tests for client certificate
744f9ee 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Test Handshake defragmentation only for TLS 1.3 only for small values
2a0667e 
Signed-off-by: Waleed Elmelegy <[email protected]>
@waleed-elmelegy-arm @minosgalanakis
Add missing client certificate check in handshake defragmentation tests
9cd8a5f 
Signed-off-by: Waleed Elmelegy <[email protected]>
@minosgalanakis
ssl-opt: Updated the keywords to look up during handshake fragmentati…
78eb291 
…on tests.

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
ssl-opt: Added requires_openssl_3_x to defragmentation tests.
95468a7 
Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
ssl-opt: Adjusted the wording on handshake fragmentation tests.
7ff885d 
Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
ssl-opt: Switched to requires_protocol_version for handshake framenta…
6a97c6f 
…tion tests.

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
ssl-opt: Added tls 1_2 to handshake defregmentation.
d330eea 
Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
ssl-opt: Removed failing TLS1.2 test cases
3783d05 
TODO: This is an intermediate commit for review purposes.
All of the removed cases will need to be validated.

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
ssl-opt: Added handhsake defragmentation tests for SSL_VARIABLE_BUFFE…
13b0e66 
…R_LENGTH

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis
ssl-opt: Added handhsake defragmentation tests for client initiated r…
e05c649 
…enegotiation.

Signed-off-by: Minos Galanakis <[email protected]>
@minosgalanakis minosgalanakis force-pushed the defrag-tls-handshake-tests branch from 253d6c9 to e05c649 Compare February 16, 2025 18:45
This was referenced Feb 17, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mpg mpg mpg requested changes

@gilles-peskine-arm gilles-peskine-arm gilles-peskine-arm requested changes

@minosgalanakis minosgalanakis Awaiting requested review from minosgalanakis

Requested changes must be addressed to merge this pull request.

Assignees

@minosgalanakis minosgalanakis

Labels
component-tls needs-preceding-pr Requires another PR to be merged first needs-review Every commit must be reviewed by at least two team members, size-s Estimated task size: small (~2d)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Basic ssl-opt testing for TLS HS defragmentation
5 participants
@waleed-elmelegy-arm @mpg @minosgalanakis @gilles-peskine-arm @rojer