Fix pubkey derivation and zeroize TWString/TWData (#4484)

* Normalisation and derivation fixes

* Minor

* Removes dependency on ed25519-bip32

* FMT

* Clippy

* Add iv check

* Use ios 18.0 in CI

* Use ios 18.6

* Add switch xcode step

* Fix pubkey derivation and zeroize TWString/TWData

* Clippy

* Add xcode switch in kotlin sample CI

* Fix pubkey derivation

* Fix C++ zerozing, use sodium comparison and minor fixes

* FMT

* Remove switch step

* Fix issues

* Adds remaining fixes

* Fix tests

* Minor fixes

* Update code coverage threshold for now

* Use memzero

* Fix

---------

Co-authored-by: Sergei Boiko <[email protected]>

Author: gupnik

rust/Cargo.lock

@@ -1 +1 @@
-94.1
+94.0

rust/coverage.stats

@@ -35,11 +35,10 @@ fn aes_cbc_encrypt_impl<E: KeyIvInit + BlockEncryptMut>(
     if iv.len() != IV_SIZE {
         return Err(StreamCipherError);
     }
-    let key = if key.len() > key_size {
-        &key[0..key_size]
-    } else {
-        key
-    };
+    if key.len() < key_size {
+        return Err(StreamCipherError);
+    }
+    let key = &key[0..key_size];
     let data = padding_mode.pad(data);
     let mut blocks = blocks::<E>(&data);
     let mut cipher = E::new(key.into(), iv.into());
@@ -55,19 +54,21 @@ fn aes_cbc_decrypt_impl<D: KeyIvInit + BlockDecryptMut>(
     key_size: usize,
     padding_mode: PaddingMode,
 ) -> Result<Vec<u8>, StreamCipherError> {
-    let key = if key.len() > key_size {
-        &key[0..key_size]
-    } else {
-        key
-    };
+    if iv.len() != IV_SIZE {
+        return Err(StreamCipherError);
+    }
+    if key.len() < key_size {
+        return Err(StreamCipherError);
+    }
+    let key = &key[0..key_size];
     if data.len() % BLOCK_SIZE_AES != 0 {
         return Err(StreamCipherError);
     }
     let mut blocks = blocks::<D>(data);
     let mut cipher = D::new(key.into(), iv.into());
     cipher.decrypt_blocks_mut(&mut blocks[..]);
     let buffer = blocks.concat();
-    Ok(padding_mode.unpad(&buffer))
+    padding_mode.unpad(&buffer).map_err(|_| StreamCipherError)
 }
 
 pub fn aes_cbc_encrypt_128(

rust/tw_crypto/src/crypto_aes_cbc/mod.rs

@@ -18,6 +18,9 @@ pub type TWFFIAESPaddingMode = u32;
 #[derive(Clone, Copy, Debug)]
 pub struct InvalidPaddingMode;
 
+#[derive(Clone, Copy, Debug)]
+pub struct InvalidPadding;
+
 impl TryFrom<u32> for PaddingMode {
     type Error = InvalidPaddingMode;
 
@@ -64,22 +67,28 @@ impl PaddingMode {
         padded
     }
 
-    pub fn unpad(&self, data: &[u8]) -> Vec<u8> {
+    pub fn unpad(&self, data: &[u8]) -> Result<Vec<u8>, InvalidPadding> {
         if data.is_empty() {
-            return data.to_vec();
+            return Ok(data.to_vec());
         }
         match self {
             PaddingMode::PKCS7 => {
                 let padding_len = data[data.len() - 1] as usize;
-                if padding_len <= data.len() {
-                    data[..data.len() - padding_len].to_vec()
-                } else {
-                    data.to_vec()
+                if padding_len == 0 || padding_len > BLOCK_SIZE_AES || padding_len > data.len() {
+                    return Err(InvalidPadding);
+                }
+                // Check that all padding bytes are equal to padding_len
+                if !data[data.len() - padding_len..]
+                    .iter()
+                    .all(|&b| b as usize == padding_len)
+                {
+                    return Err(InvalidPadding);
                 }
+                Ok(data[..data.len() - padding_len].to_vec())
             },
             // Zero padding can be ambiguous, so we return the original data
             // and the caller can strip the padding if needed
-            PaddingMode::Zero => data.to_vec(),
+            PaddingMode::Zero => Ok(data.to_vec()),
         }
     }
 }

rust/tw_crypto/src/crypto_aes_cbc/padding.rs

@@ -13,20 +13,24 @@ type Aes128Ctr64BE = Ctr64BE<Aes128>;
 type Aes192Ctr64BE = Ctr64BE<Aes192>;
 type Aes256Ctr64BE = Ctr64BE<Aes256>;
 
+const MAX_DATA_SIZE: usize = 10 * 1024 * 1024; // 10 MB
+
 fn aes_ctr_process<C: KeyIvInit + StreamCipher>(
     data: &[u8],
     iv: &[u8],
     key: &[u8],
     key_size: usize,
 ) -> Result<Vec<u8>, StreamCipherError> {
+    if data.len() > MAX_DATA_SIZE {
+        return Err(StreamCipherError);
+    }
     if iv.len() != IV_SIZE {
         return Err(StreamCipherError);
     }
-    let key = if key.len() > key_size {
-        &key[0..key_size]
-    } else {
-        key
-    };
+    if key.len() < key_size {
+        return Err(StreamCipherError);
+    }
+    let key = &key[0..key_size];
     let mut cipher = C::new(key.into(), iv.into());
     let mut data_vec = data.to_vec();
     cipher.try_apply_keystream(&mut data_vec)?;

rust/tw_crypto/src/crypto_aes_ctr/mod.rs

@@ -4,7 +4,7 @@
 // terms governing use, modification, and redistribution, is contained in the
 // file LICENSE at the root of the source code distribution tree.
 
-use bip32::ChildNumber;
+use bip32::{ChainCode, ChildNumber};
 use tw_hash::H256;
 use tw_keypair::traits::DerivableKeyTrait;
 use tw_keypair::{ecdsa::nist256p1, tw::Curve};
@@ -37,9 +37,28 @@ impl BIP32PrivateKey for nist256p1::PrivateKey {
 }
 
 impl BIP32PublicKey for nist256p1::PublicKey {
-    fn derive_child(&self, other: &[u8], _child_number: ChildNumber) -> Result<Self> {
-        let other = H256::try_from(other).map_err(|_| Error::InvalidKeyData)?;
-        <nist256p1::PublicKey as DerivableKeyTrait>::derive_child(self, other)
-            .map_err(|_| Error::DerivationFailed)
+    fn curve() -> Curve {
+        Curve::Nist256p1
+    }
+
+    fn derive_child(
+        &self,
+        chain_code: &ChainCode,
+        child_number: ChildNumber,
+    ) -> Result<(Self, ChainCode)> {
+        let (tweak, chain_code) = self.derive_tweak(chain_code, child_number)?;
+        // We should technically loop here if the tweak is zero or overflows
+        // the order of the underlying elliptic curve group, incrementing the
+        // index, however per "Child key derivation (CKD) functions":
+        // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions
+        //
+        // > "Note: this has probability lower than 1 in 2^127."
+        //
+        // ...so instead, we simply return an error if this were ever to happen,
+        // as the chances of it happening are vanishingly small.
+        let tweak = H256::try_from(&tweak[..]).map_err(|_| Error::InvalidKeyData)?;
+        let public_key = <nist256p1::PublicKey as DerivableKeyTrait>::derive_child(self, tweak)
+            .map_err(|_| Error::DerivationFailed)?;
+        Ok((public_key, chain_code))
     }
 }

rust/tw_crypto/src/crypto_hd_node/ecdsa/nist256p1/mod.rs

@@ -4,7 +4,7 @@
 // terms governing use, modification, and redistribution, is contained in the
 // file LICENSE at the root of the source code distribution tree.
 
-use bip32::ChildNumber;
+use bip32::{ChainCode, ChildNumber};
 use tw_hash::H256;
 use tw_keypair::traits::DerivableKeyTrait;
 use tw_keypair::{ecdsa::secp256k1, tw::Curve};
@@ -37,9 +37,28 @@ impl BIP32PrivateKey for secp256k1::PrivateKey {
 }
 
 impl BIP32PublicKey for secp256k1::PublicKey {
-    fn derive_child(&self, other: &[u8], _child_number: ChildNumber) -> Result<Self> {
-        let other = H256::try_from(other).map_err(|_| Error::InvalidKeyData)?;
-        <secp256k1::PublicKey as DerivableKeyTrait>::derive_child(self, other)
-            .map_err(|_| Error::DerivationFailed)
+    fn curve() -> Curve {
+        Curve::Secp256k1
+    }
+
+    fn derive_child(
+        &self,
+        chain_code: &ChainCode,
+        child_number: ChildNumber,
+    ) -> Result<(Self, ChainCode)> {
+        let (tweak, chain_code) = self.derive_tweak(chain_code, child_number)?;
+        // We should technically loop here if the tweak is zero or overflows
+        // the order of the underlying elliptic curve group, incrementing the
+        // index, however per "Child key derivation (CKD) functions":
+        // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions
+        //
+        // > "Note: this has probability lower than 1 in 2^127."
+        //
+        // ...so instead, we simply return an error if this were ever to happen,
+        // as the chances of it happening are vanishingly small.
+        let tweak = H256::try_from(&tweak[..]).map_err(|_| Error::InvalidKeyData)?;
+        let public_key = <secp256k1::PublicKey as DerivableKeyTrait>::derive_child(self, tweak)
+            .map_err(|_| Error::DerivationFailed)?;
+        Ok((public_key, chain_code))
     }
 }

rust/tw_crypto/src/crypto_hd_node/ecdsa/secp256k1/mod.rs

@@ -11,7 +11,7 @@ mod v2;
 
 use std::str::FromStr;
 
-use bip32::{ChildNumber, DerivationPath};
+use bip32::{ChainCode, ChildNumber, DerivationPath};
 use derivation::DerivationScheme;
 use key::{XPRV_SIZE, XPUB_SIZE};
 use tw_keypair::{ed25519, tw::Curve};
@@ -38,7 +38,7 @@ impl BIP32PrivateKey for ed25519::cardano::ExtendedPrivateKey {
         let bytes = self.to_zeroizing_vec();
         let bytes: [u8; XPRV_SIZE] = bytes.as_slice()[..XPRV_SIZE]
             .try_into()
-            .expect("Should not fail");
+            .map_err(|_| Error::InvalidKeyData)?;
         let bip32_xpr = key::XPrv::from_bytes_verified(bytes).map_err(|_| Error::InvalidKeyData)?;
         let child: key::XPrv = bip32_xpr.derive(DerivationScheme::V2, child_number.0);
         Self::try_from(child.as_ref()).map_err(|_| Error::InvalidKeyData)
@@ -58,14 +58,26 @@ impl BIP32PrivateKey for ed25519::cardano::ExtendedPrivateKey {
 }
 
 impl BIP32PublicKey for ed25519::cardano::ExtendedPublicKey {
-    fn derive_child(&self, _other: &[u8], child_number: ChildNumber) -> Result<Self> {
+    fn curve() -> Curve {
+        Curve::Ed25519ExtendedCardano
+    }
+
+    fn derive_child(
+        &self,
+        _chain_code: &ChainCode,
+        child_number: ChildNumber,
+    ) -> Result<(Self, ChainCode)> {
         let bytes = self.to_vec();
-        let bytes: [u8; XPUB_SIZE] = bytes[..XPUB_SIZE].try_into().expect("Should not fail");
+        let bytes: [u8; XPUB_SIZE] = bytes[..XPUB_SIZE]
+            .try_into()
+            .map_err(|_| Error::InvalidKeyData)?;
         let bip32_xpub = key::XPub::from_bytes(bytes);
         let child: key::XPub = bip32_xpub
             .derive(DerivationScheme::V2, child_number.0)
             .map_err(|_| Error::DerivationFailed)?;
-        Self::try_from(child.as_ref()).map_err(|_| Error::InvalidKeyData)
+        let public_key = Self::try_from(child.as_ref()).map_err(|_| Error::InvalidKeyData)?;
+        let chaincode = *child.chain_code();
+        Ok((public_key, chaincode))
     }
 }
 

rust/tw_crypto/src/crypto_hd_node/ed25519/cardano/mod.rs

@@ -7,7 +7,7 @@
 pub mod cardano;
 pub mod waves;
 
-use bip32::ChildNumber;
+use bip32::{ChainCode, ChildNumber};
 use tw_keypair::{ed25519, tw::Curve};
 
 use crate::crypto_hd_node::error::{Error, Result};
@@ -40,9 +40,28 @@ impl BIP32PrivateKey for ed25519::sha512::PrivateKey {
 }
 
 impl BIP32PublicKey for ed25519::sha512::PublicKey {
-    fn derive_child(&self, other: &[u8], child_number: ChildNumber) -> Result<Self> {
+    fn curve() -> Curve {
+        Curve::Ed25519
+    }
+
+    fn derive_child(
+        &self,
+        chain_code: &ChainCode,
+        child_number: ChildNumber,
+    ) -> Result<(Self, ChainCode)> {
+        let (tweak, chain_code) = self.derive_tweak(chain_code, child_number)?;
+        // We should technically loop here if the tweak is zero or overflows
+        // the order of the underlying elliptic curve group, incrementing the
+        // index, however per "Child key derivation (CKD) functions":
+        // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions
+        //
+        // > "Note: this has probability lower than 1 in 2^127."
+        //
+        // ...so instead, we simply return an error if this were ever to happen,
+        // as the chances of it happening are vanishingly small.
         if child_number.is_hardened() {
-            Self::try_from(other).map_err(|_| Error::InvalidKeyData)
+            let public_key = Self::try_from(&tweak[..]).map_err(|_| Error::InvalidKeyData)?;
+            Ok((public_key, chain_code))
         } else {
             Err(Error::InvalidChildNumber)
         }
@@ -74,9 +93,28 @@ impl BIP32PrivateKey for ed25519::blake2b::PrivateKey {
 }
 
 impl BIP32PublicKey for ed25519::blake2b::PublicKey {
-    fn derive_child(&self, other: &[u8], child_number: ChildNumber) -> Result<Self> {
+    fn curve() -> Curve {
+        Curve::Ed25519Blake2bNano
+    }
+
+    fn derive_child(
+        &self,
+        chain_code: &ChainCode,
+        child_number: ChildNumber,
+    ) -> Result<(Self, ChainCode)> {
+        let (tweak, chain_code) = self.derive_tweak(chain_code, child_number)?;
+        // We should technically loop here if the tweak is zero or overflows
+        // the order of the underlying elliptic curve group, incrementing the
+        // index, however per "Child key derivation (CKD) functions":
+        // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions
+        //
+        // > "Note: this has probability lower than 1 in 2^127."
+        //
+        // ...so instead, we simply return an error if this were ever to happen,
+        // as the chances of it happening are vanishingly small.
         if child_number.is_hardened() {
-            Self::try_from(other).map_err(|_| Error::InvalidKeyData)
+            let public_key = Self::try_from(&tweak[..]).map_err(|_| Error::InvalidKeyData)?;
+            Ok((public_key, chain_code))
         } else {
             Err(Error::InvalidChildNumber)
         }

rust/tw_crypto/src/crypto_hd_node/ed25519/mod.rs

@@ -4,7 +4,7 @@
 // terms governing use, modification, and redistribution, is contained in the
 // file LICENSE at the root of the source code distribution tree.
 
-use bip32::ChildNumber;
+use bip32::{ChainCode, ChildNumber};
 use tw_keypair::{ed25519, tw::Curve};
 
 use crate::crypto_hd_node::error::{Error, Result};
@@ -37,9 +37,28 @@ impl BIP32PrivateKey for ed25519::waves::PrivateKey {
 }
 
 impl BIP32PublicKey for ed25519::waves::PublicKey {
-    fn derive_child(&self, other: &[u8], child_number: ChildNumber) -> Result<Self> {
+    fn curve() -> Curve {
+        Curve::Curve25519Waves
+    }
+
+    fn derive_child(
+        &self,
+        chain_code: &ChainCode,
+        child_number: ChildNumber,
+    ) -> Result<(Self, ChainCode)> {
+        let (tweak, chain_code) = self.derive_tweak(chain_code, child_number)?;
+        // We should technically loop here if the tweak is zero or overflows
+        // the order of the underlying elliptic curve group, incrementing the
+        // index, however per "Child key derivation (CKD) functions":
+        // https://github.com/bitcoin/bips/blob/master/bip-0032.mediawiki#child-key-derivation-ckd-functions
+        //
+        // > "Note: this has probability lower than 1 in 2^127."
+        //
+        // ...so instead, we simply return an error if this were ever to happen,
+        // as the chances of it happening are vanishingly small.
         if child_number.is_hardened() {
-            Self::try_from(other).map_err(|_| Error::InvalidKeyData)
+            let public_key = Self::try_from(&tweak[..]).map_err(|_| Error::InvalidKeyData)?;
+            Ok((public_key, chain_code))
         } else {
             Err(Error::InvalidChildNumber)
         }