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

peer handshake violates TLS transparency #2413

Closed
codedot opened this issue Feb 27, 2018 · 22 comments
Closed

peer handshake violates TLS transparency #2413

codedot opened this issue Feb 27, 2018 · 22 comments
Labels
Tech Debt Non-urgent improvements

Comments

@codedot
Copy link

codedot commented Feb 27, 2018

Current design of the peer protocol handshake limits alternative implementations in languages other than C/C++ as described in this article. Indeed, src/ripple/overlay/README.md refers to the current implementation rather than describing the protocol:

* `Session-Signature`

    This field must be present. It contains a cryptographic token formed
    from the SHA512 hash of the shared data exchanged during SSL handshaking.
    For more details see the corresponding source code.

apparently referring to lines 31-93 in src/ripple/overlay/impl/TMHello.cpp.

OpenSSL routines SSL_get_finished and SSL_get_peer_finished are being used to access Finished messages sent over the socket which violates TLS socket transparency. These low-lever routines are only available in C/C++ for a reason.

Requesting protocol upgrade to RTXP/1.3 with Session-Signature (HTTP header) and nodeproof (the corresponding field of the Hello message in protobuf protocol) replaced by a more portable mechanism which would not be language-specific and could be implemented in other programming languages, for example using Node.js.

@JoelKatz
Copy link
Collaborator

Do you have any suggestions for what such a mechanism might be? The purpose of this code is to ensure that the two servers have ends of the same TLS connection to protect against attacks where someone proxies a connection between two servers and retains the ability to modify data.

Consider if server A wants to connect to server B and server B wants to connect to server A. Without some mechanism, two connections would have to be retained as whoever received the connection could not be confident that the inbound connection was in fact made by the intended peer.

@codedot
Copy link
Author

codedot commented Feb 27, 2018

Yes, absolutely, it was exactly these considerations that were presented in Why is it unlikely to have a complete and alternative implementation of Ripple? which I mentioned in my bug report. While the point of mutual authentication between peers is valid, the problem is that the OpenSSL routines SSL_get_finished and SSL_get_peer_finished are not available in most platforms.

The standard mechanism to ensure SSL/TLS connection is issued by trusted parties is to have keys. Node.js supports keys both for server side and client side. Why not use them to identify peers instead of reinventing the wheel by creating another pair of keys and messing with the TLS socket? At least, this would allow the rippled server to have a signed certificate, not just a key generated randomly or from a seed written in some plain text configuration file.

Still, even if having a key generated exactly by

require("crypto").createECDH("secp256k1").generateKeys("base64", "compressed")

is so important (although, I wonder why it would be?), at least consider using OpenSSL routines which are exposed in Node.js (thus likely to be exposed in other platforms).

P. S. I am sure it is not too much to hope that this time technical discussion will continue along technical issues without me receiving threats in personal messages.

@MarkusTeufelberger
Copy link
Collaborator

As far as I understand the issue, it is necessary (or at least desirable) to protect rippled <--> rippled TLS connections against active MITMs. They way this is currently done relies on signing messages that are hard to obtain with some TLS implementations (since in many cases the actual messages sent/received when establishing a connection are not that interesting to people who just want to create a TLS secured connection).

A different option might be to communicate node keys out of band (difficult, maybe centralized, does it really scale...?) or to introduce trusted third parties (signed certificates instead of just random raw keys).

Maybe a possible solution could be the second one, Let's Encrypt certificates are free and while it would be a hassle to buy a domain(!) just to run a server, it would be at least a second option to ensure MITM protection while enabling more diverse server implementations that don't have to be able to access protocol messages just for an initial handshake.

This might also improve clustering, currently one has to add nodes explicitly one by one in there, it would be nice to dynamically add any peer with a key signed by an internal CA for example.

@donovanhide
Copy link
Contributor

This issue is over three years old :-)

https://groups.google.com/forum/#!topic/golang-nuts/ULtJi9oR7dI

This is probably the correct Internet standard to have adopted:

https://tools.ietf.org/html/rfc5929

Although beware:

https://mitls.org/pages/attacks/3SHAKE#channelbindings

@MarkusTeufelberger
Copy link
Collaborator

Yeah, I was wondering how rubblelabs did this. ;-)

@donovanhide
Copy link
Contributor

I'm not doing any peer protocol stuff. Deleted it all when Vinnie started making undocumented changes to undocumented protocols.

Original poster is correct in saying that finished messages are esoteric and limited to OpenSSL. There was a whole interesting series of changes to Fabric along similar lines, using the more standard tls-unique:
https://jira.hyperledger.org/browse/FAB-1046?jql=text%20~%20%22tls-unique%22

@MarkusTeufelberger
Copy link
Collaborator

It would probably be easier to just rip this stuff out of a custom version of rippled and run a few "bridge" nodes between a reimplementation and the C++ version to keep the networks connected... but the protobuf peer protocol would still need to be reimplemented.

addaleax pushed a commit to nodejs/node that referenced this issue Mar 11, 2018

Verified

This commit was signed with the committer’s verified signature. The key has expired.
addaleax Anna Henningsen
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codedot
Copy link
Author

codedot commented Mar 12, 2018

Starting from v10.0.0-nightly2018031198a14e026b, Node.js exposes SSL_get_finished and SSL_get_peer_finished from OpenSSL as tlsSocket.getFinished() and tlsSocket.getPeerFinished() in the TLS/SSL module.

@donovanhide
Copy link
Contributor

openssl/openssl#5509 (comment)
Lol.

@JoelKatz
Copy link
Collaborator

JoelKatz commented Mar 12, 2018

Maybe I'm missing something, but how is using "tls-unique" any different from using the SSL_get_finished/SSL_get_peer_finished? And wouldn't using "tls-unique" also violate TLS transparency?

In any event, if this is an actual issue for anyone, they're welcome to submit a pull request. The simplest way to do it is to add support for some new form of MITM rejection while still supporting the existing scheme for a few more versions.

@donovanhide
Copy link
Contributor

donovanhide commented Mar 12, 2018

The difference is mostly around the logic of which side to use in which situation:

https://boringssl.googlesource.com/boringssl/+/af0e32cb84f0c9cc65b9233a3414d2562642b342/ssl/ssl_lib.c#2928

Also it has an Internet RFC that defines it.

I assumed "transparency" meant ease of implementation using a TLS library other than openssl. For instance in Go, you only ever get the side that you are meant to get:

https://golang.org/src/crypto/tls/conn.go#L1372

I'm no TLS expert, but I see that tls-unique, and by implication both finished messages, use is discouraged due to the Triple Handshake Attack:

https://www.ietf.org/mail-archive/web/tls/current/msg18263.html

I'd consider hiring an TLS expert such as @richsalz :-)

@donovanhide
Copy link
Contributor

As per the @richsalz recommendation in the other thread, this seems to be the current recommendation for application level authentication of other peers over TLS connections.

https://tools.ietf.org/html/rfc5705

Seems well supported in current and forthcoming TLS library releases.

https://www.openssl.org/docs/man1.0.2/ssl/SSL_export_keying_material.html
https://www.gnutls.org/manual/html_node/Deriving-keys-for-other-applications_002fprotocols.html
https://go-review.googlesource.com/c/go/+/85115
https://github.com/google/boringssl/blob/master/include/openssl/ssl.h#L1525-L1533

targos pushed a commit to nodejs/node that referenced this issue Mar 17, 2018

Verified

This commit was signed with the committer’s verified signature.
targos Michaël Zasso
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Mar 20, 2018

Unverified

The email in this signature doesn’t match the committer email.
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@codedot
Copy link
Author

codedot commented Mar 21, 2018

Node.js v9.9.0 has been released with Finished messages exposed. It should now be possible to develop a Node.js-based client for the Ripple peer network, for example starting from this skeleton.

@nbougalis
Copy link
Contributor

That's awesome, @codedot. I know you opened another issue for improved documentation; we're working on that. In the meantime, we can discuss details in that issue.

MayaLekova pushed a commit to MayaLekova/node that referenced this issue May 8, 2018
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: nodejs#19102
Fixes: nodejs#19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Aug 7, 2018

Unverified

The email in this signature doesn’t match the committer email.
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Aug 7, 2018

Unverified

The email in this signature doesn’t match the committer email.
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Aug 7, 2018

Unverified

The email in this signature doesn’t match the committer email.
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Aug 9, 2018

Unverified

The email in this signature doesn’t match the committer email.
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit to nodejs/node that referenced this issue Aug 16, 2018

Unverified

The email in this signature doesn’t match the committer email.
Exposes SSL_get_finished and SSL_get_peer_finished routines in OpenSSL
as tlsSocket.getFinished and tlsSocket.getPeerFinished, respectively.

PR-URL: #19102
Fixes: #19055
Refs: XRPLF/rippled#2413
Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@nbougalis
Copy link
Contributor

I'm supportive. I think we should improve the existing logic to take advantage of TLS features that make this possible.

Anyone want to submit a PR?

@mDuo13 mDuo13 added the Tech Debt Non-urgent improvements label Aug 2, 2019
@MarkusTeufelberger
Copy link
Collaborator

See also #3049

@MarkusTeufelberger
Copy link
Collaborator

Some ways to do this in other languages:

NodeJS: https://nodejs.org/api/tls.html#tls_tlssocket_getfinished and https://nodejs.org/api/tls.html#tls_tlssocket_getpeerfinished

Python: https://docs.python.org/3/library/ssl.html#ssl.SSLSocket.get_channel_binding (the tls-unique cb data should be either the "get_finished" or "get_peer_finished" message, see https://github.com/python/cpython/blob/40dad9545aad4ede89abbab1c1beef5303d9573e/Modules/_ssl.c#L2712-L2727). This is NOT enough for this use case unfortunately, so there would be a need to extend the Python standard library similar to NodeJS.
https://www.pyopenssl.org/en/stable/api/ssl.html#OpenSSL.SSL.Connection.get_finished and the get_peer_finished() function should work. Note that pyOpenSSL should only be used to make TLS connections - for everything else, there's cryptography or the stdlib!

Golang: https://golang.org/src/crypto/tls/conn.go (clientFinished/serverFinished fields in the Conn struct)

Rust: Didn't find anything immediately, but I'm sure it would be not too hard to get a patch into rustls if it is not possible to extract this message already.

About RFC5705: I didn't find much in Python for example, other than again using pyOpenSSL, so I'm not so sure how much one can win with this approach.
I'm not sure how/where the exported key material would be used though. Maybe I'm misunderstanding something, but pulling 100 bytes of EKM from both ends should result in the same data if there's no active MITM, but create a different result in case there is? In that case, it might really be much easier to sign + send EKM data instead of XORed and hashed finished messages.

@MarkusTeufelberger
Copy link
Collaborator

Libp2p has an interesting construct for doing this:

https://github.com/libp2p/specs/blob/master/tls/tls.md

They send a self-signed certificate on every connection that contains the host key + a signature over the TLS key signed with the host key. In addition to the normal cert checks when establishing a valid TLS connection, they also require a check of that signature.

These certs could be easily rotated or re-generated (in the extreme case one for each connection attempt) and these would also prevent MITM attacks (either the TLS key is not available to the MITM or the signature of the TLS key exposes him).

@movitto
Copy link
Contributor

movitto commented Aug 20, 2019

We did it in Ruby!

https://github.com/DevNullProd/XRBP/blob/master/lib/xrbp/overlay/handshake.rb

Complexity should be considered in the context of all of this, there are obviously many different alternative solutions which could be implemented but since this is a protocol handshake, simplicity should be highly valued.

The lack of exposure of the 'finished' and 'peer_finished' methods to any programming languages should not be seen as an excuse to not be able to implement the handshake. We submitted the patch adding this to the ruby openssl bindings upstream!

ruby/openssl#250

I actually like the current handshake, it's simple and works. But certainly if other solutions provide better tradeoffs, they should be explored.

@fanatid
Copy link
Contributor

fanatid commented Apr 13, 2020

If somebody interested in handshake on Rust, I made small dirty PoC: https://github.com/fanatid/rust-ripple-p2p

@nbougalis
Copy link
Contributor

I think that we've taken this issue as far as it can go, given the current situation. We may want to improve the handshake in the future to avoid the "hash the finished messages" construct, but it is acceptable for now.

@MarkusTeufelberger
Copy link
Collaborator

Did anything change about the TLS handshake in #3049? This is as much a problem as ever, https://github.com/ripple/rippled/blob/2b448683736398faa4e58d8f386c32356ad1d3c2/src/ripple/overlay/impl/Handshake.cpp#L46-L49 refers to this issue here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tech Debt Non-urgent improvements
Projects
None yet
8 participants