Verify peer's public key earlier:

[intelliot]

High Level Overview of Change

Fix an issue introduced in 1.10.0-b1.

No stable versions of rippled (including 1.9.4) are affected.

Context of Change

An issue was introduced in #4195 / 5a15229 (part of 1.10.0-b1) and is fixed here.

Type of Change

• Bug fix (non-breaking change which fixes an issue)

---

Acknowledgements:
Aaron Hook for responsibly disclosing this issue.

Bug Bounties and Responsible Disclosures:
We welcome reviews of the rippled code and urge researchers to
responsibly disclose any issues they may find.

To report a bug, please send a detailed report to:

    bugs@xrpl.org

[scottschurr]

👍 LGTM.

[nbougalis]

Clean and simple. 👍 from me.

[nbougalis]

Actually, veto. This change is actually insufficient: the shared secret which is signed is identical, so an an attacker can just duplicate the Session-Signature field.

Under normal circumstances, this makes no sense: you'd just cause the connection to drop. But with the "accidental misconfiguration detector" code introduced with commit 5a15229 there's now an incentive: by switching the direction of the attack, it would just cause servers that connect to the attacker to shut down.

I am going to stack a [FOLD] commit on top of this; feel free to squash it. It will disable the duplicate detector, eliminating this attack vector and I will enhance PR #4395 to properly handle this case.

[intelliot]

Created #4438 to avoid the distraction of the stale approvals above, and also to preserve history in this PR for easier auditability (e.g. you can easily confirm that #4438 is just this PR squashed, with no other edits).

Superseded by #4438.

[ximinez]

Actually, veto. This change is actually insufficient: the shared secret which is signed is identical, so an an attacker can just duplicate the Session-Signature field.

I think the original fix is sufficient. For an attacker to duplicate the Session-Signature field, and have the signature be valid, they'd have to duplicate the entire message. The Instance-Cookie field would have to be unchanged (else the signature wouldn't match) (initialized as peerInstanceID at https://github.com/XRPLF/rippled/blob/develop/src/ripple/overlay/impl/Handshake.cpp#L306-L320). That is checked before the node is allowed to shut down here https://github.com/XRPLF/rippled/blob/develop/src/ripple/overlay/impl/Handshake.cpp#L306-L320. Thus the attack would look like a self-connection, and would NOT shut down the node.

I haven't tried this experimentally.

[nbougalis]

Hey @ximinez! Hope all is well. Thanks for the comment.

Unfortunately, the original fix is NOT sufficient.

Some background

The underlying problem here is two-fold: (1) the Instance-Cookie isn't mixed into the value being signed; and (2) both endpoints sign the same value.

By way of background, for those who aren't familiar with the details:

• The Instance-Cookie is a 64-bit value that every server randomly generates at startup.
• The Session-Signature field is produced by a node signing a 256-bit value V using its node private key.
• The value V which is generated from the SSL session such that both endpoints produce the same value.
• Lastly rippled uses RFC 6979 deterministic signatures, by fixing the nonce needed to be deterministically derived from the message. That is, given a key K and a message M, the resulting signature S is "fixed".

Now for the good stuff...

Consider two peers, Alice, a normal, well-behaved honest peer, and Mallory, a malicious peer.

The original fix prevents Mallory from initiating a connection to Alice and causing her to shutdown because, per the protocol, Mallory needs to send an HTTP Upgrade request with a valid Session-Signature and, since Mallory does not possess Alice's key, he cannot pretend to know Alice's identity.

But that's NOT the only scenario. What if Alice attempts to make an outgoing connection to Mallory? Now, per the protocol, Alice sends an HTTP Upgrade request, which includes her Instance-Cookie and (importantly) a Session-Signature.

And now Mallory now has all the information needed to trick Alice into thinking her node identity is shared: the Session-Signature that Alice already sent. All he needs to do is produce a response with a different Instance-Cookie and the Session-Signature that Alice has already produced.

When Alice processes Mallory's response what she will see a valid signature and an Instance-Cookie that is different from her's. And Alice will proceed to shut down. Oops. 😢

The solution (which I will implement and submit as a fix for #4395) is to mix the Instance-Cookie value into the value being signed, along with an extra "direction" indicator and use this new hardened value for the new Session-EKM-Signature checks introduced in #4395.

Wait, isn't this scheme already broken?

"It sounds like the existing Session-Signature mechanism is broken already: Mallory can just spoof the Session-Signature as described above right now, regardless of this fix." Yes, he can blindly and immediately (as in, during the same session) play back a Session-Signature field to the server that sent it, but that's fine. If he does so, all he'll achieve is to cause the connection to drop since it's detected as a "self-connection". He's not hurting anyone other than himself.

[ximinez]

Hi @nbougalis! That all makes perfect sense, including the future fix. I had completely forgotten how the handshake works.

That said, instead of rolling the original fix back entirely, what if we just remove the call to stop the server, but keep logging that different message? That can at least alert operators who check their logs that they have a potential problem. Then once we have a stronger handshake protocol, we can start doing the automatic shutdown...