Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dht)!: fixes MAC related key vuln for propagated cleartext msgs #3907

Merged
merged 2 commits into from
May 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 23 additions & 13 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion comms/core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
10 changes: 4 additions & 6 deletions comms/core/src/peer_manager/node_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ use std::{

use blake2::{
digest::{Update, VariableOutput},
VarBlake2b,
Blake2bVar,
};
use serde::{de, Deserialize, Deserializer, Serialize};
use tari_utilities::{
Expand Down Expand Up @@ -74,13 +74,11 @@ impl NodeId {
pub fn from_key<K: ByteArray>(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)
}

Comment on lines 74 to 84
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hash invocation should use domain separation to avoid unexpected collisions elsewhere. This is supported directly using Blake2 personalization.

Expand Down
1 change: 0 additions & 1 deletion comms/core/src/utils/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,3 @@ pub mod cidr;
pub mod datetime;
pub mod mpsc;
pub mod multiaddr;
pub mod signature;
51 changes: 0 additions & 51 deletions comms/core/src/utils/signature.rs

This file was deleted.

2 changes: 1 addition & 1 deletion comms/dht/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
54 changes: 28 additions & 26 deletions comms/dht/src/crypt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand Down Expand Up @@ -80,21 +80,16 @@ pub fn encrypt(cipher_key: &CipherKey, plain_text: &[u8]) -> Vec<u8> {
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,
Expand All @@ -105,29 +100,36 @@ 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,
flags: DhtMessageFlags,
expires: Option<EpochTime>,
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());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this guaranteed to have a fixed-length byte representation?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this has a fixed-length representation, and types are differentiated using a flag.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible, if desired, to use variable-length representations. In this case, the length (as a fixed-length representation!) should be prepended after the (fixed-length) flag.

Copy link
Member Author

@sdbondi sdbondi May 24, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right, each variant of NodeDestination has a single byte representation prepended to the encoding. Gotcha, both the NodeId and CommsPublicKey have fixed-length representations so I don't think we'll have a need for variable-length encodings here.

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)]
Expand Down
Loading