Skip to content

Commit

Permalink
fix(comms): validate onion3 checksum (#5440)
Browse files Browse the repository at this point in the history
Description
---
Validates onion3 checksum

Motivation and Context
---
Spec:
https://github.com/torproject/torspec/blob/main/rend-spec-v3.txt#L2258

Previously invalid onion addresses would be accepted. 

How Has This Been Tested?
---
New unit test

What process can a PR reviewer use to test or verify this change?
---
Look at unit test. Network still works.

Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
  • Loading branch information
sdbondi committed Jun 6, 2023
1 parent 602f416 commit 0dfdb3a
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 10 deletions.
1 change: 1 addition & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions comms/core/Cargo.toml
Expand Up @@ -39,6 +39,7 @@ prost = "=0.9.0"
rand = "0.7.3"
serde = "1.0.119"
serde_derive = "1.0.119"
sha3 = "0.9"
snow = { version = "=0.9.1", features = ["default-resolver"] }
thiserror = "1.0.26"
tokio = { version = "1.23", features = ["rt-multi-thread", "time", "sync", "signal", "net", "macros", "io-util"] }
Expand Down
78 changes: 68 additions & 10 deletions comms/core/src/connection_manager/common.rs
Expand Up @@ -22,6 +22,7 @@

use std::{convert::TryInto, net::Ipv6Addr};

use digest::Digest;
use log::*;
use tokio::io::{AsyncRead, AsyncWrite};

Expand Down Expand Up @@ -224,14 +225,6 @@ fn validate_address(addr: &Multiaddr, allow_test_addrs: bool) -> Result<(), Conn
.next()
.ok_or_else(|| ConnectionManagerError::InvalidMultiaddr("Multiaddr was empty".to_string()))?;

let expect_end_of_address = |mut iter: multiaddr::Iter<'_>| match iter.next() {
Some(p) => Err(ConnectionManagerError::InvalidMultiaddr(format!(
"Unexpected multiaddress component '{}'",
p
))),
None => Ok(()),
};

/// Returns [true] if the address is a unicast link-local address (fe80::/10).
#[inline]
const fn is_unicast_link_local(addr: &Ipv6Addr) -> bool {
Expand All @@ -245,7 +238,6 @@ fn validate_address(addr: &Multiaddr, allow_test_addrs: bool) -> Result<(), Conn
})?;

validate_tcp_port(tcp)?;

expect_end_of_address(addr_iter)
},

Expand Down Expand Up @@ -286,14 +278,27 @@ fn validate_address(addr: &Multiaddr, allow_test_addrs: bool) -> Result<(), Conn
"A zero onion port is not valid in the onion spec".to_string(),
)),
Protocol::Onion(_, _) => Err(ConnectionManagerError::OnionV2NotSupported),
Protocol::Onion3(_) => expect_end_of_address(addr_iter),
Protocol::Onion3(addr) => {
expect_end_of_address(addr_iter)?;
validate_onion3_address(&addr)
},
p => Err(ConnectionManagerError::InvalidMultiaddr(format!(
"Unsupported address type '{}'",
p
))),
}
}

fn expect_end_of_address(mut iter: multiaddr::Iter<'_>) -> Result<(), ConnectionManagerError> {
match iter.next() {
Some(p) => Err(ConnectionManagerError::InvalidMultiaddr(format!(
"Unexpected multiaddress component '{}'",
p
))),
None => Ok(()),
}
}

fn validate_tcp_port(expected_tcp: Protocol) -> Result<(), ConnectionManagerError> {
match expected_tcp {
Protocol::Tcp(0) => Err(ConnectionManagerError::InvalidMultiaddr(
Expand All @@ -307,6 +312,35 @@ fn validate_tcp_port(expected_tcp: Protocol) -> Result<(), ConnectionManagerErro
}
}

/// Validates the onion3 version and checksum as per https://github.com/torproject/torspec/blob/main/rend-spec-v3.txt#LL2258C6-L2258C6
fn validate_onion3_address(addr: &multiaddr::Onion3Addr<'_>) -> Result<(), ConnectionManagerError> {
const ONION3_PUBKEY_SIZE: usize = 32;
const ONION3_CHECKSUM_SIZE: usize = 2;

let (pub_key, checksum_version) = addr.hash().split_at(ONION3_PUBKEY_SIZE);
let (checksum, version) = checksum_version.split_at(ONION3_CHECKSUM_SIZE);

if version != b"\x03" {
return Err(ConnectionManagerError::InvalidMultiaddr(
"Invalid version in onion address".to_string(),
));
}

let calculated_checksum = sha3::Sha3_256::new()
.chain(".onion checksum")
.chain(pub_key)
.chain(version)
.finalize();

if calculated_checksum[..2] != *checksum {
return Err(ConnectionManagerError::InvalidMultiaddr(
"Invalid checksum in onion address".to_string(),
));
}

Ok(())
}

#[cfg(test)]
mod test {
use multiaddr::multiaddr;
Expand Down Expand Up @@ -368,4 +402,28 @@ mod test {
validate_address(addr, true).unwrap_err();
}
}

#[test]
fn validate_onion3_checksum() {
let valid: Multiaddr = "/onion3/vww6ybal4bd7szmgncyruucpgfkqahzddi37ktceo3ah7ngmcopnpyyd:1234"
.parse()
.unwrap();

validate_address(&valid, false).unwrap();

// Change one byte
let invalid: Multiaddr = "/onion3/www6ybal4bd7szmgncyruucpgfkqahzddi37ktceo3ah7ngmcopnpyyd:1234"
.parse()
.unwrap();

validate_address(&invalid, false).unwrap_err();

// Randomly generated
let invalid: Multiaddr = "/onion3/pd6sf3mqkkkfrn4rk5odgcr2j5sn7m523a4tm7pzpuotk2b7rpuhaeym:1234"
.parse()
.unwrap();

let err = validate_address(&invalid, false).unwrap_err();
assert!(matches!(err, ConnectionManagerError::InvalidMultiaddr(_)));
}
}

0 comments on commit 0dfdb3a

Please sign in to comment.