Skip to content

Security: fix certificate pinning in HexStringCertTrustManager#448

Merged
generalbytes merged 1 commit intoGENERALBYTESCOM:masterfrom
johnzweng:bugfix/fix-hexstring-certmanager
Nov 5, 2020
Merged

Security: fix certificate pinning in HexStringCertTrustManager#448
generalbytes merged 1 commit intoGENERALBYTESCOM:masterfrom
johnzweng:bugfix/fix-hexstring-certmanager

Conversation

@johnzweng
Copy link
Contributor

@johnzweng johnzweng commented Nov 5, 2020

TL;DR:

Certificate pinning in HexStringCertTrustManager could easily be circumvented, this PR fixes this.

Details:

The method argument chain in X509TrustManager.checkClientTrusted(X509Certificate[] chain, String authType) receives a list of certificates which it received from a TLS server during the TLS handshake in the Certificate message directly after the ServerHello message.

This list of certificates contains at least the server's own certificate but may also contain an arbitrary number of addidtional certificates. (This normally might be intermediate certificates provided by the server to enable the TLS client to build a valid trust chain from the server's own certificate up to a known trusted root-certificate.)

The key points are:

  • certificates are public knowledge (anybody can retrieve a server's TLS certificate by just connecting to it)
  • a TLS server can include any certificate in its Certificate message
  • only the first certificate (the server's own, chain[0]) should be considered as trusted (i.e. trusting that the server possesses the matching private key) as otherwise the rest of the TLS session setup would fail anyway.

In the current implementation a man-in-the-middle could just setup a TLS server using ANY self-created TLS cert and just needs to add the certificate of the actual real server in its Certificate message (which it can easily obtain from the real server). --> Thus the current implementation does not provide certificate pinning.

See for reference:

TLS-handshake

@johnzweng
Copy link
Contributor Author

johnzweng commented Nov 5, 2020

As reference:
a similar bug was discovered in the past in the OkHttp library (CVE-2016-2402):

The researcher who found CVE-2016-2402 back then also wrote a nice overview on this kind of bug: https://koz.io/pinning-cve-2016-2402/

This article also includes this nice overview of a possible attack:

pinning-attack-seq-diagram

@generalbytes generalbytes merged commit 8b278e3 into GENERALBYTESCOM:master Nov 5, 2020
@generalbytes
Copy link
Contributor

Thank you. Where can we pay the bounty? :)

@generalbytes
Copy link
Contributor

We checked also remaining trust managers in our internal source code part and issue has been found only in HexStringCertTrustManager used by LndWallet.

@johnzweng
Copy link
Contributor Author

johnzweng commented Nov 6, 2020

Thanks, if you want to send some BTC you can send it to: 3MwbtpaBajqU7KM7VmBj52Se82hn3Nnv4m 🙂

Or if you prefer Lightning ⚡️, you can send me something via https://john.zweng.at/lightning

@generalbytes
Copy link
Contributor

Bounty paid in 3d2a78e0ec46c31c9a06beaeb9f7f23f7138ff4615ace1b5a5d2f041331c121e

@johnzweng
Copy link
Contributor Author

Thank you! 🙂

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants