Skip to content
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.

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

Calling Secp256k1::randomize on a verification-only context will abort the process #82

Closed
apoelstra opened this issue Nov 12, 2018 · 3 comments · Fixed by bitcoin-core/secp256k1#587

Comments

@apoelstra
Copy link
Member

apoelstra commented Nov 12, 2018

TIL that secp256k1_context_randomize actually requires a signing context, despite the docs not mentioning this. See bitcoin-core/secp256k1#573

The following code can trigger a process abort in a unit test (run with cargo test --features "rand")

diff --git a/src/lib.rs b/src/lib.rs
index 81cbc57..2b0a4a7 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -1179,6 +1179,13 @@ mod tests {
 
         assert_tokens(&sig, &[Token::BorrowedBytes(&SIG_BYTES[..])]);
     }
+
+    #[cfg(feature="rand")]
+    #[test]
+    fn test_randomize() {
+        let mut s = Secp256k1::verification_only();
+        s.randomize(&mut ::rand::thread_rng());
+    }
 }

 #[cfg(all(test, feature = "unstable"))] 
@apoelstra
Copy link
Member Author

apoelstra commented Nov 12, 2018

As a secondary bug, there are no unit tests for the context randomization function.

Edit: Ah, there sorta are, but they're spread throughout the rest of the tests and I evidently only thought to add it to signing tests.

gmaxwell added a commit to bitcoin-core/secp256k1 that referenced this issue Feb 21, 2019
6198375 Make randomization of a non-signing context a noop (Tim Ruffing)

Pull request description:

  Before this commit secp256k1_context_randomize called illegal_callback
  when called on a context not initialized for signing. This is not
  documented. Moreover, it is not desirable because non-signing contexts
  may use randomization in the future.

  This commit makes secp256k1_context_randomize a noop in this case. This
  is safe because the context cannot be used for signing anyway.

  This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.

Tree-SHA512: 34ddfeb004d9da8f4a77c739fa2110544c28939378e779226da52f410a0e36b3aacb3ebd2e3f3918832a9027684c161789cfdc27a133f2f0e0f1c47e8363029c
@real-or-random
Copy link
Collaborator

real-or-random commented Feb 27, 2019

This has been fixed upstream.

As a secondary bug, there are no unit tests for the context randomization function.

Edit: Ah, there sorta are, but they're spread throughout the rest of the tests and I evidently only thought to add it to signing tests.

Closing here for now but just reply if you think that we should improve that.

@apoelstra
Copy link
Member Author

I guess we ought to reopen until we update our C code to use the new upstream.

@apoelstra apoelstra reopened this Feb 27, 2019
real-or-random added a commit to romanz/secp256k1-zkp that referenced this issue Mar 23, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.
real-or-random added a commit to romanz/secp256k1-zkp that referenced this issue Apr 1, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.
real-or-random added a commit to romanz/secp256k1-zkp that referenced this issue Apr 5, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.
real-or-random added a commit to romanz/secp256k1-zkp that referenced this issue Apr 12, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.
jonasnick pushed a commit to jonasnick/secp256k1-zkp that referenced this issue May 6, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.
apoelstra pushed a commit to apoelstra/secp256k1-zkp that referenced this issue May 29, 2019
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jan 17, 2020
Summary:
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.

This is a backport of secp256k1's PR587

Depends on D4971

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D4980
deadalnix pushed a commit to Bitcoin-ABC/secp256k1 that referenced this issue Jan 18, 2020
Summary:
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes #573 and it fixes rust-bitcoin/rust-secp256k1#82.

This is a backport of secp256k1's PR587

Depends on D4971

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D4980
sickpig pushed a commit to sickpig/BitcoinUnlimited that referenced this issue Mar 24, 2020
Summary:
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes BitcoinUnlimited#573 and it fixes rust-bitcoin/rust-secp256k1#82.

This is a backport of secp256k1's PR587

Depends on D4971

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D4980
sickpig pushed a commit to sickpig/BitcoinUnlimited that referenced this issue Mar 24, 2020
Summary:
Before this commit secp256k1_context_randomize called illegal_callback
when called on a context not initialized for signing. This is not
documented. Moreover, it is not desirable because non-signing contexts
may use randomization in the future.

This commit makes secp256k1_context_randomize a noop in this case. This
is safe because the context cannot be used for signing anyway.

This fixes BitcoinUnlimited#573 and it fixes rust-bitcoin/rust-secp256k1#82.

This is a backport of secp256k1's PR587

Depends on D4971

Test Plan:
  ninja check-secp256k1

Reviewers: #bitcoin_abc, Fabien

Reviewed By: #bitcoin_abc, Fabien

Differential Revision: https://reviews.bitcoinabc.org/D4980
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants