From 5d7d55d97f1251619911e4555a925cc03b50c7ed Mon Sep 17 00:00:00 2001 From: Martin Stefcek <35243812+Cifko@users.noreply.github.com> Date: Mon, 23 May 2022 16:03:21 +0200 Subject: [PATCH 1/3] fix: hash in cucumber (#4124) Description --- Fixes wrong hash in cucumber test. How Has This Been Tested? --- ` npm test -- --name "As a wallet I want to submit a transaction"` --- integration_tests/helpers/util.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration_tests/helpers/util.js b/integration_tests/helpers/util.js index 05c3415602..ef34cb7b6f 100644 --- a/integration_tests/helpers/util.js +++ b/integration_tests/helpers/util.js @@ -253,7 +253,7 @@ const getTransactionOutputHash = function (output) { // features.maturity Buffer.from([parseInt(output.features.maturity)]), // features.flags - Buffer.from([output.features.flags]), + Buffer.from(toLittleEndian(output.features.flags, 16)), ]); // features.parent_public_key features = Buffer.concat([ From 1e96d45535f4af967a761fd71521eb68bbb1b371 Mon Sep 17 00:00:00 2001 From: Stan Bondi Date: Tue, 24 May 2022 15:20:20 +0400 Subject: [PATCH 2/3] fix(dht)!: fixes MAC related key vuln for propagated cleartext msgs (#3907) Description --- - changes challenge in signed messages to `e = H(P||R||H(m))` - remove minor from DHT versioning Motivation and Context --- This only affects _signed cleartext_ messages that are propagated through adversarial nodes. Encrypted propagated messages are unaffected because only the recipient can decrypt the MAC and so cannot forge a signature. Nonce malleability ```text e = H(P||m) for P Attacker chooses R' = s.G - eP s' any scalar and transmits for P Verifier: s'.G ?= R' + eP ?= s'.G - eP + eP ?= s'.G (true for any s') ``` Related key ```text sig = for P e = H(R||m) Attacker: sig' = for P' s' = s + a.H(R||m) P' = P + a.G Verifier: s'.G ?= R + eP' ?= R + e(k + a).G ``` All signed cleartext and encrypted DHT messages are incompatible with current network: Namely, - Peer discovery - Wallet Propagated/SAF messages How Has This Been Tested? --- New unit tests that verify OriginMac is secure against these vulnerabilities Tested base node manually, and normal cleartext block and tx propagation messages continue to work Tested wallet, direct send transactions are still compatible --- Cargo.lock | 36 ++-- comms/core/Cargo.toml | 2 +- comms/core/src/peer_manager/node_id.rs | 10 +- comms/core/src/utils/mod.rs | 1 - comms/core/src/utils/signature.rs | 51 ----- comms/dht/Cargo.toml | 2 +- comms/dht/src/crypt.rs | 54 +++--- comms/dht/src/envelope.rs | 58 ++++-- comms/dht/src/inbound/decryption.rs | 65 +++---- comms/dht/src/lib.rs | 1 + comms/dht/src/origin_mac.rs | 183 ++++++++++++++++++ comms/dht/src/outbound/broadcast.rs | 33 ++-- comms/dht/src/outbound/serialize.rs | 3 +- comms/dht/src/proto/envelope.proto | 7 +- comms/dht/src/store_forward/error.rs | 12 +- .../dht/src/store_forward/saf_handler/task.rs | 41 ++-- comms/dht/src/test_utils/makers.rs | 28 +-- comms/dht/src/version.rs | 54 ++---- 18 files changed, 386 insertions(+), 255 deletions(-) delete mode 100644 comms/core/src/utils/signature.rs create mode 100644 comms/dht/src/origin_mac.rs diff --git a/Cargo.lock b/Cargo.lock index 4e63d878cd..8b049902ac 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -165,7 +165,7 @@ version = "0.2.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ca5162d1b961cb589a8ca08a2aa7cabc6341e05e0bf18d66a07697900b5d2ad0" dependencies = [ - "blake2", + "blake2 0.9.2", "password-hash", ] @@ -488,6 +488,15 @@ dependencies = [ "opaque-debug 0.3.0", ] +[[package]] +name = "blake2" +version = "0.10.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b9cf849ee05b2ee5fba5e36f97ff8ec2533916700fc0758d40d92136a42f3388" +dependencies = [ + "digest 0.10.3", +] + [[package]] name = "block" version = "0.1.6" @@ -1902,6 +1911,7 @@ checksum = "f2fb860ca6fafa5552fb6d0e816a69c8e49f0908bf524e30a90d97c85892d506" dependencies = [ "block-buffer 0.10.2", "crypto-common", + "subtle", ] [[package]] @@ -6147,7 +6157,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "6142f7c25e94f6fd25a32c3348ec230df9109b463f59c8c7acc4bd34936babb7" dependencies = [ "aes-gcm 0.9.4", - "blake2", + "blake2 0.9.2", "chacha20poly1305", "rand 0.8.5", "rand_core 0.6.3", @@ -6602,7 +6612,7 @@ name = "tari_bulletproofs" version = "4.1.2" source = "git+https://github.com/tari-project/bulletproofs?tag=v4.1.2#bdad5232c447a3acc1384b950574c6890a444e85" dependencies = [ - "blake2", + "blake2 0.9.2", "byteorder", "curve25519-dalek-ng", "digest 0.9.0", @@ -6621,7 +6631,7 @@ dependencies = [ name = "tari_collectibles" version = "0.1.0" dependencies = [ - "blake2", + "blake2 0.9.2", "clap 3.1.8", "derivative", "diesel", @@ -6707,7 +6717,7 @@ dependencies = [ "anyhow", "async-trait", "bitflags 1.3.2", - "blake2", + "blake2 0.10.4", "bytes 1.1.0", "chrono", "cidr", @@ -6865,7 +6875,7 @@ dependencies = [ "async-trait", "bincode", "bitflags 1.3.2", - "blake2", + "blake2 0.9.2", "bytes 0.5.6", "chrono", "config", @@ -6923,7 +6933,7 @@ version = "0.13.0" source = "git+https://github.com/tari-project/tari-crypto.git?tag=v0.13.0#c4dd4c0e53528720642b54f42083c6d9e392ee29" dependencies = [ "base64 0.10.1", - "blake2", + "blake2 0.9.2", "cbindgen", "curve25519-dalek-ng", "digest 0.9.0", @@ -6955,7 +6965,7 @@ version = "0.1.0" dependencies = [ "anyhow", "async-trait", - "blake2", + "blake2 0.9.2", "bytecodec", "clap 3.1.8", "digest 0.9.0", @@ -7014,7 +7024,7 @@ version = "0.32.0" dependencies = [ "argon2", "arrayvec 0.7.2", - "blake2", + "blake2 0.9.2", "chacha20", "chrono", "clear_on_drop", @@ -7190,7 +7200,7 @@ name = "tari_mmr" version = "0.32.0" dependencies = [ "bincode", - "blake2", + "blake2 0.9.2", "criterion 0.2.11", "croaring", "digest 0.9.0", @@ -7249,7 +7259,7 @@ dependencies = [ name = "tari_script" version = "0.12.0" dependencies = [ - "blake2", + "blake2 0.9.2", "digest 0.9.0", "rand 0.8.5", "serde", @@ -7345,7 +7355,7 @@ version = "0.32.0" dependencies = [ "anyhow", "async-trait", - "blake2", + "blake2 0.9.2", "bytecodec", "clap 3.1.8", "config", @@ -7390,7 +7400,7 @@ dependencies = [ "argon2", "async-trait", "bincode", - "blake2", + "blake2 0.9.2", "chrono", "clear_on_drop", "crossbeam-channel 0.3.9", diff --git a/comms/core/Cargo.toml b/comms/core/Cargo.toml index 74f267c9f0..5fc7ed055b 100644 --- a/comms/core/Cargo.toml +++ b/comms/core/Cargo.toml @@ -19,7 +19,7 @@ tari_utilities = { git = "https://github.com/tari-project/tari_utilities.git", t anyhow = "1.0.53" async-trait = "0.1.36" bitflags = "1.0.4" -blake2 = "0.9.0" +blake2 = "0.10.4" bytes = { version = "1", features = ["serde"] } chrono = { version = "0.4.19", default-features = false, features = ["serde", "clock"] } cidr = "0.1.0" diff --git a/comms/core/src/peer_manager/node_id.rs b/comms/core/src/peer_manager/node_id.rs index 55d1217ec2..bdfcf0070d 100644 --- a/comms/core/src/peer_manager/node_id.rs +++ b/comms/core/src/peer_manager/node_id.rs @@ -32,7 +32,7 @@ use std::{ use blake2::{ digest::{Update, VariableOutput}, - VarBlake2b, + Blake2bVar, }; use serde::{de, Deserialize, Deserializer, Serialize}; use tari_utilities::{ @@ -74,13 +74,11 @@ impl NodeId { pub fn from_key(key: &K) -> Self { let bytes = key.as_bytes(); let mut buf = [0u8; NodeId::byte_size()]; - VarBlake2b::new(NodeId::byte_size()) + Blake2bVar::new(NodeId::byte_size()) .expect("NodeId::byte_size() is invalid") .chain(bytes) - .finalize_variable(|hash| { - // Safety: output size and buf size are equal - buf.copy_from_slice(hash) - }); + // Safety: output size and buf size are equal + .finalize_variable(&mut buf).unwrap(); NodeId(buf) } diff --git a/comms/core/src/utils/mod.rs b/comms/core/src/utils/mod.rs index 59823a19d7..0c33c5f3cc 100644 --- a/comms/core/src/utils/mod.rs +++ b/comms/core/src/utils/mod.rs @@ -27,4 +27,3 @@ pub mod cidr; pub mod datetime; pub mod mpsc; pub mod multiaddr; -pub mod signature; diff --git a/comms/core/src/utils/signature.rs b/comms/core/src/utils/signature.rs deleted file mode 100644 index 54bd92dd3e..0000000000 --- a/comms/core/src/utils/signature.rs +++ /dev/null @@ -1,51 +0,0 @@ -// Copyright 2019, The Tari Project -// -// Redistribution and use in source and binary forms, with or without modification, are permitted provided that the -// following conditions are met: -// -// 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following -// disclaimer. -// -// 2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the -// following disclaimer in the documentation and/or other materials provided with the distribution. -// -// 3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote -// products derived from this software without specific prior written permission. -// -// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, -// INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE -// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR -// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, -// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE -// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - -use digest::Digest; -use rand::{CryptoRng, Rng}; -use tari_crypto::{ - keys::{PublicKey, SecretKey}, - signatures::{SchnorrSignature, SchnorrSignatureError}, - tari_utilities::message_format::MessageFormat, -}; - -use crate::types::{Challenge, CommsPublicKey, CommsSecretKey, Signature}; - -pub fn sign_challenge( - rng: &mut R, - secret_key: CommsSecretKey, - challenge: Challenge, -) -> Result -where - R: CryptoRng + Rng, -{ - let nonce = ::K::random(rng); - SchnorrSignature::sign(secret_key, nonce, &challenge.finalize()) -} - -/// Verify that the signature is valid for the challenge -pub fn verify_challenge(public_key: &CommsPublicKey, signature: &[u8], challenge: Challenge) -> bool { - match Signature::from_binary(signature) { - Ok(signature) => signature.verify_challenge(public_key, &challenge.finalize()), - Err(_) => false, - } -} diff --git a/comms/dht/Cargo.toml b/comms/dht/Cargo.toml index c0ffdbd6ea..7c33c47ab0 100644 --- a/comms/dht/Cargo.toml +++ b/comms/dht/Cargo.toml @@ -27,7 +27,7 @@ diesel = { version = "1.4.7", features = ["sqlite", "serde_json", "chrono", "num diesel_migrations = "1.4.0" libsqlite3-sys = { version = "0.22.2", features = ["bundled"], optional = true } digest = "0.9.0" -futures = { version = "^0.3.1" } +futures = "^0.3.1" log = "0.4.8" log-mdc = "0.1.0" prost = "=0.9.0" diff --git a/comms/dht/src/crypt.rs b/comms/dht/src/crypt.rs index 964d4d6a92..ee97d7d0b9 100644 --- a/comms/dht/src/crypt.rs +++ b/comms/dht/src/crypt.rs @@ -28,7 +28,7 @@ use chacha20::{ Key, Nonce, }; -use digest::Digest; +use digest::{Digest, FixedOutput}; use rand::{rngs::OsRng, RngCore}; use tari_comms::types::{Challenge, CommsPublicKey}; use tari_crypto::{ @@ -80,21 +80,16 @@ pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Vec { let nonce_ga = Nonce::from_slice(&nonce); let mut cipher = ChaCha20::new(&cipher_key.0, nonce_ga); - // Cloning the plain text to avoid a caller thinking we have encrypted in place and losing the integral nonce added - // below - let mut plain_text_clone = plain_text.to_vec(); - - cipher.apply_keystream(plain_text_clone.as_mut_slice()); - - let mut ciphertext_integral_nonce = Vec::with_capacity(nonce.len() + plain_text_clone.len()); - ciphertext_integral_nonce.extend(&nonce); - ciphertext_integral_nonce.append(&mut plain_text_clone); - ciphertext_integral_nonce + let mut buf = vec![0u8; plain_text.len() + nonce.len()]; + buf[..nonce.len()].copy_from_slice(&nonce[..]); + buf[nonce.len()..].copy_from_slice(plain_text); + cipher.apply_keystream(&mut buf[nonce.len()..]); + buf } -/// Generates a challenge for the origin MAC. -pub fn create_origin_mac_challenge(header: &DhtMessageHeader, body: &[u8]) -> Challenge { - create_origin_mac_challenge_parts( +/// Generates a 32-byte hashed challenge that commits to the message header and body +pub fn create_message_challenge(header: &DhtMessageHeader, body: &[u8]) -> [u8; 32] { + create_message_challenge_parts( header.version, &header.destination, header.message_type, @@ -105,8 +100,8 @@ pub fn create_origin_mac_challenge(header: &DhtMessageHeader, body: &[u8]) -> Ch ) } -/// Generates a challenge for the origin MAC. -pub fn create_origin_mac_challenge_parts( +/// Generates a 32-byte hashed challenge that commits to all message parts +pub fn create_message_challenge_parts( protocol_version: DhtProtocolVersion, destination: &NodeDestination, message_type: DhtMessageType, @@ -114,20 +109,27 @@ pub fn create_origin_mac_challenge_parts( expires: Option, ephemeral_public_key: Option<&CommsPublicKey>, body: &[u8], -) -> Challenge { +) -> [u8; 32] { let mut mac_challenge = Challenge::new(); - mac_challenge.update(&protocol_version.to_bytes()); - mac_challenge.update(destination.as_inner_bytes()); + mac_challenge.update(&protocol_version.as_bytes()); + mac_challenge.update(destination.to_inner_bytes()); mac_challenge.update(&(message_type as i32).to_le_bytes()); mac_challenge.update(&flags.bits().to_le_bytes()); - if let Some(t) = expires { - mac_challenge.update(&t.as_u64().to_le_bytes()); - } - if let Some(e_pk) = ephemeral_public_key.as_ref() { - mac_challenge.update(e_pk.as_bytes()); - } + let expires = expires.map(|t| t.as_u64().to_le_bytes()).unwrap_or_default(); + mac_challenge.update(&expires); + + let e_pk = ephemeral_public_key + .map(|e_pk| { + let mut buf = [0u8; 32]; + // CommsPublicKey::as_bytes returns 32-bytes + buf.copy_from_slice(e_pk.as_bytes()); + buf + }) + .unwrap_or_default(); + mac_challenge.update(&e_pk); + mac_challenge.update(&body); - mac_challenge + mac_challenge.finalize_fixed().into() } #[cfg(test)] diff --git a/comms/dht/src/envelope.rs b/comms/dht/src/envelope.rs index ef75e16a3f..97ee8e0e13 100644 --- a/comms/dht/src/envelope.rs +++ b/comms/dht/src/envelope.rs @@ -140,8 +140,8 @@ impl DhtMessageType { pub struct DhtMessageHeader { pub version: DhtProtocolVersion, pub destination: NodeDestination, - /// Encoded DhtOrigin. This can refer to the same peer that sent the message - /// or another peer if the message is being propagated. + /// Encoded OriginMac. Depending on message flags, this may be encrypted. This can refer to the same peer that sent + /// the message or another peer if the message is being propagated. pub origin_mac: Vec, pub ephemeral_public_key: Option, pub message_type: DhtMessageType, @@ -152,7 +152,7 @@ pub struct DhtMessageHeader { impl DhtMessageHeader { pub fn is_valid(&self) -> bool { - if self.flags.contains(DhtMessageFlags::ENCRYPTED) { + if self.flags.is_encrypted() { !self.origin_mac.is_empty() && self.ephemeral_public_key.is_some() } else { true @@ -203,7 +203,7 @@ impl TryFrom for DhtMessageHeader { }; let expires = header.expires.and_then(timestamp_to_datetime); - let version = DhtProtocolVersion::try_from((header.major, header.minor))?; + let version = DhtProtocolVersion::try_from(header.major)?; Ok(Self { version, @@ -234,7 +234,6 @@ impl From for DhtHeader { let expires = header.expires.map(epochtime_to_datetime); Self { major: header.version.as_major(), - minor: header.version.as_minor(), ephemeral_public_key: header .ephemeral_public_key .as_ref() @@ -260,7 +259,7 @@ impl DhtEnvelope { } /// Represents the ways a destination node can be represented. -#[derive(Clone, Debug, Deserialize, Serialize, PartialEq, Eq)] +#[derive(Clone, Debug, PartialEq, Eq)] pub enum NodeDestination { /// The sender has chosen not to disclose the message destination, or the destination is /// the peer being sent to. @@ -272,14 +271,24 @@ pub enum NodeDestination { } impl NodeDestination { - /// Returns the slice of bytes of the `CommsPublicKey` or `NodeId`. Returns an empty slice if the destination is + /// Returns the bytes of the `CommsPublicKey` or `NodeId`. Returns an empty slice if the destination is /// `Unknown`. - pub fn as_inner_bytes(&self) -> &[u8] { - use NodeDestination::{NodeId, PublicKey, Unknown}; + pub fn to_inner_bytes(&self) -> [u8; 33] { + // It is important that there is no ambiguity between fields when e.g. using bytes as part of hash pre-image + // so each type of NodeDestination is assigned a value that differentiates them. + let mut buf = [0u8; 33]; match self { - Unknown => &[], - PublicKey(pk) => pk.as_bytes(), - NodeId(node_id) => node_id.as_bytes(), + NodeDestination::Unknown => buf, + NodeDestination::PublicKey(pk) => { + buf[0] = 1; + buf[1..].copy_from_slice(pk.as_bytes()); + buf + }, + NodeDestination::NodeId(node_id) => { + buf[0] = 2; + buf[1..=NodeId::byte_size()].copy_from_slice(node_id.as_bytes()); + buf + }, } } @@ -401,3 +410,28 @@ impl From for Destination { } } } + +#[cfg(test)] +mod tests { + use super::*; + + mod node_destination { + use rand::rngs::OsRng; + use tari_crypto::keys::PublicKey; + use tari_utilities::hex::{to_hex, Hex}; + + use super::*; + + #[test] + fn to_inner_bytes() { + assert!(NodeDestination::Unknown.to_inner_bytes().iter().all(|b| *b == 0)); + let (_, pk) = CommsPublicKey::random_keypair(&mut OsRng); + assert!(to_hex(&NodeDestination::PublicKey(Box::new(pk.clone())).to_inner_bytes()).contains(&pk.to_hex())); + let node_id = NodeId::from_public_key(&pk); + assert!( + to_hex(&NodeDestination::NodeId(Box::new(node_id.clone())).to_inner_bytes()) + .contains(&node_id.to_hex()) + ); + } + } +} diff --git a/comms/dht/src/inbound/decryption.rs b/comms/dht/src/inbound/decryption.rs index 941aa2290b..0b0207bd58 100644 --- a/comms/dht/src/inbound/decryption.rs +++ b/comms/dht/src/inbound/decryption.rs @@ -20,7 +20,7 @@ // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -use std::{sync::Arc, task::Poll, time::Duration}; +use std::{convert::TryInto, sync::Arc, task::Poll, time::Duration}; use futures::{future::BoxFuture, task::Context}; use log::*; @@ -30,10 +30,7 @@ use tari_comms::{ message::EnvelopeBody, peer_manager::NodeIdentity, pipeline::PipelineError, - types::{Challenge, CommsPublicKey}, - utils::signature, }; -use tari_utilities::ByteArray; use thiserror::Error; use tower::{layer::Layer, Service, ServiceExt}; @@ -42,7 +39,7 @@ use crate::{ crypt::CipherKey, envelope::DhtMessageHeader, inbound::message::{DecryptedDhtMessage, DhtInboundMessage}, - proto::envelope::OriginMac, + origin_mac::{OriginMac, OriginMacError, ProtoOriginMac}, DhtConfig, }; @@ -52,14 +49,14 @@ const LOG_TARGET: &str = "comms::middleware::decryption"; enum DecryptionError { #[error("Failed to validate origin MAC signature")] OriginMacInvalidSignature, - #[error("Origin MAC contained an invalid public key")] - OriginMacInvalidPublicKey, #[error("Origin MAC not provided for encrypted message")] OriginMacNotProvided, #[error("Failed to decrypt origin MAC")] OriginMacDecryptedFailed, #[error("Failed to decode clear-text origin MAC")] OriginMacClearTextDecodeFailed, + #[error("Origin MAC error: {0}")] + OriginMacError(#[from] OriginMacError), #[error("Ephemeral public key not provided for encrypted message")] EphemeralKeyNotProvided, #[error("Message rejected because this node could not decrypt a message that was addressed to it")] @@ -175,7 +172,8 @@ where S: Service Err(err @ OriginMacNotProvided) | Err(err @ EphemeralKeyNotProvided) | Err(err @ EncryptedMessageNoDestination) | - Err(err @ OriginMacInvalidSignature) => { + Err(err @ OriginMacInvalidSignature) | + Err(err @ OriginMacError(_)) => { // This message should not have been propagated, or has been manipulated in some way. Ban the source of // this message. connectivity @@ -203,10 +201,8 @@ where S: Service ) -> Result { let dht_header = &message.dht_header; - let mac_challenge = crypt::create_origin_mac_challenge(dht_header, &message.body); - if !dht_header.flags.is_encrypted() { - return Self::success_not_encrypted(message, mac_challenge).await; + return Self::success_not_encrypted(message).await; } trace!( target: LOG_TARGET, @@ -230,11 +226,12 @@ where S: Service // Decrypt and verify the origin let authenticated_origin = match Self::attempt_decrypt_origin_mac(&shared_secret, dht_header) { - Ok((public_key, signature)) => { + Ok(origin_mac) => { // If this fails, discard the message because we decrypted and deserialized the message with our shared // ECDH secret but the message could not be authenticated - Self::authenticate_origin_mac(&public_key, &signature, mac_challenge)?; - public_key + let msg_challenge = crypt::create_message_challenge(&message.dht_header, &message.body); + Self::authenticate_origin_mac(&origin_mac, &msg_challenge)?; + origin_mac.into_signer_public_key() }, Err(err) => { trace!( @@ -304,7 +301,7 @@ where S: Service fn attempt_decrypt_origin_mac( shared_secret: &CipherKey, dht_header: &DhtMessageHeader, - ) -> Result<(CommsPublicKey, Vec), DecryptionError> { + ) -> Result { let encrypted_origin_mac = Some(&dht_header.origin_mac) .filter(|b| !b.is_empty()) // This should not have been sent/propagated @@ -312,21 +309,15 @@ where S: Service let decrypted_bytes = crypt::decrypt(shared_secret, encrypted_origin_mac) .map_err(|_| DecryptionError::OriginMacDecryptedFailed)?; - let origin_mac = - OriginMac::decode(decrypted_bytes.as_slice()).map_err(|_| DecryptionError::OriginMacDecryptedFailed)?; - // Check the public key here, because it is possible (rare but possible) for an failed decrypted message to pass - // protobuf decoding of the relatively simple OriginMac struct but with invalid data - let public_key = CommsPublicKey::from_bytes(&origin_mac.public_key) - .map_err(|_| DecryptionError::OriginMacInvalidPublicKey)?; - Ok((public_key, origin_mac.signature)) + let origin_mac = ProtoOriginMac::decode(decrypted_bytes.as_slice()) + .map_err(|_| DecryptionError::OriginMacDecryptedFailed)?; + + let origin_mac = origin_mac.try_into()?; + Ok(origin_mac) } - fn authenticate_origin_mac( - public_key: &CommsPublicKey, - signature: &[u8], - challenge: Challenge, - ) -> Result<(), DecryptionError> { - if signature::verify_challenge(public_key, signature, challenge) { + fn authenticate_origin_mac(origin_mac: &OriginMac, message: &[u8]) -> Result<(), DecryptionError> { + if origin_mac.verify(message) { Ok(()) } else { Err(DecryptionError::OriginMacInvalidSignature) @@ -367,20 +358,18 @@ where S: Service .map_err(|_| DecryptionError::MessageBodyDecryptionFailed) } - async fn success_not_encrypted( - message: DhtInboundMessage, - mac_challenge: Challenge, - ) -> Result { + async fn success_not_encrypted(message: DhtInboundMessage) -> Result { let authenticated_pk = if message.dht_header.origin_mac.is_empty() { None } else { - let origin_mac = OriginMac::decode(message.dht_header.origin_mac.as_slice()) - .map_err(|_| DecryptionError::OriginMacClearTextDecodeFailed)?; - let public_key = CommsPublicKey::from_bytes(&origin_mac.public_key) - .map_err(|_| DecryptionError::OriginMacInvalidPublicKey)?; + let origin_mac: OriginMac = ProtoOriginMac::decode(message.dht_header.origin_mac.as_slice()) + .map_err(|_| DecryptionError::OriginMacClearTextDecodeFailed)? + .try_into()?; + + let mac_challenge = crypt::create_message_challenge(&message.dht_header, &message.body); - Self::authenticate_origin_mac(&public_key, &origin_mac.signature, mac_challenge)?; - Some(public_key) + Self::authenticate_origin_mac(&origin_mac, &mac_challenge)?; + Some(origin_mac.into_signer_public_key()) }; match EnvelopeBody::decode(message.body.as_slice()) { diff --git a/comms/dht/src/lib.rs b/comms/dht/src/lib.rs index aa3a81e9d7..69b4b31c9b 100644 --- a/comms/dht/src/lib.rs +++ b/comms/dht/src/lib.rs @@ -102,6 +102,7 @@ pub use dedup::DedupLayer; mod filter; mod logging_middleware; +mod origin_mac; mod peer_validator; mod proto; mod rpc; diff --git a/comms/dht/src/origin_mac.rs b/comms/dht/src/origin_mac.rs new file mode 100644 index 0000000000..ac5c59a01a --- /dev/null +++ b/comms/dht/src/origin_mac.rs @@ -0,0 +1,183 @@ +// Copyright 2022, The Tari Project +// +// Redistribution and use in source and binary forms, with or without modification, are permitted provided that the +// following conditions are met: +// +// 1. Redistributions of source code must retain the above copyright notice, this list of conditions and the following +// disclaimer. +// +// 2. Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the +// following disclaimer in the documentation and/or other materials provided with the distribution. +// +// 3. Neither the name of the copyright holder nor the names of its contributors may be used to endorse or promote +// products derived from this software without specific prior written permission. +// +// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, +// INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE +// DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR +// SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, +// WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE +// USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + +use std::convert::TryFrom; + +use digest::{Digest, FixedOutput}; +use rand::rngs::OsRng; +use tari_comms::types::{Challenge, CommsPublicKey, CommsSecretKey, Signature}; +use tari_crypto::keys::PublicKey; +use tari_utilities::ByteArray; + +#[derive(Debug, Clone)] +pub struct OriginMac { + signer_public_key: CommsPublicKey, + signature: Signature, +} + +fn construct_origin_mac_hash( + signer_public_key: &CommsPublicKey, + public_nonce: &CommsPublicKey, + message: &[u8], +) -> [u8; 32] { + // e = H_mac(P||R||m) + Challenge::with_params(&[], &[], b"TARIDHTORIGINMAC") + .chain(signer_public_key.as_bytes()) + .chain(public_nonce.as_bytes()) + .chain(message) + .finalize_fixed() + .into() +} + +impl OriginMac { + /// Create a new signed [OriginMac](self::OriginMac) for the given message. + pub fn new_signed(signer_secret_key: CommsSecretKey, message: &[u8]) -> Self { + let (nonce_s, nonce_pk) = CommsPublicKey::random_keypair(&mut OsRng); + let signer_public_key = CommsPublicKey::from_secret_key(&signer_secret_key); + let challenge = construct_origin_mac_hash(&signer_public_key, &nonce_pk, message); + let signature = Signature::sign(signer_secret_key, nonce_s, &challenge) + .expect("challenge is [u8;32] but SchnorrSignature::sign failed"); + + Self { + signer_public_key, + signature, + } + } + + /// Returns true if the provided message valid for this origin MAC, otherwise false. + pub fn verify(&self, message: &[u8]) -> bool { + let challenge = construct_origin_mac_hash(&self.signer_public_key, self.signature.get_public_nonce(), message); + self.signature.verify_challenge(&self.signer_public_key, &challenge) + } + + /// Consume this instance, returning the public key of the signer. + pub fn into_signer_public_key(self) -> CommsPublicKey { + self.signer_public_key + } + + /// Converts to a protobuf struct + pub fn to_proto(&self) -> ProtoOriginMac { + ProtoOriginMac { + signer_public_key: self.signer_public_key.to_vec(), + public_nonce: self.signature.get_public_nonce().to_vec(), + signature: self.signature.get_signature().to_vec(), + } + } +} + +impl TryFrom for OriginMac { + type Error = OriginMacError; + + fn try_from(origin_mac: ProtoOriginMac) -> Result { + let signer_public_key = CommsPublicKey::from_bytes(&origin_mac.signer_public_key) + .map_err(|_| OriginMacError::InvalidSignerPublicKey)?; + + let public_nonce = + CommsPublicKey::from_bytes(&origin_mac.public_nonce).map_err(|_| OriginMacError::InvalidPublicNonce)?; + + let signature = + CommsSecretKey::from_bytes(&origin_mac.signature).map_err(|_| OriginMacError::InvalidSignature)?; + + Ok(Self { + signer_public_key, + signature: Signature::new(public_nonce, signature), + }) + } +} + +/// The Message Authentication Code (MAC) message format of the decrypted `DhtHeader::origin_mac` field +#[derive(Clone, prost::Message)] +pub struct ProtoOriginMac { + #[prost(bytes, tag = "1")] + pub signer_public_key: Vec, + #[prost(bytes, tag = "2")] + pub public_nonce: Vec, + #[prost(bytes, tag = "3")] + pub signature: Vec, +} + +#[derive(Debug, thiserror::Error)] +pub enum OriginMacError { + #[error("Failed to decrypt origin MAC")] + DecryptedFailed, + #[error("Failed to validate origin MAC signature")] + InvalidSignature, + #[error("Origin MAC contained an invalid public nonce")] + InvalidPublicNonce, + #[error("Origin MAC contained an invalid signer public key")] + InvalidSignerPublicKey, + #[error("Origin MAC failed to verify")] + VerificationFailed, +} + +#[cfg(test)] +mod test { + use tari_crypto::keys::SecretKey; + + use super::*; + const MSG: &[u8] = b"100% genuine"; + + fn setup() -> (OriginMac, CommsSecretKey) { + let signer_k = CommsSecretKey::random(&mut OsRng); + (OriginMac::new_signed(signer_k.clone(), MSG), signer_k) + } + + #[test] + fn it_secures_the_message() { + let (mac, _) = setup(); + assert!(mac.verify(MSG)); + assert!(!mac.verify(b"99.9% genuine")); + } + + #[test] + fn it_is_secure_against_related_key_attack() { + let (mut mac, signer_k) = setup(); + let signer_pk = CommsPublicKey::from_secret_key(&signer_k); + let msg = construct_origin_mac_hash(&signer_pk, mac.signature.get_public_nonce(), MSG); + let msg_scalar = CommsSecretKey::from_bytes(&msg).unwrap(); + + // Some `a` key + let (bad_signer_k, bad_signer_pk) = CommsPublicKey::random_keypair(&mut OsRng); + mac.signer_public_key = &bad_signer_pk + &signer_pk; + // s' = s + e.a + mac.signature = Signature::new( + mac.signature.get_public_nonce().clone(), + mac.signature.get_signature() + (&msg_scalar * bad_signer_k), + ); + + assert!(!mac.verify(MSG)); + } + + #[test] + fn it_secures_the_public_nonce() { + let (mut mac, signer_k) = setup(); + let (nonce_k, _) = CommsPublicKey::random_keypair(&mut OsRng); + // Get the original hashed challenge + let signer_pk = CommsPublicKey::from_secret_key(&signer_k); + let msg = construct_origin_mac_hash(&signer_pk, mac.signature.get_public_nonce(), MSG); + + // Change to . Note: We need signer_k because the Signature interface does not provide a way to + // change just the public nonce, an attacker does not need the secret key. + mac.signature = Signature::sign(signer_k, nonce_k, &msg).unwrap(); + assert!(!mac.verify(MSG)); + } +} diff --git a/comms/dht/src/outbound/broadcast.rs b/comms/dht/src/outbound/broadcast.rs index 5fb8f9cea7..0169492a2e 100644 --- a/comms/dht/src/outbound/broadcast.rs +++ b/comms/dht/src/outbound/broadcast.rs @@ -36,11 +36,10 @@ use tari_comms::{ message::{MessageExt, MessageTag}, peer_manager::{NodeId, NodeIdentity, Peer}, pipeline::PipelineError, - types::{Challenge, CommsPublicKey}, - utils::signature, + types::CommsPublicKey, }; -use tari_crypto::keys::PublicKey; -use tari_utilities::{epoch_time::EpochTime, hex::Hex, message_format::MessageFormat, ByteArray}; +use tari_crypto::{keys::PublicKey, tari_utilities::epoch_time::EpochTime}; +use tari_utilities::{hex::Hex, ByteArray}; use tokio::sync::oneshot; use tower::{layer::Layer, Service, ServiceExt}; @@ -52,13 +51,14 @@ use crate::{ dedup, discovery::DhtDiscoveryRequester, envelope::{datetime_to_epochtime, datetime_to_timestamp, DhtMessageFlags, DhtMessageHeader, NodeDestination}, + origin_mac::OriginMac, outbound::{ message::{DhtOutboundMessage, OutboundEncryption, SendFailure}, message_params::FinalSendMessageParams, message_send_state::MessageSendState, SendMessageResponse, }, - proto::envelope::{DhtMessageType, OriginMac}, + proto::envelope::DhtMessageType, version::DhtProtocolVersion, DhtConfig, }; @@ -499,7 +499,7 @@ where S: Service // Encrypt the message with the body let encrypted_body = crypt::encrypt(&shared_ephemeral_secret, &body); - let mac_challenge = crypt::create_origin_mac_challenge_parts( + let mac_challenge = crypt::create_message_challenge_parts( self.protocol_version, destination, message_type, @@ -509,9 +509,10 @@ where S: Service &encrypted_body, ); // Sign the encrypted message - let origin_mac = create_origin_mac(&self.node_identity, mac_challenge)?; + let origin_mac = + OriginMac::new_signed(self.node_identity.secret_key().clone(), &mac_challenge).to_proto(); // Encrypt and set the origin field - let encrypted_origin_mac = crypt::encrypt(&shared_ephemeral_secret, &origin_mac); + let encrypted_origin_mac = crypt::encrypt(&shared_ephemeral_secret, &origin_mac.to_encoded_bytes()); Ok(( Some(Arc::new(e_public_key)), Some(encrypted_origin_mac.into()), @@ -522,7 +523,7 @@ where S: Service trace!(target: LOG_TARGET, "Encryption not requested for message"); if include_origin { - let mac_challenge = crypt::create_origin_mac_challenge_parts( + let mac_challenge = crypt::create_message_challenge_parts( self.protocol_version, destination, message_type, @@ -531,8 +532,9 @@ where S: Service None, &body, ); - let origin_mac = create_origin_mac(&self.node_identity, mac_challenge)?; - Ok((None, Some(origin_mac.into()), body)) + let origin_mac = + OriginMac::new_signed(self.node_identity.secret_key().clone(), &mac_challenge).to_proto(); + Ok((None, Some(origin_mac.to_encoded_bytes().into()), body)) } else { Ok((None, None, body)) } @@ -541,15 +543,6 @@ where S: Service } } -fn create_origin_mac(node_identity: &NodeIdentity, mac_challenge: Challenge) -> Result, DhtOutboundError> { - let signature = signature::sign_challenge(&mut OsRng, node_identity.secret_key().clone(), mac_challenge)?; - let mac = OriginMac { - public_key: node_identity.public_key().to_vec(), - signature: signature.to_binary()?, - }; - Ok(mac.to_encoded_bytes()) -} - #[cfg(test)] mod test { use std::time::Duration; diff --git a/comms/dht/src/outbound/serialize.rs b/comms/dht/src/outbound/serialize.rs index f4947a1e70..592fde4c5d 100644 --- a/comms/dht/src/outbound/serialize.rs +++ b/comms/dht/src/outbound/serialize.rs @@ -88,8 +88,7 @@ where destination_node_id.short_str() ); let dht_header = custom_header.map(DhtHeader::from).unwrap_or_else(|| DhtHeader { - major: protocol_version.as_major(), - minor: protocol_version.as_minor(), + major: protocol_version.as_major() as u32, origin_mac: origin_mac.map(|b| b.to_vec()).unwrap_or_else(Vec::new), ephemeral_public_key: ephemeral_public_key.map(|e| e.to_vec()).unwrap_or_else(Vec::new), message_type: dht_message_type as i32, diff --git a/comms/dht/src/proto/envelope.proto b/comms/dht/src/proto/envelope.proto index ce8575f804..65ef739c08 100644 --- a/comms/dht/src/proto/envelope.proto +++ b/comms/dht/src/proto/envelope.proto @@ -24,7 +24,7 @@ enum DhtMessageType { message DhtHeader { uint32 major = 1; - uint32 minor = 2; +// uint32 minor = 2; oneof destination { // The sender has chosen not to disclose the message destination bool unknown = 3; @@ -55,8 +55,3 @@ message DhtEnvelope { bytes body = 2; } -// The Message Authentication Code (MAC) message format of the decrypted `DhtHeader::origin_mac` field -message OriginMac { - bytes public_key = 1; - bytes signature = 2; -} diff --git a/comms/dht/src/store_forward/error.rs b/comms/dht/src/store_forward/error.rs index f44200946a..5876bc92be 100644 --- a/comms/dht/src/store_forward/error.rs +++ b/comms/dht/src/store_forward/error.rs @@ -30,7 +30,13 @@ use tari_comms::{ use tari_utilities::byte_array::ByteArrayError; use thiserror::Error; -use crate::{actor::DhtActorError, envelope::DhtMessageError, outbound::DhtOutboundError, storage::StorageError}; +use crate::{ + actor::DhtActorError, + envelope::DhtMessageError, + origin_mac::OriginMacError, + outbound::DhtOutboundError, + storage::StorageError, +}; /// Error type for SAF #[derive(Debug, Error)] @@ -45,8 +51,8 @@ pub enum StoreAndForwardError { DhtOutboundError(#[from] DhtOutboundError), #[error("Received stored message has an invalid destination")] InvalidDestination, - #[error("Received stored message has an invalid origin signature")] - InvalidOriginMac, + #[error("Received stored message has an invalid origin signature: {0}")] + InvalidOriginMac(#[from] OriginMacError), #[error("Invalid envelope body")] InvalidEnvelopeBody, #[error("DHT header is invalid")] diff --git a/comms/dht/src/store_forward/saf_handler/task.rs b/comms/dht/src/store_forward/saf_handler/task.rs index 59c5d6df08..eb8b553dc3 100644 --- a/comms/dht/src/store_forward/saf_handler/task.rs +++ b/comms/dht/src/store_forward/saf_handler/task.rs @@ -20,7 +20,10 @@ // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -use std::{convert::TryInto, sync::Arc}; +use std::{ + convert::{TryFrom, TryInto}, + sync::Arc, +}; use chrono::{DateTime, NaiveDateTime, Utc}; use futures::{future, stream, StreamExt}; @@ -30,8 +33,7 @@ use tari_comms::{ message::{EnvelopeBody, MessageTag}, peer_manager::{NodeId, NodeIdentity, Peer, PeerFeatures, PeerManager, PeerManagerError}, pipeline::PipelineError, - types::{Challenge, CommsPublicKey}, - utils::signature, + types::CommsPublicKey, }; use tari_utilities::{convert::try_convert_all, ByteArray}; use tokio::sync::mpsc; @@ -41,11 +43,12 @@ use crate::{ actor::DhtRequester, crypt, dedup, - envelope::{timestamp_to_datetime, DhtMessageFlags, DhtMessageHeader, NodeDestination}, + envelope::{timestamp_to_datetime, DhtMessageHeader, NodeDestination}, inbound::{DecryptedDhtMessage, DhtInboundMessage}, + origin_mac::{OriginMac, OriginMacError, ProtoOriginMac}, outbound::{OutboundMessageRequester, SendMessageParams}, proto::{ - envelope::{DhtMessageType, OriginMac}, + envelope::DhtMessageType, store_forward::{ stored_messages_response::SafResponseType, StoredMessage as ProtoStoredMessage, @@ -552,7 +555,7 @@ where S: Service header: &DhtMessageHeader, body: &[u8], ) -> Result<(Option, EnvelopeBody), StoreAndForwardError> { - if header.flags.contains(DhtMessageFlags::ENCRYPTED) { + if header.flags.is_encrypted() { let ephemeral_public_key = header.ephemeral_public_key.as_ref().expect( "[store and forward] DHT header is invalid after validity check because it did not contain an \ ephemeral_public_key", @@ -565,8 +568,7 @@ where S: Service ); let shared_secret = crypt::generate_ecdh_secret(node_identity.secret_key(), ephemeral_public_key); let decrypted = crypt::decrypt(&shared_secret, &header.origin_mac)?; - let mac_challenge = crypt::create_origin_mac_challenge(header, body); - let authenticated_pk = Self::authenticate_message(&decrypted, mac_challenge)?; + let authenticated_pk = Self::authenticate_message(&decrypted, header, body)?; trace!( target: LOG_TARGET, @@ -584,8 +586,7 @@ where S: Service let authenticated_pk = if header.origin_mac.is_empty() { None } else { - let mac_challenge = crypt::create_origin_mac_challenge(header, body); - Some(Self::authenticate_message(&header.origin_mac, mac_challenge)?) + Some(Self::authenticate_message(&header.origin_mac, header, body)?) }; let envelope_body = EnvelopeBody::decode(body).map_err(|_| StoreAndForwardError::MalformedMessage)?; Ok((authenticated_pk, envelope_body)) @@ -593,17 +594,21 @@ where S: Service } fn authenticate_message( - origin_mac_body: &[u8], - challenge: Challenge, + cleartext_origin_mac_body: &[u8], + header: &DhtMessageHeader, + body: &[u8], ) -> Result { - let origin_mac = OriginMac::decode(origin_mac_body)?; - let public_key = - CommsPublicKey::from_bytes(&origin_mac.public_key).map_err(|_| StoreAndForwardError::InvalidOriginMac)?; + let origin_mac = ProtoOriginMac::decode(cleartext_origin_mac_body)?; + let origin_mac = OriginMac::try_from(origin_mac)?; - if signature::verify_challenge(&public_key, &origin_mac.signature, challenge) { - Ok(public_key) + let challenge = crypt::create_message_challenge(header, body); + + if origin_mac.verify(&challenge) { + Ok(origin_mac.into_signer_public_key()) } else { - Err(StoreAndForwardError::InvalidOriginMac) + Err(StoreAndForwardError::InvalidOriginMac( + OriginMacError::VerificationFailed, + )) } } diff --git a/comms/dht/src/test_utils/makers.rs b/comms/dht/src/test_utils/makers.rs index 7354d1e72a..e2f14ce75a 100644 --- a/comms/dht/src/test_utils/makers.rs +++ b/comms/dht/src/test_utils/makers.rs @@ -27,14 +27,10 @@ use tari_comms::{ multiaddr::Multiaddr, peer_manager::{NodeId, NodeIdentity, Peer, PeerFeatures, PeerFlags, PeerManager}, transports::MemoryTransport, - types::{Challenge, CommsDatabase, CommsPublicKey, CommsSecretKey}, - utils::signature, + types::{CommsDatabase, CommsPublicKey, CommsSecretKey}, Bytes, }; -use tari_crypto::{ - keys::PublicKey, - tari_utilities::{message_format::MessageFormat, ByteArray}, -}; +use tari_crypto::keys::PublicKey; use tari_storage::lmdb_store::{LMDBBuilder, LMDBConfig}; use tari_test_utils::{paths::create_temporary_data_path, random}; @@ -42,8 +38,9 @@ use crate::{ crypt, envelope::{DhtMessageFlags, DhtMessageHeader, NodeDestination}, inbound::DhtInboundMessage, + origin_mac::OriginMac, outbound::message::DhtOutboundMessage, - proto::envelope::{DhtEnvelope, DhtMessageType, OriginMac}, + proto::envelope::{DhtEnvelope, DhtMessageType}, version::DhtProtocolVersion, }; @@ -88,7 +85,7 @@ pub fn make_dht_header( let mut origin_mac = Vec::new(); if include_origin { - let challenge = crypt::create_origin_mac_challenge_parts( + let challenge = crypt::create_message_challenge_parts( DhtProtocolVersion::latest(), &destination, DhtMessageType::None, @@ -97,7 +94,7 @@ pub fn make_dht_header( Some(e_public_key), message, ); - origin_mac = make_valid_origin_mac(node_identity, challenge); + origin_mac = make_valid_origin_mac(node_identity, &challenge); if flags.is_encrypted() { let shared_secret = crypt::generate_ecdh_secret(e_secret_key, node_identity.public_key()); origin_mac = crypt::encrypt(&shared_secret, &origin_mac); @@ -119,15 +116,10 @@ pub fn make_dht_header( } } -pub fn make_valid_origin_mac(node_identity: &NodeIdentity, challenge: Challenge) -> Vec { - let mac = OriginMac { - public_key: node_identity.public_key().to_vec(), - signature: signature::sign_challenge(&mut OsRng, node_identity.secret_key().clone(), challenge) - .unwrap() - .to_binary() - .unwrap(), - }; - mac.to_encoded_bytes() +pub fn make_valid_origin_mac(node_identity: &NodeIdentity, message: &[u8]) -> Vec { + OriginMac::new_signed(node_identity.secret_key().clone(), message) + .to_proto() + .to_encoded_bytes() } pub fn make_dht_inbound_message( diff --git a/comms/dht/src/version.rs b/comms/dht/src/version.rs index 26fc01b923..8a2816cf7f 100644 --- a/comms/dht/src/version.rs +++ b/comms/dht/src/version.rs @@ -21,10 +21,9 @@ // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use std::{ - convert::{TryFrom, TryInto}, + convert::TryFrom, fmt, fmt::{Display, Formatter}, - io::Write, }; use serde::{Deserialize, Serialize}; @@ -35,8 +34,8 @@ use crate::envelope::DhtMessageError; #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(try_from = "u32", into = "u32")] pub enum DhtProtocolVersion { - V1 { minor: u32 }, - V2 { minor: u32 }, + V1 = 1, + V2, } impl DhtProtocolVersion { @@ -47,44 +46,28 @@ impl DhtProtocolVersion { /// Returns v1 version pub fn v1() -> Self { - DhtProtocolVersion::V1 { minor: 0 } + DhtProtocolVersion::V1 } /// Returns v2 version pub fn v2() -> Self { - DhtProtocolVersion::V2 { minor: 0 } + DhtProtocolVersion::V2 } /// Returns the byte representation for the version - pub fn to_bytes(self) -> Vec { - let mut buf = Vec::with_capacity(4 * 2); - buf.write_all(&self.as_major().to_le_bytes()).unwrap(); - buf.write_all(&self.as_minor().to_le_bytes()).unwrap(); - buf + pub fn as_bytes(self) -> [u8; 4] { + self.as_major().to_le_bytes() } /// Returns the major version number pub fn as_major(&self) -> u32 { - use DhtProtocolVersion::{V1, V2}; - match self { - V1 { .. } => 1, - V2 { .. } => 2, - } - } - - /// Returns the minor version number - pub fn as_minor(&self) -> u32 { - use DhtProtocolVersion::{V1, V2}; - match self { - V1 { minor } => *minor, - V2 { minor } => *minor, - } + *self as u32 } } impl Display for DhtProtocolVersion { fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { - write!(f, "v{}.{}", self.as_major(), self.as_minor()) + write!(f, "v{}", self.as_major()) } } @@ -92,19 +75,12 @@ impl TryFrom for DhtProtocolVersion { type Error = DhtMessageError; fn try_from(value: u32) -> Result { - (value, 0).try_into() - } -} - -impl TryFrom<(u32, u32)> for DhtProtocolVersion { - type Error = DhtMessageError; - - fn try_from((major, minor): (u32, u32)) -> Result { - use DhtProtocolVersion::{V1, V2}; - match major { - 0..=1 => Ok(V1 { minor }), - 2 => Ok(V2 { minor }), - n => Err(DhtMessageError::InvalidProtocolVersion(n)), + if value == DhtProtocolVersion::V1 as u32 { + Ok(DhtProtocolVersion::V1) + } else if value == DhtProtocolVersion::V2 as u32 { + Ok(DhtProtocolVersion::V2) + } else { + Err(DhtMessageError::InvalidProtocolVersion(value)) } } } From 5ac6133a8ab0707dfd97cf1647d709256bb9c05b Mon Sep 17 00:00:00 2001 From: Stan Bondi Date: Tue, 24 May 2022 16:28:33 +0400 Subject: [PATCH 3/3] fix(comms)!: commit to public key and nonce in identity sig (#3928) Description --- - commit to public nonce and public key on identity signature Motivation and Context --- Schnorr signatures are not secure unless public nonce and public key are committed to How Has This Been Tested? --- Existing tests (internal to identity signature) --- .../src/peer_manager/identity_signature.rs | 32 +++++++++++++++---- 1 file changed, 26 insertions(+), 6 deletions(-) diff --git a/comms/core/src/peer_manager/identity_signature.rs b/comms/core/src/peer_manager/identity_signature.rs index 085731c80e..f1f2d3d39e 100644 --- a/comms/core/src/peer_manager/identity_signature.rs +++ b/comms/core/src/peer_manager/identity_signature.rs @@ -27,7 +27,7 @@ use digest::Digest; use prost::Message; use rand::rngs::OsRng; use serde::{Deserialize, Serialize}; -use tari_crypto::keys::SecretKey; +use tari_crypto::keys::PublicKey as PublicKeyTrait; use tari_utilities::ByteArray; use crate::{ @@ -64,9 +64,17 @@ impl IdentitySignature { addresses: I, updated_at: DateTime, ) -> Self { - let challenge = Self::construct_challenge(Self::LATEST_VERSION, features, addresses, updated_at); - let nonce = CommsSecretKey::random(&mut OsRng); - let signature = Signature::sign(secret_key.clone(), nonce, &challenge.finalize()) + let public_key = CommsPublicKey::from_secret_key(secret_key); + let (secret_nonce, public_nonce) = CommsPublicKey::random_keypair(&mut OsRng); + let challenge = Self::construct_challenge( + &public_key, + &public_nonce, + Self::LATEST_VERSION, + features, + addresses, + updated_at, + ); + let signature = Signature::sign(secret_key.clone(), secret_nonce, &challenge.finalize()) .expect("unreachable panic: challenge hash digest is the correct length"); Self { version: Self::LATEST_VERSION, @@ -110,17 +118,29 @@ impl IdentitySignature { return false; } - let challenge = Self::construct_challenge(self.version, features, addresses, self.updated_at); + let challenge = Self::construct_challenge( + public_key, + self.signature.get_public_nonce(), + self.version, + features, + addresses, + self.updated_at, + ); self.signature.verify_challenge(public_key, &challenge.finalize()) } fn construct_challenge<'a, I: IntoIterator>( + public_key: &CommsPublicKey, + public_nonce: &CommsPublicKey, version: u8, features: PeerFeatures, addresses: I, updated_at: DateTime, ) -> Challenge { + // e = H(P||R||m) let challenge = Challenge::new() + .chain(public_key.as_bytes()) + .chain(public_nonce.as_bytes()) .chain(version.to_le_bytes()) .chain(u64::try_from(updated_at.timestamp()).unwrap().to_le_bytes()) .chain(features.bits().to_le_bytes()); @@ -177,7 +197,7 @@ impl From<&IdentitySignature> for proto::identity::IdentitySignature { mod test { use std::str::FromStr; - use tari_crypto::keys::PublicKey; + use tari_crypto::keys::{PublicKey, SecretKey}; use super::*; use crate::peer_manager::{NodeId, PeerFlags};