keyfork-shard: assert shared secrets are contributory

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

@@ -234,7 +234,7 @@ impl Client {
                 }
 
                 let depth = path.len() as u8;
-                Ok(ExtendedPrivateKey::new_from_parts(
+                Ok(ExtendedPrivateKey::from_parts(
                     &d.data,
                     depth,
                     d.chain_code,

crates/derive/keyfork-derive-openpgp/src/lib.rs

@@ -59,13 +59,17 @@ pub enum Error {
 #[allow(missing_docs)]
 pub type Result<T, E = Error> = std::result::Result<T, E>;
 
-/// Create an OpenPGP Cert with derived keys from the given derivation response, keys, and User
-/// ID.
+/// Create an OpenPGP Cert with private key data, with derived keys from the given derivation
+/// response, keys, and User ID.
+///
+/// Certificates are created with a default expiration of one day, but may be configured to expire
+/// later using the `KEYFORK_OPENPGP_EXPIRE` environment variable using values such as "15d" (15
+/// days), "1m" (one month), or "2y" (two years).
 ///
 /// # Errors
 /// The function may error for any condition mentioned in [`Error`].
 pub fn derive(xprv: XPrv, keys: &[KeyFlags], userid: &UserID) -> Result<Cert> {
-    let primary_key_flags = match keys.get(0) {
+    let primary_key_flags = match keys.first() {
         Some(kf) if kf.for_certification() => kf,
         _ => return Err(Error::NotCert),
     };
@@ -109,7 +113,7 @@ pub fn derive(xprv: XPrv, keys: &[KeyFlags], userid: &UserID) -> Result<Cert> {
     let cert = cert.insert_packets(vec![Packet::from(userid.clone()), binding.into()])?;
     let policy = sequoia_openpgp::policy::StandardPolicy::new();
 
-    // Set certificate expiration to one day
+    // Set certificate expiration to configured expiration or (default) one day
     let mut keypair = primary_key.clone().into_keypair()?;
     let signatures =
         cert.set_expiration_time(&policy, None, &mut keypair, Some(expiration_date))?;

crates/derive/keyfork-derive-util/src/extended_key/private_key.rs

@@ -124,9 +124,9 @@ mod serde_with {
         K: PrivateKey + Clone,
     {
         let variable_len_bytes = <&[u8]>::deserialize(deserializer)?;
-        let bytes: [u8; 32] = variable_len_bytes
-            .try_into()
-            .expect(bug!("unable to parse serialized private key; no support for static len"));
+        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))
     }
 }
@@ -179,13 +179,20 @@ where
             .into_bytes();
         let (private_key, chain_code) = hash.split_at(KEY_SIZE / 8);
 
-        Self::new_from_parts(
+        assert!(
+            !private_key.iter().all(|byte| *byte == 0),
+            bug!("hmac function returned all-zero master key")
+        );
+
+        Self::from_parts(
             private_key
                 .try_into()
                 .expect(bug!("KEY_SIZE / 8 did not give a 32 byte slice")),
             0,
             // Checked: chain_code is always the same length, hash is static size
-            chain_code.try_into().expect(bug!("Invalid chain code length")),
+            chain_code
+                .try_into()
+                .expect(bug!("Invalid chain code length")),
         )
     }
 
@@ -205,9 +212,9 @@ where
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::new_from_parts(key, 4, *chain_code);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code);
     /// ```
-    pub fn new_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]) -> Self {
         Self {
             private_key: K::from_bytes(key),
             depth,
@@ -229,7 +236,7 @@ where
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::new_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));
     /// ```
     pub fn private_key(&self) -> &K {
@@ -262,7 +269,7 @@ where
     /// # }
     /// ```
     pub fn extended_public_key(&self) -> ExtendedPublicKey<K::PublicKey> {
-        ExtendedPublicKey::new_from_parts(self.public_key(), self.depth, self.chain_code)
+        ExtendedPublicKey::from_parts(self.public_key(), self.depth, self.chain_code)
     }
 
     /// Return a public key for the current [`PrivateKey`].
@@ -301,7 +308,7 @@ where
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::new_from_parts(key, 4, *chain_code);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code);
     /// assert_eq!(xprv.depth(), 4);
     /// ```
     pub fn depth(&self) -> u8 {
@@ -321,7 +328,7 @@ where
     /// #   b"AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA";
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
-    /// let xprv = ExtendedPrivateKey::<PrivateKey>::new_from_parts(key, 4, *chain_code);
+    /// let xprv = ExtendedPrivateKey::<PrivateKey>::from_parts(key, 4, *chain_code);
     /// assert_eq!(chain_code, &xprv.chain_code());
     /// ```
     pub fn chain_code(&self) -> [u8; 32] {

crates/derive/keyfork-derive-util/src/extended_key/public_key.rs

@@ -60,11 +60,11 @@ where
     /// let chain_code: &[u8; 32] = //
     /// #   b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
     /// let pubkey = PublicKey::from_bytes(key);
-    /// let xpub = ExtendedPublicKey::<PublicKey>::new_from_parts(pubkey, 0, *chain_code);
+    /// let xpub = ExtendedPublicKey::<PublicKey>::from_parts(pubkey, 0, *chain_code);
     /// # Ok(())
     /// # }
     /// ```
-    pub fn new_from_parts(public_key: K, depth: u8, chain_code: ChainCode) -> Self {
+    pub fn from_parts(public_key: K, depth: u8, chain_code: ChainCode) -> Self {
         Self {
             public_key,
             depth,
@@ -86,7 +86,7 @@ where
     /// # let chain_code: &[u8; 32] = b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
     /// # let pubkey = PublicKey::from_bytes(key);
     /// let xpub = //
-    /// # ExtendedPublicKey::<PublicKey>::new_from_parts(pubkey, 0, *chain_code);
+    /// # ExtendedPublicKey::<PublicKey>::from_parts(pubkey, 0, *chain_code);
     /// let pubkey = xpub.public_key();
     /// # Ok(())
     /// # }
@@ -121,7 +121,7 @@ where
     /// # let chain_code: &[u8; 32] = b"BBBBBBBBBBBBBBBBBBBBBBBBBBBBBBBB";
     /// # let pubkey = PublicKey::from_bytes(key);
     /// let xpub = //
-    /// # ExtendedPublicKey::<PublicKey>::new_from_parts(pubkey, 0, *chain_code);
+    /// # ExtendedPublicKey::<PublicKey>::from_parts(pubkey, 0, *chain_code);
     /// let index = DerivationIndex::new(0, false)?;
     /// let child = xpub.derive_child(&index)?;
     /// # Ok(())

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

@@ -102,6 +102,10 @@ pub enum PrivateKeyError {
     /// For the given algorithm, the private key must be nonzero.
     #[error("The provided private key must be nonzero, but is not")]
     NonZero,
+
+    /// A scalar could not be constructed for the given algorithm.
+    #[error("A scalar could not be constructed for the given algorithm")]
+    InvalidScalar,
 }
 
 #[cfg(feature = "secp256k1")]
@@ -130,20 +134,19 @@ impl PrivateKey for k256::SecretKey {
     }
 
     fn derive_child(&self, other: &PrivateKeyBytes) -> Result<Self, Self::Err> {
-        if other.iter().all(|n| n == &0) {
-            return Err(PrivateKeyError::NonZero);
-        }
-        let other = *other;
-        // Checked: See above nonzero check
-        let scalar = Option::<NonZeroScalar>::from(NonZeroScalar::from_repr(other.into()))
-            .expect(bug!("Should have been able to get a NonZeroScalar"));
+        use k256::elliptic_curve::ScalarPrimitive;
+        use k256::{Scalar, Secp256k1};
+
+        // Construct a scalar from bytes
+        let scalar = ScalarPrimitive::<Secp256k1>::from_bytes(other.into());
+        let scalar = Option::<ScalarPrimitive<Secp256k1>>::from(scalar);
+        let scalar = scalar.ok_or(PrivateKeyError::InvalidScalar)?;
+        let scalar = Scalar::from(scalar);
 
         let derived_scalar = self.to_nonzero_scalar().as_ref() + scalar.as_ref();
-        Ok(
-            Option::<NonZeroScalar>::from(NonZeroScalar::new(derived_scalar))
-                .map(Into::into)
-                .expect(bug!("Should be able to make Key")),
-        )
+        let nonzero_scalar = Option::<NonZeroScalar>::from(NonZeroScalar::new(derived_scalar))
+            .ok_or(PrivateKeyError::NonZero)?;
+        Ok(Self::from(nonzero_scalar))
     }
 }
 
@@ -202,9 +205,7 @@ impl PrivateKey for TestPrivateKey {
     type Err = PrivateKeyError;
 
     fn from_bytes(b: &PrivateKeyBytes) -> Self {
-        Self {
-            key: *b
-        }
+        Self { key: *b }
     }
 
     fn to_bytes(&self) -> PrivateKeyBytes {

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

@@ -77,6 +77,10 @@ pub enum PublicKeyError {
     #[error("The provided public key must be nonzero, but is not")]
     NonZero,
 
+    /// A scalar could not be constructed for the given algorithm.
+    #[error("A scalar could not be constructed for the given algorithm")]
+    InvalidScalar,
+
     /// Public key derivation is unsupported for this algorithm.
     #[error("Public key derivation is unsupported for this algorithm")]
     DerivationUnsupported,
@@ -85,7 +89,7 @@ pub enum PublicKeyError {
 #[cfg(feature = "secp256k1")]
 use k256::{
     elliptic_curve::{group::prime::PrimeCurveAffine, sec1::ToEncodedPoint},
-    AffinePoint, NonZeroScalar,
+    AffinePoint,
 };
 
 #[cfg(feature = "secp256k1")]
@@ -105,14 +109,16 @@ impl PublicKey for k256::PublicKey {
     }
 
     fn derive_child(&self, other: PrivateKeyBytes) -> Result<Self, Self::Err> {
-        if other.iter().all(|n| n == &0) {
-            return Err(PublicKeyError::NonZero);
-        }
-        // Checked: See above
-        let scalar = Option::<NonZeroScalar>::from(NonZeroScalar::from_repr(other.into()))
-            .expect(bug!("Should have been able to get a NonZeroScalar"));
+        use k256::elliptic_curve::ScalarPrimitive;
+        use k256::{Secp256k1, Scalar};
 
-        let point = self.to_projective() + (AffinePoint::generator() * *scalar);
+        // Construct a scalar from bytes
+        let scalar = ScalarPrimitive::<Secp256k1>::from_bytes(&other.into());
+        let scalar = Option::<ScalarPrimitive<Secp256k1>>::from(scalar);
+        let scalar = scalar.ok_or(PublicKeyError::InvalidScalar)?;
+        let scalar = Scalar::from(scalar);
+
+        let point = self.to_projective() + (AffinePoint::generator() * scalar);
         Ok(Self::from_affine(point.into())
             .expect(bug!("Could not from_affine after scalar arithmetic")))
     }

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

@@ -300,7 +300,7 @@ mod secp256k1 {
 
         fn try_from(value: &DerivationResponse) -> Result<Self, Self::Error> {
             match value.algorithm {
-                DerivationAlgorithm::Secp256k1 => Ok(Self::new_from_parts(
+                DerivationAlgorithm::Secp256k1 => Ok(Self::from_parts(
                     &value.data,
                     value.depth,
                     value.chain_code,
@@ -335,7 +335,7 @@ mod ed25519 {
 
         fn try_from(value: &DerivationResponse) -> Result<Self, Self::Error> {
             match value.algorithm {
-                DerivationAlgorithm::Ed25519 => Ok(Self::new_from_parts(
+                DerivationAlgorithm::Ed25519 => Ok(Self::from_parts(
                     &value.data,
                     value.depth,
                     value.chain_code,

crates/keyfork-shard/src/lib.rs

@@ -10,6 +10,7 @@ use aes_gcm::{
     aead::{consts::U12, Aead},
     Aes256Gcm, KeyInit, Nonce,
 };
+use base64::prelude::{Engine, BASE64_STANDARD};
 use hkdf::Hkdf;
 use keyfork_bug::{bug, POISONED_MUTEX};
 use keyfork_mnemonic_util::{English, Mnemonic};
@@ -23,7 +24,6 @@ use keyfork_prompt::{
 use sha2::Sha256;
 use sharks::{Share, Sharks};
 use x25519_dalek::{EphemeralSecret, PublicKey};
-use base64::prelude::{BASE64_STANDARD, Engine};
 
 // 32-byte share, 1-byte index, 1-byte threshold, 1-byte version == 36 bytes
 // Encrypted, is 52 bytes
@@ -212,7 +212,9 @@ pub trait Format {
             if let Ok(Some(qrcode_content)) =
                 keyfork_qrcode::scan_camera(std::time::Duration::from_secs(30), 0)
             {
-                let decoded_data = BASE64_STANDARD.decode(qrcode_content).unwrap();
+                let decoded_data = BASE64_STANDARD
+                    .decode(qrcode_content)
+                    .expect(bug!("qrcode should contain base64 encoded data"));
                 pubkey_data = Some(decoded_data.try_into().map_err(|_| InvalidData)?)
             } else {
                 prompt
@@ -246,10 +248,9 @@ pub trait Format {
         // create our shared key
         let our_key = EphemeralSecret::random();
         let our_pubkey_mnemonic = Mnemonic::from_bytes(PublicKey::from(&our_key).as_bytes())?;
-        let shared_secret = our_key
-            .diffie_hellman(&PublicKey::from(their_pubkey))
-            .to_bytes();
-        let hkdf = Hkdf::<Sha256>::new(None, &shared_secret);
+        let shared_secret = our_key.diffie_hellman(&PublicKey::from(their_pubkey));
+        assert!(shared_secret.was_contributory(), bug!("shared secret might be insecure"));
+        let hkdf = Hkdf::<Sha256>::new(None, shared_secret.as_bytes());
 
         let mut shared_key_data = [0u8; 256 / 8];
         hkdf.expand(b"key", &mut shared_key_data)?;
@@ -300,7 +301,10 @@ pub trait Format {
             use keyfork_qrcode::{qrencode, ErrorCorrection};
             let mut qrcode_data = our_pubkey_mnemonic.to_bytes();
             qrcode_data.extend(payload_mnemonic.as_bytes());
-            if let Ok(qrcode) = qrencode(&BASE64_STANDARD.encode(qrcode_data), ErrorCorrection::Highest) {
+            if let Ok(qrcode) = qrencode(
+                &BASE64_STANDARD.encode(qrcode_data),
+                ErrorCorrection::Highest,
+            ) {
                 prompt
                     .lock()
                     .expect(bug!(POISONED_MUTEX))
@@ -433,7 +437,10 @@ pub fn remote_decrypt(w: &mut impl Write) -> Result<(), Box<dyn std::error::Erro
         {
             use keyfork_qrcode::{qrencode, ErrorCorrection};
             let qrcode_data = key_mnemonic.to_bytes();
-            if let Ok(qrcode) = qrencode(&BASE64_STANDARD.encode(qrcode_data), ErrorCorrection::Highest) {
+            if let Ok(qrcode) = qrencode(
+                &BASE64_STANDARD.encode(qrcode_data),
+                ErrorCorrection::Highest,
+            ) {
                 pm.prompt_message(PromptMessage::Text(format!(
                     concat!(
                         "A QR code will be displayed after this prompt. ",
@@ -464,7 +471,14 @@ pub fn remote_decrypt(w: &mut impl Write) -> Result<(), Box<dyn std::error::Erro
             if let Ok(Some(qrcode_content)) =
                 keyfork_qrcode::scan_camera(std::time::Duration::from_secs(QRCODE_TIMEOUT), 0)
             {
-                let decoded_data = BASE64_STANDARD.decode(qrcode_content).unwrap();
+                let decoded_data = BASE64_STANDARD
+                    .decode(qrcode_content)
+                    .expect(bug!("qrcode should contain base64 encoded data"));
+                assert_eq!(
+                    decoded_data.len(),
+                    ENCRYPTED_LENGTH as usize,
+                    bug!("invalid payload data")
+                );
                 let _ = pubkey_data.insert(decoded_data[..32].try_into().map_err(|_| InvalidData)?);
                 let _ = payload_data.insert(decoded_data[32..].to_vec());
             } else {
@@ -500,8 +514,9 @@ pub fn remote_decrypt(w: &mut impl Write) -> Result<(), Box<dyn std::error::Erro
             bug!("invalid payload data")
         );
 
-        let shared_secret = our_key.diffie_hellman(&PublicKey::from(pubkey)).to_bytes();
-        let hkdf = Hkdf::<Sha256>::new(None, &shared_secret);
+        let shared_secret = our_key.diffie_hellman(&PublicKey::from(pubkey));
+        assert!(shared_secret.was_contributory(), bug!("shared secret might be insecure"));
+        let hkdf = Hkdf::<Sha256>::new(None, shared_secret.as_bytes());
 
         let mut shared_key_data = [0u8; 256 / 8];
         hkdf.expand(b"key", &mut shared_key_data)?;

crates/keyfork/src/cli/derive.rs

@@ -20,8 +20,12 @@ pub enum DeriveSubcommands {
     /// Derive an OpenPGP Transferable Secret Key (private key). The key is encoded using OpenPGP
     /// ASCII Armor, a format usable by most programs using OpenPGP.
     ///
-    /// The key is generated with a 24-hour expiration time. The operation to set the expiration
-    /// time to a higher value is left to the user to ensure the key is usable by the user.
+    /// Certificates are created with a default expiration of one day, but may be configured to
+    /// expire later using the `KEYFORK_OPENPGP_EXPIRE` environment variable using values such as
+    /// "15d" (15 days), "1m" (one month), or "2y" (two years).
+    ///
+    /// It is recommended to use the default expiration of one day and to change the expiration
+    /// using an external utility, to ensure the Certify key is usable.
     #[command(name = "openpgp")]
     OpenPGP {
         /// Default User ID for the certificate, using the OpenPGP User ID format.

crates/keyfork/src/cli/wizard.rs

@@ -38,7 +38,7 @@ fn derive_key(seed: [u8; 32], index: u8) -> Result<Cert> {
     let chain = DerivationIndex::new(u32::from_be_bytes(pgp_u32), true)?;
     let mut shrd_u32 = [0u8; 4];
     shrd_u32[..].copy_from_slice(&"shrd".bytes().collect::<Vec<u8>>());
-    let account = DerivationIndex::new(u32::from_be_bytes(pgp_u32), true)?;
+    let account = DerivationIndex::new(u32::from_be_bytes(shrd_u32), true)?;
     let subkey = DerivationIndex::new(u32::from(index), true)?;
     let path = DerivationPath::default()
         .chain_push(chain)
@@ -132,8 +132,8 @@ fn generate_shard_secret(
         for i in 0..keys_per_shard {
             pm.prompt_message(Message::Text(format!(
                 "Please remove all keys and insert key #{} for user #{}",
-                i + 1,
-                index + 1,
+                (i as u16) + 1,
+                (index as u16) + 1,
             )))?;
             let card_backend = loop {
                 if let Some(c) = PcscBackend::cards(None)?.next().transpose()? {