secret_connection: fix bogus identity key handling #164
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `SecretConnection { remote_pubkey }` field is intended to store the
remote peer's static Ed25519 identity key.
However, during the AKE verification process, it was previously
temporarily initialized to the remote peer's ephemeral X25519 key.
This only worked because previously it's using Signatory's
`ed25519::PublicKey` type, which does not apply point decompression
(as that involves solving the curve equation, which requires a basic
curve arithmetic backend).
However, this did cause sporadic failures in the Secret Connection AKE
after attempting to switch this key type out for ed25519-dalek's
`PublicKey` type, which does decompress points (#162). This caused
sporadic failures whever the X25519 ephemeral key failed to decompress
as an Ed25519 public key (which is mere coincidence, as attempting to do
so makes no sense mathematically).
This commit changes `remote_pubkey` to an `Option` and doesn't attempt
to store any key until validated. This is perhaps needlessly fallible,
but at least addresses the immediate problem.
There doesn't appear to be any immediate security impact from this:
while in combination with other bugs it could potentially be used to
forge an identity key, the keys are cryptographically validated by
subsequent steps, and any failure to validate them aborts the handshake.
Upon success, the identity key is always overwritten with the correct
one. A security vulnerability would require some way to successfully
complete the handshake in spite of the cryptographic errors and without
the key being overwritten.
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
SecretConnection { remote_pubkey }field is intended to store the remote peer's static Ed25519 identity key.However, during the AKE verification process, it was previously temporarily initialized to the remote peer's ephemeral X25519 key. This only worked because previously it's using Signatory's
ed25519::PublicKeytype, which does not apply point decompression (as that involves solving the curve equation, which requires at least a basic curve arithmetic backend).However, this did cause sporadic failures in the Secret Connection AKE after attempting to switch this key type out for ed25519-dalek's
PublicKeytype, which does decompress points (#162). This caused sporadic failures whever the X25519 ephemeral key failed to decompress as an Ed25519 public key (which is mere coincidence, as attempting to doso makes no sense mathematically).
This commit changes
remote_pubkeyto anOptionand doesn't attempt to store any key until validated. This is perhaps needlessly fallible, but at least addresses the immediate problem.There doesn't appear to be any immediate security impact from this: while in combination with other bugs it could potentially be used to forge an identity key, the keys are cryptographically validated by subsequent steps, and any failure to validate them aborts the handshake.
Upon success, the identity key is always overwritten with the correct one. A security vulnerability would require some way to successfully complete the handshake in spite of the cryptographic errors and without
the key being overwritten.