keyfork-derive-util: add note about potential side-channel when verifying keys

crates/daemon/keyforkd-client/src/lib.rs

@@ -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(
+                ExtendedPrivateKey::from_parts(&d.data, depth, d.chain_code)
-                    &d.data,
+                    .map_err(|_| Error::InvalidKey)
-                    depth,
-                    d.chain_code,
-                ))
             }
             _ => Err(Error::InvalidResponse),
         }

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<T, E = Error> = std::result::Result<T, E>;
@@ -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 function may return an error if the derived master key could not be parsed as a key for
-    /// * The given seed had an incorrect length.
+    /// the given algorithm (such as exceeding values on the secp256k1 curve).
-    /// * A `HmacSha512` can't be constructed.
     ///
     /// # Examples
     /// ```rust
@@ -167,11 +167,11 @@ where
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed);
     /// ```
-    pub fn new(seed: impl as_private_key::AsPrivateKey) -> Self {
+    pub fn new(seed: impl as_private_key::AsPrivateKey) -> Result<Self> {
         Self::new_internal(seed.as_private_key())
     }
 
-    fn new_internal(seed: &[u8]) -> Self {
+    fn new_internal(seed: &[u8]) -> Result<Self> {
         let hash = HmacSha512::new_from_slice(&K::key().bytes().collect::<Vec<_>>())
             .expect(bug!("HmacSha512 InvalidLength should be infallible"))
             .chain_update(seed)
@@ -179,6 +179,10 @@ where
             .into_bytes();
         let (private_key, chain_code) = hash.split_at(KEY_SIZE / 8);
 
+        // NOTE: Could potentially cause side-channel attacks, but Rust will likely optimize any
+        // possible comparison I could make anyways. This is kept as-is for clarity's sake, but can
+        // potentially leak information about the first few bytes of a key, such as if they all
+        // happen to be zero.
         assert!(
             !private_key.iter().all(|byte| *byte == 0),
             bug!("hmac function returned all-zero master key")
@@ -214,11 +218,16 @@ where
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
     /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code);
     /// ```
-    pub fn from_parts(key: &[u8; 32], depth: u8, chain_code: [u8; 32]) -> Self {
+    pub fn from_parts(key: &[u8; 32], depth: u8, chain_code: [u8; 32]) -> Result<Self> {
-        Self {
+        match K::from_bytes(key) {
-            private_key: K::from_bytes(key),
+            Ok(key) => {
+                Ok(Self {
+                    private_key: key,
                     depth,
                     chain_code,
+                })
+            }
+            Err(_) => Err(Error::InvalidKey),
         }
     }
 
@@ -232,12 +241,15 @@ where
     /// #   public_key::TestPublicKey as PublicKey,
     /// #   private_key::TestPrivateKey as PrivateKey,
     /// # };
+    /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
     /// let key: &[u8; 32] = //
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code)?;
-    /// assert_eq!(xprv.private_key(), &PrivateKey::from_bytes(key));
+    /// assert_eq!(xprv.private_key(), &PrivateKey::from_bytes(key)?);
+    /// # Ok(())
+    /// # }
     /// ```
     pub fn private_key(&self) -> &K {
         &self.private_key
@@ -262,7 +274,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::<PrivateKey>::new(*seed);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed)?;
     /// let xpub = xprv.extended_public_key();
     /// assert_eq!(known_key, xpub.public_key().to_bytes());
     /// # Ok(())
@@ -286,7 +298,7 @@ where
     /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
     /// let seed: &[u8; 64] = //
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed)?;
     /// let pubkey = xprv.public_key();
     /// # Ok(())
     /// # }
@@ -304,12 +316,15 @@ where
     /// #   public_key::TestPublicKey as PublicKey,
     /// #   private_key::TestPrivateKey as PrivateKey,
     /// # };
+    /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
     /// let key: &[u8; 32] = //
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code)?;
     /// assert_eq!(xprv.depth(), 4);
+    /// # Ok(())
+    /// # }
     /// ```
     pub fn depth(&self) -> u8 {
         self.depth
@@ -324,12 +339,15 @@ where
     /// #   public_key::TestPublicKey as PublicKey,
     /// #   private_key::TestPrivateKey as PrivateKey,
     /// # };
+    /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
     /// let key: &[u8; 32] = //
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::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 +369,7 @@ where
     /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
     /// let seed: &[u8; 64] = //
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
-    /// let root_xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed);
+    /// let root_xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed)?;
     /// let path = DerivationPath::default()
     ///     .chain_push(DerivationIndex::new(44, true)?)
     ///     .chain_push(DerivationIndex::new(0, true)?)
@@ -397,7 +415,7 @@ where
     /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
     /// let seed: &[u8; 64] = //
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
-    /// let root_xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed);
+    /// let root_xprv = ExtendedPrivateKey::<PrivateKey>::new(*seed)?;
     /// let bip44_wallet = DerivationPath::default()
     ///     .chain_push(DerivationIndex::new(44, true)?)
     ///     .chain_push(DerivationIndex::new(0, true)?)

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<Self, Self::Err>;
 
     /// 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 {
+    fn from_bytes(b: &PrivateKeyBytes) -> Result<Self, Self::Err> {
-        Self::from_slice(b).expect(bug!("Invalid private key bytes"))
+        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 {
+    fn from_bytes(b: &PrivateKeyBytes) -> Result<Self, Self::Err> {
-        Self::from_bytes(b)
+        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 {
+    fn from_bytes(b: &PrivateKeyBytes) -> Result<Self, Self::Err> {
-        Self { key: *b }
+        Ok(Self { key: *b })
     }
 
     fn to_bytes(&self) -> PrivateKeyBytes {

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] {

crates/derive/keyfork-derive-util/src/request.rs

@@ -70,7 +70,7 @@ impl DerivationAlgorithm {
         match self {
             #[cfg(feature = "ed25519")]
             Self::Ed25519 => {
-                let key = ExtendedPrivateKey::<ed25519_dalek::SigningKey>::new(seed);
+                let key = ExtendedPrivateKey::<ed25519_dalek::SigningKey>::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::<k256::SecretKey>::new(seed);
+                let key = ExtendedPrivateKey::<k256::SecretKey>::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::<TestPrivateKey>::new(seed);
+                let key = ExtendedPrivateKey::<TestPrivateKey>::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<Self, Self::Error> {
             match value.algorithm {
-                DerivationAlgorithm::Secp256k1 => Ok(Self::from_parts(
+                DerivationAlgorithm::Secp256k1 => {
-                    &value.data,
+                    Self::from_parts(&value.data, value.depth, value.chain_code).map_err(Into::into)
-                    value.depth,
+                }
-                    value.chain_code,
-                )),
                 _ => Err(Self::Error::Algorithm),
             }
         }
@@ -335,11 +333,9 @@ mod ed25519 {
 
         fn try_from(value: &DerivationResponse) -> Result<Self, Self::Error> {
             match value.algorithm {
-                DerivationAlgorithm::Ed25519 => Ok(Self::from_parts(
+                DerivationAlgorithm::Ed25519 => {
-                    &value.data,
+                    Self::from_parts(&value.data, value.depth, value.chain_code).map_err(Into::into)
-                    value.depth,
+                }
-                    value.chain_code,
-                )),
                 _ => Err(Self::Error::Algorithm),
             }
         }

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::<SecretKey>::new(varlen_seed);
+            let xkey = ExtendedPrivateKey::<SecretKey>::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::<SigningKey>::new(varlen_seed);
+            let xkey = ExtendedPrivateKey::<SigningKey>::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::<SigningKey>::new(seed);
+    let xkey = ExtendedPrivateKey::<SigningKey>::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::<SigningKey>::new(seed);
+    let mut xkey = ExtendedPrivateKey::<SigningKey>::new(seed).unwrap();
     for i in 0..=u32::from(u8::MAX) {
         xkey = xkey
             .derive_child(&DerivationIndex::new(i, true).unwrap())

crates/keyfork-shard/src/openpgp.rs

@@ -259,6 +259,7 @@ impl<P: PromptHandler> Format for OpenPGP<P> {
         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(

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<Cert> {
         .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)
@@ -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(())
 }