Bump deps (signatory/stdtx/k256/tendermint/yubihsm) #162
Conversation
tarcieri
force-pushed
the
update-signatory-stdtx-k256-yubihsm
branch
from
1f5ed87 to
ad7abd0
Compare
|
Getting some Secret Connection-related failures in the test suite: Will investigate (or would appreciate any help) |
|
I've found the cause of the Secret Connection failures, and it appears to be a somewhat confusing (non-critical) bug in the original code which previously didn't cause a breakage due to lack of Ed25519 point decompression: These lines are attempting to parse an ephemeral X25519 public key as an Ed25519 one. This just simply appears to be the wrong key: the When the handshake completes, it's overwritten with the correct key here: This succeeded before because it was using Signatory's I'll open a branch that attempts to fix just this issue. Edit: opened #164 which appears to address the issue, and rebased this PR on it. |
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.
tarcieri
force-pushed
the
update-signatory-stdtx-k256-yubihsm
branch
from
ad7abd0 to
4f692dd
Compare
tony-iqlusion
changed the title
tarcieri
force-pushed
the
update-signatory-stdtx-k256-yubihsm
branch
2 times, most recently
from
b486ffd to
1cfc62e
Compare
Codecov Report
@@ Coverage Diff @@
## develop #162 +/- ##
===========================================
+ Coverage 22.64% 22.97% +0.32%
===========================================
Files 60 61 +1
Lines 2446 2429 -17
===========================================
+ Hits 554 558 +4
+ Misses 1892 1871 -21
Continue to review full report at Codecov.
|
tarcieri
force-pushed
the
update-signatory-stdtx-k256-yubihsm
branch
2 times, most recently
from
919cdf1 to
4fe5186
Compare
Upgrades the following dependencies to the latest versions: - `signatory` v0.22.0 - `stdtx` v0.3.0 - `k256` v0.5.9 - `tendermint` v0.16.0 - `yubihsm` v0.35.0-rc This commit also moves away from Signatory-based abstractions in several places, leaning more directly on `ed25519-dalek` and the `k256` crate, the latter of which now contains full ECDSA/secp256k1 support. Additionally, it completely removes use of `signatory-dalek` and `rust-secp256k1`/`signatory-secp256k1`. The rationale is that ed25519-dalek has natively adopted the `Signer` and `Verifier` traits which Signatory originally provided, which eliminates the need to use it through an abstraction layer/wrapper. The `signatory-dalek` crate has also since been retired.
tarcieri
force-pushed
the
update-signatory-stdtx-k256-yubihsm
branch
from
4fe5186 to
a603499
Compare
Upgrades the following dependencies to the latest versions:
signatoryv0.22.0stdtxv0.3.0k256v0.5.9tendermintv0.16.0yubihsmv0.35.0-rcThis commit also moves away from Signatory-based abstractions in several places, leaning more directly on
ed25519-dalekand thek256crate, the latter of which now contains full ECDSA/secp256k1 support. Additionally, it completely removes use ofsignatory-dalekandrust-secp256k1/signatory-secp256k1.The rationale is that ed25519-dalek has natively adopted the
SignerandVerifiertraits which Signatory originally provided, which eliminates the need to use it through an abstraction layer/wrapper. Thesignatory-dalekcrate has also since been retired.