Skip to content

WIP: eliminate leak of node secret key from KeysInterface #1362

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

Closed
wants to merge 1 commit into from
Closed

WIP: eliminate leak of node secret key from KeysInterface #1362

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
4 changes: 2 additions & 2 deletions lightning-background-processor/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,7 @@ mod tests {
use bitcoin::network::constants::Network;
use lightning::chain::{BestBlock, Confirm, chainmonitor};
use lightning::chain::channelmonitor::ANTI_REORG_DELAY;
use lightning::chain::keysinterface::{InMemorySigner, Recipient, KeysInterface, KeysManager};
use lightning::chain::keysinterface::{InMemorySigner, KeysInterface, KeysManager};
use lightning::chain::transaction::OutPoint;
use lightning::get_event_msg;
use lightning::ln::channelmanager::{BREAKDOWN_TIMEOUT, ChainParameters, ChannelManager, SimpleArcChannelManager};
Expand Down Expand Up @@ -428,7 +428,7 @@ mod tests {
let network_graph = Arc::new(NetworkGraph::new(genesis_block.header.block_hash()));
let net_graph_msg_handler = Some(Arc::new(NetGraphMsgHandler::new(network_graph.clone(), Some(chain_source.clone()), logger.clone())));
let msg_handler = MessageHandler { chan_handler: Arc::new(test_utils::TestChannelMessageHandler::new()), route_handler: Arc::new(test_utils::TestRoutingMessageHandler::new() )};
let peer_manager = Arc::new(PeerManager::new(msg_handler, keys_manager.get_node_secret(Recipient::Node).unwrap(), &seed, logger.clone(), IgnoringMessageHandler{}));
let peer_manager = Arc::new(PeerManager::new(msg_handler, keys_manager.get_shared_secret_producer(), &seed, logger.clone(), IgnoringMessageHandler{}));
let node = Node { node: manager, net_graph_msg_handler, peer_manager, chain_monitor, persister, tx_broadcaster, network_graph, logger, best_block };
nodes.push(node);
}
Expand Down
5 changes: 3 additions & 2 deletions lightning-net-tokio/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,6 +475,7 @@ mod tests {
use std::sync::atomic::{AtomicBool, Ordering};
use std::sync::{Arc, Mutex};
use std::time::Duration;
use lightning::chain::keysinterface::EphemeralSharedSecretProducer;

pub struct TestLogger();
impl lightning::util::logger::Logger for TestLogger {
Expand Down Expand Up @@ -560,7 +561,7 @@ mod tests {
let a_manager = Arc::new(PeerManager::new(MessageHandler {
chan_handler: Arc::clone(&a_handler),
route_handler: Arc::clone(&a_handler),
}, a_key.clone(), &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
}, EphemeralSharedSecretProducer::new(a_key.clone()), &[1; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));

let (b_connected_sender, mut b_connected) = mpsc::channel(1);
let (b_disconnected_sender, mut b_disconnected) = mpsc::channel(1);
Expand All @@ -574,7 +575,7 @@ mod tests {
let b_manager = Arc::new(PeerManager::new(MessageHandler {
chan_handler: Arc::clone(&b_handler),
route_handler: Arc::clone(&b_handler),
}, b_key.clone(), &[2; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));
}, EphemeralSharedSecretProducer::new(b_key.clone()), &[2; 32], Arc::new(TestLogger()), Arc::new(lightning::ln::peer_handler::IgnoringMessageHandler{})));

// We bind on localhost, hoping the environment is properly configured with a local
// address. This may not always be the case in containers and the like, so if this test is
Expand Down
128 changes: 111 additions & 17 deletions lightning/src/chain/keysinterface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,9 @@ use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::sha256d::Hash as Sha256dHash;
use bitcoin::hash_types::WPubkeyHash;

use bitcoin::secp256k1::ecdh::SharedSecret;
use bitcoin::secp256k1::key::{SecretKey, PublicKey};
use bitcoin::secp256k1::{Secp256k1, Signature, Signing};
use bitcoin::secp256k1::{Secp256k1, Signature, Signing, All, SignOnly};
use bitcoin::secp256k1::recovery::RecoverableSignature;
use bitcoin::secp256k1;

Expand All @@ -37,7 +38,7 @@ use util::ser::{Writeable, Writer, Readable, ReadableArgs};
use chain::transaction::OutPoint;
use ln::{chan_utils, PaymentPreimage};
use ln::chan_utils::{HTLCOutputInCommitment, make_funding_redeemscript, ChannelPublicKeys, HolderCommitmentTransaction, ChannelTransactionParameters, CommitmentTransaction, ClosingTransaction};
use ln::msgs::UnsignedChannelAnnouncement;
use ln::msgs::{UnsignedChannelAnnouncement, UnsignedChannelUpdate, UnsignedNodeAnnouncement};
use ln::script::ShutdownScript;

use prelude::*;
Expand Down Expand Up @@ -392,16 +393,60 @@ pub enum Recipient {
PhantomNode,
}

///
pub trait SharedSecretProduce: Send + Sync {
///
fn public_key(&self, secp_ctx: &secp256k1::Secp256k1<SignOnly>) -> PublicKey;
///
fn shared_secret(&self, other: &PublicKey) -> SharedSecret;
///
fn do_clone(&self) -> Box<dyn SharedSecretProduce>;
}

///
pub struct EphemeralSharedSecretProducer {
///
pub secret_key: SecretKey
}

impl SharedSecretProduce for EphemeralSharedSecretProducer {
fn public_key(&self, secp_ctx: &Secp256k1<SignOnly>) -> PublicKey {
PublicKey::from_secret_key(&secp_ctx, &self.secret_key)
}

fn shared_secret(&self, other: &PublicKey) -> SharedSecret {
SharedSecret::new(other, &self.secret_key)
}

fn do_clone(&self) -> Box<dyn SharedSecretProduce> {
Box::new(Self { secret_key: self.secret_key })
}
}

impl EphemeralSharedSecretProducer {
///
pub fn new(secret_key: SecretKey) -> Box<dyn SharedSecretProduce> {
Box::new( Self { secret_key })
}
}

/// A trait to describe an object which can get user secrets and key material.
pub trait KeysInterface {
/// A type which implements Sign which will be returned by get_channel_signer.
type Signer : Sign;

/// Get node secret key (aka node_id or network_key) based on the provided [`Recipient`].
/// A shared secret producer using the node key
fn get_shared_secret_producer(&self) -> Box<dyn SharedSecretProduce>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite sure I understand the need for this trait here, and super not a fan of box-dyn in our interface.

Copy link
Member Author

Choose a reason for hiding this comment

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

The trait replaces SecretKey in several places. It seems particularly necessary in DirectionalNoiseState, so that low-level protocol enum doesn't get exposed to the full KeysInterface.

The Box<dyn> was expedient for the draft - a generic would propagate into a lot of peer protocol structs.


/// ECDH
fn shared_secret(&self, recipient: Recipient, other: &PublicKey) -> Result<SharedSecret, ()>;

/// Get node public key (AKA node ID)
///
/// This method must return the same value each time it is called with a given `Recipient`
/// parameter.
fn get_node_secret(&self, recipient: Recipient) -> Result<SecretKey, ()>;
Copy link
Member Author

Choose a reason for hiding this comment

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

The goal is to eliminate this function.

fn get_node_key(&self, recipient: Recipient, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<PublicKey, ()>;

/// Get a script pubkey which we send funds to when claiming on-chain contestable outputs.
///
/// This method should return a different value each time it is called, to avoid linking
Expand Down Expand Up @@ -441,6 +486,12 @@ pub trait KeysInterface {
/// The secret key used to sign the invoice is dependent on the [`Recipient`].
fn sign_invoice(&self, hrp_bytes: &[u8], invoice_data: &[u5], receipient: Recipient) -> Result<RecoverableSignature, ()>;

/// Sign a node announcement
fn sign_node_announcement(&self, msg: &UnsignedNodeAnnouncement, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>;

/// Sign a channel update
fn sign_channel_update(&self, msg: &UnsignedChannelUpdate, secp_ctx: &Secp256k1<secp256k1::All>) -> Result<Signature, ()>;

/// Get secret key material as bytes for use in encrypting and decrypting inbound payment data.
///
/// If the implementor of this trait supports [phantom node payments], then every node that is
Expand Down Expand Up @@ -1118,17 +1169,21 @@ impl KeysManager {

Ok(spend_tx)
}
}

impl KeysInterface for KeysManager {
type Signer = InMemorySigner;

fn get_node_secret(&self, recipient: Recipient) -> Result<SecretKey, ()> {
match recipient {
Recipient::Node => Ok(self.node_secret.clone()),
Recipient::PhantomNode => Err(())
}
}
}

impl KeysInterface for KeysManager {
type Signer = InMemorySigner;

fn get_shared_secret_producer(&self) -> Box<dyn SharedSecretProduce> {
EphemeralSharedSecretProducer::new(self.node_secret)
}

fn get_inbound_payment_key_material(&self) -> KeyMaterial {
self.inbound_payment_key.clone()
Expand Down Expand Up @@ -1169,12 +1224,30 @@ impl KeysInterface for KeysManager {

fn sign_invoice(&self, hrp_bytes: &[u8], invoice_data: &[u5], recipient: Recipient) -> Result<RecoverableSignature, ()> {
let preimage = construct_invoice_preimage(&hrp_bytes, &invoice_data);
let secret = match recipient {
Recipient::Node => self.get_node_secret(Recipient::Node)?,
Recipient::PhantomNode => return Err(()),
};
let secret = self.get_node_secret(recipient)?;
Ok(self.secp_ctx.sign_recoverable(&hash_to_message!(&Sha256::hash(&preimage)), &secret))
}

fn sign_node_announcement(&self, msg: &UnsignedNodeAnnouncement, secp_ctx: &Secp256k1<All>) -> Result<Signature, ()> {
let msghash = hash_to_message!(&Sha256dHash::hash(&msg.encode()[..])[..]);
Ok(secp_ctx.sign(&msghash, &self.node_secret))

}

fn sign_channel_update(&self, msg: &UnsignedChannelUpdate, secp_ctx: &Secp256k1<All>) -> Result<Signature, ()> {
let msghash = hash_to_message!(&Sha256dHash::hash(&msg.encode()[..])[..]);
Ok(secp_ctx.sign(&msghash, &self.node_secret))
}

fn shared_secret(&self, recipient: Recipient, other: &PublicKey) -> Result<SharedSecret, ()> {
let secret = self.get_node_secret(recipient)?;
Ok(SharedSecret::new(other, &secret))
}

fn get_node_key(&self, recipient: Recipient, secp_ctx: &Secp256k1<All>) -> Result<PublicKey, ()> {
let secret = self.get_node_secret(recipient)?;
Ok(PublicKey::from_secret_key(&secp_ctx, &secret))
}
}

/// Similar to [`KeysManager`], but allows the node using this struct to receive phantom node
Expand Down Expand Up @@ -1207,11 +1280,8 @@ pub struct PhantomKeysManager {
impl KeysInterface for PhantomKeysManager {
type Signer = InMemorySigner;

fn get_node_secret(&self, recipient: Recipient) -> Result<SecretKey, ()> {
match recipient {
Recipient::Node => self.inner.get_node_secret(Recipient::Node),
Recipient::PhantomNode => Ok(self.phantom_secret.clone()),
}
fn get_shared_secret_producer(&self) -> Box<dyn SharedSecretProduce> {
EphemeralSharedSecretProducer::new(self.inner.node_secret)
}

fn get_inbound_payment_key_material(&self) -> KeyMaterial {
Expand Down Expand Up @@ -1243,6 +1313,23 @@ impl KeysInterface for PhantomKeysManager {
let secret = self.get_node_secret(recipient)?;
Ok(self.inner.secp_ctx.sign_recoverable(&hash_to_message!(&Sha256::hash(&preimage)), &secret))
}

fn sign_node_announcement(&self, msg: &UnsignedNodeAnnouncement, secp_ctx: &Secp256k1<All>) -> Result<Signature, ()> {
self.inner.sign_node_announcement(msg, secp_ctx)
}

fn sign_channel_update(&self, msg: &UnsignedChannelUpdate, secp_ctx: &Secp256k1<All>) -> Result<Signature, ()> {
self.inner.sign_channel_update(msg, secp_ctx)
}

fn shared_secret(&self, recipient: Recipient, other: &PublicKey) -> Result<SharedSecret, ()> {
let secret = self.get_node_secret(recipient)?;
Ok(SharedSecret::new(other, &secret))
}

fn get_node_key(&self, recipient: Recipient, secp_ctx: &Secp256k1<All>) -> Result<PublicKey, ()> {
Ok(PublicKey::from_secret_key(&secp_ctx, &self.get_node_secret(recipient)?))
}
}

impl PhantomKeysManager {
Expand Down Expand Up @@ -1275,6 +1362,13 @@ impl PhantomKeysManager {
pub fn derive_channel_keys(&self, channel_value_satoshis: u64, params: &[u8; 32]) -> InMemorySigner {
self.inner.derive_channel_keys(channel_value_satoshis, params)
}

pub(crate) fn get_node_secret(&self, recipient: Recipient) -> Result<SecretKey, ()> {
match recipient {
Recipient::Node => Ok(self.inner.node_secret.clone()),
Recipient::PhantomNode => Ok(self.phantom_secret.clone())
}
}
}

// Ensure that BaseSign can have a vtable
Expand Down
11 changes: 8 additions & 3 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6265,13 +6265,13 @@ mod tests {
use ln::channel::{Channel,InboundHTLCOutput,OutboundHTLCOutput,InboundHTLCState,OutboundHTLCState,HTLCOutputInCommitment,HTLCCandidate,HTLCInitiator,TxCreationKeys};
use ln::channel::MAX_FUNDING_SATOSHIS;
use ln::features::InitFeatures;
use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate};
use ln::msgs::{ChannelUpdate, DataLossProtect, DecodeError, OptionalField, UnsignedChannelUpdate, UnsignedNodeAnnouncement};
use ln::script::ShutdownScript;
use ln::chan_utils;
use ln::chan_utils::{ChannelPublicKeys, HolderCommitmentTransaction, CounterpartyChannelTransactionParameters, htlc_success_tx_weight, htlc_timeout_tx_weight};
use chain::BestBlock;
use chain::chaininterface::{FeeEstimator,ConfirmationTarget};
use chain::keysinterface::{InMemorySigner, Recipient, KeyMaterial, KeysInterface, BaseSign};
use chain::keysinterface::{InMemorySigner, Recipient, KeyMaterial, KeysInterface, BaseSign, SharedSecretProduce};
use chain::transaction::OutPoint;
use util::config::UserConfig;
use util::enforcing_trait_impls::EnforcingSigner;
Expand All @@ -6280,6 +6280,7 @@ mod tests {
use util::test_utils::OnGetShutdownScriptpubkey;
use util::logger::Logger;
use bitcoin::secp256k1::{Secp256k1, Message, Signature, All};
use bitcoin::secp256k1::ecdh::SharedSecret;
use bitcoin::secp256k1::ffi::Signature as FFISignature;
use bitcoin::secp256k1::key::{SecretKey,PublicKey};
use bitcoin::secp256k1::recovery::RecoverableSignature;
Expand Down Expand Up @@ -6319,7 +6320,7 @@ mod tests {
impl KeysInterface for Keys {
type Signer = InMemorySigner;

fn get_node_secret(&self, _recipient: Recipient) -> Result<SecretKey, ()> { panic!(); }
fn get_shared_secret_producer(&self) -> Box<dyn SharedSecretProduce> { panic!(); }
fn get_inbound_payment_key_material(&self) -> KeyMaterial { panic!(); }
fn get_destination_script(&self) -> Script {
let secp_ctx = Secp256k1::signing_only();
Expand All @@ -6340,6 +6341,10 @@ mod tests {
fn get_secure_random_bytes(&self) -> [u8; 32] { [0; 32] }
fn read_chan_signer(&self, _data: &[u8]) -> Result<Self::Signer, DecodeError> { panic!(); }
fn sign_invoice(&self, _hrp_bytes: &[u8], _invoice_data: &[u5], _recipient: Recipient) -> Result<RecoverableSignature, ()> { panic!(); }
fn sign_node_announcement(&self, _msg: &UnsignedNodeAnnouncement, _secp_ctx: &Secp256k1<All>) -> Result<Signature, ()> { panic!(); }
fn sign_channel_update(&self, _msg: &UnsignedChannelUpdate, _secp_ctx: &Secp256k1<All>) -> Result<Signature, ()> { panic!(); }
fn shared_secret(&self, _recipient: Recipient, _other: &PublicKey) -> Result<SharedSecret, ()> { panic!(); }
fn get_node_key(&self, _recipient: Recipient, _secp_ctx: &Secp256k1<secp256k1::All>) -> Result<PublicKey, ()> { panic!(); }
}

fn public_from_secret_hex(secp_ctx: &Secp256k1<All>, hex: &str) -> PublicKey {
Expand Down
Loading