search

rust-lang / rustup

The Rust toolchain installer
https://rust-lang.github.io/rustup/
Apache License 2.0
6.15k stars 884 forks source link

Force strong TLS 1.2 cipher suites in download/src/lib.rs because servers offer weak cipher suites #2294

Closed x448 closed 3 months ago

x448 commented 4 years ago

Describe the problem you are trying to solve

rustup-init.sh was recently updated to force strong TLS 1.2-1.3 cipher suites for downloading files (if supported by local tools). However, rustup itself isn't doing the same.

Schannel in Windows 7, 8, and 8.1 doesn't support the two strong cipher suites offered by rust servers (as of April 23, 2020), so this request is limited in scope to curl-backend + OpenSSL.

Describe the solution you'd like

Use the same strong TLS 1.2-1.3 cipher suites as rustup-init.sh (if supported by OpenSSL) when rustup is using curl-backend + OpenSSL.

One way is to configure reqwest to use rustls instead of native-tls.

rustls only supports 9 cipher suites and they're the same 9 we want enabled.

/// A list of all the cipher suites supported by rustls.
pub static ALL_CIPHERSUITES: [&SupportedCipherSuite; 9] =
    [// TLS1.3 suites
     &TLS13_CHACHA20_POLY1305_SHA256,
     &TLS13_AES_256_GCM_SHA384,
     &TLS13_AES_128_GCM_SHA256,

     // TLS1.2 suites
     &TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,
     &TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,
     &TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,
     &TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,
     &TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,
     &TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256];

Author of reqwest says,

If you wish to try this out progressively, you can enable both default-tls and rustls-tls, and maybe check a config or env var to call either .use_default_tls() or .use_rustls_tls().

Thanks @kinnison and @seanmonstar for helpful feedback and suggestions.

Notes

kinnison commented 4 years ago

We are migrating away from the cURL binding toward reqwest as our only fetcher. We've defaulted to that for a while and eventually I'm going to drop cURL entirely. As such you probably need to investigate how we might be able to configure reqwest appropriately. This may need work on reqwest or hyper or some TLS bindings as well.

x448 commented 4 years ago

That seems reasonable.

However, reqwest is using Schannel on Windows. So Windows 7, 8.0 and 8.1 will connect to cargo.io and other rust-lang servers using only weak cipher suites because Schannel doesn't have required strong TLS 1.2 cipher suites until Windows 10. And Microsoft won't sunset Windows 8-8.1 until 2023.

Also, Active Directory policies can disable specific Schannel cipher suites without users knowing, which can create support issues for rustup and cargo.

I don't know if I'm the best person to ask @seanmonstar for security-related changes to reqwest. You have more gravitas and contacts in the rust community to make such a request.

Is there anyone assigned to be the official Rust security lead? In hindsight, Ken Thompson's hack showed the world in 1984 that compiler updates over the internet today might be a high value target.

seanmonstar commented 4 years ago

I can't comment much on schannel. As an additional option, reqwest can be configured to use rustls (and disable native-tls), which I'd recommend over OpenSSL.

x448 commented 4 years ago

Thanks, I updated the issue based on helpful feedback from both of you.

The only 9 cipher suites supported by rustls are the same 9 we want enabled. :+1:

kinnison commented 4 years ago

Okay, so two things - one we need to configure rustup to use rustls rather than native-tls in the reqwest dependency, and then second we need to get further into deprecating our use of cURL or even removing it?

@x448 Do you want to check if we're already using rustls and if not, cook up a PR to make it so?

kinnison commented 4 years ago

I'd note that we need to be certain that things like the SSL CA certificate bundles etc which tend to be platform specific and configured by openssl will need to be verified as still working under rustls.

x448 commented 4 years ago

@seanmonstar, would you need to add rustls-native-certs as an option in reqwest before we can use it in rustup?

@kinnison I'm not seeing rustls being used (currently) by rustup. I think reqwest uses native-tls by default.

There's no clear-cut winner between webpki-roots (bundled root certs) and rustls-native-certs (OS provided root certs) which are both written by @ctz (author of rustls).

Although I prefer webpki-roots due to bad experience with Windows cert stores, it's "underlying data is MPL-licensed, and src/lib.rs is therefore a derived work." I don't know if this license issue matters to rustup, just pointing it out in case.

rustls-native-certs avoids the MPL license issue and will use OS-provided certs on MacOS, Windows, Linux and possibly more. Its README has a nice comparison vs webpki-roots.

rustls-native-certs is fairly new (Nov 2019 first release) and has almost 40 .toml files on GitHub that use it. By comparison, webpki-roots is in ~350 .toml files.

Maybe we should consider our own webpki-rust-root crate because rustup isn't a browser and will only connect to rust-lang controlled domains hosted by Amazon CloudFront. It can embed just the required/anticipated root certs like "Amazon Root CA 1", etc.. This would bypass the license issue and reduce needlessly trusted root certs.

If some unlucky users have employers that want to decrypt HTTPS traffic to gmail.com, etc. we can use native certs to be compatible with MITM decryption appliances. A 2nd rustup binary or command line option would be a "red pill" for some rust programmers compared to silent fallback.

FWIW, Windows cert store can get messy even without snooping employers, e.g. Dell's eDellRoot happened after Lenovo's Superfish: https://www.kb.cert.org/vuls/id/925497/

kinnison commented 4 years ago

I wouldn't want to not be using the native certificate bundles. It's essential for permitting corporate situations where they MITM SSL etc.

x448 commented 4 years ago

We can have reqwest always use hyper-rustls which it already supports. And hyper-rustls uses rustls-native-certs by default since Nov 23, 2019.

This would give us the desired cipher suites without having to detect their support & specify them on various platforms and it would also use native certificate bundles.

I'm new to rust, so I'm not yet confident of my understanding of .toml files for rustup and reqwest.

seanmonstar commented 4 years ago

If you wish to try this out progressively, you can enable both default-tls and rustls-tls, and maybe check a config or env var to call either .use_default_tls() or .use_rustls_tls().

Shnatsel commented 4 years ago

I don't know how much fuzz testing is/was done for ring, rustls, and reqwest.

Merely feeding real-world data to ring has already revealed issues, so I do not expect it to hold up particularly well under fuzz testing. However, this is irrelevant to rustup unless ring has extensive use of unsafe code. The worst issues code detectable via fuzzing in safe code are panics, infinite loops or OOMs, and I don't think those are critical for rustup use cases.

https://github.com/trailofbits/siderophile/ may help you identify interesting targets for fuzzing that reach the largest amount of unsafe code, although I'm not sure how well it would work for an FFI-heavy crate such as ring.

kinnison commented 4 years ago

I believe ring has quite a bit of unsafe code. I don't mind using reqwest with the OpenSSL backend so long as we can configure its cipher suites appropriately. Is that perhaps an easier thing to do ?

djc commented 4 years ago

(The rustls/webpki/ring audit is starting next week, should take about 2 weeks.)

x448 commented 3 years ago

Third-party security audit of ring, webpki, and rustls:

djc commented 3 years ago

I was looking at the Cargo manifests for rustup and the download crate and found it pretty confusing that there seem to be like three download backends all enabled by default. Why is that?

kinnison commented 3 years ago

@djc There's two backends, one with two options for TLS provider, because we've still not managed to bottom the problem which means reqwest wasn't working on armel on snapcraft, and we're not certain yet about the TLS provider change, so while we have a default, we're providing options for testing.

djc commented 2 years ago

What's the current outlook on this?

rami3l commented 3 months ago

What's the current outlook on this?

@djc 2 years later, do you think we can conclude this in the context of #3790?

djc commented 3 months ago

IMO the migration to rustls by default (which has been merged to main, but not yet released) adequately addresses this issue.

rami3l commented 3 months ago

IMO the migration to rustls by default (which has been merged to main, but not yet released) adequately addresses this issue.

I'm closing this as resolved by https://github.com/rust-lang/rustup/pull/3798 then. Thanks a lot :)