Skip to content

Randomize initial onion packet data. #403

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Dec 3, 2019
Merged

Randomize initial onion packet data. #403

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 3 additions & 2 deletions fuzz/fuzz_targets/chanmon_fail_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,9 +166,10 @@ impl KeysInterface for KeyProvider {
}
}

fn get_session_key(&self) -> SecretKey {
fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
let id = self.session_id.fetch_add(1, atomic::Ordering::Relaxed);
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, id, 10, self.node_id]).unwrap()
(SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, id, 10, self.node_id]).unwrap(),
[0; 32])
}

fn get_channel_id(&self) -> [u8; 32] {
Expand Down
5 changes: 3 additions & 2 deletions fuzz/fuzz_targets/full_stack_target.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,9 +277,10 @@ impl KeysInterface for KeyProvider {
}
}

fn get_session_key(&self) -> SecretKey {
fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
let ctr = self.counter.fetch_add(1, Ordering::Relaxed) as u8;
SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 13, ctr]).unwrap()
(SecretKey::from_slice(&[0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 13, ctr]).unwrap(),
[0; 32])
}

fn get_channel_id(&self) -> [u8; 32] {
Expand Down
15 changes: 11 additions & 4 deletions lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ pub trait KeysInterface: Send + Sync {
/// Get a new set of ChannelKeys for per-channel secrets. These MUST be unique even if you
/// restarted with some stale data!
fn get_channel_keys(&self, inbound: bool) -> ChannelKeys;
/// Get a secret for construting an onion packet
fn get_session_key(&self) -> SecretKey;
/// Get a secret and PRNG seed for construting an onion packet
fn get_onion_rand(&self) -> (SecretKey, [u8; 32]);
/// Get a unique temporary channel id. Channels will be referred to by this until the funding
/// transaction is created, at which point they will use the outpoint in the funding
/// transaction.
Expand Down Expand Up @@ -258,13 +258,20 @@ impl KeysInterface for KeysManager {
}
}

fn get_session_key(&self) -> SecretKey {
fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
let mut sha = self.unique_start.clone();

let child_ix = self.session_child_index.fetch_add(1, Ordering::AcqRel);
let child_privkey = self.session_master_key.ckd_priv(&self.secp_ctx, ChildNumber::from_hardened_idx(child_ix as u32).expect("key space exhausted")).expect("Your RNG is busted");
sha.input(&child_privkey.private_key.key[..]);
SecretKey::from_slice(&Sha256::from_engine(sha).into_inner()).expect("Your RNG is busted")

let mut rng_seed = sha.clone();
// Not exactly the most ideal construction, but the second value will get fed into
// ChaCha so it is another step harder to break.
rng_seed.input(b"RNG Seed Salt");
sha.input(b"Session Key Salt");
(SecretKey::from_slice(&Sha256::from_engine(sha).into_inner()).expect("Your RNG is busted"),
Sha256::from_engine(rng_seed).into_inner())
}

fn get_channel_id(&self) -> [u8; 32] {
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4199,7 +4199,7 @@ mod tests {
}

fn get_channel_keys(&self, _inbound: bool) -> ChannelKeys { self.chan_keys.clone() }
fn get_session_key(&self) -> SecretKey { panic!(); }
fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) { panic!(); }
fn get_channel_id(&self) -> [u8; 32] { [0; 32] }
}

Expand Down
19 changes: 17 additions & 2 deletions lightning/src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,21 @@ impl<'a> ChannelManager<'a> {
};

let pending_forward_info = if next_hop_data.hmac == [0; 32] {
#[cfg(test)]
{
// In tests, make sure that the initial onion pcket data is, at least, non-0.
// We could do some fancy randomness test here, but, ehh, whatever.
// This checks for the issue where you can calculate the path length given the
// onion data as all the path entries that the originator sent will be here
// as-is (and were originally 0s).
// Of course reverse path calculation is still pretty easy given naive routing
// algorithms, but this fixes the most-obvious case.
let mut new_packet_data = [0; 19*65];
chacha.process(&msg.onion_routing_packet.hop_data[65..], &mut new_packet_data[0..19*65]);
assert_ne!(new_packet_data[0..65], [0; 65][..]);
assert_ne!(new_packet_data[..], [0; 19*65][..]);
}

// OUR PAYMENT!
// final_expiry_too_soon
if (msg.cltv_expiry as u64) < self.latest_block_height.load(Ordering::Acquire) as u64 + (CLTV_CLAIM_BUFFER + LATENCY_GRACE_PERIOD_BLOCKS) as u64 {
Expand Down Expand Up @@ -1089,14 +1104,14 @@ impl<'a> ChannelManager<'a> {
}
}

let session_priv = self.keys_manager.get_session_key();
let (session_priv, prng_seed) = self.keys_manager.get_onion_rand();

let cur_height = self.latest_block_height.load(Ordering::Acquire) as u32 + 1;

let onion_keys = secp_call!(onion_utils::construct_onion_keys(&self.secp_ctx, &route, &session_priv),
APIError::RouteError{err: "Pubkey along hop was maliciously selected"});
let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height)?;
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &payment_hash);
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, prng_seed, &payment_hash);

let _ = self.total_consistency_lock.read().unwrap();

Expand Down
10 changes: 5 additions & 5 deletions lightning/src/ln/functional_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1435,7 +1435,7 @@ fn do_channel_reserve_test(test_recv: bool) {
let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
let onion_keys = onion_utils::construct_onion_keys(&secp_ctx, &route, &session_priv).unwrap();
let (onion_payloads, htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &our_payment_hash);
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash);
let msg = msgs::UpdateAddHTLC {
channel_id: chan_1.2,
htlc_id,
Expand Down Expand Up @@ -4815,7 +4815,7 @@ fn test_onion_failure() {
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap();
let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
onion_payloads[0].realm = 3;
msg.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &payment_hash);
msg.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
}, ||{}, true, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));//XXX incremented channels idx here

// final node failure
Expand All @@ -4825,7 +4825,7 @@ fn test_onion_failure() {
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap();
let (mut onion_payloads, _htlc_msat, _htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
onion_payloads[1].realm = 3;
msg.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &payment_hash);
msg.onion_routing_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
}, ||{}, false, Some(PERM|1), Some(msgs::HTLCFailChannelUpdate::ChannelClosed{short_channel_id: channels[1].0.contents.short_channel_id, is_permanent: true}));

// the following three with run_onion_failure_test_with_fail_intercept() test only the origin node
Expand Down Expand Up @@ -5003,7 +5003,7 @@ fn test_onion_failure() {
route.hops[1].cltv_expiry_delta += CLTV_FAR_FAR_AWAY + route.hops[0].cltv_expiry_delta + 1;
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::new(), &route, &session_priv).unwrap();
let (onion_payloads, _, htlc_cltv) = onion_utils::build_onion_payloads(&route, height).unwrap();
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &payment_hash);
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &payment_hash);
msg.cltv_expiry = htlc_cltv;
msg.onion_routing_packet = onion_packet;
}, ||{}, true, Some(21), None);
Expand Down Expand Up @@ -5253,7 +5253,7 @@ fn test_update_add_htlc_bolt2_receiver_check_max_htlc_limit() {
let cur_height = nodes[0].node.latest_block_height.load(Ordering::Acquire) as u32 + 1;
let onion_keys = onion_utils::construct_onion_keys(&Secp256k1::signing_only(), &route, &session_priv).unwrap();
let (onion_payloads, _htlc_msat, htlc_cltv) = onion_utils::build_onion_payloads(&route, cur_height).unwrap();
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, &our_payment_hash);
let onion_packet = onion_utils::construct_onion_packet(onion_payloads, onion_keys, [0; 32], &our_payment_hash);

let mut msg = msgs::UpdateAddHTLC {
channel_id: chan.2,
Expand Down
16 changes: 13 additions & 3 deletions lightning/src/ln/onion_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,8 +161,18 @@ fn xor_bufs(dst: &mut[u8], src: &[u8]) {
}
}


const ZERO:[u8; 21*65] = [0; 21*65];
pub(super) fn construct_onion_packet(mut payloads: Vec<msgs::OnionHopData>, onion_keys: Vec<OnionKeys>, associated_data: &PaymentHash) -> msgs::OnionPacket {
pub(super) fn construct_onion_packet(payloads: Vec<msgs::OnionHopData>, onion_keys: Vec<OnionKeys>, prng_seed: [u8; 32], associated_data: &PaymentHash) -> msgs::OnionPacket {
let mut packet_data = [0; 20*65];

let mut chacha = ChaCha20::new(&prng_seed, &[0; 8]);
chacha.process(&[0; 20*65], &mut packet_data);

construct_onion_packet_with_init_noise(payloads, onion_keys, packet_data, associated_data)
}

fn construct_onion_packet_with_init_noise(mut payloads: Vec<msgs::OnionHopData>, onion_keys: Vec<OnionKeys>, mut packet_data: [u8; 20*65], associated_data: &PaymentHash) -> msgs::OnionPacket {
let mut buf = Vec::with_capacity(21*65);
buf.resize(21*65, 0);

Expand All @@ -181,7 +191,6 @@ pub(super) fn construct_onion_packet(mut payloads: Vec<msgs::OnionHopData>, onio
res
};

let mut packet_data = [0; 20*65];
let mut hmac_res = [0; 32];

for (i, (payload, keys)) in payloads.iter_mut().zip(onion_keys.iter()).rev().enumerate() {
Expand Down Expand Up @@ -547,7 +556,8 @@ mod tests {
},
);

let packet = super::construct_onion_packet(payloads, onion_keys, &PaymentHash([0x42; 32]));
let packet = [0; 20*65];
let packet = super::construct_onion_packet_with_init_noise(payloads, onion_keys, packet, &PaymentHash([0x42; 32]));
// Just check the final packet encoding, as it includes all the per-hop vectors in it
// anyway...
assert_eq!(packet.encode(), hex::decode("0002eec7245d6b7d2ccb30380bfbe2a3648cd7a942653f5aa340edcea1f283686619e5f14350c2a76fc232b5e46d421e9615471ab9e0bc887beff8c95fdb878f7b3a716a996c7845c93d90e4ecbb9bde4ece2f69425c99e4bc820e44485455f135edc0d10f7d61ab590531cf08000179a333a347f8b4072f216400406bdf3bf038659793d4a1fd7b246979e3150a0a4cb052c9ec69acf0f48c3d39cd55675fe717cb7d80ce721caad69320c3a469a202f1e468c67eaf7a7cd8226d0fd32f7b48084dca885d56047694762b67021713ca673929c163ec36e04e40ca8e1c6d17569419d3039d9a1ec866abe044a9ad635778b961fc0776dc832b3a451bd5d35072d2269cf9b040f6b7a7dad84fb114ed413b1426cb96ceaf83825665ed5a1d002c1687f92465b49ed4c7f0218ff8c6c7dd7221d589c65b3b9aaa71a41484b122846c7c7b57e02e679ea8469b70e14fe4f70fee4d87b910cf144be6fe48eef24da475c0b0bcc6565ae82cd3f4e3b24c76eaa5616c6111343306ab35c1fe5ca4a77c0e314ed7dba39d6f1e0de791719c241a939cc493bea2bae1c1e932679ea94d29084278513c77b899cc98059d06a27d171b0dbdf6bee13ddc4fc17a0c4d2827d488436b57baa167544138ca2e64a11b43ac8a06cd0c2fba2d4d900ed2d9205305e2d7383cc98dacb078133de5f6fb6bed2ef26ba92cea28aafc3b9948dd9ae5559e8bd6920b8cea462aa445ca6a95e0e7ba52961b181c79e73bd581821df2b10173727a810c92b83b5ba4a0403eb710d2ca10689a35bec6c3a708e9e92f7d78ff3c5d9989574b00c6736f84c199256e76e19e78f0c98a9d580b4a658c84fc8f2096c2fbea8f5f8c59d0fdacb3be2802ef802abbecb3aba4acaac69a0e965abd8981e9896b1f6ef9d60f7a164b371af869fd0e48073742825e9434fc54da837e120266d53302954843538ea7c6c3dbfb4ff3b2fdbe244437f2a153ccf7bdb4c92aa08102d4f3cff2ae5ef86fab4653595e6a5837fa2f3e29f27a9cde5966843fb847a4a61f1e76c281fe8bb2b0a181d096100db5a1a5ce7a910238251a43ca556712eaadea167fb4d7d75825e440f3ecd782036d7574df8bceacb397abefc5f5254d2722215c53ff54af8299aaaad642c6d72a14d27882d9bbd539e1cc7a527526ba89b8c037ad09120e98ab042d3e8652b31ae0e478516bfaf88efca9f3676ffe99d2819dcaeb7610a626695f53117665d267d3f7abebd6bbd6733f645c72c389f03855bdf1e4b8075b516569b118233a0f0971d24b83113c0b096f5216a207ca99a7cddc81c130923fe3d91e7508c9ac5f2e914ff5dccab9e558566fa14efb34ac98d878580814b94b73acbfde9072f30b881f7f0fff42d4045d1ace6322d86a97d164aa84d93a60498065cc7c20e636f5862dc81531a88c60305a2e59a985be327a6902e4bed986dbf4a0b50c217af0ea7fdf9ab37f9ea1a1aaa72f54cf40154ea9b269f1a7c09f9f43245109431a175d50e2db0132337baa0ef97eed0fcf20489da36b79a1172faccc2f7ded7c60e00694282d93359c4682135642bc81f433574aa8ef0c97b4ade7ca372c5ffc23c7eddd839bab4e0f14d6df15c9dbeab176bec8b5701cf054eb3072f6dadc98f88819042bf10c407516ee58bce33fbe3b3d86a54255e577db4598e30a135361528c101683a5fcde7e8ba53f3456254be8f45fe3a56120ae96ea3773631fcb3873aa3abd91bcff00bd38bd43697a2e789e00da6077482e7b1b1a677b5afae4c54e6cbdf7377b694eb7d7a5b913476a5be923322d3de06060fd5e819635232a2cf4f0731da13b8546d1d6d4f8d75b9fce6c2341a71b0ea6f780df54bfdb0dd5cd9855179f602f9172307c7268724c3618e6817abd793adc214a0dc0bc616816632f27ea336fb56dfd").unwrap());
Expand Down
1 change: 1 addition & 0 deletions lightning/src/util/chacha20.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,7 @@ mod real_chacha {
self.offset = 0;
}

#[inline] // Useful cause input may be 0s on stack that should be optimized out
Copy link

Choose a reason for hiding this comment

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

A warn against too smart compiler which would align stack after function jump?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm? Not sure what you're asking.

Copy link

Choose a reason for hiding this comment

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

Wasn't asking anything, just wanted to be sure that inline was to prevent compiler alignement.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure what you mean by "alignment", but, in any case, afaiu, #[inline] has two meanings:

  • for pub fns it allows inlining across crates, which is normally only possible with lto,
  • for regular fns its just a hint to the compiler that it should try inlining, which it is free to completely ignore if it likes.

pub fn process(&mut self, input: &[u8], output: &mut [u8]) {
assert!(input.len() == output.len());
let len = input.len();
Expand Down
6 changes: 3 additions & 3 deletions lightning/src/util/test_utils.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,10 @@ impl keysinterface::KeysInterface for TestKeysInterface {
fn get_shutdown_pubkey(&self) -> PublicKey { self.backing.get_shutdown_pubkey() }
fn get_channel_keys(&self, inbound: bool) -> keysinterface::ChannelKeys { self.backing.get_channel_keys(inbound) }

fn get_session_key(&self) -> SecretKey {
fn get_onion_rand(&self) -> (SecretKey, [u8; 32]) {
match *self.override_session_priv.lock().unwrap() {
Some(key) => key.clone(),
None => self.backing.get_session_key()
Some(key) => (key.clone(), [0; 32]),
None => self.backing.get_onion_rand()
}
}

Expand Down