Extend ssl-opt testing for TLS HS defragmentation #10007

minosgalanakis · 2025-02-24T23:59:42Z

Description

Continue the work on #9928 adding more specially designed tests in ssl-opt.sh. Currently it is focused on buffer resizing and renegotiation for TLS 1.2. The scope may be expanded.

PR checklist

Please remove the segment/s on either side of the | symbol as appropriate, and add any relevant link/s to the end of the line.
If the provided content is part of the present PR remove the # symbol.

changelog not required because: Testing changes
development PR provided #here
TF-PSA-Crypto PR not required because: No changes to crypto
framework PR not required: No changes to framework
3.6 PR provided [Backport 3.6] Extend ssl-opt testing for TLS HS defragmentation #10065
2.28 PR not required because: Feature we are testing has not been backported to 2.28
tests provided

mpg

Just a first pass, making myself familiar with things. I haven't looked at the buffer resizing part, only renego.

It seems to me that we need to distinguish two things:

the party that fragments
the party that initiates renegotiation

So far it looks like we're testing both options for the party that fragments, but the party that initiates the renegotiation is always the client. I'm not sure if we need the full matrix, but we need to make a conscious decision about our test plan.

IMO the hardest case is when the client is both initiating and fragmenting (tested in this PR), and the ClientHello that initiates the renegotiation is fragmented (not tested so far).

Server-initiated renego is very simple: it's just the server sending a HelloRequest (empty, so can't be fragmented) followed by a handshake that's identical to client-initiated renego. Basically, only the client can actually start the renego, all the server can do is ask the client to start it. So, I'm not overly worried about not having tests for server-initiated renego with fragmentation, though as a matter of principle it might be good to have one.

tests/ssl-opt.sh

mpg · 2025-03-11T10:13:01Z

tests/ssl-opt.sh

-            "$P_SRV debug_level=4 force_version=tls12 auth_mode=required" \
-            "$O_NEXT_CLI -tls1_2 -split_send_frag 32 -cert $DATA_FILES_PATH/server5.crt -key $DATA_FILES_PATH/server5.key" \
+requires_config_enabled MBEDTLS_SSL_RENEGOTIATION
+run_test    "Handshake defragmentation with client-initiated renegotation: len=256" \


Typo: renegotIation (multiple occurrences).

tests/ssl-opt.sh

gilles-peskine-arm

Looks promising at dee46de, but we're missing what's perhaps the most important case: a server receives a fragmented ClientHello for renegotiation.

tests/ssl-opt.sh

gilles-peskine-arm · 2025-03-10T16:37:58Z

tests/ssl-opt.sh

+requires_protocol_version tls12
+requires_certificate_authentication
+requires_config_enabled MBEDTLS_SSL_MAX_FRAGMENT_LENGTH
+requires_config_enabled MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH


Why is buffer resizing relevant?

It was requested by @mpg andscoped in as part of the possible extended tests in the design discussions. This PR is exploring how it could be introduced, but I am happy to move forward either way.

Actually Gilles I think it was you who brought it up, back when the original PR was introducing a new pointer. IIRC the reasoning was that any new pointer comes with a risk of use-after-free, so what's the one thing that frees (reallocates) in_buf other than ssl_free() (which also sets all the pointers to NULL) - it's buffer resizing. Back then I think you said that based on reading the code it looked OK (pointer updated in the relevant place) but as a matter of principle we should have a test. After that I included it in my list of tests we wanted.

Now that we don't have an extra pointer any more, I agree it seems less important. However since Minos already wrote the tests cases, I'm inclined to keep them.

tests/ssl-opt.sh

minosgalanakis · 2025-03-11T17:26:11Z

The force push to 92ec60f is simply the rebase on updated feature branch.

minosgalanakis · 2025-03-12T09:15:38Z

@mpg , @gilles-peskine-arm All the comments have been addressed, and the CI is clear.

mpg

Can't complete my review today, posting the feedback I have so far.

Next commit to review: fd0c850 ssl-opt: Added coverage for client-initiated fragmented HS renegotiation tests.

tests/ssl-opt.sh

Signed-off-by: Minos Galanakis <[email protected]>

…ffer depth. Signed-off-by: Minos Galanakis <[email protected]>

…tiation delay Signed-off-by: Minos Galanakis <[email protected]>

gilles-peskine-arm

LGTM except for a typo that affects test case filtering.

tests/ssl-opt.sh

gilles-peskine-arm · 2025-03-17T15:12:39Z

tests/ssl-opt.sh

+            -c "client hello, adding renegotiation extension" \
+            -c "found renegotiation extension" \
+            -c "=> renegotiate"
+


There's no test case for renegotiation when Mbed TLS is the party that initiates the renegotiation. However, as noted by Manuel, server-initiated renegotiation just means a non-fragmentable (because empty) HelloRequest from the server, followed by the same exchanges as client-initiated renegotiation. So for our purposes here, testing both sides of the initiation doesn't really matter. What matters is testing both a fragmented renegotiation ClientHello (i.e. OpenSSL is the fragmenting client and Mbed TLS is the defragmenting server) and a fragmented renegotiation ServerHello (i.e. OpenSSL is the fragmenting server and Mbed TLS is the defragmenting client), and this is done.

To be clear, this comment states that there's an apparent testing gap, but it's ok because it's not really an actual testing gap, so no action is needed.

mpg

Thanks for addressing my feedback, looks pretty good to me now!

Only left two minor comments that probably wouldn't be worth another round on their own, but since we already need another round for the filter-impacting typo found by Gilles, you may consider addressing those as well.

tests/ssl-opt.sh

Signed-off-by: Minos Galanakis <[email protected]>

… initiated reneg Signed-off-by: Minos Galanakis <[email protected]>

mpg

Looks all good to me now, thanks!

gilles-peskine-arm

LGTM

gilles-peskine-arm · 2025-03-18T11:25:23Z

tests/ssl-opt.sh

+            -c "client hello, adding renegotiation extension" \
+            -c "found renegotiation extension" \
+            -c "=> renegotiate"
+


To be clear, this comment states that there's an apparent testing gap, but it's ok because it's not really an actual testing gap, so no action is needed.

minosgalanakis changed the base branch from development to features/tls-defragmentation/development February 25, 2025 00:00

minosgalanakis changed the title ~~Exntend ssl-opt testing for TLS HS defragmentation~~ Extend ssl-opt testing for TLS HS defragmentation Feb 25, 2025

minosgalanakis force-pushed the task9887_extend_defragmentation_tests branch 2 times, most recently from 20501e7 to d2ab8b5 Compare March 5, 2025 10:46

minosgalanakis force-pushed the task9887_extend_defragmentation_tests branch 5 times, most recently from d93a7b2 to 8158b39 Compare March 7, 2025 14:33

mpg requested changes Mar 11, 2025

View reviewed changes

gilles-peskine-arm requested changes Mar 11, 2025

View reviewed changes

minosgalanakis force-pushed the task9887_extend_defragmentation_tests branch from dee46de to 92ec60f Compare March 11, 2025 17:20

minosgalanakis force-pushed the task9887_extend_defragmentation_tests branch from 92ec60f to 54e275d Compare March 11, 2025 17:30

minosgalanakis added size-s Estimated task size: small (~2d) priority-high High priority - will be reviewed soon labels Mar 12, 2025

github-project-automation bot added this to Roadmap pull requests (new board) Mar 12, 2025

github-project-automation bot moved this to In Development in Roadmap pull requests (new board) Mar 12, 2025

minosgalanakis added the needs-review Every commit must be reviewed by at least two team members, label Mar 12, 2025

mpg reviewed Mar 12, 2025

View reviewed changes

tests/ssl-opt.sh Outdated Show resolved Hide resolved

tests/ssl-opt.sh Outdated Show resolved Hide resolved

mpg changed the base branch from features/tls-defragmentation/development to development March 13, 2025 08:38

mpg changed the base branch from development to features/tls-defragmentation/development March 13, 2025 08:39

mpg changed the base branch from features/tls-defragmentation/development to development March 13, 2025 10:52

minosgalanakis added 6 commits March 13, 2025 11:06

ssl-opt: Added fragmented HS tests for SSL_VARIABLE_BUFFER_LENGTH.

daa14a4

Signed-off-by: Minos Galanakis <[email protected]>

ssl-opt: Added fragmented HS tests for client-initiated renegotiation.

5aaa6e0

Signed-off-by: Minos Galanakis <[email protected]>

ssl-opt: Added fragmented HS tests for server-initiated renegotiation.

529188f

Signed-off-by: Minos Galanakis <[email protected]>

Added Mock Renegotiation negative test for testing.

05009c7

Signed-off-by: Minos Galanakis <[email protected]>

programs -> ssl_client2.c: Added option renego_delay to set record bu…

7134e52

…ffer depth. Signed-off-by: Minos Galanakis <[email protected]>

sll-opt: Added refence fix for the Mock HS Defrag test using renegoti…

87be69a

…tiation delay Signed-off-by: Minos Galanakis <[email protected]>

minosgalanakis force-pushed the task9887_extend_defragmentation_tests branch from 7949ab0 to f475a15 Compare March 14, 2025 00:15

minosgalanakis mentioned this pull request Mar 14, 2025

[Backport 3.6] Extend ssl-opt testing for TLS HS defragmentation #10065

Merged

6 tasks

minosgalanakis moved this from In Development to In Review in Roadmap pull requests (new board) Mar 14, 2025

minosgalanakis requested review from gilles-peskine-arm and mpg March 17, 2025 09:29

gilles-peskine-arm requested changes Mar 17, 2025

View reviewed changes

github-project-automation bot moved this from In Review to In Development in Roadmap pull requests (new board) Mar 17, 2025

gilles-peskine-arm added needs-work and removed needs-review Every commit must be reviewed by at least two team members, labels Mar 17, 2025

mpg previously approved these changes Mar 18, 2025

View reviewed changes

tests/ssl-opt.sh Outdated Show resolved Hide resolved

tests/ssl-opt.sh Outdated Show resolved Hide resolved

ssl-opt: Fixed a minor typo.

dfc082e

Signed-off-by: Minos Galanakis <[email protected]>

minosgalanakis dismissed mpg’s stale review via dfc082e March 18, 2025 10:25

ssl-opt: Added 4 and 128 bytes tests to HS defragmentation for server…

625c8fd

… initiated reneg Signed-off-by: Minos Galanakis <[email protected]>

mpg approved these changes Mar 18, 2025

View reviewed changes

mpg added needs-review Every commit must be reviewed by at least two team members, needs-ci Needs to pass CI tests and removed needs-work labels Mar 18, 2025

mpg requested a review from gilles-peskine-arm March 18, 2025 10:41

gilles-peskine-arm approved these changes Mar 18, 2025

View reviewed changes

github-project-automation bot moved this from In Development to Has Approval in Roadmap pull requests (new board) Mar 18, 2025

gilles-peskine-arm 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 18, 2025

gilles-peskine-arm enabled auto-merge March 18, 2025 11:29

gilles-peskine-arm added this pull request to the merge queue Mar 18, 2025

Merged via the queue into Mbed-TLS:development with commit 94b9972 Mar 18, 2025
6 checks passed

github-project-automation bot moved this from Has Approval to Done in Roadmap pull requests (new board) Mar 18, 2025

This was referenced Mar 25, 2025

Issue9887 extend add basic defragmentation tests #9990

Closed

Extended ssl-opt tests for TLS HS defragmentation #9987

Closed

minosgalanakis mentioned this pull request Apr 22, 2025

Properly support smaller fragment / record size #10099

Open

Extend ssl-opt testing for TLS HS defragmentation #10007

Extend ssl-opt testing for TLS HS defragmentation #10007

Uh oh!

Conversation

minosgalanakis commented Feb 24, 2025 • edited by mpg Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR checklist

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mpg Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

minosgalanakis Mar 11, 2025

Choose a reason for hiding this comment

Uh oh!

mpg Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

minosgalanakis commented Mar 11, 2025

Uh oh!

minosgalanakis commented Mar 12, 2025

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gilles-peskine-arm Mar 17, 2025

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mpg left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm left a comment

Choose a reason for hiding this comment

Uh oh!

gilles-peskine-arm Mar 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

minosgalanakis commented Feb 24, 2025 •

edited by mpg

Loading

gilles-peskine-arm left a comment •

edited

Loading