Incorrect mbedtls_ssl_write usage causes ranges of bytes to get lost

[LBPHacker]

I did this

Using mbedtls 3.6.2 = Mbed-TLS/mbedtls@107ea89daaef and nghttp2 1.50.0 = nghttp2/nghttp2@87fef4ab71be, HTTP/2 requests with large-ish (600kB+) request bodies often fail with HTTP/2 stream 1 was not closed cleanly: PROTOCOL_ERROR (err 1). Wireshark traces I have taken show that the data stream ends sooner than the content-length header would suggest, and the server gives up after some inactivity and fails the request. Matching up frames with known data shows that ranges of bytes inside the TLS stream occasionally get lost.

This has prompted me to look at how data is passed to mbedtls:

curl/lib/vtls/mbedtls.c

Lines 1169 to 1194 in 7eb8c04

	static ssize_t mbed_send(struct Curl_cfilter *cf, struct Curl_easy *data,
	const void *mem, size_t len,
	CURLcode *curlcode)
	{
	struct ssl_connect_data *connssl = cf->ctx;
	struct mbed_ssl_backend_data *backend =
	(struct mbed_ssl_backend_data *)connssl->backend;
	int ret = -1;
	(void)data;
	DEBUGASSERT(backend);
	ret = mbedtls_ssl_write(&backend->ssl, (unsigned char *)mem, len);
	if(ret < 0) {
	CURL_TRC_CF(data, cf, "mbedtls_ssl_write(len=%zu) -> -0x%04X",
	len, -ret);
	*curlcode = ((ret == MBEDTLS_ERR_SSL_WANT_WRITE)
	#ifdef TLS13_SUPPORT
	|| (ret == MBEDTLS_ERR_SSL_RECEIVED_NEW_SESSION_TICKET)
	#endif
	)? CURLE_AGAIN : CURLE_SEND_ERROR;
	ret = -1;
	}
	return ret;
	}

Given the documentation on mbedtls_ssl_write, which states that when MBEDTLS_ERR_SSL_WANT_WRITE is returned, the same arguments must be passed the next time, this seems incorrect: simply returning CURLE_AGAIN allows for more data to be given to this function than the amount given at the previous call.

I have made a change locally that makes sure that when MBEDTLS_ERR_SSL_WANT_WRITE is returned, the exact same arguments are passed the next time around, even if mbed_send has more data to send than the last time (and return an appropriate ret), and indeed, this fixes my issue.

This is not a pull request because the underlying issue seems to be one of design, and I am not willing to tackle fixing it: as far as I can tell, libcurl is designed with write(2)-like behaviour in mind, i.e. ranges of bytes that are reported sent have been sent (or the stream breaks later, etc.), and ranges reported to have been left unsent are not sent, but also not committed anywhere. The issue with mbedtls seems to be that it effectively commits ranges of bytes internally, which is not compatible with this design.

I expected the following

I expected the aforementioned requests to succeed. Excuse me for filling this field with such an abstract answer; the issue I am facing is also abstract.

curl/libcurl version

curl 8.10.1 = 7eb8c04

operating system

Windows 10.0.19045.3570

[LBPHacker]

All that said, if it can be guaranteed that any data ever presented to mbed_send is effectively committed, in the sense that it will not change between calls even in the case of partial writes (I have been unable to confirm this), a potential fix may be as simple as remembering how much data was passed to mbedtls_ssl_write in the previous call if it returned MBEDTLS_ERR_SSL_WANT_WRITE and trying to send only as much the next call.

[icing]

@LBPHacker nice find. I think we'll need to adapt the repeated write behaviour for that backend.

[LBPHacker]

Thanks for confirming. Does this mean that this repeated write behaviour is already present somewhere in the codebase?

[icing]

Thanks for confirming. Does this mean that this repeated write behaviour is already present somewhere in the codebase?

No, we have to adapt our mbedtls backend implementation.

[icing]

I propose a fix for this in #15846. I was able to reproduce the problem with a special, local setup, but could not make it into a reliable test case. Would be nice if you could verify that the PR works for you.

[LBPHacker]

Thank you, I will test this in a bit. However, I have concerns regarding the fix.

As far as I can tell, it is essentially what I proposed in my second comment, but I have as yet been unable to confirm that libcurl in any way prevents passing mbed_send data different from the last call. In fact, I am fairly sure that when using CURLOPT_CONNECT_ONLY, calling curl_easy_send from user code ultimately resolves to a call to mbed_send, which suggests that it is entirely up to user code to keep in mind and deal with this pickiness of mbedtls.

Further, calling mbedtls_ssl_write with 0 bytes of data is allowed and produces a TLS frame, and most likely can also yield MBEDTLS_ERR_SSL_WANT_WRITE (I have seen no indication of the contrary). As far as I can tell, curl_easy_send passes such empty ranges of bytes to mbed_send without question, so this is also doable from user code. However, the fix uses 0 as a special value to indicate "not blocked in mbedtls".

Are these concerns valid? Am I missing context here?

[icing]

Regarding the use of curl_easy_send() in this context, there are indeed questions. In general, having a EAGAIN-like API that partially commits data is always problematic.

I think we need to document in KNOWN BUGS, that a CURL_EAGAIN expects the subsequent curl_easy_send() to be called with at least the amount of bytes from before. And failure to do so will not work with backends like mbedTLS. There is no sane way around this.

As to 0-length writes, I think they should semantically be a NOP, meaning curl_easy_send() should be allowed to do an early return. For mbedTLS and the PR, I'll add a flag so that the filter can handle this.

[LBPHacker]

That is reasonable, thanks.

I have just confirmed that the proposed fix makes my very specific use case of large requests over h2 work.

[icing]

Just pushed the change to the PR. If your system is still hot, would be nice to get a verification. Thanks!

[LBPHacker]

Nope, that last change broke it. Most likely because this line:

curl/lib/vtls/mbedtls.c

Line 1223 in 6d416d6

if((*curlcode == CURLE_AGAIN) && !backend->send_blocked_len) {

still uses backend->send_blocked_len to decide whether the write is being blocked, and that variable never gets reset to 0.

[icing]

Arg, my bad. Just pushed a fix.

[LBPHacker]

Working as intended now 👍