libp2p / rust-libp2p

The Rust Implementation of the libp2p networking stack.
https://libp2p.io
MIT License
4.48k stars 928 forks source link

ring is a bad dependency; get rid of it #1396

Closed llebout closed 3 years ago

llebout commented 4 years ago

Hello, I'd like to suggest the removal of ring as a dependency all-together. That also includes rustls and webpki. (rustls is used by the websocket transport)

Why:

I suggest using the openssl crate instead. That supports both OpenSSL and LibreSSL and any other library that offers an openssl-compatible interface. Or individual crates that implement the primitives needed by libp2p such as: https://github.com/RustCrypto -- but I don't think they're that mature yet.

I started porting libp2p crates to openssl but it seems that ring types are exposed in public interfaces, so that would be a breaking change. Also, usage of ring really doesnt seem contained, so it's not so trivial to do so either.

burdges commented 4 years ago

We're a couple years away from any viable ring and rustls alternatives I think, assuming some qualified crypto devs begin work tomorrow.

Avoid openssl. It's poorly written and dangerous. I'd expect some yanked ring versions prevent vulnerabilities, but maybe nobody really knows.

We need TLS for transport layer encryption and authentication, which means rustls. SECIO is far worse and should be turned off asap. We need UDP for our parachain piece distribution eventually, maybe some analog of Bittorrent's uTP. We'll want QUIC for UDP protocols, meaning more TLS. I'd think all three Rust QUIC implementations use rustls, but the better one quinn definitely does.

RustCrypto provides only symmetric primitives, but not asymmetric primitives, and definitely not the painful and dangerous "agile" crypto demanzded by TLS. All interfaces in RustCrypto require const generics before stabilization. I'm unsure about side channel protections and performance in RustCrypto, well all that crypto gets written in assemble for good reasons. I hear Mozilla should produce RFCs this year for Rust and LLVM support for constant-time code, not sure about the performance impact.

We could/should eventually develop Noise, which avoids TLS' protocol agility mess, and nQUIC = QUIC+Noise, and ProtocolLabs wants to take that route, but doing so sounds like quite a long road.

llebout commented 4 years ago

Avoid openssl. It's poorly written and dangerous.

LibreSSL is here to fix that. And the openssl crate is compatible with it.

llebout commented 4 years ago

Also, this could look like an alternative: https://github.com/google/tink

llebout commented 4 years ago

Someone has started porting already, but gave up, apparently. https://github.com/denim2x/tink/tree/rust/rust

burdges commented 4 years ago

Tink's roadmap says nothing about TLS, handshakes, key exchange, etc., so afaik not relevant.

We'd ideally want everything except TLS 1.3 disabled, but LibreSSL only supports TLS 1.3 for clients, not yet servers: https://github.com/libressl-portable/portable/issues/228 I'd worry that LibreSSL might inherits far more bugs from OpenSSL than BoringSSL does, but maybe not.

Aside from rustls, your choices are NSS, miTLS, and BoringSSL, according to https://github.com/tlswg/tlswg-wiki/blob/master/IMPLEMENTATIONS.md GnuTLS sounds worse than OpssenSSL. And picotls uses OpenSSL. CycloneSSL only supports 32 bit architectures. Ain't clear if wolfSSL runs properly on powerful CPUs either.

In essence, ring provides an idiomatic safe rust interface into BoringSSL's crypto primitives, so anything more direct will be much worse. I'd expect Mozilla has okay but not idiomatic NSS bindings somewhere. I think miTLS sounds interesting, but maybe slow.

I'd think a ring fork sounds easiest in the short term, assuming the problem really warrants so much work. I'd think optional native rust code could support additional architectures, but one must avoid side channel attacks, downgrade attacks, etc., otherwise you only made our problems worse.

I suspect rustls shall remain the favored TLS library within the Rust ecosystem: https://jbp.io/2019/07/01/rustls-vs-openssl-performance.html I'd also expect that replacing ring in rustls means outdoing all that assembly the BoringSSL developers curated from OpenSSL, not so likely.

llebout commented 4 years ago

GnuTLS sounds worse than OpssenSSL

Why exactly?

Ain't clear if wolfSSL runs properly on powerful CPUs either.

wolfSSL also has an OpenSSL compatible API. Also: https://www.wolfssl.com/docs/benchmarks/#intel_sgx - https://www.wolfssl.com/performance-comparison-tls-1-3-wolfssl-openssl/ Performance looks good.

Also wolfSSL supports the NTRU quantum-resistant scheme, which is interesting for libp2p.

I'd think a ring fork sounds easiest in the short term, assuming the problem really warrants so much work. I'd think optional native rust code could support additional architectures, but one must avoid side channel attacks, downgrade attacks, etc., otherwise you only made our problems worse.

This is why I suggest OpenSSL/LibreSSL. TLS 1.3 support in LibreSSL will come soon enough.

I suspect rustls shall remain the favored TLS library within the Rust ecosystem: https://jbp.io/2019/07/01/rustls-vs-openssl-performance.html I'd also expect that replacing ring in rustls means outdoing all that assembly the BoringSSL developers curated from OpenSSL, not so likely.

Extreme performance is not much of a concern for everyone.


About QUIC:

quinn: "Pluggable cryptography, with a standard implementation backed by rustls and ring" -- which means it should be quite trivial to add another backend.

burdges commented 4 years ago

You're suggesting we move away from the TLS implementation favored by the Rust ecosystem and instead use C bindings far more pervasively and less safely. It's clear doing this would increase our long term maintenance costs, increase our vulnerability surface, and complicate others developing on libp2p and substrate.

It appears quinn has traits for cryptography in https://docs.rs/quinn/0.5.2/quinn/crypto/index.html but those traits appears slightly ad hoc and lack any impl, and all quinn's cryptographic types wrap rustls types. I think crypto traits should wait for const generics to stabilize, so quinn supporting anything besides rustls sounds unlikely anytime soon.

It appears ring paused the mass yanks after 0.14, although more yanking could follow the the next applicable CVE or BoringSSL churn from Google, but right now everything looks fine. If more mass yanks happen, someone could attempt a semver trick ring 0.17+ wrapper for ring 0.15, 0.16, etc., like the rand project uses, which maybe ring would publish, but regardless if wrappers work then forks or [patch.*] work. We'd want these wrappers for version miss matches anyways, so not sure if this fix even constitutes extra work.

It's possible the Rust Security WG might curtail the yanking https://github.com/rust-secure-code/wg/issues/16 somewhat by providing other avenues. All crates should behave slightly more like ring, in that CVEs should propagate downstream automatically, and the security WG exists to make this happen sanely. I'd think CVEs should eventually cause cargo warnings, even when maintainers keep the insecure version up.

p.s. I do not think NTRU support sounds relevant right now. All well maintained TLS implementations should adopt the results of the NIST post-quantum competition, but currently all we know is https://csrc.nist.gov/news/2019/pqc-standardization-process-2nd-round-candidates

llebout commented 4 years ago

Just to note:

It's not only about the mass yanks, as mentionned earlier. More so, portability.

And the yanks they made werent for security vulnerabilities, they were a way to create attention and virtual incentive towards purchasing the maintainer's support contracts.

burdges commented 4 years ago

I do agree ring should add pure rust backup implementations, but maybe side channel protections caused concerns. If so, there are the upcoming RFCs for better constant-time code generation in Rust and LLVM, which makes non-assembler implementations safer.

In the short term, one could actually do a ring-on-dalek fork that uses curve25519-dalek and ed25519-dalek for everything asymmetric, and RustCrypto for everything symmetric. It restricts the cipher suits considerably, and symmetric stuff gets slower, but doing this sounds far easier than using C bindings everywhere.

Afaik, we're already using pure rust crates like dalek, etc. for anything in substrate itself that likes strange platforms, like non-browser WASM or SGX. I've no idea if/why libp2p should run on those platforms, although it's obviously nice. It appears code for these platforms exists in some cases, so again forks work fine.

It's clearly claimed that some mass yanks stemmed from churn in boringssl, so those comments actually say he wont audit google's assembler changes, which sounds perfectly reasonable.

Demi-Marie commented 4 years ago

We need TLS for transport layer encryption and authentication, which means rustls. SECIO is far worse and should be turned off asap. We need UDP for our parachain piece distribution eventually, maybe some analog of Bittorrent's uTP. We'll want QUIC for UDP protocols, meaning more TLS. I'd think all three Rust QUIC implementations use rustls, but the better one quinn definitely does.

Actually, only quinn-proto (for which quinn is just a wrapper) uses rustls. Quiche uses BoringSSL and will soon be able to use OpenSSL, and Mozilla’s neqo uses NSS. Since quiche will soon be able to use OpenSSL, it will likely be able to use rustls via MesaLink. Quiche’s use of BoringSSL and neqo’s use of NSS are likely because Cloudflare and Mozilla use BoringSSL and NSS, respectively, for all other TLS traffic.

That said, quinn-proto is by far the best choice for libp2p. Quiche’s API is really annoying for libp2p, since everything is !Send and !Sync. Neqo simply isn’t ready yet. Quiche’s API is fantastic for high-throughput production servers, as manual packet dispatch and per-core state machines allow locks to be avoided and improve performance, but is quite annoying in the context of libp2p, where being Send and Sync is important.

This should be closed as “won’t fix”.

burdges commented 4 years ago

Very informative, thank you. If we ever need a fix I think the easiest would simply be a ring fork that merges some of the numerous PRs that Brian Smith rejects for not being constant-time enough or whatever. We're not the only ones with this issue, so others contribute to such a fork too. I think @tomaka can say if and when that should happen since he is the main one running libp2p in strange places.

newpavlov commented 4 years ago

I really hope that one day rustls will support an optional pure-Rust backend based on the RustCrypto crates. When I was talking with @ctz on IRC (I think it was more than a year ago), he said that he is open to it. I think we are really close to it in terms of implemented primitives, but not quite there yet. Also another blocker is making webpki independent from ring and unfortunately I am not sure if Brian will be open to that idea.

Demi-Marie commented 4 years ago

@newpavlov I would be more than willing to fork webpki if someone would agree to help maintain it. x509-signature might be a good basis for such a fork.

Demi-Marie commented 4 years ago

That said, a lot of cryptographic code is hand-written assembler for good reasons. The fastest ChaCha20 implementation I am aware of is a formally verified assembler version (generated via the verified Jasmine compiler, IIRC). Not using optimized implementations means a significant performance hit.

Demi-Marie commented 4 years ago

What is the status of miTLS? It is formally verified and is by far my preferred option if it is of production quality yet.