Skip to content

Use Into<Message> in signing api #755

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 23, 2024
Merged

Use Into<Message> in signing api #755

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/sign_verify.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn verify<C: Verification>(
let sig = ecdsa::Signature::from_compact(&sig)?;
let pubkey = PublicKey::from_slice(&pubkey)?;

Ok(secp.verify_ecdsa(&msg, &sig, &pubkey).is_ok())
Ok(secp.verify_ecdsa(msg, &sig, &pubkey).is_ok())
}

fn sign<C: Signing>(
Expand All @@ -26,7 +26,7 @@ fn sign<C: Signing>(
let msg = sha256::Hash::hash(msg);
let msg = Message::from_digest_slice(msg.as_ref())?;
let seckey = SecretKey::from_slice(&seckey)?;
Ok(secp.sign_ecdsa(&msg, &seckey))
Ok(secp.sign_ecdsa(msg, &seckey))
}

fn main() {
Expand Down
4 changes: 2 additions & 2 deletions examples/sign_verify_recovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ fn recover<C: Verification>(
let id = ecdsa::RecoveryId::try_from(i32::from(recovery_id))?;
let sig = ecdsa::RecoverableSignature::from_compact(&sig, id)?;

secp.recover_ecdsa(&msg, &sig)
secp.recover_ecdsa(msg, &sig)
}

fn sign_recovery<C: Signing>(
Expand All @@ -26,7 +26,7 @@ fn sign_recovery<C: Signing>(
let msg = sha256::Hash::hash(msg);
let msg = Message::from_digest_slice(msg.as_ref())?;
let seckey = SecretKey::from_slice(&seckey)?;
Ok(secp.sign_ecdsa_recoverable(&msg, &seckey))
Ok(secp.sign_ecdsa_recoverable(msg, &seckey))
}

fn main() {
Expand Down
14 changes: 7 additions & 7 deletions no_std_test/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,12 @@ fn start(_argc: isize, _argv: *const *const u8) -> isize {
let public_key = PublicKey::from_secret_key(&secp, &secret_key);
let message = Message::from_digest_slice(&[0xab; 32]).expect("32 bytes");

let sig = secp.sign_ecdsa(&message, &secret_key);
assert!(secp.verify_ecdsa(&message, &sig, &public_key).is_ok());
let sig = secp.sign_ecdsa(message, &secret_key);
assert!(secp.verify_ecdsa(message, &sig, &public_key).is_ok());

let rec_sig = secp.sign_ecdsa_recoverable(&message, &secret_key);
assert!(secp.verify_ecdsa(&message, &rec_sig.to_standard(), &public_key).is_ok());
assert_eq!(public_key, secp.recover_ecdsa(&message, &rec_sig).unwrap());
let rec_sig = secp.sign_ecdsa_recoverable(message, &secret_key);
assert!(secp.verify_ecdsa(message, &rec_sig.to_standard(), &public_key).is_ok());
assert_eq!(public_key, secp.recover_ecdsa(message, &rec_sig).unwrap());
let (rec_id, data) = rec_sig.serialize_compact();
let new_rec_sig = ecdsa::RecoverableSignature::from_compact(&data, rec_id).unwrap();
assert_eq!(rec_sig, new_rec_sig);
Expand All @@ -121,8 +121,8 @@ fn start(_argc: isize, _argv: *const *const u8) -> isize {
let public_key = PublicKey::from_secret_key(&secp_alloc, &secret_key);
let message = Message::from_digest_slice(&[0xab; 32]).expect("32 bytes");

let sig = secp_alloc.sign_ecdsa(&message, &secret_key);
assert!(secp_alloc.verify_ecdsa(&message, &sig, &public_key).is_ok());
let sig = secp_alloc.sign_ecdsa(message, &secret_key);
assert!(secp_alloc.verify_ecdsa(message, &sig, &public_key).is_ok());
unsafe { libc::printf("Verified alloc Successfully!\n\0".as_ptr() as _) };
}

Expand Down
25 changes: 14 additions & 11 deletions src/ecdsa/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,7 @@ impl Signature {
/// The signature must be normalized or verification will fail (see [`Signature::normalize_s`]).
#[inline]
#[cfg(feature = "global-context")]
pub fn verify(&self, msg: &Message, pk: &PublicKey) -> Result<(), Error> {
pub fn verify(&self, msg: impl Into<Message>, pk: &PublicKey) -> Result<(), Error> {
SECP256K1.verify_ecdsa(msg, self, pk)
}
}
Expand Down Expand Up @@ -243,10 +243,11 @@ impl<'de> serde::Deserialize<'de> for Signature {
impl<C: Signing> Secp256k1<C> {
fn sign_ecdsa_with_noncedata_pointer(
&self,
msg: &Message,
msg: impl Into<Message>,
sk: &SecretKey,
noncedata: Option<&[u8; 32]>,
) -> Signature {
let msg = msg.into();
unsafe {
let mut ret = ffi::Signature::new();
let noncedata_ptr = match noncedata {
Expand All @@ -272,7 +273,7 @@ impl<C: Signing> Secp256k1<C> {

/// Constructs a signature for `msg` using the secret key `sk` and RFC6979 nonce
/// Requires a signing-capable context.
pub fn sign_ecdsa(&self, msg: &Message, sk: &SecretKey) -> Signature {
pub fn sign_ecdsa(&self, msg: impl Into<Message>, sk: &SecretKey) -> Signature {
self.sign_ecdsa_with_noncedata_pointer(msg, sk, None)
}

Expand All @@ -283,7 +284,7 @@ impl<C: Signing> Secp256k1<C> {
/// Requires a signing-capable context.
pub fn sign_ecdsa_with_noncedata(
&self,
msg: &Message,
msg: impl Into<Message>,
sk: &SecretKey,
noncedata: &[u8; 32],
) -> Signature {
Expand All @@ -292,13 +293,14 @@ impl<C: Signing> Secp256k1<C> {

fn sign_grind_with_check(
&self,
msg: &Message,
msg: impl Into<Message>,
sk: &SecretKey,
check: impl Fn(&ffi::Signature) -> bool,
) -> Signature {
let mut entropy_p: *const ffi::types::c_void = ptr::null();
let mut counter: u32 = 0;
let mut extra_entropy = [0u8; 32];
let msg = msg.into();
Copy link
Member

@tcharding tcharding Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why we have a local var here but on line 262 we just chain the calls? Did you mean to change both?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the local variable to ensure that the object lives through the entire time that we are using the pointer.

If we're still chaining the calls on line 262 then that also needs to be changed. Thanks for beating me to the review!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I thought as much.

Out of interest how can the value not exist the entire time we are using it when its pass by value, so is part of the stack frame, calling Into is just borrow checker stuff and as_c_ptr is type checker stuff, neither of which effects the actual data (I think). Then the value is used in a single function call (the ffi function call).

Copy link
Member

@apoelstra apoelstra Oct 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you mean by Into being "just borrow checker stuff". Into::into takes an abstract message by value and returns a Message by value. Then as_c_ptr borrows this object and returns a raw pointer whose lifetime must not exceed the lifetime of the Message. The borrow checker is barely involved with any of this, and even if it was, it can only do sanity checks; it never affects the semantics of code.

But if we don't give the Message a variable binding, its lifetime will only consist of one line of code. If we give it a binding it'll live until the end of the function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tcharding note that this is the same underlying issue as this lint: https://doc.rust-lang.org/rustc/lints/listing/warn-by-default.html#temporary-cstring-as-ptr

(one day we can write an attribute to add these lints for user types)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just musings and for my education, so please only respond if it amuses you to do so.

When sign_ecdsa_with_noncedata_pointer is compiled is the parametr msg: impl Into<Message>, just 32 bytes in the stack frame?

I'm not sure what you mean by Into being "just borrow checker stuff".

I misspoke, FTR I don't know the exact correct terminology for all the parses of the compiler.

I read the link above but that is different in that a str has to have memory backing it but in our case I thought the memory backing it would be the 32 bytes in the stack frame that was passed in as msg (in function sign_ecdsa_with_noncedata_pointer) - so I can't understand why creating a local variable is fixing the problem.

Said another way, I get that at the end of msg.into().as_c_ptr() that there are no guarantees that the pointer is valid, but I don't get why we cannot tell it is valid because we know where the value is on the stack already because it was passed in with the function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(I think I can map C functions to opcodes but I do not know exactly how to map Rust functions to opcodes.)

Copy link
Contributor Author

@liamaharon liamaharon Oct 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why we have a local var here but on line 262 we just chain the calls? Did you mean to change both?

@tcharding it's because here it's used in a loop. impl Into<Message> doesn't implement Copy, so if we .into it inside the loop we lose ownership and cannot proceed with more iterations.

Screenshot 2024-10-22 at 10 51 27

By calling .into outside of the loop, we get the Message type which implements Copy so it can be used inside the loop as many times as needed.

We need the local variable to ensure that the object lives through the entire time that we are using the pointer.

If we're still chaining the calls on line 262 then that also needs to be changed. Thanks for beating me to the review!

@apoelstra interesting. So once Rust passes .into().as_c_ptr() to the ffi, it marks the memory as unused and so that memory could be recycled before the C code finishes doing what it needs with that memory slice? I wonder if that is what was causing the UB in the release build.

I've updated all instances of chaining to have the .into() in a higher scope so the memory is held onto.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read the link above but that is different in that a str has to have memory backing it but in our case I thought the memory backing it would be the 32 bytes in the stack frame

How could it be? The 32 bytes in the stack frame belong to an object of a completely different type (an opaque Into<Message> vs a Message). And we are telling the compiler that we don't need the Into<Message> anymore and it can reclaim the memory.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to read a book on how the Rust compiler works. Thanks for your patience.

loop {
unsafe {
let mut ret = ffi::Signature::new();
Expand Down Expand Up @@ -338,7 +340,7 @@ impl<C: Signing> Secp256k1<C> {
/// Requires a signing capable context.
pub fn sign_ecdsa_grind_r(
&self,
msg: &Message,
msg: impl Into<Message>,
sk: &SecretKey,
bytes_to_grind: usize,
) -> Signature {
Expand All @@ -352,7 +354,7 @@ impl<C: Signing> Secp256k1<C> {
/// signature implementation of bitcoin core. In average, this function
/// will perform two signing operations.
/// Requires a signing capable context.
pub fn sign_ecdsa_low_r(&self, msg: &Message, sk: &SecretKey) -> Signature {
pub fn sign_ecdsa_low_r(&self, msg: impl Into<Message>, sk: &SecretKey) -> Signature {
self.sign_grind_with_check(msg, sk, compact_sig_has_zero_first_bit)
}
}
Expand All @@ -372,20 +374,21 @@ impl<C: Verification> Secp256k1<C> {
/// # let (secret_key, public_key) = secp.generate_keypair(&mut rand::thread_rng());
/// #
/// let message = Message::from_digest_slice(&[0xab; 32]).expect("32 bytes");
/// let sig = secp.sign_ecdsa(&message, &secret_key);
/// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Ok(()));
/// let sig = secp.sign_ecdsa(message, &secret_key);
/// assert_eq!(secp.verify_ecdsa(message, &sig, &public_key), Ok(()));
///
/// let message = Message::from_digest_slice(&[0xcd; 32]).expect("32 bytes");
/// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Err(Error::IncorrectSignature));
/// assert_eq!(secp.verify_ecdsa(message, &sig, &public_key), Err(Error::IncorrectSignature));
/// # }
/// ```
#[inline]
pub fn verify_ecdsa(
&self,
msg: &Message,
msg: impl Into<Message>,
sig: &Signature,
pk: &PublicKey,
) -> Result<(), Error> {
let msg = msg.into();
unsafe {
if ffi::secp256k1_ecdsa_verify(
self.ctx.as_ptr(),
Expand Down
46 changes: 24 additions & 22 deletions src/ecdsa/recovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ impl RecoverableSignature {
/// verify-capable context.
#[inline]
#[cfg(feature = "global-context")]
pub fn recover(&self, msg: &Message) -> Result<key::PublicKey, Error> {
pub fn recover(&self, msg: impl Into<Message>) -> Result<key::PublicKey, Error> {
crate::SECP256K1.recover_ecdsa(msg, self)
}
}
Expand All @@ -154,10 +154,11 @@ impl From<ffi::RecoverableSignature> for RecoverableSignature {
impl<C: Signing> Secp256k1<C> {
fn sign_ecdsa_recoverable_with_noncedata_pointer(
&self,
msg: &Message,
msg: impl Into<Message>,
sk: &key::SecretKey,
noncedata_ptr: *const super_ffi::types::c_void,
) -> RecoverableSignature {
let msg = msg.into();
let mut ret = ffi::RecoverableSignature::new();
unsafe {
// We can assume the return value because it's not possible to construct
Expand All @@ -182,7 +183,7 @@ impl<C: Signing> Secp256k1<C> {
/// Requires a signing-capable context.
pub fn sign_ecdsa_recoverable(
&self,
msg: &Message,
msg: impl Into<Message>,
sk: &key::SecretKey,
) -> RecoverableSignature {
self.sign_ecdsa_recoverable_with_noncedata_pointer(msg, sk, ptr::null())
Expand All @@ -195,7 +196,7 @@ impl<C: Signing> Secp256k1<C> {
/// Requires a signing-capable context.
pub fn sign_ecdsa_recoverable_with_noncedata(
&self,
msg: &Message,
msg: impl Into<Message>,
sk: &key::SecretKey,
noncedata: &[u8; 32],
) -> RecoverableSignature {
Expand All @@ -209,9 +210,10 @@ impl<C: Verification> Secp256k1<C> {
/// `msg`. Requires a verify-capable context.
pub fn recover_ecdsa(
&self,
msg: &Message,
msg: impl Into<Message>,
sig: &RecoverableSignature,
) -> Result<key::PublicKey, Error> {
let msg = msg.into();
unsafe {
let mut pk = super_ffi::PublicKey::new();
if ffi::secp256k1_ecdsa_recover(
Expand Down Expand Up @@ -252,15 +254,15 @@ mod tests {
let (sk, pk) = full.generate_keypair(&mut rand::thread_rng());

// Try signing
assert_eq!(sign.sign_ecdsa_recoverable(&msg, &sk), full.sign_ecdsa_recoverable(&msg, &sk));
let sigr = full.sign_ecdsa_recoverable(&msg, &sk);
assert_eq!(sign.sign_ecdsa_recoverable(msg, &sk), full.sign_ecdsa_recoverable(msg, &sk));
let sigr = full.sign_ecdsa_recoverable(msg, &sk);

// Try pk recovery
assert!(vrfy.recover_ecdsa(&msg, &sigr).is_ok());
assert!(full.recover_ecdsa(&msg, &sigr).is_ok());
assert!(vrfy.recover_ecdsa(msg, &sigr).is_ok());
assert!(full.recover_ecdsa(msg, &sigr).is_ok());

assert_eq!(vrfy.recover_ecdsa(&msg, &sigr), full.recover_ecdsa(&msg, &sigr));
assert_eq!(full.recover_ecdsa(&msg, &sigr), Ok(pk));
assert_eq!(vrfy.recover_ecdsa(msg, &sigr), full.recover_ecdsa(msg, &sigr));
assert_eq!(full.recover_ecdsa(msg, &sigr), Ok(pk));
}

#[test]
Expand All @@ -280,7 +282,7 @@ mod tests {
let sk = SecretKey::from_slice(&ONE).unwrap();
let msg = Message::from_digest_slice(&ONE).unwrap();

let sig = s.sign_ecdsa_recoverable(&msg, &sk);
let sig = s.sign_ecdsa_recoverable(msg, &sk);

assert_eq!(Ok(sig), RecoverableSignature::from_compact(&[
0x66, 0x73, 0xff, 0xad, 0x21, 0x47, 0x74, 0x1f,
Expand All @@ -306,7 +308,7 @@ mod tests {
let msg = Message::from_digest_slice(&ONE).unwrap();
let noncedata = [42u8; 32];

let sig = s.sign_ecdsa_recoverable_with_noncedata(&msg, &sk, &noncedata);
let sig = s.sign_ecdsa_recoverable_with_noncedata(msg, &sk, &noncedata);

assert_eq!(Ok(sig), RecoverableSignature::from_compact(&[
0xb5, 0x0b, 0xb6, 0x79, 0x5f, 0x31, 0x74, 0x8a,
Expand All @@ -331,14 +333,14 @@ mod tests {

let (sk, pk) = s.generate_keypair(&mut rand::thread_rng());

let sigr = s.sign_ecdsa_recoverable(&msg, &sk);
let sigr = s.sign_ecdsa_recoverable(msg, &sk);
let sig = sigr.to_standard();

let msg = crate::random_32_bytes(&mut rand::thread_rng());
let msg = Message::from_digest_slice(&msg).unwrap();
assert_eq!(s.verify_ecdsa(&msg, &sig, &pk), Err(Error::IncorrectSignature));
assert_eq!(s.verify_ecdsa(msg, &sig, &pk), Err(Error::IncorrectSignature));

let recovered_key = s.recover_ecdsa(&msg, &sigr).unwrap();
let recovered_key = s.recover_ecdsa(msg, &sigr).unwrap();
assert!(recovered_key != pk);
}

Expand All @@ -353,9 +355,9 @@ mod tests {

let (sk, pk) = s.generate_keypair(&mut rand::thread_rng());

let sig = s.sign_ecdsa_recoverable(&msg, &sk);
let sig = s.sign_ecdsa_recoverable(msg, &sk);

assert_eq!(s.recover_ecdsa(&msg, &sig), Ok(pk));
assert_eq!(s.recover_ecdsa(msg, &sig), Ok(pk));
}

#[test]
Expand All @@ -371,9 +373,9 @@ mod tests {

let (sk, pk) = s.generate_keypair(&mut rand::thread_rng());

let sig = s.sign_ecdsa_recoverable_with_noncedata(&msg, &sk, &noncedata);
let sig = s.sign_ecdsa_recoverable_with_noncedata(msg, &sk, &noncedata);

assert_eq!(s.recover_ecdsa(&msg, &sig), Ok(pk));
assert_eq!(s.recover_ecdsa(msg, &sig), Ok(pk));
}

#[test]
Expand All @@ -386,10 +388,10 @@ mod tests {

// Zero is not a valid sig
let sig = RecoverableSignature::from_compact(&[0; 64], RecoveryId::Zero).unwrap();
assert_eq!(s.recover_ecdsa(&msg, &sig), Err(Error::InvalidSignature));
assert_eq!(s.recover_ecdsa(msg, &sig), Err(Error::InvalidSignature));
// ...but 111..111 is
let sig = RecoverableSignature::from_compact(&[1; 64], RecoveryId::Zero).unwrap();
assert!(s.recover_ecdsa(&msg, &sig).is_ok());
assert!(s.recover_ecdsa(msg, &sig).is_ok());
}

#[test]
Expand Down
6 changes: 4 additions & 2 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,9 @@ impl SecretKey {
/// Constructs an ECDSA signature for `msg` using the global [`SECP256K1`] context.
#[inline]
#[cfg(feature = "global-context")]
pub fn sign_ecdsa(&self, msg: Message) -> ecdsa::Signature { SECP256K1.sign_ecdsa(&msg, self) }
pub fn sign_ecdsa(&self, msg: impl Into<Message>) -> ecdsa::Signature {
SECP256K1.sign_ecdsa(msg, self)
}

/// Returns the [`Keypair`] for this [`SecretKey`].
///
Expand Down Expand Up @@ -737,7 +739,7 @@ impl PublicKey {
pub fn verify<C: Verification>(
&self,
secp: &Secp256k1<C>,
msg: &Message,
msg: impl Into<Message>,
sig: &ecdsa::Signature,
) -> Result<(), Error> {
secp.verify_ecdsa(msg, sig, self)
Expand Down
Loading
Loading