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

Hostname verification should be required by default when OpenSSL::SSL::VERIFY_PEER is configured #8

Closed
tarcieri opened this issue Mar 24, 2015 · 14 comments

Comments

@tarcieri
Copy link
Collaborator

Even if OpenSSL::SSL::VERIFY_PEER is configured, I/O is allowed with a remote server before the subject has been verified. VERIFY_PEER only checks the cert chain is rooted in the local truststore. It does not check if the subject is valid in and of itself.

My understanding is the ssl_socket.post_connection_check(hostname) method must be called to ensure the subject is correctly verified. However, communication is allowed to remote services without verifying the subject.

I would suggest throwing an exception if VERIFY_PEER is configured and I/O is attempted without first calling post_connection_check

It would also be nice if this all happened automatically simply by passing hostname into OpenSSL::SSL::SSLSocket (which AFAICT only affects SNI presently, and not subject verification)

@tarcieri
Copy link
Collaborator Author

One way to solve this without breaking backwards compatibility is to add a new type of verification mode, e.g. OpenSSL::SSL::VERIFY_HOST. Ideally I think if this mode were used, it would require the hostname be passed in when OpenSSL::SSL::SSLSocket is created, and then always call post_connection_check for you.

@zzak
Copy link
Member

zzak commented Jun 9, 2016

I believe this is fixed in ruby/ruby@61a3fff which made it's way here soon after.

Can you please verify (no pun intended)? /cc @rhenium

@rhenium
Copy link
Member

rhenium commented Jun 15, 2016

It only converted SSLSocket#initialize to C and doesn't fix this.

I'm afraid adding a new verify mode confuses people because the existing modes are directly mapped to OpenSSL's SSL_* constants.

How about adding SSLContext#verify_hostname=(true|false)? This injects OpenSSL::SSL.verify_certificate_identity into verify_callback so that the handshake fails.
https://github.com/ruby/openssl/compare/master...rhenium:topic/verify-hostname-on-connect2?w=1

@tarcieri
Copy link
Collaborator Author

tarcieri commented Jun 15, 2016

@rhenium yeah agreed re:

I'm afraid adding a new verify mode confuses people because the existing modes are directly mapped to OpenSSL's SSL_* constants.

What I'd actually suggest now in 20/20 hindsight of all of this stuff is having hostname verification automatically performed when OpenSSL::SSL::SSLSocket#hostname= has been set. Right now this just does SNI, but I think if it's been set the hostname should be automatically verified too.

You could additionally add SSLContext#verify_hostname=(true|false), and have it default to true.

@rhenium
Copy link
Member

rhenium commented Jun 16, 2016

While I agree this is beneficial in most cases, this is our extension so I rather think this is something that should be enabled by explicitly calling SSLContext#set_params.

@tarcieri
Copy link
Collaborator Author

tarcieri commented Jun 16, 2016

I think this is severe enough to warrant a breaking change. The Rust hyper project just did a similar breaking change (or as one commenter described it, "bugfix"):

hyperium/hyper#811

I was very surprised to discover that setting OpenSSL::SSL::SSLSocket#hostname= does not perform hostname verification by default. This resulted in CVE-2015-1828: HTTPS MitM vulnerability in http.rb, because we weren't explicitly calling the (at the time undocumented) #post_connection_check method.

I think security libraries should have good defaults, and hostname verification definitely seems like the sort of thing that should be on by default to me.

Users who are not calling #hostname= will not be impacted, and ones who are are probably more concerned with security than ones who aren't.

At the very least it should warn: no hostname verification performed!!!

@rhenium
Copy link
Member

rhenium commented Jun 17, 2016

It is a definitely natural and desirable behavior that HTTP libraries perform hostname verification by default, but openssl is basically a wrapper for OpenSSL.

I think openssl should follow the default of OpenSSL unless the user requests to use "Ruby's recommended default" by calling SSLContext#set_params. There are already have some exceptions, for example we have a default DH parameter to enable a server to use DHE cipher suites by default (with a warning), though.

/cc @nahi @zzak @hsbt @nurse

@nurse
Copy link
Member

nurse commented Jun 17, 2016

As rhenium says, ext/openssl is basically a wrapper of OpenSSL. In the such wrapper area, Ruby shouldn't extend OpenSSL, because OpenSSL sometimes introduces incompatible changes...

But ext/openssl has also some utility methods. They can be extended to be more useful and secure.

Anyway as usual Ruby's development process, it needs to list up use cases and the extension is useful and secure in some use cases and not harmful for other use cases.

The idea "if it sets hostname with hostname=, it should automatically call post_connection_check" sounds reasonable on the usual use case, but I don't think about edge cases.

@tarcieri
Copy link
Collaborator Author

As rhenium says, ext/openssl is basically a wrapper of OpenSSL. In the such wrapper area, Ruby shouldn't extend OpenSSL, because OpenSSL sometimes introduces incompatible changes...

Hostname verification is not a feature of OpenSSL itself, except in the unreleased 1.1.0 series. Yet it is a part of the Ruby OpenSSL extension (the current implementation is my code), so what you're saying isn't strictly true.

How hostname verification should interact with OpenSSL requires thinking about it outside of a purely "wrapper" context.

For me, at least, it was very unexpected that setting the hostname did not cause hostname verification to be performed by default.

@rhenium
Copy link
Member

rhenium commented Jun 30, 2016

Sorry for my silence.

I think you meant X509_VERIFY_PARAM_set1_host() (by the way it is already in 1.0.2), but it's unrelated to SNI. Combining SNI and the hostname verification is completely new.

I'm all for having such feature as an option, but just I don't think it should be the default. I can add another reason: the hostname verification is useful for HTTP, but it is not necessarily for other application-layer protocols.

Again I think SSLContext#set_params is the right place to enable it. This sets the system certificate store to cert_store. So some kind of server certificate verification is always necessary here, and it's clear that HTTP is one of the main targets of this method.

In my quick grep '.set_params' against gems on rubygems.org (using akr/gem-codesearch), I didn't find a gem that will break if we add it to SSLContext#set_params.

@tarcieri
Copy link
Collaborator Author

hostname verification is useful for HTTP, but it is not necessarily for other application-layer protocols

@rhenium hostname verification is an important part of the usage of SSL/TLS in virtually any protocol. I say this as someone who just added SSL/TLS support to redis-rb and also sent a patch to add hostname verification to ruby-net-ldap.

This paper covers several cases where MitM attacks were possible in non-browser software because hostname verification wasn't performed: https://www.cs.utexas.edu/~shmat/shmat_ccs12.pdf

@tarcieri tarcieri changed the title Subject is not ensured to be verified even when OpenSSL::SSL::VERIFY_PEER is configured Hostname verification should be required by default when OpenSSL::SSL::VERIFY_PEER is configured Jun 30, 2016
@rhenium
Copy link
Member

rhenium commented Jul 1, 2016

@tarcieri It's true when the cert_store is the system default one, but one may set up its own CA/self-signed certificate and the certificates do not necessarily contain the server's hostname. This shouln't be prevented as OpenSSL::SSL is still for general TLS and it's clear that the hostname verification is an application-layer thing.

rhenium added a commit to rhenium/ruby-openssl that referenced this issue Jul 9, 2016
If a client sets this to true and enables SNI with SSLSocket#hostname=,
the hostname verification on the server certificate is performed
automatically during the handshake using
OpenSSL::SSL.verify_certificate_identity().

Currently an user who wants to do the hostname verification needs to
call SSLSocket#post_connection_check explicitly after the TLS connection
is established.

This commit also enables the option in SSLContext::DEFAULT_PARAMS.
Applications using SSLContext#set_params may be affected by this.

[GH ruby#8]
rhenium added a commit to rhenium/ruby-openssl that referenced this issue Jul 9, 2016
If a client sets this to true and enables SNI with SSLSocket#hostname=,
the hostname verification on the server certificate is performed
automatically during the handshake using
OpenSSL::SSL.verify_certificate_identity().

Currently an user who wants to do the hostname verification needs to
call SSLSocket#post_connection_check explicitly after the TLS connection
is established.

This commit also enables the option in SSLContext::DEFAULT_PARAMS.
Applications using SSLContext#set_params may be affected by this.

[GH ruby#8]
rhenium added a commit to rhenium/ruby-openssl that referenced this issue Jul 10, 2016
If a client sets this to true and enables SNI with SSLSocket#hostname=,
the hostname verification on the server certificate is performed
automatically during the handshake using
OpenSSL::SSL.verify_certificate_identity().

Currently an user who wants to do the hostname verification needs to
call SSLSocket#post_connection_check explicitly after the TLS connection
is established.

This commit also enables the option in SSLContext::DEFAULT_PARAMS.
Applications using SSLContext#set_params may be affected by this.

[GH ruby#8]
rhenium added a commit to rhenium/ruby-openssl that referenced this issue Jul 23, 2016
If a client sets this to true and enables SNI with SSLSocket#hostname=,
the hostname verification on the server certificate is performed
automatically during the handshake using
OpenSSL::SSL.verify_certificate_identity().

Currently an user who wants to do the hostname verification needs to
call SSLSocket#post_connection_check explicitly after the TLS connection
is established.

This commit also enables the option in SSLContext::DEFAULT_PARAMS.
Applications using SSLContext#set_params may be affected by this.

[GH ruby#8]
rhenium added a commit to rhenium/ruby-openssl that referenced this issue Jul 24, 2016
Add new ossl_pkey_read_raw() function to ossl_pkey.c. This decodes
+data+ using password +pass+ from DER/PEM encoding, and return EVP_PKEY.
The next commit will make use of this - it adds PKCS ruby#8 PrivateKeyInfo
format support.
rhenium added a commit to rhenium/ruby-openssl that referenced this issue Jul 24, 2016
PKCS ruby#8 format is widely used outside OpenSSL world. This commit allows
passing "PrivateKeyInfo" as the format keyword argument of PKey#to_der
and #export. Also make OpenSSL::PKey.read handle DER encoded PKCS ruby#8
EncryptedPrivateKeyInfo format.
@tarcieri
Copy link
Collaborator Author

Fixed in #60

@zzak
Copy link
Member

zzak commented Jul 28, 2016

🎉

rhenium added a commit to rhenium/ruby-openssl that referenced this issue Mar 18, 2017
OpenSSL::PKey::PKey#private_to_der, #private_to_pem are added to the
generic PKey class that serialize the private key to PKCS ruby#8
{Encrypted,}PrivateKeyInfo format, in DER- and PEM- encoding
respectively. Also add #public_to_der and #public_to_pem for symmetry.
They serializes the public key into X.509 SubjectPublicKeyInfo format.

OpenSSL::PKey.read now reads DER-encoded PKCS ruby#8 keys. PEM-encoded keys
have been already handled by PEM_read_bio_PrivateKey().
rhenium added a commit to rhenium/ruby-openssl that referenced this issue Mar 23, 2017
OpenSSL::PKey::PKey#private_to_der, #private_to_pem are added to the
generic PKey class. They serialize the private key to PKCS ruby#8
{Encrypted,}PrivateKeyInfo format, in DER- and PEM- encoding,
respectively. For symmetry, also add #public_to_der and #public_to_pem
that serialize the public key into X.509 SubjectPublicKeyInfo format.

OpenSSL::PKey.read now reads DER-encoded PKCS ruby#8 keys as well as the
"raw" private keys. PEM-encoded PKCS ruby#8 keys have been already handled
by PEM_read_bio_PrivateKey().
bdewater pushed a commit to bdewater/openssl that referenced this issue Nov 24, 2019
OpenSSL::PKey::PKey#private_to_der, #private_to_pem are added to the
generic PKey class. They serialize the private key to PKCS ruby#8
{Encrypted,}PrivateKeyInfo format, in DER- and PEM- encoding,
respectively. For symmetry, also add #public_to_der and #public_to_pem
that serialize the public key into X.509 SubjectPublicKeyInfo format.

OpenSSL::PKey.read now reads DER-encoded PKCS ruby#8 keys as well as the
"raw" private keys. PEM-encoded PKCS ruby#8 keys have been already handled
by PEM_read_bio_PrivateKey().
bdewater pushed a commit to bdewater/openssl that referenced this issue Nov 24, 2019
OpenSSL::PKey::PKey#private_to_der, #private_to_pem are added to the
generic PKey class. They serialize the private key to PKCS ruby#8
{Encrypted,}PrivateKeyInfo format, in DER- and PEM- encoding,
respectively. For symmetry, also add #public_to_der and #public_to_pem
that serialize the public key into X.509 SubjectPublicKeyInfo format.

OpenSSL::PKey.read now reads DER-encoded PKCS ruby#8 keys as well as the
"raw" private keys. PEM-encoded PKCS ruby#8 keys have been already handled
by PEM_read_bio_PrivateKey().
ioquatix pushed a commit that referenced this issue Nov 25, 2019
OpenSSL::PKey::PKey#private_to_der, #private_to_pem are added to the
generic PKey class. They serialize the private key to PKCS #8
{Encrypted,}PrivateKeyInfo format, in DER- and PEM- encoding,
respectively. For symmetry, also add #public_to_der and #public_to_pem
that serialize the public key into X.509 SubjectPublicKeyInfo format.

OpenSSL::PKey.read now reads DER-encoded PKCS #8 keys as well as the
"raw" private keys. PEM-encoded PKCS #8 keys have been already handled
by PEM_read_bio_PrivateKey().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants