mbedtls
mbedtls copied to clipboard
Mbed-TLS
Making MBEDTLS_SSL_MAX_CONTENT_LEN dynamic
Currently MBEDTLS_SSL_MAX_CONTENT_LEN is set in the config at build-time.
However normal operation with bulk transfer mandates setting this to 16384.
In the case that you are on a small target and making multiple connections for different reasons, this can result in running out of memory needlessly. Any connections that will never perform large transfers and still would work correctly with MBEDTLS_SSL_MAX_CONTENT_LEN set to, eg, 2048, are basically burning 30KB of memory for no gain if 16384 was mandatory because even one connection wanted to do bulk transfer.
#define MBEDTLS_SSL_BUFFER_LEN ( MBEDTLS_SSL_MAX_CONTENT_LEN \
+ MBEDTLS_SSL_COMPRESSION_ADD \
+ 29 /* counter + header + IV */ \
+ MBEDTLS_SSL_MAC_ADD \
+ MBEDTLS_SSL_PADDING_ADD \
)
...
/*
* Setup an SSL context
*/
int mbedtls_ssl_setup( mbedtls_ssl_context *ssl,
const mbedtls_ssl_config *conf )
{
int ret;
const size_t len = MBEDTLS_SSL_BUFFER_LEN;
ssl->conf = conf;
/*
* Prepare base structures
*/
if( ( ssl-> in_buf = mbedtls_calloc( 1, len ) ) == NULL ||
( ssl->out_buf = mbedtls_calloc( 1, len ) ) == NULL )
{
...
What is the feeling about a patch that:
-
puts this value in a member of
mbedtls_ssl_context -
defaults to continuing to initialize that member to
MBEDTLS_SSL_MAX_CONTENT_LEN, so nothing breaks with the change -
convert
MBEDTLS_SSL_BUFFER_LEN, which currently does arithmetic onMBEDTLS_SSL_MAX_CONTENT_LEN, to be a constant added to the effective content length held in the member ofmbedtls_ssl_contextwhere it is currently used -
add a small api like
mbedtls_ssl_set_max_content_len(ssl, len)to allow overriding the default before callingmbedtls_ssl_setup()that actually does the allocation.
I'll be happy to provide such a patch and align it to any maintainer comments.
Hi @lws-team Thank yo for your suggestion. We will consider it. You are correct that the standard mandates up to 16 KiB, but in real use case, the bottle neck is the certificate being sent. A certificate chain usually does not reach 16 KB. In my view, setting this value dynamically is a risk that it will exceed the e constraints. Nonetheless, we will discuss it.
@RonEld I have a "real use case" which is bulk POST data sent from a browser for file upload, it breaks with an SSL error unless MBEDTLS_SSL_MAX_CONTENT_LEN is 16384, that is unrelated to any cert chain size. But the same application wants to make both client and server mode wss connections using ws protocols that can't send or receive more than a much lower packet size at long intervals. As it is I can't even implement the required number of connections with the available memory because there is only a build-time setting for ALL connections.
I looked closer into this yesterday... the buffer length of 16384 is the mandatory default for both sides per RFC6066. In an optional extension, per-connection, the client (only) may propose to negotiate it down to 512/1024/2048/4096 (only), and the server (only) may accept the proposal or reject it. The server may not make a counter-proposal. The existence of MBEDTLS_SSL_MAX_CONTENT_LEN itself already violates this, then.
For client connections, an api setting the buffer size can also imply the same 512/1024/2048/4096 restriction to the extension. Assuming the server allows it, that would be safe with both sides aware of the logical limit. RFC6066's extension provides no way for the server to do that for the client, but that's no worse than setting MBEDTLS_SSL_MAX_CONTENT_LEN globally today.
@RonEld Here is a second use-case where the excessive allocation at mbedtls made a big problem
https://community.letsencrypt.org/t/increasing-tls-sni-01-server-timeout/47208/5
Let's Encrypt fire 4 simultaneous SNI challenges at us, which complete after the TLS handshake, ie, there is no payload. Since elsewhere the app supports bulk file upload, mbedTLS today must be configured for 16384-size buffers in that application. The extra needless allocation during the parallel challenge causes the heap to underrun, meaning accepts have to be deferred to keep us from exhausting the memory, and resulting in our missing the timout on the server side. To solve it, Let's Encrypt graciously increased their timeout, but the root cause is the inability to control mbedTLS allocation per-connection.
@lws-team thank you for your enhancement on the use cases. We will take that into consideration
@lws-team As an open source project, we welcome contributions, as long as they follow our coding standards and the CLA has been signed, as described here
Thanks, "coding standards" worked but "here" points to a 404 for me. I don't mind a normal CLA for this.
I already prepared a 4-patch series, which follows the style of the code around it, but it needs to finish testing here. I'll check it against the coding standards and send a first try at a PR tomorrow. I took the approach to align the new api with the fragmentation extension possibilities, since for client where it's possible they need to cooperate; that creates some granularity (eg, between 2048 and 4096) but even at 4096 it is 1/4 of the burnt memory than (2 x 16384) per connection. We can discuss it and anything else that catches your eyes after the PR... I expect we have to go around a few times, no worries.
I'm sorry, the "here" should have pointed to https://github.com/ARMmbed/mbedtls#contributing
@lws-team I have one comment,for the record, on your note of RFC 6066. This RFC defines the usage of max_fragment_length which is covered by Mbed TLS in MBEDTLS_SSL_MAX_FRAGMENT_LENGTH definition. You can see the table in https://github.com/ARMmbed/mbedtls/blob/development/library/ssl_tls.c#L148. MBEDTLS_SSL_MAX_CONTENT_LEN defines the physical maximal limit of the transport buffers you can use, due to constrained environments. RFC5246 defines the maximal length to be 16KB, but not mandates to 16KB:
The record layer fragments information blocks into TLSPlaintext records carrying data in chunks of 2^14 bytes or less.
So, the existence of MBEDTLS_SSL_MAX_CONTENT_LEN doesn't violate this.
Nonetheless, we will consider your use cases
... the violation is in order to function in all cases as a server, MBEDTLS_SSL_MAX_CONTENT_LEN must be exactly 16384. That sets the size of both the RX and TX buffers for the connection. For TX, the implementation can easily take care to fragment at a modified MBEDTLS_SSL_MAX_CONTENT_LEN. But there is no way for the server to inform the client it is operating with an RX buffer <16384. If the peer sends something longer than the modified MBEDTLS_SSL_MAX_CONTENT_LEN in that case, in the assumption that's always open to it to do, the connection dies.
The server cannot control if the client will deploy the fragmentation extension for a connection. The server is not allowed to do anything other than accept the client's proposed fragmentation limit, or not acknowledge it and use exactly 16384, not MBEDTLS_SSL_MAX_CONTENT_LEN.
The fragmentation extension defines index 0 not as MBEDTLS_SSL_MAX_CONTENT_LEN, as mbedTLS has it, but as 16384. mbedtls lets me override MBEDTLS_SSL_MAX_CONTENT_LEN to be anything, and it has no way to inform the client of its consequent implementation limitation that deviates from the client's expectation. So this is a violation.
I'm not complaining about that, since for small targets every connection having 32KB+ of buffer no matter the nature of the traffic is intolerable. What I mean to point out is the shortcoming of not being able to negotiate server fragmentation size with the client is already enshrined in the workaround that is MBEDTLS_SSL_MAX_CONTENT_LEN. What these patches will do is let you leave MBEDTLS_SSL_MAX_CONTENT_LEN at 16384 and choose connection-by-connection to reduce it, according to what you know about the traffic on each connection. To the extent that is a violation of the server needing to have an RX buffer of 16384, this is no worse than what you have, and better to the extent you can deviate from it on precisely the connections you know will act within the customized limit.
@lws-team You are correct, and this is why we state:
/*
* Maxium fragment length in bytes,
* determines the size of each of the two internal I/O buffers.
*
* Note: the RFC defines the default size of SSL / TLS messages. If you
* change the value here, other clients / servers may not be able to
* communicate with you anymore. Only change this value if you control
* both sides of the connection and have it reduced at both sides, or
* if you're using the Max Fragment Length extension and you know all your
* peers are using it too!
*/
Anyway, I think this discussion is becoming a bit out of topic.
I was about to open a ticket requesting to separate sizes of input and output buffers. The reason for it is that (due to lack of Max Fragment Size extension support in the current deployment of the Internet), we indeed need to be prepared to accept 16KB records. However, we're free to send (much) smaller from our side. Thus, currently to communicate with anyone on the Internet, we need 32KB of buffer. We could easily reduce that to 20KB or less. The patch would be trivial.
Being able to dynamically specify buffer sizes per a mbedtls_ssl_context would be even better, but #1208 is a massive patch. I'm not surprised it's not merged still :-). Anyway, just a note (perhaps, to myself). I hope to read/review thru these tickets next few days. Thanks.
Being able to dynamically specify buffer sizes per a mbedtls_ssl_context would be even better, but #1208 is a massive patch.
Do you have a proposal to acheive the same "even better" result with less code?
I'm not surprised it's not merged still :-)
My surprise is there is no path to merge anything from non arm.com guys. Just a queue of rotting patches.
Also there's a big basic architectural blocker you guys need to address, which is follow OpenSSL and remove what are internal structs from the abi, which currently creates a hump any non-trivial patch cannot get over. Even without this "patch racism" Arm-only problem...
@lws-team : First of all, hello Andy, didn't recognize you at once, nice to meet you again!
Do you have a proposal to acheive the same "even better" result with less code?
I might have. But I'm coming here from issues like https://github.com/ARMmbed/mbedtls/issues/1356 , and optimizations to mbedTLS would be something like 4th level of recursion from top-level scope of my work. On the other hand, I consider it's important to look/leave "anchors" for possible future work, so happy that this discussion can be held.
you guys need to address
Just to clarify, I'm coming here from (unrelated) work done as a Linaro engineer, but nothing beyond that. However, reading thru this and #1208, I do wonder if there could be closer cooperation between Arm and Linaro on mbedTLS matters. So, my comments could be treated as initial seed effort, for me to raise something to decision-makers on Linaro side, to talk about with decision-makers on Arm side.
And of course, if "something" happens, that something will be just regular participation of Linaro engineers in public code reviews, hopefully building trust with Arm colleagues allowing them to merge patches faster (or reject with more confidence). And if nothing happens, I'll just disappear the same way as I popped out of nowhere.
@lws-team : With the above in mind, I'd like to share my opinion on the specific matter and discussion of this ticket. First of all, thanks for raising it, it's important matter, and I'm once again happy to be able to have found it, and see detailed discussion, which is again, I can only attribute that to your thoroughness, @lws-team .
That said:
The existence of MBEDTLS_SSL_MAX_CONTENT_LEN itself already violates this, then. "patch racism" illegal [from https://github.com/ARMmbed/mbedtls/pull/1208]
I much like that style and such terminology myself, but then I know that it - such a style, it doesn't have to be true. Existence of MBEDTLS_SSL_MAX_CONTENT_LEN itself is not a violation, but redefining it may be. So, if you would like to put "Not a single comma violated from RFC5246" on your product, just don't redefine it (the default value is fine). But I think it's fair to leave a freedom to other users to take responsibility of pragmatic optimizations where they need.
And I think it would be important outlook on the matter to find a suitable (even if maybe interim and partial) solution. As long there's no need to declare MBEDTLS_SSL_MAX_CONTENT_LEN "violation" and "illegal", there's no need to seek for "ultimately righteous solution" in the shape of 1500 changed lines (https://github.com/ARMmbed/mbedtls/pull/1208).
Sadly, there's no "ultimately righteous solution", as usual, there's a set of conflicting requirements and "choose M from N" situation. A pragmatic solution would optimize for number of changes though an reviewability. That's how I'd approach the task, and if I fail to find time for that, that's how I'd suggest to do it to other interested parties who may come later to this ticket. (Instead of, for example, starting to complain that another guy already tried to do that, his patches stuck, so nothing can be done at all).
Thanks (and sorry for expressing disagreement).
We could easily reduce that to 20KB or less. The patch would be trivial.
I went on and posted proof of concept of such a patch as https://github.com/ARMmbed/mbedtls/pull/1368.
Why this great fix was not added in 5 (FIVE!) years? It's super strange to use "define" there: I have mqtt connect and https operations. This connections may have different parameters. BearSSL uses dymanic buffer for MFLN and work great - less than 20Kb RAM for tls connection.
There is another possible bug. MbedTLS can't detect MFLN correctly. Tried BearSSL - and it can use 1Kb but MbedTLS can't do such. May be it's a bug in detection and working with it in mbedTLS ?
@igor-d-n The initial buffer size for the handshake is defined at compile-time, but for application data it will be dynamic if you enable MBEDTLS_SSL_VARIABLE_BUFFER_LENGTH. That covers most of the use cases for the requirement here. Is this enough for you, or do you need different handshake sizes for the different connections?
Regarding MFLN, it's supposed to be supported in Mbed TLS. If it isn't working correctly, please report a bug with clear instructions to reproduce.
That covers most of the use cases for the requirement here. Is this enough for you, or do you need different handshake sizes for the different connections?
it still requires upfront allocation of large contiguous chunks of memory that pushes up peak memory usage of each connection. i have a set of patches that make buffers fully dynamic but they're not quite submit-ready yet, i'd like to do more internal testing.
Regarding MFLN, it's supposed to be supported in Mbed TLS. If it isn't working correctly, please report a bug with clear instructions to reproduce.
it's currently broken in stream mode, as documented in https://github.com/Mbed-TLS/mbedtls/issues/1840 . i submitted patches there.
