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

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

Closed
x448 opened this issue Apr 23, 2020 · 20 comments

Comments

@x448
Copy link
Contributor

x448 commented Apr 23, 2020

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

@x448 x448 changed the title Force strong TLS 1.2 cipher suites in download/src/lib.rs for curl-backend+OpenSSL Force strong TLS 1.2 cipher suites in download/src/lib.rs for curl-backend+OpenSSL because servers offer weak cipher suites Apr 23, 2020
@kinnison
Copy link
Contributor

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
Copy link
Contributor Author

x448 commented Apr 27, 2020

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
Copy link
Contributor

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 x448 changed the title Force strong TLS 1.2 cipher suites in download/src/lib.rs for curl-backend+OpenSSL because servers offer weak cipher suites Force strong TLS 1.2 cipher suites in download/src/lib.rs because servers offer weak cipher suites Apr 27, 2020
@x448
Copy link
Contributor Author

x448 commented Apr 27, 2020

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. 👍

@kinnison
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor Author

x448 commented Apr 29, 2020

@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
Copy link
Contributor

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
Copy link
Contributor Author

x448 commented Apr 30, 2020

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
Copy link
Contributor

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
Copy link
Member

Shnatsel commented May 7, 2020

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
Copy link
Contributor

kinnison commented May 8, 2020

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
Copy link
Contributor

djc commented May 20, 2020

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

@x448
Copy link
Contributor Author

x448 commented Apr 8, 2021

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

@djc
Copy link
Contributor

djc commented Apr 12, 2021

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
Copy link
Contributor

@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
Copy link
Contributor

djc commented Jun 14, 2022

What's the current outlook on this?

@rami3l
Copy link
Member

rami3l commented Jul 10, 2024

What's the current outlook on this?

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

@djc
Copy link
Contributor

djc commented Jul 10, 2024

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

@rami3l
Copy link
Member

rami3l commented Jul 10, 2024

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 #3798 then. Thanks a lot :)

@rami3l rami3l closed this as completed Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants