search

sfackler / rust-openssl

OpenSSL bindings for Rust
1.38k stars 740 forks source link

Utf8Error in servername() #930

Closed Geal closed 6 years ago

Geal commented 6 years ago

Hello,

we encountered a panic on Utf8Error in sozu's SNI callback: https://github.com/sozu-proxy/sozu/issues/468

Here is the stacktrace:

#0  0x00007fc229562ef6 strlen (libc.so.6)
#1  0x00005641f94b3972 _ZN3std3sys4unix9backtrace8printing15resolve_symname17h87ab3cf14001972fE (sozu)
#2  0x00005641f949fcbf _ZN3std10sys_common9backtrace5print17hb44e22d18fe0726aE (sozu)
#3  0x00005641f94b78e9 _ZN3std9panicking12default_hook28_$u7b$$u7b$closure$u7d$$u7d$17h0e8381ee05fcbda7E (sozu)
#4  0x00005641f94b7419 _ZN3std9panicking12default_hook17hcdc4e6d5fb1cd52eE (sozu)
#5  0x00005641f90b6751 _ZN4sozu19register_panic_hook28_$u7b$$u7b$closure$u7d$$u7d$17h867de89b978e26d3E (sozu)
#6  0x00005641f94b7d56 _ZN3std9panicking20rust_panic_with_hook17hc72289ea25e833b7E (sozu)
#7  0x00005641f94b7b73 _ZN3std9panicking11begin_panic17h7ebefca6ed78d043E.llvm.568FF184 (sozu)
#8  0x00005641f94b7ae3 _ZN3std9panicking15begin_panic_fmt17h18438b23b942b607E (sozu)
#9  0x00005641f94b7a72 rust_begin_unwind (sozu)
#10 0x00005641f9505963 _ZN4core9panicking9panic_fmt17h4a59e77d46fd0e12E (sozu)
#11 0x00005641f93eb0e3 _ZN4core6result13unwrap_failed17h83e7ab6bf846021cE (sozu)
#12 0x00005641f93ea0e6 _ZN7openssl3ssl6SslRef10servername17hba168f2bfc1ed363E (sozu)
#13 0x00005641f9158d65 _ZN7openssl3ssl9callbacks7raw_sni17h5e503e2f78ac5e6dE (sozu)
#14 0x00007fc22a5301a9 ssl_parse_clienthello_tlsext (libssl.so.1.0.0)
#15 0x00007fc22a516516 ssl3_get_client_hello (libssl.so.1.0.0)
#16 0x00007fc22a51ae0f ssl3_accept (libssl.so.1.0.0)
#17 0x00007fc22a52a6c0 ssl23_accept (libssl.so.1.0.0)
#18 0x00005641f916897b _ZN7openssl3ssl3Ssl6accept17ha1555ccee0ad2c00E (sozu)
#19 0x00005641f91baede _ZN8sozu_lib7network8protocol7openssl12TlsHandshake8readable17hc61156c9c301f4e9E (sozu)
#20 0x00005641f919243c _ZN94_$LT$sozu_lib..network..https_openssl..TlsClient$u20$as$u20$sozu_lib..network..ProxyClient$GT$5ready17hbbf1af0a0f838de3E (sozu)
#21 0x00005641f9135152 _ZN8sozu_lib7network5proxy6Server5ready17hd083acbf83ec3da7E (sozu)
#22 0x00005641f912dc55 _ZN8sozu_lib7network5proxy6Server3run17h026ea3f7a21b30eaE (sozu)
#23 0x00005641f8fc5b34 _ZN4sozu6worker20begin_worker_process17he43247cf2974d896E (sozu)
#24 0x00005641f90b30a0 _ZN4sozu4main17hd14bc33c2d53b417E (sozu)
#25 0x00005641f8f28853 _ZN3std2rt10lang_start28_$u7b$$u7b$closure$u7d$$u7d$17ha13a03662e143d68E (sozu)
#26 0x00005641f94b79f3 _ZN3std9panicking3try7do_call17hf73418d1b42a2b0bE.llvm.568FF184 (sozu)
#27 0x00005641f94becfa __rust_maybe_catch_panic (sozu)
#28 0x00005641f94b3338 _ZN3std2rt19lang_start_internal17h9fffff712402d563E (sozu)
#29 0x00005641f90b6784 main (sozu)
#30 0x00007fc22950149b __libc_start_main (libc.so.6)
#31 0x00005641f8f1d08a _start (sozu)

I think it is caused by the following code:

https://github.com/sfackler/rust-openssl/blob/master/openssl/src/ssl/mod.rs#L2376

Are there cases where openssl could return a string that is not encoded in valid UTF8?

If neede, here is sozu's SNI callback code: https://github.com/sozu-proxy/sozu/blob/master/lib/src/network/https_openssl.rs#L678-L713

sfackler commented 6 years ago

Hmm, that unwrap seems like the wrong thing to do there, yeah. Whatever client is connecting to sozu is technically violating the SNI spec, but we shouldn't be panicking when they do so!

"HostName" contains the fully qualified DNS hostname of the server, as understood by the client. The hostname is represented as a byte string using ASCII encoding without a trailing dot. This allows the support of internationalized domain names through the use of A-labels defined in [RFC5890]. DNS hostnames are case-insensitive. The algorithm to compare hostnames is described in [RFC5890], Section 2.3.2.4.

Literal IPv4 and IPv6 addresses are not permitted in "HostName".

https://tools.ietf.org/html/rfc6066#section-3

We could decide that returning a &str here is wrong, add servername2 that returns a &[u8], deprecate servername and fix it all up in the next breaking release.

Alternatively, we could turn the panic into returning None since the value isn't actually a valid HostName, and add a servername_raw that returns a &[u8] and doesn't do the filtering.