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

Does not do any TLS hostname verification by default #472

Closed
frewsxcv opened this Issue Apr 22, 2015 · 25 comments

Comments

Projects
None yet
@frewsxcv
Copy link
Contributor

frewsxcv commented Apr 22, 2015

Currently, hyper does not provide any hostname verification when working with TLS. Ideally, Hyper would use a library similar to https://github.com/pyca/service_identity to prevent MITM attacks from happening.

@seanmonstar

This comment has been minimized.

Copy link
Member

seanmonstar commented Apr 22, 2015

The issue here is that hyper needs to pick something by default.

It's already possible to do it yourself with Client.set_ssl_verifier().

@frewsxcv frewsxcv changed the title TLS does not do any hostname verification Does not do any TLS hostname verification Apr 23, 2015

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Apr 23, 2015

For what it's worth, I just implemented RFC 6125-compliant hostname verification for Ruby's OpenSSL extension (a.k.a. CVE-2015-1855):

https://github.com/ruby/openssl/pull/12/files

I'd be happy to do a Rust implementation.

@frewsxcv

This comment has been minimized.

Copy link
Contributor Author

frewsxcv commented Apr 26, 2015

Worth noting this is also an issue in Servo: servo/servo#4954

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Apr 26, 2015

I just started an RFC6125 hostname verification library for Rust:

https://github.com/tarcieri/pkixnames

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Apr 26, 2015

One thing I'm curious about... what type is used for representing presented identifiers? I think I'd like the internal implementation of this library to be done completely as Vec<u8>s

@veeti

This comment has been minimized.

Copy link

veeti commented Apr 26, 2015

Hyper doesn't seem to do any TLS verification right now with the default client settings. Any untrusted certificate goes.

@seanmonstar

This comment has been minimized.

Copy link
Member

seanmonstar commented Apr 26, 2015

@tarcieri I don't know what presented identifiers means in this context.

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Apr 26, 2015

@seanmonstar the subjectAlternativeNames (SANs) or Common Names (CNs) in X.509 certificates

@seanmonstar

This comment has been minimized.

Copy link
Member

seanmonstar commented Apr 26, 2015

Ah, I have no idea. That would be in the openssl lib, no?

On Sun, Apr 26, 2015, 2:21 PM Tony Arcieri notifications@github.com wrote:

@seanmonstar https://github.com/seanmonstar the subjectAlternativeNames
(SANs) or Common Names (CNs) in certificates


Reply to this email directly or view it on GitHub
#472 (comment).

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Apr 26, 2015

It's beginning to seem like rust-openssl is probably where I should be asking about this 😉

@seanmonstar seanmonstar changed the title Does not do any TLS hostname verification Does not do any TLS hostname verification by default Aug 19, 2015

@samfoo samfoo referenced this issue Dec 2, 2015

Closed

Add HSTS pinning #8580

@frewsxcv

This comment has been minimized.

Copy link
Contributor Author

frewsxcv commented Jan 23, 2016

Since it hasn't been explicitly stated in here, here's a relevant project: https://github.com/briansmith/webpki

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented Jan 23, 2016

@frewsxcv +1 for that

@zmanian

This comment has been minimized.

Copy link

zmanian commented May 8, 2016

What is a good API for disabling TLS verification when the user desires it?

Environment variable? Argument to the connection object?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented May 8, 2016

Disabling security based on environment variables sounds dangerous.

@frewsxcv

This comment has been minimized.

Copy link
Contributor Author

frewsxcv commented May 8, 2016

What is a good API for disabling TLS verification when the user desires it?

Hopefully, this will not be possible.

seanmonstar added a commit that referenced this issue May 8, 2016

feat(ssl): enable hostname verification by default for OpenSSL
Additionally disables SSLv2 and SSLv3, as those are universally considered
unsafe.

Closes #472

@seanmonstar seanmonstar self-assigned this May 8, 2016

@MoSal

This comment has been minimized.

Copy link

MoSal commented May 8, 2016

@zmanian @frewsxcv
There are use-cases where skipping TLS verification is necessary.

In libcurl, the options CURLOPT_SSL_VERIFYPEER and CURLOPT_SSL_VERIFYHOST are available and you can set them to 0.

The curl tool has the option -k,--insecure which relies on the library options mentioned above.

@sfackler

This comment has been minimized.

Copy link
Contributor

sfackler commented May 9, 2016

An easy approach would be to make your own implementation of SslClient that avoids the check.

@AGWA

This comment has been minimized.

Copy link

AGWA commented May 9, 2016

Although it should go without saying that disabling certificate verification is a terrible thing to do, there will be developers who will want to disable it and will find a way to do so whether or not Hyper provides an API for it. Rather than leaving developers to their own devices, I think Hyper should provide an API to disable verification for two reasons:

  1. It will be easier to spot in code audits. Searching for the API call that disables verification is easier than spotting and analyzing a custom SslClient implementation. It would even be possible to search GitHub and file bug reports against projects that are disabling verification.
  2. Developers are likely to search the Web for the invalid certificate error message and find a blog post or Stack Overflow answer that contains an example SslClient to disable verification. This example might come without a sufficient disclaimer that it's doing something very bad. In contrast, if there's an "official" way to disable verification, it can come with a big warning and be named in such a way that may cause at least some developers to think twice before using it. For example, in Go's TLS library, the property to disable verification is named InsecureSkipVerify.
@jimmycuadra

This comment has been minimized.

Copy link

jimmycuadra commented May 9, 2016

I'm -1 on an option to disable verification. Rebuttal to AGWA's two points above: 1) An entire type that does nothing but circumvent verification has a bigger footprint than an environment variable. I don't think making it easy to grep for is a good thing to optimize for. 2) The kind of developer that would do what you're describing will still just copy/paste whatever they read on Stack Overflow, regardless of whether or not the the flag has "insecure" in the name.

bors-servo added a commit to servo/servo that referenced this issue May 10, 2016

Auto merge of #11115 - mbrubeck:openssl-verify, r=jdm
Use openssl-verify to check certificate + hostname

Fixes #4954.  r? @jdm

This is based on hyperium/hyper#472, though it doesn't re-use that code directly because Servo configures its own OpenSSL context.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/11115)
<!-- Reviewable:end -->
@mhristache

This comment has been minimized.

Copy link

mhristache commented May 29, 2016

I have a bunch of use cases where disabling certificate validation is needed, mainly related to use on machines not connected to internet (the case at my company):

  • self signed certificates are used on most of the machines
  • the CA files are not up to date (e.g. machines are using older OSs)

More than that, it should also be possible to enable old, insecure ciphers in a relatively easy way mainly to be able to talk to clients/servers not supporting TLS1.2 (everything using Java 1.7 or older).

Hyper should be flexible enough to support these use cases. Defaults can be to enable most secure options but give the user the possibility to configure hyper according to his/her needs.

I have opened #811 for this.

Edit: fix a typo

@sfackler

This comment has been minimized.

Copy link
Contributor

sfackler commented May 29, 2016

@maximih Hyper already allows all of the configuration you asked for by manually configuring the Openssl object: http://hyper.rs/hyper/v0.9.6/hyper/net/struct.Openssl.html

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented May 29, 2016

Note that running an X.509 PKI that broken only offers the illusion of security.

Re: TLS 1.2 and Java 1.7, it's supported out-of-the-box by the Sun JSSE provider.

@seanmonstar

This comment has been minimized.

Copy link
Member

seanmonstar commented May 29, 2016

mainly related to use on machines not connected to internet

Just noticed this, it seems like in this case it may be a waste to use TLS at all. If talking over a local network that you own and control, you can get performance improvements by not encrypting everything. Unless there are others on your local network that you want to protect yourself from, I guess?

Hyper should be flexible enough to support these use cases. Defaults can be to enable most secure options but give the user the possibility to configure hyper according to his/her needs.

This is exactly the current case. Default is to be secure. If one wants to alter away from the defaults, you can utilize the Client::with_connector(HttpsConnector::new(some_configured_openssl)). You can pick all the ciphers, protocol versions, and validation you want or don't want.

@MoSal

This comment has been minimized.

Copy link

MoSal commented May 29, 2016

Just noticed this, it seems like in this case it may be a waste to use TLS at all.

Not necessarily. There are other local use cases that benefit from skipping verification. For example:

  • Testing server performance with and without TLS.
  • (Easily) isolating potential server TLS issues.

Using curl/libcurl for those use-cases is trivial and just works.

@tarcieri

This comment has been minimized.

Copy link

tarcieri commented May 29, 2016

@MoSal I buy @AGWA's argument that having a standard notation that's easy to grep for is desirable.

That said, while I'm glad you think curl makes it super easy to shut off the security, I'm not sure that's actually a desirable property. I think the world would be a better place if there were no -k option to curl, and people needed to explicitly type --insecure, for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.