From c9b80df60bba3158249c58b3f968131e25280fb9 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 6 May 2025 19:27:49 +0200 Subject: [PATCH 1/6] Implement signing keys and signing --- Cargo.lock | 5 + crates/bitwarden-crypto/Cargo.toml | 1 + crates/bitwarden-crypto/src/cose.rs | 5 + crates/bitwarden-crypto/src/error.rs | 6 + crates/bitwarden-crypto/src/keys/key_id.rs | 7 + crates/bitwarden-crypto/src/keys/mod.rs | 1 + .../src/keys/signing_crypto_key.rs | 421 ++++++++++++++++++ crates/bitwarden-crypto/src/lib.rs | 1 + crates/bitwarden-crypto/src/signing/mod.rs | 27 ++ 9 files changed, 474 insertions(+) create mode 100644 crates/bitwarden-crypto/src/keys/signing_crypto_key.rs create mode 100644 crates/bitwarden-crypto/src/signing/mod.rs diff --git a/Cargo.lock b/Cargo.lock index 21e2fd57..dfd61c8f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -426,6 +426,7 @@ dependencies = [ "ciborium", "coset", "criterion", + "ed25519-dalek", "generic-array", "hkdf", "hmac", @@ -1261,6 +1262,7 @@ dependencies = [ "fiat-crypto", "rustc_version", "subtle", + "zeroize", ] [[package]] @@ -1459,8 +1461,11 @@ checksum = "4a3daa8e81a3963a60642bcc1f90a670680bd4a77535faa384e9d1c79d620871" dependencies = [ "curve25519-dalek", "ed25519", + "rand_core 0.6.4", + "serde", "sha2", "subtle", + "zeroize", ] [[package]] diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index 7cae1f34..cc90f98c 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -33,6 +33,7 @@ cbc = { version = ">=0.1.2, <0.2", features = ["alloc", "zeroize"] } chacha20poly1305 = { version = "0.10.1" } ciborium = { version = ">=0.2.2, <0.3" } coset = { version = ">=0.3.8, <0.4" } +ed25519-dalek = { version = "2.1.1", features = ["rand_core"] } generic-array = { version = ">=0.14.7, <1.0", features = ["zeroize"] } hkdf = ">=0.12.3, <0.13" hmac = ">=0.12.1, <0.13" diff --git a/crates/bitwarden-crypto/src/cose.rs b/crates/bitwarden-crypto/src/cose.rs index c98a9f89..a8769be9 100644 --- a/crates/bitwarden-crypto/src/cose.rs +++ b/crates/bitwarden-crypto/src/cose.rs @@ -16,6 +16,11 @@ use crate::{ /// the draft was never published as an RFC, we use a private-use value for the algorithm. pub(crate) const XCHACHA20_POLY1305: i64 = -70000; +// Labels +// +// The label used for the namespace ensuring strong domain separation when using signatures. +pub(crate) const SIGNING_NAMESPACE: i64 = -80000; + /// Encrypts a plaintext message using XChaCha20Poly1305 and returns a COSE Encrypt0 message pub(crate) fn encrypt_xchacha20_poly1305( plaintext: &[u8], diff --git a/crates/bitwarden-crypto/src/error.rs b/crates/bitwarden-crypto/src/error.rs index d2604b12..9536de1c 100644 --- a/crates/bitwarden-crypto/src/error.rs +++ b/crates/bitwarden-crypto/src/error.rs @@ -56,6 +56,12 @@ pub enum CryptoError { #[error("Invalid nonce length")] InvalidNonceLength, + + #[error("Invalid signature")] + InvalidSignature, + + #[error("Invalid namespace")] + InvalidNamespace, } #[derive(Debug, Error)] diff --git a/crates/bitwarden-crypto/src/keys/key_id.rs b/crates/bitwarden-crypto/src/keys/key_id.rs index ef7459ac..525a7418 100644 --- a/crates/bitwarden-crypto/src/keys/key_id.rs +++ b/crates/bitwarden-crypto/src/keys/key_id.rs @@ -9,6 +9,7 @@ pub(crate) const KEY_ID_SIZE: usize = 16; /// A key id is a unique identifier for a single key. There is a 1:1 mapping between key ID and key /// bytes, so something like a user key rotation is replacing the key with ID A with a new key with /// ID B. +#[derive(Clone)] pub(crate) struct KeyId(uuid::Uuid); /// Fixed length identifiers for keys. @@ -38,6 +39,12 @@ impl From for [u8; KEY_ID_SIZE] { } } +impl From<&KeyId> for Vec { + fn from(key_id: &KeyId) -> Self { + key_id.0.as_bytes().to_vec() + } +} + impl From<[u8; KEY_ID_SIZE]> for KeyId { fn from(bytes: [u8; KEY_ID_SIZE]) -> Self { KeyId(Uuid::from_bytes(bytes)) diff --git a/crates/bitwarden-crypto/src/keys/mod.rs b/crates/bitwarden-crypto/src/keys/mod.rs index efdbc52d..823421e9 100644 --- a/crates/bitwarden-crypto/src/keys/mod.rs +++ b/crates/bitwarden-crypto/src/keys/mod.rs @@ -14,6 +14,7 @@ mod asymmetric_crypto_key; pub use asymmetric_crypto_key::{ AsymmetricCryptoKey, AsymmetricEncryptable, AsymmetricPublicCryptoKey, }; +mod signing_crypto_key; mod user_key; pub use user_key::UserKey; mod device_key; diff --git a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs new file mode 100644 index 00000000..a5a3104d --- /dev/null +++ b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs @@ -0,0 +1,421 @@ +//! This file implements creation and verification of detached signatures + +use ciborium::{value::Integer, Value}; +use coset::{ + iana::{self, Algorithm, EllipticCurve, EnumI64, KeyOperation, KeyType, OkpKeyParameter}, + CborSerializable, CoseKey, CoseSign1, Label, RegisteredLabel, RegisteredLabelWithPrivate, +}; +use ed25519_dalek::Signer; +use rand::rngs::OsRng; + +use super::key_id::KeyId; +use crate::{cose::SIGNING_NAMESPACE, error::Result, signing::SigningNamespace, CryptoError}; + +#[allow(unused)] +enum SigningCryptoKeyEnum { + Ed25519(ed25519_dalek::SigningKey), +} + +#[allow(unused)] +enum VerifyingKeyEnum { + Ed25519(ed25519_dalek::VerifyingKey), +} + +#[allow(unused)] +struct SigningKey { + id: KeyId, + inner: SigningCryptoKeyEnum, +} + +#[allow(unused)] +struct VerifyingKey { + id: KeyId, + inner: VerifyingKeyEnum, +} + +#[allow(unused)] +impl SigningKey { + fn make_ed25519() -> Result { + Ok(SigningKey { + id: KeyId::make(), + inner: SigningCryptoKeyEnum::Ed25519(ed25519_dalek::SigningKey::generate(&mut OsRng)), + }) + } + + fn cose_algorithm(&self) -> Algorithm { + match &self.inner { + SigningCryptoKeyEnum::Ed25519(_) => Algorithm::EdDSA, + } + } + + fn to_cose(&self) -> Result> { + match &self.inner { + SigningCryptoKeyEnum::Ed25519(key) => { + coset::CoseKeyBuilder::new_okp_key() + .key_id((&self.id).into()) + .algorithm(Algorithm::EdDSA) + // Note: X does not refer to the X coordinate of the public key curve point, but + // to the verifying key, as represented by the curve spec. In the + // case of Ed25519, this is the compressed Y coordinate. This was ill-defined in + // earlier drafts of the standard. https://www.rfc-editor.org/rfc/rfc9053.html#name-octet-key-pair + // + // Note: By the standard, the public key is optional (but RECOMMENDED) here, and + // can be derived on the fly. + .param( + OkpKeyParameter::X.to_i64(), + Value::Bytes(key.verifying_key().to_bytes().into()), + ) + .param( + OkpKeyParameter::D.to_i64(), + Value::Bytes(key.to_bytes().into()), + ) + .param( + OkpKeyParameter::Crv.to_i64(), + Value::Integer(Integer::from(EllipticCurve::Ed25519.to_i64())), + ) + .add_key_op(KeyOperation::Sign) + .add_key_op(KeyOperation::Verify) + .build() + .to_vec() + .map_err(|_| CryptoError::InvalidKey) + } + } + } + + fn from_cose(bytes: &[u8]) -> Result { + let cose_key = CoseKey::from_slice(bytes).map_err(|_| CryptoError::InvalidKey)?; + let (key_id, Some(algorithm), key_type) = (cose_key.key_id, cose_key.alg, cose_key.kty) + else { + return Err(CryptoError::InvalidKey); + }; + let key_id: [u8; 16] = key_id + .as_slice() + .try_into() + .map_err(|_| CryptoError::InvalidKey)?; + let key_id: KeyId = key_id.into(); + + match (key_type, algorithm) { + (kty, alg) + if kty == RegisteredLabel::Assigned(KeyType::OKP) + && alg == RegisteredLabelWithPrivate::Assigned(Algorithm::EdDSA) => + { + // https://www.rfc-editor.org/rfc/rfc9053.html#name-octet-key-pair + let (mut crv, mut x, mut d) = (None, None, None); + for (key, value) in &cose_key.params { + if let Label::Int(i) = key { + let key = OkpKeyParameter::from_i64(*i).ok_or(CryptoError::InvalidKey)?; + match key { + OkpKeyParameter::Crv => { + crv.replace(value); + } + OkpKeyParameter::X => { + x.replace(value); + } + OkpKeyParameter::D => { + d.replace(value); + } + _ => (), + } + } + } + + let (Some(_x), Some(d), Some(crv)) = (x, d, crv) else { + return Err(CryptoError::InvalidKey); + }; + let crv: i128 = crv.as_integer().ok_or(CryptoError::InvalidKey)?.into(); + if crv == EllipticCurve::Ed25519.to_i64().into() { + let secret_key_bytes: &[u8; 32] = d + .as_bytes() + .ok_or(CryptoError::InvalidKey)? + .as_slice() + .try_into() + .map_err(|_| CryptoError::InvalidKey)?; + let key = ed25519_dalek::SigningKey::from_bytes(secret_key_bytes); + Ok(SigningKey { + id: key_id, + inner: SigningCryptoKeyEnum::Ed25519(key), + }) + } else { + Err(CryptoError::InvalidKey) + } + } + _ => Err(CryptoError::InvalidKey), + } + } + + pub(crate) fn sign(&self, namespace: &SigningNamespace, data: &[u8]) -> Signature { + Signature::from( + coset::CoseSign1Builder::new() + .protected( + coset::HeaderBuilder::new() + .algorithm(self.cose_algorithm()) + .key_id((&self.id).into()) + .value( + SIGNING_NAMESPACE, + ciborium::Value::Integer(Integer::from(namespace.as_i64())), + ) + .build(), + ) + .create_detached_signature(data, &[], |pt| self.sign_raw(pt)) + .build(), + ) + } + + /// Signs the given byte array with the signing key. + /// This should never be used directly, but only through the `sign` method, to enforce + /// strong domain separation of the signatures. + fn sign_raw(&self, data: &[u8]) -> Vec { + match &self.inner { + SigningCryptoKeyEnum::Ed25519(key) => key.sign(data).to_bytes().to_vec(), + } + } + + fn to_verifying_key(&self) -> VerifyingKey { + match &self.inner { + SigningCryptoKeyEnum::Ed25519(key) => VerifyingKey { + id: self.id.clone(), + inner: VerifyingKeyEnum::Ed25519(key.verifying_key()), + }, + } + } +} + +#[allow(unused)] +impl VerifyingKey { + fn to_cose(&self) -> Result> { + match &self.inner { + VerifyingKeyEnum::Ed25519(key) => coset::CoseKeyBuilder::new_okp_key() + .key_id((&self.id).into()) + .algorithm(Algorithm::EdDSA) + .param( + OkpKeyParameter::Crv.to_i64(), + Value::Integer(Integer::from(EllipticCurve::Ed25519.to_i64())), + ) + .param( + OkpKeyParameter::X.to_i64(), + Value::Bytes(key.to_bytes().to_vec()), + ) + .add_key_op(KeyOperation::Verify) + .build() + .to_vec() + .map_err(|_| CryptoError::InvalidKey), + } + } + + fn from_cose(bytes: &[u8]) -> Result { + let cose_key = coset::CoseKey::from_slice(bytes).map_err(|_| CryptoError::InvalidKey)?; + + let (key_id, Some(algorithm), key_type) = (cose_key.key_id, cose_key.alg, cose_key.kty) + else { + return Err(CryptoError::InvalidKey); + }; + let key_id: [u8; 16] = key_id + .as_slice() + .try_into() + .map_err(|_| CryptoError::InvalidKey)?; + let key_id: KeyId = key_id.into(); + + match (key_type, algorithm) { + (kty, alg) + if kty == RegisteredLabel::Assigned(KeyType::OKP) + && alg == RegisteredLabelWithPrivate::Assigned(Algorithm::EdDSA) => + { + let (mut crv, mut x) = (None, None); + for (key, value) in &cose_key.params { + if let coset::Label::Int(i) = key { + let key = OkpKeyParameter::from_i64(*i).ok_or(CryptoError::InvalidKey)?; + match key { + OkpKeyParameter::Crv => { + crv.replace(value); + } + OkpKeyParameter::X => { + x.replace(value); + } + _ => (), + } + } + } + let (Some(x), Some(crv)) = (x, crv) else { + return Err(CryptoError::InvalidKey); + }; + + let crv: i128 = crv.as_integer().ok_or(CryptoError::InvalidKey)?.into(); + if crv == iana::EllipticCurve::Ed25519.to_i64().into() { + let verifying_key_bytes: &[u8; 32] = x + .as_bytes() + .ok_or(CryptoError::InvalidKey)? + .as_slice() + .try_into() + .map_err(|_| CryptoError::InvalidKey)?; + let verifying_key = + ed25519_dalek::VerifyingKey::from_bytes(verifying_key_bytes) + .map_err(|_| CryptoError::InvalidKey)?; + Ok(VerifyingKey { + id: key_id, + inner: VerifyingKeyEnum::Ed25519(verifying_key), + }) + } else { + Err(CryptoError::InvalidKey) + } + } + _ => Err(CryptoError::InvalidKey), + } + } + + /// Verifies the signature of the given data, for the given namespace. + /// This should never be used directly, but only through the `verify` method, to enforce + /// strong domain separation of the signatures. + pub(crate) fn verify( + &self, + namespace: &SigningNamespace, + signature: &Signature, + data: &[u8], + ) -> bool { + let Some(_alg) = &signature.inner().protected.header.alg else { + return false; + }; + + let mut signature_namespace = signature.namespace(); + let Ok(signature_namespace) = signature.namespace() else { + return false; + }; + if signature_namespace != *namespace { + return false; + } + + signature + .inner() + .verify_detached_signature(data, &[], |sig, data| self.verify_raw(sig, data)) + .is_ok() + } + + /// Verifies the signature of the given data, for the given namespace. + /// This should never be used directly, but only through the `verify` method, to enforce + /// strong domain separation of the signatures. + fn verify_raw(&self, signature: &[u8], data: &[u8]) -> Result<()> { + match &self.inner { + VerifyingKeyEnum::Ed25519(key) => { + let sig = ed25519_dalek::Signature::from_bytes( + signature + .try_into() + .map_err(|_| crate::error::CryptoError::InvalidSignature)?, + ); + key.verify_strict(data, &sig) + .map_err(|_| crate::error::CryptoError::InvalidSignature) + } + } + } +} + +/// A signature cryptographically attests to a (namespace, data) pair. The namespace is included in +/// the signature object, the data is not. One data object can be signed multiple times, with +/// different namespaces / by different signers, depending on the application needs. +#[allow(unused)] +struct Signature(CoseSign1); + +impl From for Signature { + fn from(cose_sign1: CoseSign1) -> Self { + Signature(cose_sign1) + } +} + +#[allow(unused)] +impl Signature { + fn from_bytes(bytes: &[u8]) -> Result { + let cose_sign1 = CoseSign1::from_slice(bytes).map_err(|_| CryptoError::InvalidSignature)?; + Ok(Signature(cose_sign1)) + } + + fn to_bytes(&self) -> Result> { + self.0 + .clone() + .to_vec() + .map_err(|_| CryptoError::InvalidSignature) + } + + fn inner(&self) -> &CoseSign1 { + &self.0 + } + + fn namespace(&self) -> Result { + let mut namespace = None; + for (key, value) in &self.0.protected.header.rest { + if let Label::Int(key) = key { + if *key == SIGNING_NAMESPACE { + namespace.replace(value); + } + } + } + let Some(namespace) = namespace else { + return Err(CryptoError::InvalidNamespace); + }; + let Some(namespace) = namespace.as_integer() else { + return Err(CryptoError::InvalidNamespace); + }; + let namespace: i128 = namespace.into(); + SigningNamespace::try_from_i64(namespace as i64) + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_sign_roundtrip() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let verifying_key = signing_key.to_verifying_key(); + let data = b"Hello, world!"; + let namespace = SigningNamespace::EncryptionMetadata; + + let signature = signing_key.sign(&namespace, data); + assert!(verifying_key.verify(&namespace, &signature, data)); + } + + #[test] + fn test_changed_signature_fails() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let verifying_key = signing_key.to_verifying_key(); + let data = b"Hello, world!"; + let namespace = SigningNamespace::EncryptionMetadata; + + let signature = signing_key.sign(&namespace, data); + assert!(!verifying_key.verify(&namespace, &signature, b"Goodbye, world!")); + } + + #[test] + fn test_changed_namespace_fails() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let verifying_key = signing_key.to_verifying_key(); + let data = b"Hello, world!"; + let namespace = SigningNamespace::EncryptionMetadata; + let other_namespace = SigningNamespace::Test; + + let signature = signing_key.sign(&namespace, data); + assert!(!verifying_key.verify(&other_namespace, &signature, data)); + } + + #[test] + fn test_cose_roundtrip_encode_signing() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let cose = signing_key.to_cose().unwrap(); + let parsed_key = SigningKey::from_cose(&cose).unwrap(); + + assert_eq!( + signing_key.to_cose().unwrap(), + parsed_key.to_cose().unwrap() + ); + } + + #[test] + fn test_cose_roundtrip_encode_verifying() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let cose = signing_key.to_verifying_key().to_cose().unwrap(); + let parsed_key = VerifyingKey::from_cose(&cose).unwrap(); + + assert_eq!( + signing_key.to_verifying_key().to_cose().unwrap(), + parsed_key.to_cose().unwrap() + ); + } +} diff --git a/crates/bitwarden-crypto/src/lib.rs b/crates/bitwarden-crypto/src/lib.rs index d3a0e304..70ee3aff 100644 --- a/crates/bitwarden-crypto/src/lib.rs +++ b/crates/bitwarden-crypto/src/lib.rs @@ -31,6 +31,7 @@ pub use wordlist::EFF_LONG_WORD_LIST; mod store; pub use store::{KeyStore, KeyStoreContext}; mod cose; +mod signing; mod traits; mod xchacha20; pub use traits::{Decryptable, Encryptable, IdentifyKey, KeyId, KeyIds}; diff --git a/crates/bitwarden-crypto/src/signing/mod.rs b/crates/bitwarden-crypto/src/signing/mod.rs new file mode 100644 index 00000000..6f9f2166 --- /dev/null +++ b/crates/bitwarden-crypto/src/signing/mod.rs @@ -0,0 +1,27 @@ +use crate::CryptoError; + +/// Signing is domain-separated within bitwarden, to prevent cross protocol attacks. +/// +/// A new signed entity or protocol shall use a new signing namespace. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum SigningNamespace { + #[allow(dead_code)] + EncryptionMetadata = 1, + #[cfg(test)] + Test = -1, +} + +impl SigningNamespace { + pub fn as_i64(&self) -> i64 { + *self as i64 + } + + pub fn try_from_i64(value: i64) -> Result { + match value { + 1 => Ok(Self::EncryptionMetadata), + #[cfg(test)] + -1 => Ok(Self::Test), + _ => Err(CryptoError::InvalidNamespace), + } + } +} From 2ad9f598b090c0bf54457a3a5bd8e8752b373360 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 6 May 2025 19:31:04 +0200 Subject: [PATCH 2/6] Remove unused code --- crates/bitwarden-crypto/src/keys/signing_crypto_key.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs index a5a3104d..67e1dff8 100644 --- a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs +++ b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs @@ -275,7 +275,6 @@ impl VerifyingKey { return false; }; - let mut signature_namespace = signature.namespace(); let Ok(signature_namespace) = signature.namespace() else { return false; }; From b0c3dae5c267afb18fc6656d53da8daef482dca6 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 6 May 2025 19:42:41 +0200 Subject: [PATCH 3/6] Add test vector --- .../src/keys/signing_crypto_key.rs | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs index 67e1dff8..424a15f3 100644 --- a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs +++ b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs @@ -320,12 +320,12 @@ impl From for Signature { #[allow(unused)] impl Signature { - fn from_bytes(bytes: &[u8]) -> Result { + fn from_cose(bytes: &[u8]) -> Result { let cose_sign1 = CoseSign1::from_slice(bytes).map_err(|_| CryptoError::InvalidSignature)?; Ok(Signature(cose_sign1)) } - fn to_bytes(&self) -> Result> { + fn to_cose(&self) -> Result> { self.0 .clone() .to_vec() @@ -360,11 +360,32 @@ impl Signature { mod tests { use super::*; + const SIGNING_KEY: &[u8] = &[167, 1, 1, 2, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, 3, 39, 4, 130, 1, 2, 33, 88, 32, 92, 186, 140, 91, 228, 10, 169, 163, 132, 55, 210, 79, 96, 186, 198, 251, 255, 79, 157, 58, 28, 182, 213, 118, 51, 15, 60, 110, 161, 114, 222, 168, 35, 88, 32, 59, 136, 203, 0, 108, 23, 82, 84, 206, 163, 86, 62, 187, 196, 156, 156, 150, 80, 101, 129, 247, 112, 117, 10, 34, 54, 254, 181, 239, 214, 195, 78, 32, 6]; + const VERIFYING_KEY: &[u8] = &[166, 1, 1, 2, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, 3, 39, 4, 129, 2, 32, 6, 33, 88, 32, 92, 186, 140, 91, 228, 10, 169, 163, 132, 55, 210, 79, 96, 186, 198, 251, 255, 79, 157, 58, 28, 182, 213, 118, 51, 15, 60, 110, 161, 114, 222, 168]; + /// Uses the ´SigningNamespace::EncryptionMetadata´ namespace, "Test message" as data + const SIGNATURE: &[u8] = &[132, 88, 27, 163, 1, 39, 4, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, 58, 0, 1, 56, 127, 1, 160, 246, 88, 64, 143, 218, 162, 76, 208, 117, 94, 215, 224, 98, 89, 193, 194, 226, 144, 214, 91, 130, 129, 130, 77, 36, 79, 196, 45, 105, 120, 151, 136, 57, 230, 27, 37, 142, 55, 191, 23, 200, 237, 215, 252, 42, 182, 140, 201, 173, 199, 214, 97, 105, 107, 101, 140, 182, 105, 9, 206, 106, 210, 29, 203, 174, 178, 12]; + + #[test] + fn test_using_test_vectors() { + let signing_key = SigningKey::from_cose(SIGNING_KEY).unwrap(); + let verifying_key = VerifyingKey::from_cose(VERIFYING_KEY).unwrap(); + let signature = Signature::from_cose(SIGNATURE).unwrap(); + + let data = b"Test message"; + let namespace = SigningNamespace::EncryptionMetadata; + + assert_eq!(signing_key.to_cose().unwrap(), SIGNING_KEY); + assert_eq!(verifying_key.to_cose().unwrap(), VERIFYING_KEY); + assert_eq!(signature.to_cose().unwrap(), SIGNATURE); + + assert!(verifying_key.verify(&namespace, &signature, data)); + } + #[test] fn test_sign_roundtrip() { let signing_key = SigningKey::make_ed25519().unwrap(); let verifying_key = signing_key.to_verifying_key(); - let data = b"Hello, world!"; + let data = b"Test message"; let namespace = SigningNamespace::EncryptionMetadata; let signature = signing_key.sign(&namespace, data); @@ -375,7 +396,7 @@ mod tests { fn test_changed_signature_fails() { let signing_key = SigningKey::make_ed25519().unwrap(); let verifying_key = signing_key.to_verifying_key(); - let data = b"Hello, world!"; + let data = b"Test message"; let namespace = SigningNamespace::EncryptionMetadata; let signature = signing_key.sign(&namespace, data); @@ -386,7 +407,7 @@ mod tests { fn test_changed_namespace_fails() { let signing_key = SigningKey::make_ed25519().unwrap(); let verifying_key = signing_key.to_verifying_key(); - let data = b"Hello, world!"; + let data = b"Test message"; let namespace = SigningNamespace::EncryptionMetadata; let other_namespace = SigningNamespace::Test; From 2e8bd92e5213c57a5dbbaa0e1efe1a5ed975dc98 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Tue, 6 May 2025 19:44:34 +0200 Subject: [PATCH 4/6] Cargo fmt --- .../src/keys/signing_crypto_key.rs | 23 ++++++++++++++++--- 1 file changed, 20 insertions(+), 3 deletions(-) diff --git a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs index 424a15f3..865b3be5 100644 --- a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs +++ b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs @@ -360,10 +360,27 @@ impl Signature { mod tests { use super::*; - const SIGNING_KEY: &[u8] = &[167, 1, 1, 2, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, 3, 39, 4, 130, 1, 2, 33, 88, 32, 92, 186, 140, 91, 228, 10, 169, 163, 132, 55, 210, 79, 96, 186, 198, 251, 255, 79, 157, 58, 28, 182, 213, 118, 51, 15, 60, 110, 161, 114, 222, 168, 35, 88, 32, 59, 136, 203, 0, 108, 23, 82, 84, 206, 163, 86, 62, 187, 196, 156, 156, 150, 80, 101, 129, 247, 112, 117, 10, 34, 54, 254, 181, 239, 214, 195, 78, 32, 6]; - const VERIFYING_KEY: &[u8] = &[166, 1, 1, 2, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, 3, 39, 4, 129, 2, 32, 6, 33, 88, 32, 92, 186, 140, 91, 228, 10, 169, 163, 132, 55, 210, 79, 96, 186, 198, 251, 255, 79, 157, 58, 28, 182, 213, 118, 51, 15, 60, 110, 161, 114, 222, 168]; + const SIGNING_KEY: &[u8] = &[ + 167, 1, 1, 2, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, + 3, 39, 4, 130, 1, 2, 33, 88, 32, 92, 186, 140, 91, 228, 10, 169, 163, 132, 55, 210, 79, 96, + 186, 198, 251, 255, 79, 157, 58, 28, 182, 213, 118, 51, 15, 60, 110, 161, 114, 222, 168, + 35, 88, 32, 59, 136, 203, 0, 108, 23, 82, 84, 206, 163, 86, 62, 187, 196, 156, 156, 150, + 80, 101, 129, 247, 112, 117, 10, 34, 54, 254, 181, 239, 214, 195, 78, 32, 6, + ]; + const VERIFYING_KEY: &[u8] = &[ + 166, 1, 1, 2, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, + 3, 39, 4, 129, 2, 32, 6, 33, 88, 32, 92, 186, 140, 91, 228, 10, 169, 163, 132, 55, 210, 79, + 96, 186, 198, 251, 255, 79, 157, 58, 28, 182, 213, 118, 51, 15, 60, 110, 161, 114, 222, + 168, + ]; /// Uses the ´SigningNamespace::EncryptionMetadata´ namespace, "Test message" as data - const SIGNATURE: &[u8] = &[132, 88, 27, 163, 1, 39, 4, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, 20, 115, 137, 58, 0, 1, 56, 127, 1, 160, 246, 88, 64, 143, 218, 162, 76, 208, 117, 94, 215, 224, 98, 89, 193, 194, 226, 144, 214, 91, 130, 129, 130, 77, 36, 79, 196, 45, 105, 120, 151, 136, 57, 230, 27, 37, 142, 55, 191, 23, 200, 237, 215, 252, 42, 182, 140, 201, 173, 199, 214, 97, 105, 107, 101, 140, 182, 105, 9, 206, 106, 210, 29, 203, 174, 178, 12]; + const SIGNATURE: &[u8] = &[ + 132, 88, 27, 163, 1, 39, 4, 80, 222, 105, 244, 28, 22, 106, 70, 109, 171, 83, 154, 97, 23, + 20, 115, 137, 58, 0, 1, 56, 127, 1, 160, 246, 88, 64, 143, 218, 162, 76, 208, 117, 94, 215, + 224, 98, 89, 193, 194, 226, 144, 214, 91, 130, 129, 130, 77, 36, 79, 196, 45, 105, 120, + 151, 136, 57, 230, 27, 37, 142, 55, 191, 23, 200, 237, 215, 252, 42, 182, 140, 201, 173, + 199, 214, 97, 105, 107, 101, 140, 182, 105, 9, 206, 106, 210, 29, 203, 174, 178, 12, + ]; #[test] fn test_using_test_vectors() { From f7f1e2ed775f84ed647674274378be15408df997 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Wed, 7 May 2025 13:49:27 +0200 Subject: [PATCH 5/6] Make ed25519 dependency version a range --- crates/bitwarden-crypto/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bitwarden-crypto/Cargo.toml b/crates/bitwarden-crypto/Cargo.toml index cc90f98c..e6a9fbdb 100644 --- a/crates/bitwarden-crypto/Cargo.toml +++ b/crates/bitwarden-crypto/Cargo.toml @@ -33,7 +33,7 @@ cbc = { version = ">=0.1.2, <0.2", features = ["alloc", "zeroize"] } chacha20poly1305 = { version = "0.10.1" } ciborium = { version = ">=0.2.2, <0.3" } coset = { version = ">=0.3.8, <0.4" } -ed25519-dalek = { version = "2.1.1", features = ["rand_core"] } +ed25519-dalek = { version = ">=2.1.1, <=2.2.0", features = ["rand_core"] } generic-array = { version = ">=0.14.7, <1.0", features = ["zeroize"] } hkdf = ">=0.12.3, <0.13" hmac = ">=0.12.1, <0.13" From 6deb284ce695a0040f935f2e999143238596ba43 Mon Sep 17 00:00:00 2001 From: Bernd Schoolmann Date: Mon, 12 May 2025 16:56:27 +0200 Subject: [PATCH 6/6] Add signed object --- .../src/keys/signing_crypto_key.rs | 180 ++++++++++++++++-- 1 file changed, 169 insertions(+), 11 deletions(-) diff --git a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs index 865b3be5..06c27026 100644 --- a/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs +++ b/crates/bitwarden-crypto/src/keys/signing_crypto_key.rs @@ -143,7 +143,15 @@ impl SigningKey { } } - pub(crate) fn sign(&self, namespace: &SigningNamespace, data: &[u8]) -> Signature { + /// Signs the given payload with the signing key, under a given namespace. + /// This returns a [`Signature`] object, that does not contain the payload. + /// The payload must be stored separately, and needs to be provided when verifying the + /// signature. + /// + /// This should be used when multiple signers are required, or when signatures need to be + /// replaceable without re-uploading the object, or if the signed object should be parseable + /// by the server side, without the use of COSE on the server. + pub(crate) fn sign_detached(&self, namespace: &SigningNamespace, data: &[u8]) -> Signature { Signature::from( coset::CoseSign1Builder::new() .protected( @@ -161,6 +169,31 @@ impl SigningKey { ) } + /// Signs the given payload with the signing key, under a given namespace. + /// This returns a [`SignedObject`] object, that contains the payload. + /// The payload is included in the signature, and does not need to be provided when verifying + /// the signature. + /// + /// This should be used when only one signer is required, so that only one object needs to be + /// kept track of. + pub(crate) fn sign(&self, namespace: &SigningNamespace, data: &[u8]) -> Result { + let cose_sign1 = coset::CoseSign1Builder::new() + .protected( + coset::HeaderBuilder::new() + .algorithm(self.cose_algorithm()) + .key_id((&self.id).into()) + .value( + SIGNING_NAMESPACE, + ciborium::Value::Integer(Integer::from(namespace.as_i64())), + ) + .build(), + ) + .payload(data.to_vec()) + .create_signature(&[], |pt| self.sign_raw(pt)) + .build(); + Ok(SignedObject(cose_sign1)) + } + /// Signs the given byte array with the signing key. /// This should never be used directly, but only through the `sign` method, to enforce /// strong domain separation of the signatures. @@ -265,7 +298,7 @@ impl VerifyingKey { /// Verifies the signature of the given data, for the given namespace. /// This should never be used directly, but only through the `verify` method, to enforce /// strong domain separation of the signatures. - pub(crate) fn verify( + pub(crate) fn verify_signature( &self, namespace: &SigningNamespace, signature: &Signature, @@ -288,6 +321,27 @@ impl VerifyingKey { .is_ok() } + /// Verifies the signature of a signed object, for the given namespace, and returns the payload. + pub(crate) fn get_verified_payload( + &self, + namespace: &SigningNamespace, + signature: &SignedObject, + ) -> Result> { + let Some(_alg) = &signature.inner().protected.header.alg else { + return Err(CryptoError::InvalidSignature); + }; + + let signature_namespace = signature.namespace()?; + if signature_namespace != *namespace { + return Err(CryptoError::InvalidNamespace); + } + + signature + .inner() + .verify_signature(&[], |sig, data| self.verify_raw(sig, data))?; + signature.payload() + } + /// Verifies the signature of the given data, for the given namespace. /// This should never be used directly, but only through the `verify` method, to enforce /// strong domain separation of the signatures. @@ -356,6 +410,63 @@ impl Signature { } } +/// A signed object has a cryptographical attestation to a (namespace, data) pair. The namespace and +/// data are included in the signature object. +#[allow(unused)] +struct SignedObject(CoseSign1); + +impl From for SignedObject { + fn from(cose_sign1: CoseSign1) -> Self { + SignedObject(cose_sign1) + } +} + +#[allow(unused)] +impl SignedObject { + fn from_cose(bytes: &[u8]) -> Result { + let cose_sign1 = CoseSign1::from_slice(bytes).map_err(|_| CryptoError::InvalidSignature)?; + Ok(SignedObject(cose_sign1)) + } + + fn to_cose(&self) -> Result> { + self.0 + .clone() + .to_vec() + .map_err(|_| CryptoError::InvalidSignature) + } + + fn inner(&self) -> &CoseSign1 { + &self.0 + } + + fn namespace(&self) -> Result { + let mut namespace = None; + for (key, value) in &self.0.protected.header.rest { + if let Label::Int(key) = key { + if *key == SIGNING_NAMESPACE { + namespace.replace(value); + } + } + } + let Some(namespace) = namespace else { + return Err(CryptoError::InvalidNamespace); + }; + let Some(namespace) = namespace.as_integer() else { + return Err(CryptoError::InvalidNamespace); + }; + let namespace: i128 = namespace.into(); + SigningNamespace::try_from_i64(namespace as i64) + } + + fn payload(&self) -> Result> { + self.0 + .payload + .as_ref() + .ok_or(CryptoError::InvalidSignature) + .map(|payload| payload.to_vec()) + } +} + #[cfg(test)] mod tests { use super::*; @@ -395,29 +506,42 @@ mod tests { assert_eq!(verifying_key.to_cose().unwrap(), VERIFYING_KEY); assert_eq!(signature.to_cose().unwrap(), SIGNATURE); - assert!(verifying_key.verify(&namespace, &signature, data)); + assert!(verifying_key.verify_signature(&namespace, &signature, data)); } #[test] - fn test_sign_roundtrip() { + fn test_sign_detached_roundtrip() { let signing_key = SigningKey::make_ed25519().unwrap(); let verifying_key = signing_key.to_verifying_key(); let data = b"Test message"; let namespace = SigningNamespace::EncryptionMetadata; - let signature = signing_key.sign(&namespace, data); - assert!(verifying_key.verify(&namespace, &signature, data)); + let signature = signing_key.sign_detached(&namespace, data); + assert!(verifying_key.verify_signature(&namespace, &signature, data)); } #[test] - fn test_changed_signature_fails() { + fn test_sign_roundtrip() { let signing_key = SigningKey::make_ed25519().unwrap(); let verifying_key = signing_key.to_verifying_key(); let data = b"Test message"; let namespace = SigningNamespace::EncryptionMetadata; + let signed_object = signing_key.sign(&namespace, data).unwrap(); + let payload = verifying_key + .get_verified_payload(&namespace, &signed_object) + .unwrap(); + assert_eq!(payload, data); + } - let signature = signing_key.sign(&namespace, data); - assert!(!verifying_key.verify(&namespace, &signature, b"Goodbye, world!")); + #[test] + fn test_changed_payload_fails() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let verifying_key = signing_key.to_verifying_key(); + let data = b"Test message"; + let namespace = SigningNamespace::EncryptionMetadata; + + let signature = signing_key.sign_detached(&namespace, data); + assert!(!verifying_key.verify_signature(&namespace, &signature, b"Test message 2")); } #[test] @@ -428,8 +552,42 @@ mod tests { let namespace = SigningNamespace::EncryptionMetadata; let other_namespace = SigningNamespace::Test; - let signature = signing_key.sign(&namespace, data); - assert!(!verifying_key.verify(&other_namespace, &signature, data)); + let signature = signing_key.sign_detached(&namespace, data); + assert!(!verifying_key.verify_signature(&other_namespace, &signature, data)); + } + + #[test] + fn test_changed_namespace_fails_signed_object() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let verifying_key = signing_key.to_verifying_key(); + let data = b"Test message"; + let namespace = SigningNamespace::EncryptionMetadata; + let other_namespace = SigningNamespace::Test; + let signed_object = signing_key.sign(&namespace, data).unwrap(); + assert!(verifying_key + .get_verified_payload(&other_namespace, &signed_object) + .is_err()); + } + + #[test] + fn test_cose_roundtrip_signature() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let cose = + signing_key.sign_detached(&SigningNamespace::EncryptionMetadata, b"Test message"); + let cose = cose.to_cose().unwrap(); + let parsed_cose = Signature::from_cose(&cose).unwrap(); + assert_eq!(cose, parsed_cose.to_cose().unwrap()); + } + + #[test] + fn test_cose_roundtrip_signed_object() { + let signing_key = SigningKey::make_ed25519().unwrap(); + let cose = signing_key + .sign(&SigningNamespace::EncryptionMetadata, b"Test message") + .unwrap(); + let cose = cose.to_cose().unwrap(); + let parsed_cose = SignedObject::from_cose(&cose).unwrap(); + assert_eq!(cose, parsed_cose.to_cose().unwrap()); } #[test]