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

Add Zeroize to private key #1770

Merged
merged 6 commits into from
Apr 17, 2024
Merged

Add Zeroize to private key #1770

merged 6 commits into from
Apr 17, 2024

Conversation

mstrug-rdx
Copy link
Contributor

Summary

Added derive of Zeroize and ZeroizeOnDrop to Ed25519PrivateKey. This is required by Sargon project (will be used thorught RET).

Copy link

github-actions bot commented Apr 12, 2024

Docker tags
docker.io/radixdlt/private-scrypto-builder:8ab6d83af1

Copy link

github-actions bot commented Apr 12, 2024

Benchmark for 8ab6d83

Click to view benchmark
Test Base PR %
costing::bench_prepare_wasm 66.0±0.38ms 65.7±0.37ms -0.45%
costing::decode_sbor 10.9±0.03µs 10.9±0.02µs 0.00%
costing::decode_sbor_bytes 29.5±0.11µs 30.7±0.03µs +4.07%
costing::deserialize_wasm 1303.2±6.06µs 1292.3±4.16µs -0.84%
costing::instantiate_flash_loan 4.2±0.85ms 4.2±0.87ms 0.00%
costing::instantiate_radiswap 5.7±0.07ms 5.8±0.06ms +1.75%
costing::spin_loop 21.7±0.06ms 21.7±0.06ms 0.00%
costing::validate_sbor_payload 30.1±0.07µs 29.8±0.06µs -1.00%
costing::validate_sbor_payload_bytes 250.6±1.40ns 248.9±0.71ns -0.68%
costing::validate_secp256k1 76.8±0.18µs 76.7±0.73µs -0.13%
costing::validate_wasm 37.4±0.09ms 36.6±0.05ms -2.14%
decimal::add/0 8.4±0.00ns 8.4±0.00ns 0.00%
decimal::add/rust-native 9.9±0.01ns 9.9±0.02ns 0.00%
decimal::add/wasmer 111.2±0.15ns 114.0±0.23ns +2.52%
decimal::add/wasmer-call-native 449.8±0.40ns 454.8±1.01ns +1.11%
decimal::add/wasmi 719.4±3.28ns 628.7±4.66ns -12.61%
decimal::add/wasmi-call-native 5.6±0.02µs 5.0±0.01µs -10.71%
decimal::div/0 190.0±0.11ns 191.2±0.34ns +0.63%
decimal::from_string/0 159.0±0.43ns 155.9±0.56ns -1.95%
decimal::mul/0 142.1±0.10ns 142.0±0.25ns -0.07%
decimal::mul/rust-native 137.3±0.37ns 137.8±0.07ns +0.36%
decimal::mul/wasmer 1501.9±1.97ns 1506.1±1.68ns +0.28%
decimal::mul/wasmer-call-native 576.7±0.64ns 587.1±1.11ns +1.80%
decimal::mul/wasmi 42.9±0.16µs 42.0±0.23µs -2.10%
decimal::mul/wasmi-call-native 5.7±0.03µs 5.2±0.03µs -8.77%
decimal::pow/0 652.6±0.51ns 653.8±1.10ns +0.18%
decimal::pow/rust-native 632.0±0.51ns 629.8±0.59ns -0.35%
decimal::pow/wasmer 6.6±0.01µs 6.6±0.00µs 0.00%
decimal::pow/wasmer-call-native 1028.2±0.40ns 1042.6±3.06ns +1.40%
decimal::pow/wasmi 200.3±0.29µs 195.6±0.40µs -2.35%
decimal::pow/wasmi-call-native 5.5±0.01µs 5.1±0.01µs -7.27%
decimal::root/0 7.7±0.01µs 8.0±0.00µs +3.90%
decimal::sub/0 8.5±0.01ns 8.5±0.01ns 0.00%
decimal::to_string/0 441.3±0.96ns 447.6±1.07ns +1.43%
precise_decimal::add/0 10.1±0.02ns 10.0±0.01ns -0.99%
precise_decimal::add/rust-native 11.5±0.02ns 11.5±0.00ns 0.00%
precise_decimal::add/wasmer 118.9±0.10ns 121.1±0.67ns +1.85%
precise_decimal::add/wasmer-call-native 502.9±0.57ns 483.7±0.53ns -3.82%
precise_decimal::add/wasmi 929.9±2.73ns 796.6±1.44ns -14.33%
precise_decimal::add/wasmi-call-native 7.4±0.01µs 6.7±0.02µs -9.46%
precise_decimal::div/0 301.8±0.62ns 300.2±0.34ns -0.53%
precise_decimal::from_string/0 196.8±0.51ns 198.3±0.25ns +0.76%
precise_decimal::mul/0 343.4±0.36ns 342.3±0.77ns -0.32%
precise_decimal::mul/rust-native 301.6±0.26ns 301.8±0.28ns +0.07%
precise_decimal::mul/wasmer 3.4±0.00µs 3.5±0.00µs +2.94%
precise_decimal::mul/wasmer-call-native 832.8±1.29ns 808.6±2.12ns -2.91%
precise_decimal::mul/wasmi 109.4±0.57µs 106.1±0.21µs -3.02%
precise_decimal::mul/wasmi-call-native 7.9±0.04µs 7.1±0.05µs -10.13%
precise_decimal::pow/0 1893.9±11.87ns 1857.3±4.18ns -1.93%
precise_decimal::pow/rust-native 1491.0±4.95ns 1482.4±2.42ns -0.58%
precise_decimal::pow/wasmer 16.2±0.02µs 16.1±0.01µs -0.62%
precise_decimal::pow/wasmer-call-native 2.1±0.00µs 2.1±0.00µs 0.00%
precise_decimal::pow/wasmi 529.8±1.74µs 518.0±3.02µs -2.23%
precise_decimal::pow/wasmi-call-native 13.6±0.07µs 13.1±0.05µs -3.68%
precise_decimal::root/0 57.0±0.09µs 58.0±0.02µs +1.75%
precise_decimal::sub/0 9.6±0.03ns 9.6±0.01ns 0.00%
precise_decimal::to_string/0 762.0±4.20ns 718.8±1.21ns -5.67%
schema::validate_payload 344.9±1.26µs 341.8±0.66µs -0.90%
transaction::radiswap 5.6±0.02ms 5.4±0.02ms -3.57%
transaction::transfer 1795.7±12.18µs 1816.9±4.19µs +1.18%
transaction_processing::prepare 2.2±0.00ms 2.3±0.00ms +4.55%
transaction_processing::prepare_and_decompile 6.1±0.01ms 6.1±0.03ms 0.00%
transaction_processing::prepare_and_decompile_and_recompile 24.4±0.48ms 23.8±0.28ms -2.46%
transaction_validation::validate_manifest 42.3±0.07µs 42.3±0.05µs 0.00%
transaction_validation::verify_bls_2KB 1061.8±21.72µs 1008.2±16.68µs -5.05%
transaction_validation::verify_bls_32B 1003.4±10.79µs 1009.0±24.01µs +0.56%
transaction_validation::verify_ecdsa 74.5±0.05µs 74.6±0.20µs +0.13%
transaction_validation::verify_ed25519 55.1±0.07µs 54.6±0.06µs -0.91%

Copy link

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

Thx! But I need it on Secp256k1PrivateKey as well :)

@mstrug-rdx
Copy link
Contributor Author

Thx! But I need it on Secp256k1PrivateKey as well :)

@CyonAlexRDX Secp256k1PrivateKey internally uses SecretKey from secp256k1 library, to derive Zeroize we need to mark DefaultIsZeroes trait for SecretKey which cannot be empty by default (cannot contains 32 count of 0) because it fails to create through from_slice() constructor. Alternatively we could use new() constructor but this will not be initalized to 0's but some random number. So I am not sure if we can achieve for secp256k1 what you need.
Also there is couple threads on secp256k1 github where the project refuse to implement Zeroize: rust-bitcoin/rust-secp256k1#553

@mstrug-rdx
Copy link
Contributor Author

@CyonAlexRDX I did some implementation which uses SecretKey::non_secure_erase() as default value but it fills slice with 1's.

@CyonAlexRDX
Copy link

@CyonAlexRDX I did some implementation which uses SecretKey::non_secure_erase() as default value but it fills slice with 1's.

All ones is not a valid Secp256k1PrivateKey. It is larger than the order of the curve. So best replace the key with the key 31 zeroes followed by 1, i.e the key 0d1

Copy link

@CyonAlexRDX CyonAlexRDX left a comment

Choose a reason for hiding this comment

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

LGTM!

Maybe add a unit tests that calls zeroize() on a non 1 private key and assert that it becomes SecretKeyWrapper::default()?

@mstrug-rdx mstrug-rdx merged commit 94bf833 into develop Apr 17, 2024
28 checks passed
@mstrug-rdx mstrug-rdx deleted the feature/zeroize-private-key branch April 17, 2024 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants