Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Replace libsecp256k1 with secp256k1 #10798

Merged
paritytech-processbot merged 21 commits into from
Feb 26, 2022
Merged

Replace libsecp256k1 with secp256k1 #10798

Changes from 1 commit
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
fd1fbff
Replace libsecp256k1 with secp256k1
davxy Feb 4, 2022
e200a83
Wipe ecdsa secret key from memory on drop
davxy Feb 4, 2022
0ec1383
Some comments for a known issue
davxy Feb 4, 2022
7165034
Safer core crypto primitives `from_slice` constructor
davxy Feb 7, 2022
9e79db3
Unit test fix
davxy Feb 7, 2022
7a8c4fd
Enable use of global secp256k1 context
davxy Feb 7, 2022
6b66969
Better comments for ecdsa `Pair` drop
davxy Feb 8, 2022
d318328
Replace `libsecp256k1` with `seco256k1` in `beefy-mmr`
davxy Feb 8, 2022
d0f2b0a
Replace `libsecp256k1` with `secp256k1` in FRAME `contracts`benchmarks
davxy Feb 9, 2022
712cf19
Temporary rollback of `beefy-mmr` to libsecp256k1
davxy Feb 10, 2022
be0ea83
Cargo fmt
davxy Feb 10, 2022
f4d525b
Merge branch 'master' into davxy-replace-libsecp256k1-with-secp256k1
davxy Feb 10, 2022
1348472
Rollback of FRAME `contracts` benchmarks to `libsecp256k1`
davxy Feb 10, 2022
548e8b6
Rollback for unrelated changes
davxy Feb 14, 2022
8e23106
Typo fix
davxy Feb 14, 2022
0ec0bd2
Add comments for deprecated `ecdsa_verify` and `secp256k1_ecdsa_recover`
davxy Feb 15, 2022
d3a5ec9
Merge branch 'master' into davxy-replace-libsecp256k1-with-secp256k1
davxy Feb 16, 2022
35549b7
Merge branch 'master' into davxy-replace-libsecp256k1-with-secp256k1
davxy Feb 21, 2022
45ee0d7
Merge branch 'master' into davxy-replace-libsecp256k1-with-secp256k1
davxy Feb 22, 2022
c711f82
Merge branch 'master' into davxy-replace-libsecp256k1-with-secp256k1
davxy Feb 23, 2022
5442124
Merge branch 'master' into davxy-replace-libsecp256k1-with-secp256k1
davxy Feb 26, 2022
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
Prev Previous commit
Next Next commit
Wipe ecdsa secret key from memory on drop
  • Loading branch information
@davxy
davxy committed Feb 4, 2022
commit e200a83fe9394a82d0dd52d64c1ab8d8c6e186b5
13 changes: 13 additions & 0 deletions primitives/core/src/ecdsa.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -549,6 +549,19 @@ impl Pair {
}
}

// The `secp256k1` backend doesn't implement cleanup for their private keys.
// Currently we should take care of wiping the secret from memory.
impl Drop for Pair {
fn drop(&mut self) {
let ptr = self.secret.as_mut_ptr();
for off in 0..self.secret.len() {
unsafe {
core::ptr::write_volatile(ptr.add(off), 0);
}
}
Comment on lines +550 to +554
Copy link
Contributor

@wigy-opensource-developer wigy-opensource-developer Feb 23, 2022

Choose a reason for hiding this comment

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

Nice touch, but https://doc.rust-lang.org/core/ptr/fn.write_bytes.html might perform better on some platforms and is also more expressive. Also, have you considered Seed is also an important secret?

Copy link

@tarcieri tarcieri Feb 23, 2022

Choose a reason for hiding this comment

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

It looks like zeroize is already a dependency. You could use it here:

Suggested change
for off in 0..self.secret.len() {
unsafe {
core::ptr::write_volatile(ptr.add(off), 0);
}
}
use zeroize::Zeroize;
self.secret.zeroize();

Copy link
Member Author

@davxy davxy Feb 23, 2022

Choose a reason for hiding this comment

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

Unfortunately the secp256k1::SecretKey doesn't implement DefaultIsZeroes nor Zeroize traits :-/

Copy link
Member Author

@davxy davxy Feb 23, 2022

Choose a reason for hiding this comment

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

Nice touch, but https://doc.rust-lang.org/core/ptr/fn.write_bytes.html might perform better on some platforms and is also more expressive. Also, have you considered Seed is also an important secret?

write_volatile is less performant but it is guaranteed to not be elided or reordered by the compiler especially with optimizations enabled

Copy link

@Slesarew Slesarew Feb 28, 2022

Choose a reason for hiding this comment

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

Is there a test for this cleanup anywhere?

}
}

impl CryptoType for Public {
#[cfg(feature = "full_crypto")]
type Pair = Pair;
Expand Down