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

TLS Handshake record layer fragmentation not working #1840

Open
Irrialite opened this issue Jul 6, 2018 · 39 comments · May be fixed by #8981 or #9872
Open

TLS Handshake record layer fragmentation not working #1840

Irrialite opened this issue Jul 6, 2018 · 39 comments · May be fixed by #8981 or #9872
Labels
bug component-tls size-s Estimated task size: small (~2d)

Comments

@Irrialite
Copy link

I have a server setup that sends handshake with ssl->in_hslen set to above 22700-ish, which is above the accepted limit of 16384. Is there any plan to support this (or a workaround without changing the server) and if not, I would appreciate some tips as to how to implement this as it is quite urgent for my project.

This is the code segment that fails:
/* With TLS we don't handle fragmentation (for now) */ if( ssl->in_msglen < ssl->in_hslen ) { MBEDTLS_SSL_DEBUG_MSG( 1, ( "TLS handshake fragmentation not supported" ) ); return( MBEDTLS_ERR_SSL_FEATURE_UNAVAILABLE ); }

@RonEld
Copy link
Contributor

RonEld commented Jul 9, 2018

@Irrialite As you can see from this PR we are working an refactoring the messaging infrastructure. Message fragmentation being one of the issues addressed.
Unfortunately, as you can see, this is still work in progress, and we cannot commit on when it will be ready.

@RonEld
Copy link
Contributor

RonEld commented Jul 9, 2018

You are welcome to try this PR and ad your comments though.

@RonEld
Copy link
Contributor

RonEld commented Jul 9, 2018

Note that this PR does require integration with the rest of the library though.

@RonEld
Copy link
Contributor

RonEld commented Aug 28, 2018

@Irrialite I have just looked again at your query.
Please note that TLS specification allows only up to 16 KB for the record data, so even once we support message fragmentation, sending 22700-ish is not allowed.
The application should only send 16 KB of data. If the data is larger, then perhaps the application should send it in several chunks, and the remote peer application should be able to reconstruct the whole data buffer.

@RonEld
Copy link
Contributor

RonEld commented Aug 28, 2018

@Irrialite I would like to clarify, that my previous comment was about the Application data. I now realize you were asking about the Handshake message length. The standard doesn't limit the length of the handshake message( except that the length field is 24 bits, so the message cannot exceed 32MB).

In addition, the Fragmentation support which is being worked on, is about the DLTS fragmentation. Supporting TLS fragmentation is not at the moment planned, as usually it's not a problem.
Usually, the bottle neck of the handshake messages are the certificate messages. Is it possible that your server would send a shorter certificate chain? Have you considered using ECC based certificates?

@RonEld
Copy link
Contributor

RonEld commented Dec 4, 2018

We believe this issue has been addressed, therefore closing.
If you believe this issue remains unclear or unanswered, please reopen, with an update

@RonEld RonEld closed this as completed Dec 4, 2018
@RonEld
Copy link
Contributor

RonEld commented Dec 4, 2018

Reopening, for the sake of tracking the TLS handshake fragmentation feature

@RonEld RonEld reopened this Dec 4, 2018
@rojer
Copy link
Contributor

rojer commented Dec 24, 2018

@RonEld this is more serious when Max Fragment Length is used. if a device (using mbedTLS) asks for a small fragment, e.g. 1024 bytes, and the server complies, then handshake ends up fragmented:

handshake message: msglen = 1024, type = 11, hslen = 2675

which leads to the "fragmented handshake" failure.

@RonEld
Copy link
Contributor

RonEld commented Dec 24, 2018

@rojer Thank you for your example.
You example is exactly using the max fragment length. (MFL)
MFL is cucrrently supported for DTLS only.
I suggest you look at this article for more information.
In TLS, however, since the bottle neck is probably the certificate message, it is recommended you set your MBEDTLS_SSL_IN_CONTENT_LEN value to fit the certificate size. in your case, probably bigger than 2675.

@bl4ck5un
Copy link

@RonEld I'm running into the same problem while working on a client. So mbedtls_ssl_conf_max_frag_len is not supported for client either? I tried to set MBEDTLS_SSL_IN_CONTENT_LEN to 5120 without calling mbedtls_ssl_conf_max_frag_len. But the server still sends me messages longer than that.

@bl4ck5un
Copy link

Reading the code, I called mbedtls_ssl_conf_max_frag_len(&conf, MBEDTLS_SSL_MAX_FRAG_LEN_NONE) as well, thinking that's going to cause the client to negotiate with the server, which apparently didn't happen. Am I missing anything?

@oesh
Copy link

oesh commented May 27, 2020

We (Facebook networking team) have started working on adding handshake fragmentation support to mbedTLS. We are going to upstream the patch to the master repository once it is ready.

@rojer
Copy link
Contributor

rojer commented May 28, 2020

that would be great! looking forward to it. would be super nice to also have it backported to 2.16

@bl4ck5un
Copy link

bl4ck5un commented Jul 2, 2020

@oesh that will be wonderful!! I've been blocked by this for more than a year. How is your implementation going? Do you have a public dev branch that I might try?

@oesh
Copy link

oesh commented Aug 25, 2020

@bl4ck5un hey, I was busy with something else, but I am back to it. I do have a public dev branch, and it will be terrific to see early reports from the users. I will post an update here once I have something that passes tests, probably in the course of the following two weeks.

@oesh
Copy link

oesh commented Aug 26, 2020

that would be great! looking forward to it. would be super nice to also have it backported to 2.16

@rojer I am starting with 2.16.6, since this is what we need internally.

@oesh
Copy link

oesh commented Aug 26, 2020

Sharing some of the design considerations. Please comment as appropriate.

Memory management

MbedTLS expects the handshake message to be in ssl->in_buf. ssl->in_buf contains MBEDTLS_SSL_IN_BUFFER_LEN, which is sufficient for a single TLS record. The fragmented handshake message will not necessarily fit in MBEDTLS_SSL_BUF_LENGTH.

I see several design alternatives, from the simplest to the more complicated. All assume a new config constant, MBEDTLS_SSL_HS_MSG_MAX_LEN, which limits the size of a single handshake message.

Simplest

The simpler alternative is to increase the MBEDTLS_SSL_IN_BUFFER_LEN to be sufficient for the individual handshake message.

Advantages

  1. No changes to the TLS state machine. Easier to reason about.

Disadvantages

  1. The runtime memory footprint of a session will be permanently increased, by MBEDTLS_SSL_HS_MSG_MAX_LEN - MBEDTLS_SSL_MAX_CONTENT_LEN, which is likely to be 64K - 16K == 48K.

Agressive memory footprint optimization

Instead of passing ssl->in_msg to the TLS state machine, use a different pointer, e.g. ssl->in_hs_msg, which can point to either the ssl->in_msg, if the handshake message was not fragmented, or to a different buffer, e.g. ssl->handshake->hs_msg (similar to the DTLS handshake reassembly code).

Advantages

  1. The memory footprint will likely to be minimal.

Disadvantages

  1. This is by far the most complex approach, and it will be harder to debug and maintain.

KISS (for now)

Considering the above alternatives, I am opting for the simpler alternative for the time being. My reasoning is:

  1. The reassembly of the handshake messages can be disabled in the config parameters, hence the users who do not need the reassembly will suffer no runtime penalty.
  2. The simplest alternative is by far the easiest to get right, and will result in a smaller diff.
  3. It will be still possible to add memory optimizations later, in an incremental fashion.

Edit: adjusted the design alternatives to the 2.16 branch, which makes the "slightly optimized" alternative the simplest.

@Vitaliy69
Copy link

Any progress?

Call on my IoT STM32 device:

mbedtls_ssl_conf_max_frag_len(&conf, MBEDTLS_SSL_MAX_FRAG_LEN_512);

and got the error:

Performing the SSL/TLS handshake.../build/mbedtls-mygkMM/mbedtls-2.16.4/library/ssl_tls.c:3698: 0x7ffd936d4c80: TLS handshake fragmentation not supported
/build/mbedtls-mygkMM/mbedtls-2.16.4/library/ssl_tls.c:4369: 0x7ffd936d4c80: mbedtls_ssl_handle_message_type() returned -28800 (-0x7080)

@oesh
Copy link

oesh commented Nov 16, 2020

Update:

Support for fragmented TLS handshake messages is ready for a merge into the development branch. At the moment I am waiting for the final sign-off by the MbedTLS core devs.

In the meantime, the implementation is available in https://github.com/oesh/mbedtls/tree/hs_fragmentation__design_doc

If you want to try this out:

  1. Ensure that MBEDTLS_SSL_HS_DEFRAG_CLT is enabled in include/mbedtls/config.h
  2. If needed, adjust the maximal size of an individual handshake message via MBEDTLS_SSL_HS_DEFRAG_MAX_MSG_LENGTH. The default maximal size is 16384.
  3. Enable defragmentation in the runtime by invoking mbedtls_ssl_conf_hs_defrag_max_len( conf, len ) when setting up the TLS context. The len parameter is the maximal size of a handshake message, which should not exceed the MBEDLTS_SSL_HS_DEFRAG_MAX_MSG_LENGTH configuration parameter.

@oesh
Copy link

oesh commented Nov 16, 2020

that would be great! looking forward to it. would be super nice to also have it backported to 2.16

@rojer I am starting with 2.16.6, since this is what we need internally.

Update - I have asked the MbedTLS community whether there's any issue with backporting the feature to 2.16. The response was that since this is not a bug fix, it would be best to not risk the stability of the long-term-support versions:

https://lists.trustedfirmware.org/pipermail/mbed-tls/2020-October/000205.html

@rojer
Copy link
Contributor

rojer commented Dec 31, 2020

fwiw, Cesanta agreed to open source our mbedtls modifications, among them i have incoming handshake message defragmentation - mongoose-os/mbedtls@097ccf2

this is against 2.16, and to complete the work fragmentation of outgoing messages is required. i do plan to finish this work at some point, but no firm ETA.

@lance-proxy
Copy link

Hi all, any updates on this issue above? It seems like message fragmentation is becoming more and more common and going to be problem for IoT devices to not have support for this.

@rojer
Copy link
Contributor

rojer commented Mar 22, 2024

I've finished work mentioned in #1840 (comment) and we've had these changes in production for more than a year. As part of rebasing to 3.x i've now forward-ported the changes to 3.x and put up #8981. Feedback/test reports would be appreciated.

@zliucd
Copy link

zliucd commented Apr 2, 2024

This may often happen when TLS server sends certificates in different and sequential packets. We can use len field from TLS record header to identify missing bytes and parse them.

Though I have solved this issue in my personal project(https://github.com/zliucd/TLSfeat), but why fragmentation happens is still unknown.

@rojer
Copy link
Contributor

rojer commented Apr 2, 2024

why fragmentation happens is still unknown

wdym unknown? handshake fragmentation happens because it is being requested by the client.

@gilles-peskine-arm gilles-peskine-arm added size-s Estimated task size: small (~2d) size-m Estimated task size: medium (~1w) and removed size-s Estimated task size: small (~2d) labels May 16, 2024
@hauke
Copy link

hauke commented Dec 23, 2024

Meta is using handshake fragmentation now. The TLS handshakes with https://www.facebook.com/ and https://www.instagram.com with mbedtls 3.6.2 as client are failing for me with TLS handshake fragmentation not supported. Is there a workaround to prevent Meta and others from using handshake fragmentation?

@rojer
Copy link
Contributor

rojer commented Dec 23, 2024

@hauke they shouldn't be doing that if you don't ask for max fragment size. make sure you don't set max frag size.

@hauke
Copy link

hauke commented Dec 23, 2024

Meta is not violating rfc8446. A server is allowed to send fragmented handshake messages.

Handshake messages MAY be coalesced into a single TLSPlaintext record or fragmented across several records

I do not know why they are doing it.

@zliucd
Copy link

zliucd commented Dec 24, 2024 via email

@mpg
Copy link
Contributor

mpg commented Dec 24, 2024

I can reproduce the problem, as explained here: #9861 (comment) - increasing the debug level confirms that the client is not sending a max_fragment_length extension so this is not it. The server is choosing to fragment the message for reasons of its own, which AFAICS is perfectly allowed by the standard.

I'm afraid I can't see any workaround.

I think we'll need to implement support for this. I'll note there has been two PRs about that: #3817 and #8981 but none of them are in a good shape right now.

We'll need to pick one to revive and complete (or create a new one). This is unlikely to be done by team members in the coming days considering most of us are or will soon be on holiday.

Note: we usually have a policy of not backporting new features (which, technically, this would be) to LTS branches, but I'm strongly inclined to make an exception here and backport this to the 3.6 branch at least.

@mpg
Copy link
Contributor

mpg commented Dec 24, 2024

Things that might be worth trying out (I have no time at the moment):

  • Does the problem persist if we force the TLS version to 1.2? If yes, that's a work-around (though not satisfactory of course).
  • If not, does it go away with TLS handshake fragmentation support #8981 (which is only supposed to handle 1.2 for now)?

@rojer
Copy link
Contributor

rojer commented Dec 24, 2024

@mpg i can confirm TLS1.2 is a valid workaround:

  • programs/ssl/ssl_client2 server_name=facebook.com server_port=443 request_page=/ debug_level=4 auth_mode=none crt_file=none fails
  • programs/ssl/ssl_client2 server_name=facebook.com server_port=443 request_page=/ debug_level=4 auth_mode=none crt_file=none max_version=tls12 works

@rojer
Copy link
Contributor

rojer commented Dec 25, 2024

@mpg #9872 is the incoming messgae defragemntation part of #8981, now updated to work with TLS 1.3. it's still lacking automated tests but can be tested manually and notable works with facebook servers now. it also applies cleanly to 3.6, so backporting should be easy, if desired.

@Neustradamus
Copy link

To follow this important ticket

@mpg mpg added bug size-s Estimated task size: small (~2d) and removed enhancement size-m Estimated task size: medium (~1w) labels Jan 9, 2025
@mpg mpg moved this to 3.6.3 / 2.28.10 (final) release in Mbed TLS Epics Jan 9, 2025
@mpg mpg linked a pull request Jan 9, 2025 that will close this issue
5 tasks
@mpg mpg moved this from 3.6.3 / 2.28.10 (final) release to TLS 1.3 compatibility fix in Mbed TLS Epics Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug component-tls size-s Estimated task size: small (~2d)
Projects
Status: No status
Status: TLS 1.3 compatibility fix