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

curl: upstream backports for mbedtls #24414

Merged
merged 1 commit into from
Jun 20, 2024
Merged

Conversation

Ra2-IFV
Copy link
Contributor

@Ra2-IFV Ra2-IFV commented Jun 18, 2024

Maintainer: @krant
Compile tested: aarch64 531b3f667c40405581a398a9648a0e6c27909a87
Run tested: aarch64 531b3f667c40405581a398a9648a0e6c27909a87, just simple connection test

Description:
tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam codes, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

Fixes #24365 #24386

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 18, 2024

root@OpenWrt:~# curl -vSL https://raw.githubusercontent.com/just_for_test
> GET /just_for_test HTTP/2
> Host: raw.githubusercontent.com
> User-Agent: curl/8.9.0-20240618
> Accept: */*
>
< HTTP/2 400
< content-security-policy: default-src 'none'; style-src 'unsafe-inline'; sandbox
< strict-transport-security: max-age=31536000
< x-content-type-options: nosniff
< x-frame-options: deny
< x-xss-protection: 1; mode=block
< content-type: text/plain; charset=utf-8
< x-github-request-id: C1D0:2A8DB3:283E3C:2EE34E:66720D77
< accept-ranges: bytes
< date: Tue, 18 Jun 2024 22:43:04 GMT
< via: 1.1 varnish
< x-served-by: cache-nrt-rjtf7700064-NRT
< x-cache: MISS
< x-cache-hits: 0
< x-timer: S1718750584.917578,VS0,VE303
< vary: Authorization,Accept-Encoding,Origin
< access-control-allow-origin: *
< cross-origin-resource-policy: cross-origin
< x-fastly-request-id: b66d70ce95c51f125c6f9f1ef7fa1521e273e96c
< expires: Tue, 18 Jun 2024 22:48:04 GMT
< content-length: 20
<
400: Invalid requestroot@OpenWrt:~#
root@OpenWrt:~# curl -V
curl 8.9.0-20240618 (aarch64-openwrt-linux-gnu) libcurl/8.9.0-20240618 mbedTLS/3.6.0 zlib/1.3.1 zstd/1.5.6 nghttp2/1.62.1
Release-Date: 2024-06-18
Protocols: file ftp ftps http https ipfs ipns mqtt
Features: alt-svc HSTS HTTP2 HTTPS-proxy IPv6 Largefile libz SSL threadsafe UnixSockets zstd
root@OpenWrt:~#

@krant
Copy link
Contributor

krant commented Jun 19, 2024

Thank you for this. My only concern is the possibility next curl could be 8.8.1 - in this case our version ordering will be broke.

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 19, 2024

The future version number was manually set in the snapshots, so it *should* be fine
Check it out http://curl.se/snapshots/

@krant
Copy link
Contributor

krant commented Jun 19, 2024

Makes sense. LGTM.

@1715173329
Copy link
Member

You should back port those fixes then, not bump to a unstable version.

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 19, 2024

I tried to patch it but build fails. At least unstable is better than totally broken...

@1715173329
Copy link
Member

diff --git a/net/curl/Makefile b/net/curl/Makefile
index d62712a2c3..4e2406d915 100644
--- a/net/curl/Makefile
+++ b/net/curl/Makefile
@@ -10,7 +10,7 @@ include $(INCLUDE_DIR)/nls.mk
 
 PKG_NAME:=curl
 PKG_VERSION:=8.8.0
-PKG_RELEASE:=1
+PKG_RELEASE:=2
 
 PKG_SOURCE:=$(PKG_NAME)-$(PKG_VERSION).tar.xz
 PKG_SOURCE_URL:=https://github.com/curl/curl/releases/download/curl-$(subst .,_,$(PKG_VERSION))/ \
diff --git a/net/curl/patches/010-mbedtls-check-version-for-cipher-id.patch b/net/curl/patches/010-mbedtls-check-version-for-cipher-id.patch
new file mode 100644
index 0000000000..4bdf7c550b
--- /dev/null
+++ b/net/curl/patches/010-mbedtls-check-version-for-cipher-id.patch
@@ -0,0 +1,48 @@
+From 0c4b4c1e93c8e869af230090f32346fdfd548f21 Mon Sep 17 00:00:00 2001
+From: Stefan Eissing <stefan@eissing.org>
+Date: Wed, 22 May 2024 14:44:56 +0200
+Subject: [PATCH] mbedtls: check version for cipher id
+
+mbedtls_ssl_get_ciphersuite_id_from_ssl() seems to have been added in
+mbedtls 3.2.0. Check for that version.
+
+Closes #13749
+---
+ lib/vtls/mbedtls.c | 19 ++++++++++++-------
+ 1 file changed, 12 insertions(+), 7 deletions(-)
+
+--- a/lib/vtls/mbedtls.c
++++ b/lib/vtls/mbedtls.c
+@@ -902,8 +902,6 @@ mbed_connect_step2(struct Curl_cfilter *
+     (struct mbed_ssl_backend_data *)connssl->backend;
+   struct ssl_primary_config *conn_config = Curl_ssl_cf_get_primary_config(cf);
+   const mbedtls_x509_crt *peercert;
+-  char cipher_str[64];
+-  uint16_t cipher_id;
+ #ifndef CURL_DISABLE_PROXY
+   const char * const pinnedpubkey = Curl_ssl_cf_is_proxy(cf)?
+     data->set.str[STRING_SSL_PINNEDPUBLICKEY_PROXY]:
+@@ -932,11 +930,18 @@ mbed_connect_step2(struct Curl_cfilter *
+     return CURLE_SSL_CONNECT_ERROR;
+   }
+ 
+-  cipher_id = (uint16_t)
+-              mbedtls_ssl_get_ciphersuite_id_from_ssl(&backend->ssl);
+-  mbed_cipher_suite_get_str(cipher_id, cipher_str, sizeof(cipher_str), true);
+-  infof(data, "mbedTLS: Handshake complete, cipher is %s", cipher_str);
+-
++#if MBEDTLS_VERSION_NUMBER >= 0x03020000
++  {
++    char cipher_str[64];
++    uint16_t cipher_id;
++    cipher_id = (uint16_t)
++                mbedtls_ssl_get_ciphersuite_id_from_ssl(&backend->ssl);
++    mbed_cipher_suite_get_str(cipher_id, cipher_str, sizeof(cipher_str), true);
++    infof(data, "mbedTLS: Handshake complete, cipher is %s", cipher_str);
++  }
++#else
++  infof(data, "mbedTLS: Handshake complete");
++#endif
+   ret = mbedtls_ssl_get_verify_result(&backend->ssl);
+ 
+   if(!conn_config->verifyhost)
diff --git a/net/curl/patches/020-mbedtls-v3.6.0-workarounds.patch b/net/curl/patches/020-mbedtls-v3.6.0-workarounds.patch
new file mode 100644
index 0000000000..15966fb5ad
--- /dev/null
+++ b/net/curl/patches/020-mbedtls-v3.6.0-workarounds.patch
@@ -0,0 +1,134 @@
+From 5f9017d4e28096b4589c4d5ee4f18cae086ba777 Mon Sep 17 00:00:00 2001
+From: Stefan Eissing <stefan@eissing.org>
+Date: Fri, 31 May 2024 13:01:17 +0200
+Subject: [PATCH] mbedtls: v3.6.0 workarounds
+
+- add special sauce to disable unwanted peer verification by mbedtls
+  when negotiating TLS v1.3
+- add special sauce for MBEDTLS_ERR_SSL_RECEIVED_NEW_SESSION_TICKET
+  return code on *writing* TLS data. We assume the data had not been
+  written and EAGAIN.
+- return correct Curl error code when peer verification failed.
+- disable test_08_05 with 50 HTTP/1.1 connections, as mbedtls reports a
+  memory allocation failed during handshake.
+- bump CI mbedtls version to 3.6.0
+
+Fixes #13653
+Closes #13838
+---
+ .github/workflows/linux.yml   |  2 +-
+ lib/vtls/mbedtls.c            | 52 +++++++++++++++++++++++++++++++----
+ tests/http/test_08_caddy.py   |  3 ++
+ tests/http/test_17_ssl_use.py |  5 ++--
+ 4 files changed, 52 insertions(+), 10 deletions(-)
+
+--- a/lib/vtls/mbedtls.c
++++ b/lib/vtls/mbedtls.c
+@@ -482,6 +482,20 @@ mbed_set_selected_ciphers(struct Curl_ea
+   return CURLE_OK;
+ }
+ 
++#ifdef TLS13_SUPPORT
++static int mbed_no_verify(void *udata, mbedtls_x509_crt *crt,
++                          int depth, uint32_t *flags)
++{
++  (void)udata;
++  (void)crt;
++  (void)depth;
++  /* we clear any faults the mbedtls' own verification found.
++   * See <https://github.com/Mbed-TLS/mbedtls/issues/9210> */
++  *flags = 0;
++  return 0;
++}
++#endif
++
+ static CURLcode
+ mbed_connect_step1(struct Curl_cfilter *cf, struct Curl_easy *data)
+ {
+@@ -737,6 +751,16 @@ mbed_connect_step1(struct Curl_cfilter *
+     failf(data, "mbedTLS: ssl_config failed");
+     return CURLE_SSL_CONNECT_ERROR;
+   }
++#ifdef TLS13_SUPPORT
++  if(!verifypeer) {
++    /* Default verify behaviour changed in mbedtls v3.6.0 with TLS v1.3.
++     * On 1.3 connections, the handshake fails by default without trust
++     * anchors. We override this questionable change by installing our
++     * own verify callback that clears all errors. */
++    mbedtls_ssl_conf_verify(&backend->config, mbed_no_verify, cf);
++  }
++#endif
++
+ 
+   mbedtls_ssl_init(&backend->ssl);
+ 
+@@ -922,10 +946,16 @@ mbed_connect_step2(struct Curl_cfilter *
+     connssl->connecting_state = ssl_connect_2_writing;
+     return CURLE_OK;
+   }
++  else if(ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED) {
++    failf(data, "peer certificate could not be verified");
++    return CURLE_PEER_FAILED_VERIFICATION;
++  }
+   else if(ret) {
+     char errorbuf[128];
++    CURL_TRC_CF(data, cf, "TLS version %04X",
++                mbedtls_ssl_get_version_number(&backend->ssl));
+     mbedtls_strerror(ret, errorbuf, sizeof(errorbuf));
+-    failf(data, "ssl_handshake returned - mbedTLS: (-0x%04X) %s",
++    failf(data, "ssl_handshake returned: (-0x%04X) %s",
+           -ret, errorbuf);
+     return CURLE_SSL_CONNECT_ERROR;
+   }
+@@ -1146,8 +1176,13 @@ static ssize_t mbed_send(struct Curl_cfi
+   ret = mbedtls_ssl_write(&backend->ssl, (unsigned char *)mem, len);
+ 
+   if(ret < 0) {
+-    *curlcode = (ret == MBEDTLS_ERR_SSL_WANT_WRITE) ?
+-      CURLE_AGAIN : CURLE_SEND_ERROR;
++    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;
+   }
+ 
+@@ -1203,16 +1238,21 @@ static ssize_t mbed_recv(struct Curl_cfi
+ 
+   ret = mbedtls_ssl_read(&backend->ssl, (unsigned char *)buf,
+                          buffersize);
+-
+   if(ret <= 0) {
++    CURL_TRC_CF(data, cf, "mbedtls_ssl_read(len=%zu) -> -0x%04X",
++                buffersize, -ret);
+     if(ret == MBEDTLS_ERR_SSL_PEER_CLOSE_NOTIFY)
+       return 0;
+-
+     *curlcode = ((ret == MBEDTLS_ERR_SSL_WANT_READ)
+ #ifdef TLS13_SUPPORT
+               || (ret == MBEDTLS_ERR_SSL_RECEIVED_NEW_SESSION_TICKET)
+ #endif
+     ) ? CURLE_AGAIN : CURLE_RECV_ERROR;
++    if(*curlcode != CURLE_AGAIN) {
++      char errorbuf[128];
++      mbedtls_strerror(ret, errorbuf, sizeof(errorbuf));
++      failf(data, "ssl_read returned: (-0x%04X) %s", -ret, errorbuf);
++    }
+     return -1;
+   }
+ 
+--- a/tests/http/test_08_caddy.py
++++ b/tests/http/test_08_caddy.py
+@@ -151,6 +151,9 @@ class TestCaddy:
+             pytest.skip("h3 not supported in curl")
+         if proto == 'h3' and env.curl_uses_lib('msh3'):
+             pytest.skip("msh3 itself crashes")
++        if proto == 'http/1.1' and env.curl_uses_lib('mbedtls'):
++            pytest.skip("mbedtls 3.6.0 fails on 50 connections with: "\
++                "ssl_handshake returned: (-0x7F00) SSL - Memory allocation failed")
+         count = 50
+         curl = CurlClient(env=env)
+         urln = f'https://{env.domain1}:{caddy.port}/data10.data?[0-{count-1}]'

I don't use mbedtls, you can test if this patch works.

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 19, 2024

looks great.
I did almost the same thing with git format-patch but curl fails with Segmentation fault(lmao). Perhaps I didn't clean the code before patching it.
so what's next, should I open a new pull request?

@1715173329
Copy link
Member

1715173329 commented Jun 19, 2024

No, simply update this PR.

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 19, 2024

Okay.

@Ra2-IFV Ra2-IFV changed the title curl: Temporarily update to 8.9.0-20240618 snapshot curl: upstream backports for mbedtls Jun 19, 2024
@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 19, 2024

Force pushed, updated title and description.

@1715173329
Copy link
Member

seems like only second patch is necessary

@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 19, 2024

tested and looks good.

@1715173329
Copy link
Member

tlsv1.3 support is broken in curl 8.8.0 with mbedtls 3.6.0.
See curl/curl#13653 and Mbed-TLS/mbedtls#9210 for more details.
A workaround was implemented in upsteam code, see curl/curl@0c4b4c1 and curl/curl@5f9017d
This commit includes patches generated from upstream commits.

fix openwrt#24365 openwrt#24386

Signed-off-by: Ryan Keane <the.ra2.ifv@gmail.com>
@Ra2-IFV
Copy link
Contributor Author

Ra2-IFV commented Jun 19, 2024

Refreshed. I don't know there is a target to refresh patches. I use make prepare QUILT=1 and refresh them manually 🥲

@1715173329 1715173329 merged commit 4e09831 into openwrt:master Jun 20, 2024
12 checks passed
@Ra2-IFV Ra2-IFV deleted the curl branch June 21, 2024 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

curl8.8.0: (35) ssl_handshake returned
3 participants