-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few issues that imho should be addressed to make sure we are fully backward compatible. Also should for sure get an external audit and might be cool to add a fuzzer for (all of) the crypto stuff.
let id = secp256k1::recovery::RecoveryId::from_i32(self.0[64] as i32).ok()?; | ||
let sig = secp256k1::recovery::RecoverableSignature::from_compact(&self.0[..64], id).ok()?; | ||
|
||
secp256k1::Secp256k1::verification_only().recover(&message, &sig) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Creating the Secp256k1
is super heavy and should be cached in a lazy_static
or sth. AFAIR they are considering making the required data static, but it's not there yet in the C library.
Also see: tomusdrw/ethsign#37
.map_err(|_| SecretStringError::InvalidSeedLength)?; | ||
let public = PublicKey::from_secret_key(&secret); | ||
let public = PublicKey::from_secret_key(&secp256k1::Secp256k1::signing_only(), &secret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here (context to be cached)
let message = secp256k1::Message::parse(&blake2_256(message.as_ref())); | ||
let sig: (_, _) = self.try_into().ok()?; | ||
secp256k1::recover(&message, &sig.0, &sig.1) | ||
let message = secp256k1::Message::from_slice(&blake2_256(message.as_ref())).ok()?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be an expect
, since we know it's 32-bytes
r.0[0..64].copy_from_slice(&x.0.serialize()[..]); | ||
r.0[64] = x.1.serialize(); | ||
r.0[0..64].copy_from_slice(&x.0.serialize_compact()[..]); | ||
r.0[64] = x.1.to_i32() as u8; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might cause issues in case of overflows - you can have multiple different RecoveryId
s ending up with the same Signature
type (malleability).
Seems that libsecp256k1
was simply storing u8
, but the C library stores i32
.
secp256k1::sign(&message, &self.secret).into() | ||
let message = secp256k1::Message::from_slice(&blake2_256(message)) | ||
.expect("Message is 32 byte; qed"); | ||
let sig = secp256k1::Secp256k1::signing_only().sign_recoverable(&message, &self.secret); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here :)
signature.copy_from_slice(sig); | ||
let sig = Signature(signature); | ||
|
||
match sig.recover(message) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Message is not blake2_256
hashed here (and it used to be)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fact it wasn't caught by a test scares me.
|
||
let msg = secp256k1::Message::from_slice(msg).map_err(|_| EcdsaVerifyError::BadSignature)?; | ||
|
||
let pubkey = secp256k1::Secp256k1::verification_only().recover(&msg, &rs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Context here again.
|
||
let msg = secp256k1::Message::from_slice(msg).map_err(|_| EcdsaVerifyError::BadSignature)?; | ||
|
||
let pubkey = secp256k1::Secp256k1::verification_only().recover(&msg, &rs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and here
@@ -103,8 +103,18 @@ impl Public { | |||
/// This will convert the full public key into the compressed format. | |||
#[cfg(feature = "std")] | |||
pub fn from_full(full: &[u8]) -> Result<Self, ()> { | |||
secp256k1::PublicKey::parse_slice(full, None) | |||
.map(|k| k.serialize_compressed()) | |||
let pubkey = if full.len() == 64 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The libsecp256k1
library is handling 3 kind of keys:
full (65 bytes), raw (64 bytes; no tag) and compressed.
Here we only handle the raw
case, are you sure that secp256k1
is handling both full and compressed case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The C lib handles 33 and 65 byte signatures. But no 64 byte signatures:
https://github.com/bitcoin-core/secp256k1/blob/9a5a87e0f1276e0284446af1172056ea4693737f/src/eckey_impl.h#L17-L35
This might be a silly question, but I thought the point of libsecp256k1 was to be used from substrate? This other library actually seems more awkward than that one, is there not a way to just patch libsecp instead? |
What you mean by this? |
Ah nevermind, I see the context from the linked issue. I thought this was trying to fix another issue. |
anyone working on this? |
closing due to lack of progress. |
Fixes: #8089