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

ECH: SIGSEGV when use HTTP/3 #13818

Closed
vvb2060 opened this issue May 28, 2024 · 21 comments
Closed

ECH: SIGSEGV when use HTTP/3 #13818

vvb2060 opened this issue May 28, 2024 · 21 comments
Labels
crash HTTP/3 h3 or quic related

Comments

@vvb2060
Copy link
Contributor

vvb2060 commented May 28, 2024

I did this

curl --http3 --ech true --doh-url https://1.0.0.1/dns-query https://www.cloudflare.com/cdn-cgi/trace

* thread #1, name = 'curl', stop reason = signal SIGSEGV: address not mapped to object (fault address: 0x0)
  * frame #0: 0x0000007fb6b69880 libc.so`strlen_default + 16
    frame #1: 0x00000055556e682c curl`fetch_addr + 60
    frame #2: 0x00000055556e67a0 curl`Curl_fetch_addr + 88
    frame #3: 0x000000555574deec curl`Curl_ossl_ctx_init + 1484
    frame #4: 0x0000005555753564 curl`cf_ngtcp2_connect + 932
    frame #5: 0x00000055556d59f0 curl`cf_he_connect + 1328
    frame #6: 0x00000055556d4dfc curl`cf_setup_connect + 104
    frame #7: 0x00000055556d8a10 curl`cf_hc_connect + 1056
    frame #8: 0x00000055556d3b28 curl`Curl_conn_connect + 96
    frame #9: 0x000000555570c3f8 curl`multi_runsingle + 1564
    frame #10: 0x000000555570b938 curl`curl_multi_perform + 312
    frame #11: 0x00000055556c1e1c curl`main + 7808
    frame #12: 0x0000007fb6b685b8 libc.so`__libc_init + 112

I expected the following

exit 0

curl/libcurl version

curl 8.8.0

operating system

android arm64

@dfandrich
Copy link
Contributor

dfandrich commented May 28, 2024 via email

@vvb2060
Copy link
Contributor Author

vvb2060 commented May 28, 2024

Android test apk: https://github.com/vvb2060/curl-android/releases/tag/v8.8.0

curl 8.8.0-DEV (unknown-linux-android) libcurl/8.8.0-DEV BoringSSL zlib/1.2.11 nghttp2/1.62.1 ngtcp2/1.5.0 nghttp3/1.3.0
Release-Date: [unreleased]
Protocols: dict file ftp ftps gopher gophers http https imap imaps ipfs ipns mqtt pop3 pop3s rtsp smb smbs smtp smtps telnet tftp ws wss
Features: alt-svc AsynchDNS ECH HSTS HTTP2 HTTP3 HTTPS-proxy IPv6 Largefile libz NTLM SSL threadsafe UnixSockets

I only have Android build environment

@dfandrich
Copy link
Contributor

dfandrich commented May 28, 2024 via email

@vvb2060
Copy link
Contributor Author

vvb2060 commented May 28, 2024

curl git repository not the same as source tarball?

@dfandrich
Copy link
Contributor

Almost but not quite. But the bigger issue is that I don't know how that binary you pointed me to was been built and if and what patches have been applied. But it seems that you're the one building it, so can you confirm that there aren't any patches?

@vvb2060
Copy link
Contributor Author

vvb2060 commented May 28, 2024

I don't understand why you are so wary of unofficial builds of binaries. I've given the call stack, but I'm not familiar enough with the code, so I chose to open an issue instead of a PR.

dns = Curl_fetch_addr(data, connssl->peer.hostname, connssl->peer.port);

connssl->peer.hostname is null

@dfandrich
Copy link
Contributor

dfandrich commented May 28, 2024 via email

@bagder bagder added crash HTTP/3 h3 or quic related labels May 28, 2024
@bagder
Copy link
Member

bagder commented May 28, 2024

Looking at the stacktrace, it is for HTTP/3. If you do this without ECH, does it happen as well?

@vvb2060
Copy link
Contributor Author

vvb2060 commented May 28, 2024

curl --http3-only --ech grease --doh-url https://1.0.0.1/dns-query https://www.cloudflare.com/cdn-cgi/trace
It works (www.cloudflare.com does not support ech)

@bagder
Copy link
Member

bagder commented May 28, 2024

/cc @sftcd

@sftcd
Copy link
Contributor

sftcd commented May 28, 2024

Will take a look. I don't have a version with http3 built so might take a wee bit.

@bagder
Copy link
Member

bagder commented May 28, 2024

Will take a look. I don't have a version with http3 built so might take a wee bit.

It probably also requires using BoringSSL so that both ECH and HTTP/3 can be enabled in the same build.

I'll see if I can get a build like that going myself.

@sftcd
Copy link
Contributor

sftcd commented May 28, 2024

Yep it says boring in the above. And I guess nghttp3? (I did an apt install of libnghttp3-dev configure not yet enabling h3 for me yet;-)

@bagder
Copy link
Member

bagder commented May 28, 2024

ngtcp2 and nghttp3, yes.

@sftcd
Copy link
Contributor

sftcd commented May 28, 2024

I guess the apt versions of ngtcp2/nghttp3 might be too old, with those I get:

vquic/curl_ngtcp2.c:34:10: fatal error: ngtcp2/ngtcp2_crypto_boringssl.h: No such file or directory
   34 | #include <ngtcp2/ngtcp2_crypto_boringssl.h>

Will go find source I guess. Might take a while;-)

@bagder
Copy link
Member

bagder commented May 28, 2024

The ngtcp2 package on debian uses gnutls. That's the only TLS library they ship that supports QUIC (with curl).

@bagder
Copy link
Member

bagder commented May 28, 2024

This is how we build from source normally: https://curl.se/docs/http3.html#ngtcp2-version ... but it needs some adjustments to use BoringSSL instead of quictls.

@sftcd
Copy link
Contributor

sftcd commented May 28, 2024

OK, I have a build and it crashes as above.

@sftcd
Copy link
Contributor

sftcd commented May 28, 2024

It looks like the issue is here - my code is using connssl-->peer.hostname which is NULL in this case, which ends up with a strlen(NULL) call. Replacing that with peer->hostname avoids the crash but may or may not be the right fix and maybe other similar changes will be needed elsewhere. I can check some more tomorrow and make a PR if it looks good.

@sftcd
Copy link
Contributor

sftcd commented May 29, 2024

Two other things:

  1. thanks @vvb2060 and @bagder - I think having these experimental features such that people can test 'em and find issues is great.
  2. Does anyone know of an h3 server that does support ECH? It looks like crypto.cloudflare.com supports ECH but not h3 whereas www.cloudflare.com supports h3 but not ECH. (I've not done any tests with h3+ECH before and don't know if anyone else has either.)

@sftcd
Copy link
Contributor

sftcd commented May 29, 2024

I just pushed a branch with what I think may be the fix for this. Diif is here. Will turn that into a PR once I've tested with the various TLS providers, but comments welcome in the meantime of course.

And for the avoidance of doubt: this doesn't mean ECH with work with h3, just that it won't crash if one tries that combo.

@sftcd sftcd mentioned this issue May 29, 2024
@bagder bagder closed this as completed in 48292d8 Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crash HTTP/3 h3 or quic related
Development

No branches or pull requests

4 participants