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 1.3 generates invalidly large TLS records #7918

Closed
jethrogb opened this issue Jul 12, 2023 · 7 comments
Closed

TLS 1.3 generates invalidly large TLS records #7918

jethrogb opened this issue Jul 12, 2023 · 7 comments

Comments

@jethrogb
Copy link

jethrogb commented Jul 12, 2023

Summary

RFC 8446 §5.4 specifies:

The presence of padding does not change the overall record size limitations: the full encoded TLSInnerPlaintext MUST NOT exceed 2^14 + 1 octets

However, MbedTLS will send records with a length exceeding this limit if you call mbedtls_ssl_write with a large enough buffer.

System information

Mbed TLS version (number or commit id): 3c22366
Operating system and version: Ubuntu 20.04
Configuration (if not default, please attach mbedtls_config.h): scripts/config.py set MBEDTLS_SSL_PROTO_TLS1_3

Expected behavior

MbedTLS should not generate TLS records that are too large.

Actual behavior

Here's a PCAP that shows the too long record (length = 16416, if you decrypt it you will see 15 padding bytes). If you break on ssl_build_inner_plaintext you can see it is called with *content_size == 16384 and pad == 15, but according to the spec, *content_size + pad should be at most 16384. Conforming peers will terminate the connection with a "record_overflow" alert, so this is pretty problematic.

Steps to reproduce

You can run the code from ssl_pthread_server.c.txt to see this behavior. This is maybe a little bit verbose, it was adapted from programs/ssl/ssl_client1.c and programs/ssl/ssl_server.c

@ronald-cron-arm
Copy link
Contributor

Thanks for your report. I can see in the code that we have a maximum size for data contained in a record of MBEDTLS_SSL_OUT_CONTENT_LEN which is equal to 16384 = 2^14 by default. I have to check in the TLS 1.3 spec what exactly should be less then 2^14, the length of the encrypted/non-encrypted data, this with some additional headers?

@ronald-cron-arm ronald-cron-arm self-assigned this Jul 13, 2023
@jethrogb
Copy link
Author

MBEDTLS_SSL_OUT_CONTENT_LEN limits the plaintext length for an application data record, but that's not what the RFC is talking about.

@jethrogb
Copy link
Author

Hold on, I may be misreading the spec.

@Taowyoo
Copy link
Contributor

Taowyoo commented Jul 13, 2023

I think there still some problem with size of record:

Just reproduced error with @ 3c22366:

scripts/config.py set MBEDTLS_SSL_PROTO_TLS1_3 && scripts/config.py set MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MOD
make no_test
./programs/ssl/ssl_server2 server_addr=localhost force_version=tls13 response_size=16384 debug_level=3

Then use any browser to access https://localhost:4433/ will result a SSL error
but curl -vk --tlsv1.3 https://localhost:4433/ will success
The browser error disappeared after set response_size to be < 16384 (even just 16383).

And TLS 1.2 : ./programs/ssl/ssl_server2 server_addr=localhost force_version=tls13 response_size=16384 debug_level=3 does not have this problem

@jethrogb
Copy link
Author

@ronald-cron-arm Apologies, earlier I was referring to the wrong RFC section. The problem is that MbedTLS generates too much padding. I've updated the issue description.

@ronald-cron-arm
Copy link
Contributor

@ronald-cron-arm Apologies, earlier I was referring to the wrong RFC section. The problem is that MbedTLS generates too much padding. I've updated the issue description.

No worries and thanks for these detailed investigations. Looking at the code you point to (ssl_build_inner_plaintext() and ssl_compute_padding_length()) I agree with your findings and it seems that only the size 16384 is problematic.

@waleed-elmelegy-arm
Copy link
Contributor

This is now fixed in development after the record size limit changes I tested with:
scripts/config.py set MBEDTLS_SSL_PROTO_TLS1_3 && scripts/config.py set MBEDTLS_SSL_TLS1_3_COMPATIBILITY_MOD make no_test
./programs/ssl/ssl_server2 server_addr=localhost force_version=tls13 response_size=16384 debug_level=3
and was able to access with Firefox and chrome successfully.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: [3.6] TLS 1.3 misc for LTS
Development

No branches or pull requests

4 participants