From d04989ef30ad0b60adcd66f03ce39967b2ac2658 Mon Sep 17 00:00:00 2001 From: ryan Date: Fri, 3 May 2024 23:10:34 -0400 Subject: [PATCH] keyfork-derive-util: make key parsing fallible again, since secp256k1 isn't guaranteed correct --- crates/daemon/keyforkd-client/src/lib.rs | 13 +++-- .../src/extended_key/private_key.rs | 58 ++++++++++++------- .../keyfork-derive-util/src/private_key.rs | 20 +++---- .../keyfork-derive-util/src/public_key.rs | 4 +- .../derive/keyfork-derive-util/src/request.rs | 22 +++---- .../derive/keyfork-derive-util/src/tests.rs | 8 +-- crates/keyfork-shard/src/openpgp.rs | 1 + crates/keyfork/src/cli/wizard.rs | 19 ++++-- 8 files changed, 82 insertions(+), 63 deletions(-) diff --git a/crates/daemon/keyforkd-client/src/lib.rs b/crates/daemon/keyforkd-client/src/lib.rs index d1fce59..df6163f 100644 --- a/crates/daemon/keyforkd-client/src/lib.rs +++ b/crates/daemon/keyforkd-client/src/lib.rs @@ -15,7 +15,7 @@ //! * Derive Key //! //! ## Extended Private Keys -//! +//! //! Keyfork doesn't need to be continuously called once a key has been derived. Once an Extended //! Private Key (often shortened to "XPrv") has been created, further derivations can be performed. //! The tests for this library ensure that all levels of Keyfork derivation beyond the required two @@ -108,6 +108,10 @@ pub enum Error { /// An error encountered in Keyforkd. #[error("Error in Keyforkd: {0}")] Keyforkd(#[from] KeyforkdError), + + /// An invalid key was returned. + #[error("Invalid key returned")] + InvalidKey, } #[allow(missing_docs)] @@ -234,11 +238,8 @@ impl Client { } let depth = path.len() as u8; - Ok(ExtendedPrivateKey::from_parts( - &d.data, - depth, - d.chain_code, - )) + ExtendedPrivateKey::from_parts(&d.data, depth, d.chain_code) + .map_err(|_| Error::InvalidKey) } _ => Err(Error::InvalidResponse), } diff --git a/crates/derive/keyfork-derive-util/src/extended_key/private_key.rs b/crates/derive/keyfork-derive-util/src/extended_key/private_key.rs index cd6a147..bfd8ca1 100644 --- a/crates/derive/keyfork-derive-util/src/extended_key/private_key.rs +++ b/crates/derive/keyfork-derive-util/src/extended_key/private_key.rs @@ -27,6 +27,10 @@ pub enum Error { /// The given slice was of an inappropriate size to create a Private Key. #[error("The given slice was of an inappropriate size to create a Private Key")] InvalidSliceError(#[from] std::array::TryFromSliceError), + + /// The given data was not a valid key for the chosen key type. + #[error("The given data was not a valid key for the chosen key type")] + InvalidKey, } type Result = std::result::Result; @@ -127,7 +131,7 @@ mod serde_with { let bytes: [u8; 32] = variable_len_bytes.try_into().expect(bug!( "unable to parse serialized private key; no support for static len" )); - Ok(K::from_bytes(&bytes)) + Ok(K::from_bytes(&bytes).expect(bug!("could not deserialize key with invalid scalar"))) } } @@ -148,13 +152,9 @@ where /// Generate a new [`ExtendedPrivateKey`] from a seed, ideally from a 12-word or 24-word /// mnemonic, but may take 16-byte seeds. /// - /// # Panics - /// The method performs unchecked `try_into()` operations on a constant-sized slice. - /// /// # Errors - /// An error may be returned if: - /// * The given seed had an incorrect length. - /// * A `HmacSha512` can't be constructed. + /// The function may return an error if the derived master key could not be parsed as a key for + /// the given algorithm (such as exceeding values on the secp256k1 curve). /// /// # Examples /// ```rust @@ -167,11 +167,11 @@ where /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; /// let xprv = ExtendedPrivateKey::::new(*seed); /// ``` - pub fn new(seed: impl as_private_key::AsPrivateKey) -> Self { + pub fn new(seed: impl as_private_key::AsPrivateKey) -> Result { Self::new_internal(seed.as_private_key()) } - fn new_internal(seed: &[u8]) -> Self { + fn new_internal(seed: &[u8]) -> Result { let hash = HmacSha512::new_from_slice(&K::key().bytes().collect::>()) .expect(bug!("HmacSha512 InvalidLength should be infallible")) .chain_update(seed) @@ -214,11 +214,16 @@ where /// # b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"; /// let xprv = ExtendedPrivateKey::::from_parts(key, 4, *chain_code); /// ``` - pub fn from_parts(key: &[u8; 32], depth: u8, chain_code: [u8; 32]) -> Self { - Self { - private_key: K::from_bytes(key), - depth, - chain_code, + pub fn from_parts(key: &[u8; 32], depth: u8, chain_code: [u8; 32]) -> Result { + match K::from_bytes(key) { + Ok(key) => { + Ok(Self { + private_key: key, + depth, + chain_code, + }) + } + Err(_) => Err(Error::InvalidKey), } } @@ -232,12 +237,15 @@ where /// # public_key::TestPublicKey as PublicKey, /// # private_key::TestPrivateKey as PrivateKey, /// # }; + /// # fn main() -> Result<(), Box> { /// let key: &[u8; 32] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; /// let chain_code: &[u8; 32] = // /// # b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"; - /// let xprv = ExtendedPrivateKey::::from_parts(key, 4, *chain_code); - /// assert_eq!(xprv.private_key(), &PrivateKey::from_bytes(key)); + /// let xprv = ExtendedPrivateKey::::from_parts(key, 4, *chain_code)?; + /// assert_eq!(xprv.private_key(), &PrivateKey::from_bytes(key)?); + /// # Ok(()) + /// # } /// ``` pub fn private_key(&self) -> &K { &self.private_key @@ -262,7 +270,7 @@ where /// # 102, 201, 210, 159, 219, 222, 42, 201, 44, 196, 27, /// # 90, 221, 80, 85, 135, 79, 39, 253, 223, 35, 251 /// # ]; - /// let xprv = ExtendedPrivateKey::::new(*seed); + /// let xprv = ExtendedPrivateKey::::new(*seed)?; /// let xpub = xprv.extended_public_key(); /// assert_eq!(known_key, xpub.public_key().to_bytes()); /// # Ok(()) @@ -286,7 +294,7 @@ where /// # fn main() -> Result<(), Box> { /// let seed: &[u8; 64] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - /// let xprv = ExtendedPrivateKey::::new(*seed); + /// let xprv = ExtendedPrivateKey::::new(*seed)?; /// let pubkey = xprv.public_key(); /// # Ok(()) /// # } @@ -304,12 +312,15 @@ where /// # public_key::TestPublicKey as PublicKey, /// # private_key::TestPrivateKey as PrivateKey, /// # }; + /// # fn main() -> Result<(), Box> { /// let key: &[u8; 32] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; /// let chain_code: &[u8; 32] = // /// # b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"; - /// let xprv = ExtendedPrivateKey::::from_parts(key, 4, *chain_code); + /// let xprv = ExtendedPrivateKey::::from_parts(key, 4, *chain_code)?; /// assert_eq!(xprv.depth(), 4); + /// # Ok(()) + /// # } /// ``` pub fn depth(&self) -> u8 { self.depth @@ -324,12 +335,15 @@ where /// # public_key::TestPublicKey as PublicKey, /// # private_key::TestPrivateKey as PrivateKey, /// # }; + /// # fn main() -> Result<(), Box> { /// let key: &[u8; 32] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; /// let chain_code: &[u8; 32] = // /// # b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB"; - /// let xprv = ExtendedPrivateKey::::from_parts(key, 4, *chain_code); + /// let xprv = ExtendedPrivateKey::::from_parts(key, 4, *chain_code)?; /// assert_eq!(chain_code, &xprv.chain_code()); + /// # Ok(()) + /// # } /// ``` pub fn chain_code(&self) -> [u8; 32] { self.chain_code @@ -351,7 +365,7 @@ where /// # fn main() -> Result<(), Box> { /// let seed: &[u8; 64] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - /// let root_xprv = ExtendedPrivateKey::::new(*seed); + /// let root_xprv = ExtendedPrivateKey::::new(*seed)?; /// let path = DerivationPath::default() /// .chain_push(DerivationIndex::new(44, true)?) /// .chain_push(DerivationIndex::new(0, true)?) @@ -397,7 +411,7 @@ where /// # fn main() -> Result<(), Box> { /// let seed: &[u8; 64] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - /// let root_xprv = ExtendedPrivateKey::::new(*seed); + /// let root_xprv = ExtendedPrivateKey::::new(*seed)?; /// let bip44_wallet = DerivationPath::default() /// .chain_push(DerivationIndex::new(44, true)?) /// .chain_push(DerivationIndex::new(0, true)?) diff --git a/crates/derive/keyfork-derive-util/src/private_key.rs b/crates/derive/keyfork-derive-util/src/private_key.rs index 2a600d3..fee389f 100644 --- a/crates/derive/keyfork-derive-util/src/private_key.rs +++ b/crates/derive/keyfork-derive-util/src/private_key.rs @@ -2,8 +2,6 @@ use crate::PublicKey; use thiserror::Error; -use keyfork_bug::bug; - pub(crate) type PrivateKeyBytes = [u8; 32]; /// Functions required to use an `ExtendedPrivateKey`. @@ -26,7 +24,7 @@ pub trait PrivateKey: Sized { /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; /// let private_key = OurPrivateKey::from_bytes(key_data); /// ``` - fn from_bytes(b: &PrivateKeyBytes) -> Self; + fn from_bytes(b: &PrivateKeyBytes) -> Result; /// Convert a &Self to bytes. /// @@ -38,7 +36,7 @@ pub trait PrivateKey: Sized { /// # }; /// let key_data: &[u8; 32] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - /// let private_key = OurPrivateKey::from_bytes(key_data); + /// let private_key = OurPrivateKey::from_bytes(key_data).unwrap(); /// assert_eq!(key_data, &private_key.to_bytes()); /// ``` fn to_bytes(&self) -> PrivateKeyBytes; @@ -73,7 +71,7 @@ pub trait PrivateKey: Sized { /// # }; /// let key_data: &[u8; 32] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - /// let private_key = OurPrivateKey::from_bytes(key_data); + /// let private_key = OurPrivateKey::from_bytes(key_data).unwrap(); /// let public_key = private_key.public_key(); /// ``` fn public_key(&self) -> Self::PublicKey; @@ -120,8 +118,8 @@ impl PrivateKey for k256::SecretKey { "Bitcoin seed" } - fn from_bytes(b: &PrivateKeyBytes) -> Self { - Self::from_slice(b).expect(bug!("Invalid private key bytes")) + fn from_bytes(b: &PrivateKeyBytes) -> Result { + Self::from_slice(b).map_err(|_| PrivateKeyError::InvalidScalar) } fn to_bytes(&self) -> PrivateKeyBytes { @@ -159,8 +157,8 @@ impl PrivateKey for ed25519_dalek::SigningKey { "ed25519 seed" } - fn from_bytes(b: &PrivateKeyBytes) -> Self { - Self::from_bytes(b) + fn from_bytes(b: &PrivateKeyBytes) -> Result { + Ok(Self::from_bytes(b)) } fn to_bytes(&self) -> PrivateKeyBytes { @@ -204,8 +202,8 @@ impl PrivateKey for TestPrivateKey { type PublicKey = TestPublicKey; type Err = PrivateKeyError; - fn from_bytes(b: &PrivateKeyBytes) -> Self { - Self { key: *b } + fn from_bytes(b: &PrivateKeyBytes) -> Result { + Ok(Self { key: *b }) } fn to_bytes(&self) -> PrivateKeyBytes { diff --git a/crates/derive/keyfork-derive-util/src/public_key.rs b/crates/derive/keyfork-derive-util/src/public_key.rs index ef89244..192fe70 100644 --- a/crates/derive/keyfork-derive-util/src/public_key.rs +++ b/crates/derive/keyfork-derive-util/src/public_key.rs @@ -30,7 +30,7 @@ pub trait PublicKey: Sized { /// # }; /// let key_data: &[u8; 32] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - /// let private_key = OurPrivateKey::from_bytes(key_data); + /// let private_key = OurPrivateKey::from_bytes(key_data).unwrap(); /// let public_key_bytes = private_key.public_key().to_bytes(); /// ``` fn to_bytes(&self) -> PublicKeyBytes; @@ -56,7 +56,7 @@ pub trait PublicKey: Sized { /// # }; /// let key_data: &[u8; 32] = // /// # b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA"; - /// let private_key = OurPrivateKey::from_bytes(key_data); + /// let private_key = OurPrivateKey::from_bytes(key_data).unwrap(); /// let fingerprint = private_key.public_key().fingerprint(); /// ``` fn fingerprint(&self) -> [u8; 4] { diff --git a/crates/derive/keyfork-derive-util/src/request.rs b/crates/derive/keyfork-derive-util/src/request.rs index 94af23b..cec0b54 100644 --- a/crates/derive/keyfork-derive-util/src/request.rs +++ b/crates/derive/keyfork-derive-util/src/request.rs @@ -70,7 +70,7 @@ impl DerivationAlgorithm { match self { #[cfg(feature = "ed25519")] Self::Ed25519 => { - let key = ExtendedPrivateKey::::new(seed); + let key = ExtendedPrivateKey::::new(seed)?; let derived_key = key.derive_path(path)?; Ok(DerivationResponse::with_algo_and_xprv( self.clone(), @@ -79,7 +79,7 @@ impl DerivationAlgorithm { } #[cfg(feature = "secp256k1")] Self::Secp256k1 => { - let key = ExtendedPrivateKey::::new(seed); + let key = ExtendedPrivateKey::::new(seed)?; let derived_key = key.derive_path(path)?; Ok(DerivationResponse::with_algo_and_xprv( self.clone(), @@ -87,7 +87,7 @@ impl DerivationAlgorithm { )) } Self::TestAlgorithm => { - let key = ExtendedPrivateKey::::new(seed); + let key = ExtendedPrivateKey::::new(seed)?; let derived_key = key.derive_path(path)?; Ok(DerivationResponse::with_algo_and_xprv( self.clone(), @@ -300,11 +300,9 @@ mod secp256k1 { fn try_from(value: &DerivationResponse) -> Result { match value.algorithm { - DerivationAlgorithm::Secp256k1 => Ok(Self::from_parts( - &value.data, - value.depth, - value.chain_code, - )), + DerivationAlgorithm::Secp256k1 => { + Self::from_parts(&value.data, value.depth, value.chain_code).map_err(Into::into) + } _ => Err(Self::Error::Algorithm), } } @@ -335,11 +333,9 @@ mod ed25519 { fn try_from(value: &DerivationResponse) -> Result { match value.algorithm { - DerivationAlgorithm::Ed25519 => Ok(Self::from_parts( - &value.data, - value.depth, - value.chain_code, - )), + DerivationAlgorithm::Ed25519 => { + Self::from_parts(&value.data, value.depth, value.chain_code).map_err(Into::into) + } _ => Err(Self::Error::Algorithm), } } diff --git a/crates/derive/keyfork-derive-util/src/tests.rs b/crates/derive/keyfork-derive-util/src/tests.rs index 4872c8c..f1804a0 100644 --- a/crates/derive/keyfork-derive-util/src/tests.rs +++ b/crates/derive/keyfork-derive-util/src/tests.rs @@ -31,7 +31,7 @@ fn secp256k1() { // Tests for ExtendedPrivateKey let varlen_seed = VariableLengthSeed::new(seed); - let xkey = ExtendedPrivateKey::::new(varlen_seed); + let xkey = ExtendedPrivateKey::::new(varlen_seed).unwrap(); let derived_key = xkey.derive_path(&chain).unwrap(); assert_eq!( derived_key.chain_code().as_slice(), @@ -77,7 +77,7 @@ fn ed25519() { // Tests for ExtendedPrivateKey let varlen_seed = VariableLengthSeed::new(seed); - let xkey = ExtendedPrivateKey::::new(varlen_seed); + let xkey = ExtendedPrivateKey::::new(varlen_seed).unwrap(); let derived_key = xkey.derive_path(&chain).unwrap(); assert_eq!( derived_key.chain_code().as_slice(), @@ -110,7 +110,7 @@ fn panics_with_unhardened_derivation() { use ed25519_dalek::SigningKey; let seed = hex!("000102030405060708090a0b0c0d0e0f"); - let xkey = ExtendedPrivateKey::::new(seed); + let xkey = ExtendedPrivateKey::::new(seed).unwrap(); xkey.derive_path(&DerivationPath::from_str("m/0").unwrap()) .unwrap(); } @@ -122,7 +122,7 @@ fn panics_at_depth() { use ed25519_dalek::SigningKey; let seed = hex!("000102030405060708090a0b0c0d0e0f"); - let mut xkey = ExtendedPrivateKey::::new(seed); + let mut xkey = ExtendedPrivateKey::::new(seed).unwrap(); for i in 0..=u32::from(u8::MAX) { xkey = xkey .derive_child(&DerivationIndex::new(i, true).unwrap()) diff --git a/crates/keyfork-shard/src/openpgp.rs b/crates/keyfork-shard/src/openpgp.rs index 9260ea2..be4ec47 100644 --- a/crates/keyfork-shard/src/openpgp.rs +++ b/crates/keyfork-shard/src/openpgp.rs @@ -259,6 +259,7 @@ impl Format for OpenPGP

{ let userid = UserID::from("keyfork-sss"); let path = DerivationPath::from_str("m/7366512'/0'").expect(bug!("valid derivation path")); let xprv = XPrv::new(seed) + .expect(bug!("could not create XPrv from key")) .derive_path(&path) .expect(bug!("valid derivation")); keyfork_derive_openpgp::derive( diff --git a/crates/keyfork/src/cli/wizard.rs b/crates/keyfork/src/cli/wizard.rs index ef5d9f3..1dcdf19 100644 --- a/crates/keyfork/src/cli/wizard.rs +++ b/crates/keyfork/src/cli/wizard.rs @@ -11,11 +11,12 @@ use keyfork_derive_openpgp::{ }; use keyfork_derive_util::{DerivationIndex, DerivationPath}; use keyfork_prompt::{ + default_terminal, validators::{SecurePinValidator, Validator}, - Message, PromptHandler, DefaultTerminal, default_terminal + DefaultTerminal, Message, PromptHandler, }; -use keyfork_shard::{Format, openpgp::OpenPGP}; +use keyfork_shard::{openpgp::OpenPGP, Format}; #[derive(thiserror::Error, Debug)] #[error("Invalid PIN length: {0}")] @@ -44,7 +45,9 @@ fn derive_key(seed: [u8; 32], index: u8) -> Result { .chain_push(chain) .chain_push(account) .chain_push(subkey); - let xprv = XPrv::new(seed).derive_path(&path)?; + let xprv = XPrv::new(seed) + .expect("could not construct master key from seed") + .derive_path(&path)?; let userid = UserID::from(format!("Keyfork Shard {index}")); let cert = keyfork_derive_openpgp::derive(xprv, &subkeys, &userid)?; Ok(cert) @@ -104,7 +107,7 @@ fn generate_shard_secret( keys_per_shard: u8, output_file: &Option, ) -> Result<()> { - let seed = keyfork_entropy::generate_entropy_of_const_size::<{256 / 8}>()?; + let seed = keyfork_entropy::generate_entropy_of_const_size::<{ 256 / 8 }>()?; let mut pm = default_terminal()?; let mut certs = vec![]; let mut seen_cards: HashSet = HashSet::new(); @@ -171,7 +174,13 @@ fn generate_shard_secret( let output = File::create(output_file)?; opgp.shard_and_encrypt(threshold, certs.len() as u8, &seed, &certs[..], output)?; } else { - opgp.shard_and_encrypt(threshold, certs.len() as u8, &seed, &certs[..], std::io::stdout())?; + opgp.shard_and_encrypt( + threshold, + certs.len() as u8, + &seed, + &certs[..], + std::io::stdout(), + )?; } Ok(()) }