From a986e8d50738ff51f57e6641ab4e22537037b6e0 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Thu, 24 Nov 2022 22:34:31 +0000 Subject: [PATCH 1/6] secp256k1-sys: export two static signing-capable contexts from C --- secp256k1-sys/depend/secp256k1.c.patch | 37 +++++++++++++++++-- secp256k1-sys/depend/secp256k1.h.patch | 14 ++++--- .../depend/secp256k1/include/secp256k1.h | 3 ++ .../depend/secp256k1/src/secp256k1.c | 28 ++++++++++++++ secp256k1-sys/src/lib.rs | 6 +++ 5 files changed, 79 insertions(+), 9 deletions(-) diff --git a/secp256k1-sys/depend/secp256k1.c.patch b/secp256k1-sys/depend/secp256k1.c.patch index c39705a00..d5dfa1077 100644 --- a/secp256k1-sys/depend/secp256k1.c.patch +++ b/secp256k1-sys/depend/secp256k1.c.patch @@ -1,4 +1,33 @@ -139,149d138 +61a62,89 +> /* rust-secp: add two static contexts which are initialized for signing. We +> * need two so that we can effect updates (i.e. rerandomization) by atomic +> * pointer assignment */ +> static secp256k1_context secp256k1_context_signing_1_ = { +> { +> 1, +> SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0), +> SECP256K1_GEJ_CONST_INFINITY +> }, +> { secp256k1_default_illegal_callback_fn, 0 }, +> { secp256k1_default_error_callback_fn, 0 }, +> 0 +> }; +> secp256k1_context *secp256k1_context_signing_1 = &secp256k1_context_signing_1_; +> +> static secp256k1_context secp256k1_context_signing_2_ = { +> { +> 1, +> SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0), +> SECP256K1_GEJ_CONST_INFINITY +> }, +> { secp256k1_default_illegal_callback_fn, 0 }, +> { secp256k1_default_error_callback_fn, 0 }, +> 0 +> }; +> secp256k1_context *secp256k1_context_signing_2 = &secp256k1_context_signing_2_; +> /* end rust-secp */ +> +107,117d125 < secp256k1_context* secp256k1_context_create(unsigned int flags) { < size_t const prealloc_size = secp256k1_context_preallocated_size(flags); < secp256k1_context* ctx = (secp256k1_context*)checked_malloc(&default_error_callback, prealloc_size); @@ -10,7 +39,7 @@ < return ctx; < } < -164,174d152 +128,138d135 < secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) { < secp256k1_context* ret; < size_t prealloc_size; @@ -22,7 +51,7 @@ < return ret; < } < -183,189d160 +146,152d142 < void secp256k1_context_destroy(secp256k1_context* ctx) { < if (ctx != NULL) { < secp256k1_context_preallocated_destroy(ctx); @@ -30,7 +59,7 @@ < } < } < -206,215d176 +169,178d158 < } < < secp256k1_scratch_space* secp256k1_scratch_space_create(const secp256k1_context* ctx, size_t max_size) { diff --git a/secp256k1-sys/depend/secp256k1.h.patch b/secp256k1-sys/depend/secp256k1.h.patch index a22f8cfdd..a7ffe68f5 100644 --- a/secp256k1-sys/depend/secp256k1.h.patch +++ b/secp256k1-sys/depend/secp256k1.h.patch @@ -1,21 +1,25 @@ -226,228d225 +206a207,209 +> SECP256K1_API extern secp256k1_context *secp256k1_context_signing_1; +> SECP256K1_API extern secp256k1_context *secp256k1_context_signing_2; +> +218,220d220 < SECP256K1_API secp256k1_context* secp256k1_context_create( < unsigned int flags < ) SECP256K1_WARN_UNUSED_RESULT; -231,233d227 +231,233d230 < SECP256K1_API secp256k1_context* secp256k1_context_clone( < const secp256k1_context* ctx < ) SECP256K1_ARG_NONNULL(1) SECP256K1_WARN_UNUSED_RESULT; -248,250d241 +248,250d244 < SECP256K1_API void secp256k1_context_destroy( < secp256k1_context* ctx < ) SECP256K1_ARG_NONNULL(1); -327,330d317 +327,330d320 < SECP256K1_API SECP256K1_WARN_UNUSED_RESULT secp256k1_scratch_space* secp256k1_scratch_space_create( < const secp256k1_context* ctx, < size_t size < ) SECP256K1_ARG_NONNULL(1); -338,341d324 +338,341d327 < SECP256K1_API void secp256k1_scratch_space_destroy( < const secp256k1_context* ctx, < secp256k1_scratch_space* scratch diff --git a/secp256k1-sys/depend/secp256k1/include/secp256k1.h b/secp256k1-sys/depend/secp256k1/include/secp256k1.h index 5ba40890c..7b3a5accf 100644 --- a/secp256k1-sys/depend/secp256k1/include/secp256k1.h +++ b/secp256k1-sys/depend/secp256k1/include/secp256k1.h @@ -204,6 +204,9 @@ typedef int (*rustsecp256k1_v0_6_1_nonce_function)( */ SECP256K1_API extern const rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_no_precomp; +SECP256K1_API extern rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_1; +SECP256K1_API extern rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_2; + /** Create a secp256k1 context object (in dynamically allocated memory). * * This function uses malloc to allocate memory. It is guaranteed that malloc is diff --git a/secp256k1-sys/depend/secp256k1/src/secp256k1.c b/secp256k1-sys/depend/secp256k1/src/secp256k1.c index f2bed3002..6f3fc331e 100644 --- a/secp256k1-sys/depend/secp256k1/src/secp256k1.c +++ b/secp256k1-sys/depend/secp256k1/src/secp256k1.c @@ -59,6 +59,34 @@ static const rustsecp256k1_v0_6_1_context rustsecp256k1_v0_6_1_context_no_precom }; const rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_no_precomp = &rustsecp256k1_v0_6_1_context_no_precomp_; +/* rust-secp: add two static contexts which are initialized for signing. We + * need two so that we can effect updates (i.e. rerandomization) by atomic + * pointer assignment */ +static rustsecp256k1_v0_6_1_context rustsecp256k1_v0_6_1_context_signing_1_ = { + { + 1, + SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0), + SECP256K1_GEJ_CONST_INFINITY + }, + { rustsecp256k1_v0_6_1_default_illegal_callback_fn, 0 }, + { rustsecp256k1_v0_6_1_default_error_callback_fn, 0 }, + 0 +}; +rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_1 = &rustsecp256k1_v0_6_1_context_signing_1_; + +static rustsecp256k1_v0_6_1_context rustsecp256k1_v0_6_1_context_signing_2_ = { + { + 1, + SECP256K1_SCALAR_CONST(0, 0, 0, 0, 0, 0, 0, 0), + SECP256K1_GEJ_CONST_INFINITY + }, + { rustsecp256k1_v0_6_1_default_illegal_callback_fn, 0 }, + { rustsecp256k1_v0_6_1_default_error_callback_fn, 0 }, + 0 +}; +rustsecp256k1_v0_6_1_context *rustsecp256k1_v0_6_1_context_signing_2 = &rustsecp256k1_v0_6_1_context_signing_2_; +/* end rust-secp */ + size_t rustsecp256k1_v0_6_1_context_preallocated_size(unsigned int flags) { size_t ret = sizeof(rustsecp256k1_v0_6_1_context); /* A return value of 0 is reserved as an indicator for errors when we call this function internally. */ diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index 243c941f7..cc23ff1bf 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -510,6 +510,12 @@ extern "C" { #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_no_precomp")] pub static secp256k1_context_no_precomp: *const Context; + #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_signing_1")] + pub static secp256k1_context_signing_1: *mut Context; + + #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_signing_2")] + pub static secp256k1_context_signing_2: *mut Context; + // Contexts #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_preallocated_destroy")] pub fn secp256k1_context_preallocated_destroy(cx: *mut Context); From 7d925f62aeac180cf65bf0ac4e08213b6705f84d Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Thu, 24 Nov 2022 23:20:19 +0000 Subject: [PATCH 2/6] secp256k1-sys: make signing context pointers atomic This is in its own commit so that we can argue about it independently of the rest of the PR. I believe this is correct and safe because the docs for `AtomicPtr` say that (a) it is #[repr(C)] and (b) that it has the same in-memory representation as a *mut T. --- secp256k1-sys/src/lib.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/secp256k1-sys/src/lib.rs b/secp256k1-sys/src/lib.rs index cc23ff1bf..825ec57ca 100644 --- a/secp256k1-sys/src/lib.rs +++ b/secp256k1-sys/src/lib.rs @@ -39,6 +39,7 @@ pub mod types; pub mod recovery; use core::{slice, ptr}; +use core::sync::atomic::AtomicPtr; use types::*; /// Flag for context to enable no precomputation @@ -511,10 +512,10 @@ extern "C" { pub static secp256k1_context_no_precomp: *const Context; #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_signing_1")] - pub static secp256k1_context_signing_1: *mut Context; + pub static secp256k1_context_signing_1: AtomicPtr; #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_signing_2")] - pub static secp256k1_context_signing_2: *mut Context; + pub static secp256k1_context_signing_2: AtomicPtr; // Contexts #[cfg_attr(not(rust_secp_no_symbol_renaming), link_name = "rustsecp256k1_v0_6_1_context_preallocated_destroy")] From a62f3985b91d4cada3bdfc26ac1bb08589d3e752 Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Thu, 24 Nov 2022 22:54:56 +0000 Subject: [PATCH 3/6] context: add methods to provide immutable access and rerandomization This covers both the std and no-std cases. This is perhaps an unconventional strategy; see https://github.com/rust-bitcoin/rust-secp256k1/issues/346 for discussion leading up to it. --- src/context.rs | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/src/context.rs b/src/context.rs index 52402d0a6..bd7d7d2cb 100644 --- a/src/context.rs +++ b/src/context.rs @@ -8,6 +8,98 @@ use crate::ffi::types::{c_uint, c_void, AlignedType}; use crate::ffi::{self, CPtr}; use crate::{Error, Secp256k1}; +// This module will be renamed to `global` in later commit which eliminates the other `global` module +#[cfg(feature = "std")] +pub mod global_ { + use core::cell::RefCell; + use crate::{ffi, All, Secp256k1}; + + thread_local! { + pub static CONTEXT: RefCell> = RefCell::new(Secp256k1::new()); + } + + /// Do some operation using an immutable reference to the global signing context + /// + /// Safety: you are being given a raw pointer. Do not mutate through it. Do not + /// move it, or any reference to it, across thread boundaries. + pub unsafe fn with_global_context R, R>(f: F) -> R { + CONTEXT.with(|refcell| f(refcell.borrow().ctx as *const _)) + } + + /// Rerandomize the global signing context + /// + /// # Panics + /// + /// Will panic if called from a closure passed to `with_global_ctx`. + pub fn rerandomize_context(seed: &[u8; 32]) { + CONTEXT.with(|refcell| { + let borrow = refcell.borrow_mut(); + let bare = borrow.ctx; + unsafe { + let ret = ffi::secp256k1_context_randomize(bare, seed.as_ptr()); + assert_eq!(ret, 1); // sanity check: context_randomize cannot fail + } + }); + } +} + +#[cfg(not(feature = "std"))] +mod global_ { + use core::sync::atomic::{AtomicBool, Ordering}; + use crate::ffi; + + static LOCK: AtomicBool = AtomicBool::new(false); + + // Safety: all of the unsafe blocks in this module are due to our accessing + // extern statics (the ffi::* context objects). Rust cannot control access + // to these, so we need unsafe. + // + // In particular, we are promising here that nothing else will concurrently + // access ffi::secp256k1_context_signing_{1, 2} ... though in principle, + // any code that can see the `ffi` module will be able to do so. We can only + // make promises about our own code. + // + // Other accessors, who to be clear SHOULD NOT EXIST, will need their own + // unsafe block. As far as I know this is the best we can do. + + /// Do some operation using an immutable reference to the global signing context + pub unsafe fn with_global_ctx R, R>(f: F) -> R { + let ctx = unsafe { ffi::secp256k1_context_signing_1.load(Ordering::Acquire) }; + f(ctx as *const _) + } + + /// Do some operation using a mutable reference to the global signing context + pub fn rerandomize_context(seed: &[u8; 32]) { + if LOCK.swap(true, Ordering::Acquire) { + // Concurrent operation in progress; give up + return; + } + + unsafe { + // Obtain a pointer to the alternate context. + let alt_ctx = ffi::secp256k1_context_signing_2.load(Ordering::Acquire); + // Obtain a pointer to the signing context, atomically replacing it with + // the alternate signing context (which never gets modified). Any concurrent + // callers to `with_global_ctx` will see the alternate context, not any + // inconsistent state of this one. + // + // (Concurrent callers to `with_global_ctx_mut` will fail after checking `LOCK`.). + let ctx = ffi::secp256k1_context_signing_1.swap(alt_ctx, Ordering::Acquire); + // Do the rerandomization. Note that unlike in the `std` case, we don't + // panic if this function fails (which again, it is guaranteed not to). + // The reason is that a panic at this point in the code would leave + // `LOCK` at true (as well as both `signing_1` and `signing_2` pointing + // at the same, never-rerandomized, place) and therefore prevent any + // future rerandomizations from succeeding. + ffi::secp256k1_context_randomize(ctx, seed.as_ptr()); + // Atomically swap completely-modified context into place + ffi::secp256k1_context_signing_1.swap(ctx, Ordering::Release); + }; + + LOCK.store(false, Ordering::Release); + } +} + #[cfg(all(feature = "global-context", feature = "std"))] #[cfg_attr(docsrs, doc(cfg(all(feature = "global-context", feature = "std"))))] /// Module implementing a singleton pattern for a global `Secp256k1` context. From a03beddbf8825208706cad1a226ba65a84dc922e Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 25 Nov 2022 00:33:10 +0000 Subject: [PATCH 4/6] add a simple end-to-end test not feature-gated on std or alloc Weirdly we have no tests that don't require std or alloc. We should revisit this. In many cases it is because of the dependency on rand-std but we should really try to narrow that down to specific tests. --- src/context.rs | 4 ++-- src/lib.rs | 19 +++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/src/context.rs b/src/context.rs index bd7d7d2cb..0c30ef38f 100644 --- a/src/context.rs +++ b/src/context.rs @@ -44,7 +44,7 @@ pub mod global_ { } #[cfg(not(feature = "std"))] -mod global_ { +pub mod global_ { use core::sync::atomic::{AtomicBool, Ordering}; use crate::ffi; @@ -63,7 +63,7 @@ mod global_ { // unsafe block. As far as I know this is the best we can do. /// Do some operation using an immutable reference to the global signing context - pub unsafe fn with_global_ctx R, R>(f: F) -> R { + pub unsafe fn with_global_context R, R>(f: F) -> R { let ctx = unsafe { ffi::secp256k1_context_signing_1.load(Ordering::Acquire) }; f(ctx as *const _) } diff --git a/src/lib.rs b/src/lib.rs index 946f6c7b8..713db8200 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1054,6 +1054,25 @@ mod tests { assert_eq!(msg.0, hash.into_inner()); assert_eq!(msg, Message::from_hashed_data::(test_bytes)); } + + #[test] + fn test_sign_verify_nostd() { + let mut buf_ful = vec![AlignedType::zeroed(); Secp256k1::preallocate_size()]; + let full = Secp256k1::preallocated_new(&mut buf_ful).unwrap(); + + let sk = SecretKey::from_slice(&[3u8; 32]).unwrap(); + let pk = PublicKey::from_secret_key(&full, &sk); + let msg = Message::from_slice(&[2u8; 32]).unwrap(); + + // ECDSA + let sig = full.sign_ecdsa(&msg, &sk); + assert!(full.verify_ecdsa(&msg, &sig, &pk).is_ok()); + + // Schnorr + let kp = KeyPair::from_secret_key(&full, &sk); + let sig = full.sign_schnorr_no_aux_rand(&msg, &kp); + assert!(full.verify_schnorr(&sig, &msg, &kp.x_only_public_key().0).is_ok()); + } } #[cfg(bench)] From eac1b861c0b3688020cdc5d5f2ca8f84768cc89a Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 25 Nov 2022 00:37:20 +0000 Subject: [PATCH 5/6] f try using the new global context This commit should probably be removed before merging; it can be part of the next PR. --- src/schnorr.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/schnorr.rs b/src/schnorr.rs index cc748a6a1..80c5ab0bf 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -103,11 +103,13 @@ impl Secp256k1 { nonce_data: *const ffi::types::c_uchar, ) -> Signature { unsafe { +crate::context::global_::rerandomize_context(b"this is a 32-byte random string!"); +crate::context::global_::with_global_context(|ctx| { let mut sig = [0u8; constants::SCHNORR_SIGNATURE_SIZE]; assert_eq!( 1, ffi::secp256k1_schnorrsig_sign( - self.ctx, + ctx, sig.as_mut_c_ptr(), msg.as_c_ptr(), keypair.as_c_ptr(), @@ -116,6 +118,7 @@ impl Secp256k1 { ); Signature(sig) +}) } } From 087c2f5d9bff2d72c70bca671096545e53d70bda Mon Sep 17 00:00:00 2001 From: Andrew Poelstra Date: Fri, 25 Nov 2022 01:53:17 +0000 Subject: [PATCH 6/6] make cargo fmt shut up --- src/context.rs | 2 ++ src/schnorr.rs | 32 ++++++++++++++++---------------- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/src/context.rs b/src/context.rs index 0c30ef38f..5054d70ce 100644 --- a/src/context.rs +++ b/src/context.rs @@ -12,6 +12,7 @@ use crate::{Error, Secp256k1}; #[cfg(feature = "std")] pub mod global_ { use core::cell::RefCell; + use crate::{ffi, All, Secp256k1}; thread_local! { @@ -46,6 +47,7 @@ pub mod global_ { #[cfg(not(feature = "std"))] pub mod global_ { use core::sync::atomic::{AtomicBool, Ordering}; + use crate::ffi; static LOCK: AtomicBool = AtomicBool::new(false); diff --git a/src/schnorr.rs b/src/schnorr.rs index 80c5ab0bf..77d5fe0fd 100644 --- a/src/schnorr.rs +++ b/src/schnorr.rs @@ -103,22 +103,22 @@ impl Secp256k1 { nonce_data: *const ffi::types::c_uchar, ) -> Signature { unsafe { -crate::context::global_::rerandomize_context(b"this is a 32-byte random string!"); -crate::context::global_::with_global_context(|ctx| { - let mut sig = [0u8; constants::SCHNORR_SIGNATURE_SIZE]; - assert_eq!( - 1, - ffi::secp256k1_schnorrsig_sign( - ctx, - sig.as_mut_c_ptr(), - msg.as_c_ptr(), - keypair.as_c_ptr(), - nonce_data, - ) - ); - - Signature(sig) -}) + crate::context::global_::rerandomize_context(b"this is a 32-byte random string!"); + crate::context::global_::with_global_context(|ctx| { + let mut sig = [0u8; constants::SCHNORR_SIGNATURE_SIZE]; + assert_eq!( + 1, + ffi::secp256k1_schnorrsig_sign( + ctx, + sig.as_mut_c_ptr(), + msg.as_c_ptr(), + keypair.as_c_ptr(), + nonce_data, + ) + ); + + Signature(sig) + }) } }