This PR implements a `non_secure_erase()` method on SecretKey,
KeyPair, SharedSecret, Scalar, and DisplaySecret. The purpose
of this method is to (attempt to) overwrite secret data with
valid default values. This method can be used by libraries
to implement Zeroize on structs containing secret values.
`non_secure_erase()` attempts to avoid being optimized away or
reordered using the same mechanism as the zeroize crate: first,
using `std::ptr::write_volatile` (which will not be optimized
away) to overwrite the memory, then using a memory fence to
prevent subtle issues due to load or store reordering.
Note, however, that this method is *very unlikely* to do anything
useful on its own. Effective use involves carefully placing these
values inside non-Copy structs and pinning those structs in place.
See the [`zeroize`](https://docs.rs/zeroize) documentation for tips
and tricks, and for further discussion.
[this commit includes a squashed-in commit from tcharding to fix docs
and helpful suggestions from apoelstra and Kixunil]
e705bcffb5 Fully describe safety requirements (Tobin C. Harding)
Pull request description:
Currently we have a wildcard on safety requirements, saying more or less "plus a bunch of other stuff we don't mention". This is not helpful.
Attempt to fully describe the safety requirements of creating a context from a raw context (all, signing only, and verification only).
Fix: #544
## Note
This is best effort only, will require some thought to review. To do this I read https://doc.rust-lang.org/reference/behavior-considered-undefined.html and then I flicked through `depend/secp256k1/src/secp256k1.c` and `util.h` to look for things that could cause things in the linked to list of UB.
ACKs for top commit:
apoelstra:
ACK e705bcffb5
Kixunil:
ACK e705bcffb5
Tree-SHA512: 0180d196f6d528e3c7a06da54ef58d015df19c351d98030453ae5c5e62e0565797b06169f27f5d8b40ea0b9adba377cadd45dd306c8213d0bdc98b20651766c7
Currently we have a wildcard on safety requirements, saying more or less
"plus a bunch of other stuff we don't mention". This is not helpful.
Attempt to fully describe the safety requirements of creating a context
from a raw context (all, signing only, and verification only).
Fix: #544
Currently CI is broken because we use the latest version of `rustfmt`
and `clippy` in CI. We can resolve the `rustfmt` issue permanently by
removing the `required_version` config option. We also need to fix the
latest clippy warnings.
Done as a single patch so that all patches pass CI.
We do not need to use the `hex` module from `bitcoin_hashes` to encode
into hex, we have a function in this library.
Use library hex encoding logic, removes dependency on the `hex` module
of `bitcoin_hashes` entirely from this crate.
A recent update to clippy introduced a new class of warning.
Clippy emits:
warning: casting to the same type is unnecessary (`usize` -> `usize`)
As suggested remove the unnecessary cast.
494b07a415 Add changelog entry (Tobin C. Harding)
d0c4af0e26 Add newline after docs heading (Tobin C. Harding)
Pull request description:
~Bump version to 0.25.1 ready to release~ Add changelog entry for the recently fixed unsoundness issue.
Patch 1 is an annoyingly trivial fix to docs.
ACKs for top commit:
apoelstra:
ACK 494b07a415
Tree-SHA512: 8de8d735d3dce06683ec8e66b78b966406f42ea0a8e679e8e82143a984251addd74bea3658cc63ba9d9eada3517e461e9c28085d5261d9c0db2dceb15a8cbcc2
As is customary add a newline between rustdoc heading and content. Done
so that the code is identical to other released code (during backport
the space was added).
1e6eb6cb4d shut clippy up (Andrew Poelstra)
f961497e69 context: introduce unsafe `PreallocatedContext` trait (Andrew Poelstra)
Pull request description:
Stop this from being a generic function over all contexts, to only a function generic over contexts where we can bound the lifetime precisely. Introduces a new unsafe trait. I *believe* the only code this breaks was already unsound:
* code that tried to use one of the `*Preallocated` context markers with an incorrect lifetime
* code that tried to use `preallocated_gen_new` with a non-`*Preallocated` marker, which I believe we allowed before (I just noticed this now) and almost certainly would've led to UB on drop
Fixes#543
ACKs for top commit:
Kixunil:
ACK 1e6eb6cb4d
tcharding:
ACK 1e6eb6cb4d
Tree-SHA512: 44eb4637a2f86d5b16d40174cb9e27f37cf8eb4f29546159dbbdcd3326d01f9de2f500ba732376dd84e67ebc3528c709d2d4e2aceb8a329bcb9fb9d25c9b89cb
Fixes unsoundness in `preallocated_gen_new` which previously did not
properly constrain the lifetime of the buffer used to back the context
object. We introduce an unsafe marker trait, and impl it for our
existing preallocated-context markers.
Annoyingly the trait has to be public even though it should never be
used directly, and is only used alongside the sealed `Context` trait,
so it is de-facto sealed itself.
Fixes#543
Currently we expect non-null pointers when we take `*mut T` parameters,
however we do not check that the pointers are non-null because we never
set VERIFY in our C build. We can use the `NonNull` type to enforce
no-null-ness as long as we use `NonNull::new`. In a couple of instances
we manually check that a buffer is not empty and therefore that the
pointer to it is non-null so we can safely use `NonNull::new_unchecked`.
Replace mutable pointer parameters `*mut T` (e.g. `*mut c_void`) and
return types with `NonNull<T>`.
Fix#546
For raw pointers that can never be null Rust provides the
`core::ptr::NonNull` type. Our `Secp256k1` type has an inner field that
is a non-null pointer; use `NonNull` for it.
Fix: #534
129ba3cd58 Remove size field (Tobin C. Harding)
Pull request description:
The `Secp256k1` `size` field is a cached value that we get using `ffi::secp256k1_context_preallocated_size` or
`ffi::secp256k1_context_preallocated_clone_size`, both of which just return the result of `sizeof(rustsecp256k1_v0_6_1_context)`. Instead of caching this value we can just call
`ffi::secp256k1_context_preallocated_clone_size` in the `Drop` implementation.
Fix#537
ACKs for top commit:
apoelstra:
ACK 129ba3cd58
Tree-SHA512: 3fce7863065e4b485fd2d1fdbbfe7002fa6188f1a703d89fdda570a1a32471d298e2e33fb8c5951a56a79facb5d2b427d58e473b5cb1d68eb02ffed728392b97
The `Secp256k1` `size` field is a cached value that we get using
`ffi::secp256k1_context_preallocated_size` or
`ffi::secp256k1_context_preallocated_clone_size`, both of which just
return the result of `sizeof(rustsecp256k1_v0_6_1_context)`. Instead of
caching this value we can just call
`ffi::secp256k1_context_preallocated_clone_size` in the `Drop`
implementation.
ecdad39ef4 context: Improve rustdocs (Tobin C. Harding)
e945751d85 schnorr: Improve rustdocs (Tobin C. Harding)
47f19a78ef Use lowercase for schnorr (Tobin C. Harding)
27b3e92889 Do trivial cleanup to module level docs (Tobin C. Harding)
Pull request description:
Audit of docs in `rust-secp256k1` and do a few trivial fixes. The docs are in pretty good condition, they just need more content as described in #128 if that issue is still valid.
ACKs for top commit:
apoelstra:
ACK ecdad39ef4
Tree-SHA512: 7466090325e02331f11e34cd38625541fbe8e642882afa6ddf2cf5d11ed669c7b2b48fd5b819915392760f4c6ef4ee460c2e622b3af648f99906c3ac408045d4
513144c923 Remove serde_keypair module (Tobin C. Harding)
Pull request description:
Done while unsuccessfully trying to solve https://github.com/rust-bitcoin/rust-secp256k1/issues/514#
The `serde_keypair` module appears to be only used for testing, however it is part of the public API for the `key` module?
serde de/serialization is already implemented on `KeyPair` by way of the normal `serde` traits, there is no obvious reason for the `serde_keypair` and the `KeyPairWrapper`.
Remove the `KeyPairWrapper` and test `KeyPair` serde impls directly.
ACKs for top commit:
apoelstra:
ACK 513144c923
Tree-SHA512: 23891217f3afc7cb3bb03431946e9866ee6ae611153fca8d93fe393b5a4abbd41d4713c6aa5ab24eb2734d8c8d94a9f6aed47316284b4097aa40f49f055f36b6
6d747301e8 secp256k1: Document safety constraints (Tobin C. Harding)
85681cece7 secp256k1-sys: Document safety constraints (Tobin C. Harding)
Pull request description:
Functions that are `unsafe` should include a `# Safety` section.
- Patch 1: Documents `unsafe` methods in `secp256k1-sys`. Please not this includes a minor refactor.
- Patch 2: Documents the `unsafe` `Context` trait
Together these two patches remove `#![allow(clippy::missing_safety_doc)]` from the repo.
Fix: #447
## Note to reviewers
The only function that was curly to understand the safety of was `secp256k1_context_create`, all the other stuff should be trivial to review.
ACKs for top commit:
apoelstra:
ACK 6d747301e8
Tree-SHA512: 247216c3f9e655fe8c2854b71613b31b6241318e877e83a1e4873ce84e481975a832d05cd748577f437f88b166ff287a537d26c012568e7378caed458ec55867
The `serde_keypair` module appears to be only used for testing, however
it is part of the public API for the `key` module?
serde de/serialization is already implemented on `KeyPair` by way of the normal
`serde` traits, there is no obvious reason for the `serde_keypair`
and the `KeyPairWrapper`.
Remove the `KeyPairWrapper` and test `KeyPair` serde impls directly.
We can help readers of the HTML docs by using a link to the constant.
While we are at it shorten the doc comment so it fits on one line (in
under 100 chars).
b9eefea092 Add documentation on side channel attacks to SecretKey (Tobin C. Harding)
7cf3c6c8a4 Implement constant time comparison for SecretKey (Tobin C. Harding)
19039d9281 Remove `Ord`/`Hash` traits from SecretKey (Tobin C. Harding)
4a0c7fca6a Do not use impl_array_newtype for SecretKey (Tobin C. Harding)
Pull request description:
Add constant time comparison implementation to `SecretKey`.
This PR does as suggested [here](https://github.com/rust-bitcoin/rust-secp256k1/issues/471#issuecomment-1179783309) at the end of the issue discussion thread.
Fix: #471
ACKs for top commit:
apoelstra:
ACK b9eefea092
Tree-SHA512: 217ed101b967cc048954547bcc0b3ab09e5ccf7c58e5dcb488370caf3f5567871152505a3bfb4558e59eea4849addaf1f11e1881b6744b0c90c633fa0157a5ae
We recently added a constant time `eq` implementation however library
users can inadvertently bypass this protection if they use `AsRef`. To
help prevent this add documentation to the `SecretKey` and also to the
`AsRef` implementation.
The current trait implementations of `Ord` and `PartialOrd` for
`SecretKey` leak data when doing the comparison i.e., they are not
constant time. Since there is no real usecase for ordering secret keys
remove the trait implementations all together.
Remove `Hash` at the same time because it does not make sense to
implement it if `Ord`/`PartialOrd` are not implemented.
In preparation for changing the logic of comparison trait (Ord, Eq)
implementations on the `SecretKey` copy all the code out of
`impl_array_newtype` and implement it directly in `key.rs`.
Refactor only, no logic changes (although I removed a few unneeded
references).
Currently we are not running the formatter in CI, in preparation for
doing so run `cargo +nightly fmt` to clear all current formatting
issues.
No manual changes.
9850550734 Move AsRef impl block next to Index (Tobin C. Harding)
4d42e8e906 Derive Copy and Clone (Tobin C. Harding)
b38ae97eaf Implement stable comparison functionality (Tobin C. Harding)
630fc1fcb6 Remove len and is_empty from impl_array_newtype macros (Tobin C. Harding)
9788b6df88 Remove leading colons from impl_array_newtype methods (Tobin C. Harding)
2bb08c21e5 Remove as_[mut_]ptr from impl_array_newtype macros (Tobin C. Harding)
3e28070187 Duplicate impl_array_newtype (Tobin C. Harding)
635890322a Add newline to end of file (Tobin C. Harding)
Pull request description:
Supersedes #515
The primary aim of this PR is to fix the fact that we currently implement various comparison traits (`Ord`, `PartialEq`) by comparing the inner byte arrays. These bytes are meant to be opaque and are not guaranteed across versions of `libsecp256k1`.
This PR is a bit involved because I had to detangle all the various types (across both `secp256k1` and `secp256k1-sys`) that use the `impl_array_newtype` macro.
- Patch 1: is trivial cleanup
- Patch 2: Step one of the PR is duplicating the macro into both crates so we can tease them apart.
- Patch 3-5: Are cleanup of the now duplicated `impl_array_newtype` macros
- Patch 6: Is the meat and potatoes
- Patch 7,8: Further minor clean ups to the macro
I had a lot of fun with this PR, I hope you enjoy it too.
Fix: #463
ACKs for top commit:
apoelstra:
ACK 9850550734
Tree-SHA512: 160381e53972ff882ceb1d2d47bac56a7301a2d13bfe75d3f6ff658ab2c6fbe516ad856587c4d23f98524205918ca1a5f9b737e35c23c7a01b476c92d8d1792f
There is no obvious reason why not to derive `Copy` and `Clone` for
types that use the `impl_newtype_macro`. Derives are less surprising so
deriving makes the code marginally easier to read.
Currently we rely on the inner bytes with types that are passed across
the FFI boundry when implementing comparison functions (e.g. `Ord`,
`PartialEq`), this is incorrect because the bytes are opaque, meaning
the byte layout is not guaranteed across versions of `libsecp26k1`.
Implement stable comparison functionality by doing:
- Implement `core::cmp` traits by first coercing the data into a stable
form e.g., by serializing it.
- Add fast comparison methods to `secp256k1-sys` types that wrap types
from libsecp, add similar methods to types in `secp256k1` that wrap
`secp256k1-sys` types (just call through to inner type).
- In `secp256k1-sys` feature gate the new `core::cmp` impls on
`not(fuzzing)`, when fuzzing just derive the impls instead.
Any additional methods added to `secp256k1-sys` types are private,
justified by the fact the -sys is meant to be just a thin wrapper around
libsecp256k1, we don't want to commit to supporting additional API
functions.
Please note, the solution presented in this patch is already present for
`secp256k1::PublicKey`, this PR removes that code in favour of deriving
traits that then call down to the same logic in `secp256k1-sys`.
Currently we have a few problems with our feature gating, attempt to
audit all feature gating and fix it by doing:
1. Do not enable features on optional dependencies (`rand` and
`bitcoin-hashes`) in dev-dependencies, doing so hides broken feature
gating in unit tests.
2. Do not use unnecessary feature combinations when one feature enables
another e.g. `any(feature = "std", feature = "alloc")`.
3. Enable "std" from "rand-std" and "bitcoin-std" (and fix features
gating as for point 2).
4. Clean up code around `rand::thread_rng`, this is part of this patch
because `thread_rng` requires the "rand-std" feature.
5. Clean up CI test script to test each feature individually now that
"rand-std" and "bitcoin-hashes-std" enable "std".
An array in Rust has no concept of length, it is a fixed size data type.
Equally an array cannot be "empty", again since it is a fixed size data
type. These are methods/concepts seen in slices and vectors.
Remove the `len` and `is_empty` methods.
Thomas DuBuisson
Tobin C. Harding
kwantam
Andrew Poelstra
Arik Sosman
Elichai Turkel