Skip to content

Ensure handle_announcement_signatures always has a ErrorAction #143

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 3 commits into from
Sep 3, 2018
Merged

Ensure handle_announcement_signatures always has a ErrorAction #143

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1661d3c
Add ErrorAction in secp Error -> HandleError conv in ChannelManager
TheBlueMatt Aug 30, 2018
c531cb4
Ensure handle_announcement_signatures always has a ErrorAction
TheBlueMatt Aug 30, 2018
a9434db
Check for misuse of funding_transaction_generated and panic
TheBlueMatt Aug 31, 2018
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
11 changes: 10 additions & 1 deletion src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2113,7 +2113,16 @@ impl Channel {
if tx.txid() == self.channel_monitor.get_funding_txo().unwrap().txid {
let txo_idx = self.channel_monitor.get_funding_txo().unwrap().index as usize;
if txo_idx >= tx.output.len() || tx.output[txo_idx].script_pubkey != self.get_funding_redeemscript().to_v0_p2wsh() ||
tx.output[txo_idx].value != self.channel_value_satoshis {
tx.output[txo_idx].value != self.channel_value_satoshis {
if self.channel_outbound {
// If we generated the funding transaction and it doesn't match what it
// should, the client is really broken and we should just panic and
// tell them off. That said, because hash collisions happen with high
// probability in fuzztarget mode, if we're fuzzing we just close the
// channel and move on.
#[cfg(not(feature = "fuzztarget"))]
panic!("Client called ChannelManager::funding_transaction_generated with bogus transaction!");
}
self.channel_state = ChannelState::ShutdownComplete as u32;
self.channel_update_count += 1;
return Err(HandleError{err: "funding tx had wrong script/value", action: Some(ErrorAction::DisconnectPeer{msg: None})});
Expand Down
30 changes: 16 additions & 14 deletions src/ln/channelmanager.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,11 +189,10 @@ pub struct ChannelManager {
const CLTV_EXPIRY_DELTA: u16 = 6 * 24 * 2; //TODO?

macro_rules! secp_call {
( $res : expr ) => {
( $res: expr, $err_msg: expr, $action: expr ) => {
match $res {
Ok(key) => key,
//TODO: Make the err a parameter!
Err(_) => return Err(HandleError{err: "Key error", action: None})
Err(_) => return Err(HandleError{err: $err_msg, action: Some($action)})
}
};
}
Expand Down Expand Up @@ -475,7 +474,7 @@ impl ChannelManager {

// can only fail if an intermediary hop has an invalid public key or session_priv is invalid
#[inline]
fn construct_onion_keys_callback<T: secp256k1::Signing, FType: FnMut(SharedSecret, [u8; 32], PublicKey, &RouteHop)> (secp_ctx: &Secp256k1<T>, route: &Route, session_priv: &SecretKey, mut callback: FType) -> Result<(), HandleError> {
fn construct_onion_keys_callback<T: secp256k1::Signing, FType: FnMut(SharedSecret, [u8; 32], PublicKey, &RouteHop)> (secp_ctx: &Secp256k1<T>, route: &Route, session_priv: &SecretKey, mut callback: FType) -> Result<(), secp256k1::Error> {
let mut blinded_priv = session_priv.clone();
let mut blinded_pub = PublicKey::from_secret_key(secp_ctx, &blinded_priv);
let mut first_iteration = true;
Expand All @@ -495,7 +494,7 @@ impl ChannelManager {
}
let ephemeral_pubkey = blinded_pub;

secp_call!(blinded_priv.mul_assign(secp_ctx, &secp_call!(SecretKey::from_slice(secp_ctx, &blinding_factor))));
blinded_priv.mul_assign(secp_ctx, &SecretKey::from_slice(secp_ctx, &blinding_factor)?)?;
blinded_pub = PublicKey::from_secret_key(secp_ctx, &blinded_priv);

callback(shared_secret, blinding_factor, ephemeral_pubkey, hop);
Expand All @@ -505,7 +504,7 @@ impl ChannelManager {
}

// can only fail if an intermediary hop has an invalid public key or session_priv is invalid
fn construct_onion_keys<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, route: &Route, session_priv: &SecretKey) -> Result<Vec<OnionKeys>, HandleError> {
fn construct_onion_keys<T: secp256k1::Signing>(secp_ctx: &Secp256k1<T>, route: &Route, session_priv: &SecretKey) -> Result<Vec<OnionKeys>, secp256k1::Error> {
let mut res = Vec::with_capacity(route.hops.len());

Self::construct_onion_keys_callback(secp_ctx, route, session_priv, |shared_secret, _blinding_factor, ephemeral_pubkey, _| {
Expand Down Expand Up @@ -910,15 +909,17 @@ impl ChannelManager {
}
}

let session_priv = secp_call!(SecretKey::from_slice(&self.secp_ctx, &{
let session_priv = SecretKey::from_slice(&self.secp_ctx, &{
let mut session_key = [0; 32];
rng::fill_bytes(&mut session_key);
session_key
}));
}).expect("RNG is bad!");

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

let onion_keys = ChannelManager::construct_onion_keys(&self.secp_ctx, &route, &session_priv)?;
//TODO: This should return something other than HandleError, that's really intended for
//p2p-returns only.
let onion_keys = secp_call!(ChannelManager::construct_onion_keys(&self.secp_ctx, &route, &session_priv), "Pubkey along hop was maliciously selected", msgs::ErrorAction::IgnoreError);
Copy link

Choose a reason for hiding this comment

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

Why not extend HandleError further than p2p-returns, something like MarkNodeAsBad { node_id }, could be useful to nodes ban ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe, but best to have more Err types so that you don't end up having match arms that don't make sense in context. eg having a user call send_payment then matching the Err type and ending up with a SendErrMessage action (but they don't know where to send it) is nonsense. Maybe if the full set always makes sense in context but I'm skeptical that'll work out.

let (onion_payloads, htlc_msat, htlc_cltv) = ChannelManager::build_onion_payloads(&route, cur_height)?;
let onion_packet = ChannelManager::construct_onion_packet(onion_payloads, onion_keys, &payment_hash)?;

Expand Down Expand Up @@ -1987,19 +1988,20 @@ impl ChannelMessageHandler for ChannelManager {
match channel_state.by_id.get_mut(&msg.channel_id) {
Some(chan) => {
if chan.get_their_node_id() != *their_node_id {
return Err(HandleError{err: "Got a message for a channel from the wrong node!", action: None})
return Err(HandleError{err: "Got a message for a channel from the wrong node!", action: Some(msgs::ErrorAction::IgnoreError) })
}
if !chan.is_usable() {
return Err(HandleError{err: "Got an announcement_signatures before we were ready for it", action: None });
return Err(HandleError{err: "Got an announcement_signatures before we were ready for it", action: Some(msgs::ErrorAction::IgnoreError) });
}

let our_node_id = self.get_our_node_id();
let (announcement, our_bitcoin_sig) = chan.get_channel_announcement(our_node_id.clone(), self.genesis_hash.clone())?;

let were_node_one = announcement.node_id_1 == our_node_id;
let msghash = Message::from_slice(&Sha256dHash::from_data(&announcement.encode()[..])[..]).unwrap();
secp_call!(self.secp_ctx.verify(&msghash, &msg.node_signature, if were_node_one { &announcement.node_id_2 } else { &announcement.node_id_1 }));
secp_call!(self.secp_ctx.verify(&msghash, &msg.bitcoin_signature, if were_node_one { &announcement.bitcoin_key_2 } else { &announcement.bitcoin_key_1 }));
let bad_sig_action = msgs::ErrorAction::SendErrorMessage { msg: msgs::ErrorMessage { channel_id: msg.channel_id.clone(), data: "Invalid signature in announcement_signatures".to_string() } };
secp_call!(self.secp_ctx.verify(&msghash, &msg.node_signature, if were_node_one { &announcement.node_id_2 } else { &announcement.node_id_1 }), "Bad announcement_signatures node_signature", bad_sig_action);
secp_call!(self.secp_ctx.verify(&msghash, &msg.bitcoin_signature, if were_node_one { &announcement.bitcoin_key_2 } else { &announcement.bitcoin_key_1 }), "Bad announcement_signatures bitcoin_signature", bad_sig_action);

let our_node_sig = self.secp_ctx.sign(&msghash, &self.our_network_key);

Expand All @@ -2011,7 +2013,7 @@ impl ChannelMessageHandler for ChannelManager {
contents: announcement,
}, self.get_channel_update(chan).unwrap()) // can only fail if we're not in a ready state
},
None => return Err(HandleError{err: "Failed to find corresponding channel", action: None})
None => return Err(HandleError{err: "Failed to find corresponding channel", action: Some(msgs::ErrorAction::IgnoreError)})
}
};
let mut pending_events = self.pending_events.lock().unwrap();
Expand Down