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

ignores invalid ssl certs with CERT_NONE, allowing MITM #1782

Closed
joeyh opened this issue May 3, 2016 · 8 comments
Closed

ignores invalid ssl certs with CERT_NONE, allowing MITM #1782

joeyh opened this issue May 3, 2016 · 8 comments
Labels
enhancement ✨ security 🔐 technical issue that affects security of funds topic-network 🕸 related to logic in network.py (etc)

Comments

@joeyh
Copy link

joeyh commented May 3, 2016

Electrum falls back to using CERT_NONE when making an initial connection to a server, if the CERT_REQUIRED connection fails.

In get_socket, the abridged code is:

s = ssl.wrap_socket(s, ssl_version=ssl.PROTOCOL_SSLv23, cert_reqs=ssl.CERT_REQUIRED, ca_certs=ca_path, do_handshake_on_connect=True)

if s and self.check_host_name(s.getpeercert(), self.host):
    return s

s = ssl.wrap_socket(s, ssl_version=ssl.PROTOCOL_SSLv23, cert_reqs=ssl.CERT_NONE, ca_certs=None)

This seems to leave electrum open to MITM attacks.

Since electrum caches certs on first use, the MITM potential is probably limited to the initial uses of electrum, or when a new server is added to the list.

@ecdsa
Copy link
Member

ecdsa commented Jun 3, 2016

Electrum allows both self-signed and CA signed certificates.

Self-signed certificates are saved on first use, and used for later validation. It is indeed possible to MITM a server on the first with this model, but that is the best we can do without CA validation.

CA signed certificates are a newer feature in Electrum. I understand your comment as follows: it would be possible to MITM a CA signed server, by creating a self-signed certificate with the same name. That certificate would be saved and used later. I guess we can mitigate that by saving the names of already encountered CA signed servers.

@joeyh
Copy link
Author

joeyh commented Jun 3, 2016

ThomasV wrote:

CA signed certificates are a newer feature in Electrum. I understand your
comment as folows: it would be possible to MITM a CA signed server, by creating
a self-signed certificate with the same name. That certificate would be saved
and used later.

Correct. (But see my other bug about certificate handling too; it's
worse than this.)

The right way to handle self-signed certificates is probably to get the
user to verify the certificate, and then trust it thereafter. If
that's even worth doing given how easy it is to get a LetsEncrtypt cert.

see shy jo

@bauerj bauerj added enhancement ✨ security 🔐 technical issue that affects security of funds labels Jan 15, 2018
@bauerj
Copy link
Collaborator

bauerj commented Jan 15, 2018

Maybe we should make server operators switch to CA-signed certificates by a future Electrum version. Any thoughts on that?

@ecdsa
Copy link
Member

ecdsa commented Jan 15, 2018

@bauerj I believe this issue is being fixed by @ysangkok in the asyncio branch

@bauerj
Copy link
Collaborator

bauerj commented Jan 15, 2018

If that's the case, we should inform server operators to switch to CA-signed certs ASAP.

@ecdsa
Copy link
Member

ecdsa commented Jan 15, 2018

@bauerj that does not make sense. the issue discussed here is that it is possible to create a self-signed cert for the same name as a CA signed cert, and that certificate would be pinned. There is no need to switch to mandatory CA signed certificates to mitigate that. We just need to store the names of servers that have a CA signed cert, so that they do not get later replaced by a pinned certificate

@bauerj
Copy link
Collaborator

bauerj commented Jan 15, 2018

Yes, that's true. However, switching to mandatory CA-signed certificates would not only solve the issue you outlined but also the potential MITM attack when first connecting to a server.

@SomberNight
Copy link
Member

This has been fixed on master since merging the asyncio/aiorpcx changes.
The relevant code:

async def _get_ssl_context(self):
if self.protocol != 's':
# using plaintext TCP
return None
# see if we already have cert for this server; or get it for the first time
ca_sslc = ssl.create_default_context(ssl.Purpose.SERVER_AUTH)
if not self._is_saved_ssl_cert_available():
await self._try_saving_ssl_cert_for_first_time(ca_sslc)
# now we have a file saved in our certificate store
siz = os.stat(self.cert_path).st_size
if siz == 0:
# CA signed cert
sslc = ca_sslc
else:
# pinned self-signed cert
sslc = ssl.create_default_context(ssl.Purpose.SERVER_AUTH, cafile=self.cert_path)
sslc.check_hostname = 0
return sslc

Main points:

  • still accept both self-signed and CA signed certs
  • still trust on first use
  • when encountering a self-signed cert, if nothing is yet saved for that host, pin it
  • when encountering a CA-signed cert, if nothing is yet saved for that host, save an empty file
    • pins the fact that only CA-signed certs will be accepted for this host, forever
  • if there is a pinned self-signed cert saved for a host, and that cert has expired, delete it
    • possible MITM here but really what is the alternative? to kill that hostname forever.
  • if there is a pinned self-signed cert saved for a host, and that cert is still valid, only accept that

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement ✨ security 🔐 technical issue that affects security of funds topic-network 🕸 related to logic in network.py (etc)
Projects
None yet
Development

No branches or pull requests

4 participants