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

fails to verify ssl cert hostname for cached certs #1783

Closed
joeyh opened this Issue May 3, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@joeyh

joeyh commented May 3, 2016

For some reason, rather than using the hostname validation built into ssl, (eg, SSLContext.check_hostname), electrum contains its own implementation of hostname validation, in check_host_name. I don't know if that implementation is secure, but I noticed that it's not always called when connecting to a server.

Specifically, check_host_name is only called when there's no cached cert file for a server.

If there's a cached cert file for a server, electrum sets ca_certs to point to cert_path, which contains all the cached certs. And it neglects to verify that the hostname of the server it connected to matches the cert used by that server.

So, this attack seems to be possible:

  1. Spin up a new electrum server and get it added into electrum's server list. This is a non-malicious, perfectly above-board server.
  2. Wait a while so plenty of electrums have cached the cert for your server.
  3. Now anytime you are able to MITM an electrum user, you can redirect all their electrum traffic to a malicious server you control, which uses the same cert as the server you set up in step 1. Since electrum trusts any cert it's seen before as valid for any hostname, it will trust your malicious server.

You need to toss all this manual and insecure ssl code and use ssl.create_default_context() which handles all these details securely.

@ecdsa

This comment has been minimized.

Show comment
Hide comment
@ecdsa

ecdsa Jun 3, 2016

Member

I fail to understand how this differs from #1782.
if a server uses self-signed certificate, then there's no point verifying the hostname.
you seem to expect Electrum to use only CA signed certificates, which is not the case.

Member

ecdsa commented Jun 3, 2016

I fail to understand how this differs from #1782.
if a server uses self-signed certificate, then there's no point verifying the hostname.
you seem to expect Electrum to use only CA signed certificates, which is not the case.

@joeyh

This comment has been minimized.

Show comment
Hide comment
@joeyh

joeyh Jun 3, 2016

I think I was wrong about this one.

        cert_path = os.path.join(self.config_path, 'certs', self.host)

            s = ssl.wrap_socket(s,
                                ssl_version=ssl.PROTOCOL_SSLv23,
                                cert_reqs=ssl.CERT_REQUIRED,
                                ca_certs= (temporary_path if is_new else cert_path),
                                do_handshake_on_connect=True)

So, cert_path points at the single cached cert for the host,
and only that one pinned cert will be accepted, not any others in
~/.electrum/certs/

see shy jo

joeyh commented Jun 3, 2016

I think I was wrong about this one.

        cert_path = os.path.join(self.config_path, 'certs', self.host)

            s = ssl.wrap_socket(s,
                                ssl_version=ssl.PROTOCOL_SSLv23,
                                cert_reqs=ssl.CERT_REQUIRED,
                                ca_certs= (temporary_path if is_new else cert_path),
                                do_handshake_on_connect=True)

So, cert_path points at the single cached cert for the host,
and only that one pinned cert will be accepted, not any others in
~/.electrum/certs/

see shy jo

@ecdsa ecdsa closed this Oct 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment