Skip to content

Commit 0dbc64f

Browse files
committed
Verify the holder provided valid witnesses and uses SIGHASH_ALL
LDK checks the following: * Each input spends an output that is one of P2WPKH, P2WSH, or P2TR. These were already checked by LDK when the inputs to be contributed were provided. * All signatures use the `SIGHASH_ALL` sighash type. * P2WPKH and P2TR key path spends are valid (verifies signatures) NOTE: * When checking P2WSH spends, LDK tries to decode 70-72 byte witness elements as ECDSA signatures with a sighash flag. If the internal DER-decoding fails, then LDK just assumes it wasn't a signature and carries with checks. If the element can be decoded as an ECDSA signature, the the sighash flag must be `SIGHASH_ALL`. * When checking P2TR script-path spends, LDK assumes all elements of exactly 65 bytes with the last byte matching any valid sighash flag byte are schnorr signatures and checks that the sighash type is `SIGHASH_ALL`. If the last byte is not any valid sighash flag, the element is assumed not to be a signature and is ignored. Elements of 64 bytes are not checked because if they were schnorr signatures then they would implicitly be `SIGHASH_DEFAULT` which is an alias of `SIGHASH_ALL`.
1 parent b14bb14 commit 0dbc64f

File tree

3 files changed

+556
-61
lines changed

3 files changed

+556
-61
lines changed

lightning/src/ln/channel.rs

Lines changed: 20 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -7772,45 +7772,32 @@ where
77727772
}
77737773
}
77747774

7775-
fn verify_interactive_tx_signatures(&mut self, _witnesses: &Vec<Witness>) {
7776-
if let Some(ref mut _signing_session) = self.interactive_tx_signing_session {
7777-
// Check that sighash_all was used:
7778-
// TODO(dual_funding): Check sig for sighash
7779-
}
7775+
#[cfg(splicing)]
7776+
fn is_splicing(&self) -> bool {
7777+
self.pending_splice
7778+
.as_ref()
7779+
.and_then(|pending_splice| Some(pending_splice.funding.is_some()))
7780+
.unwrap_or(false)
77807781
}
77817782

7782-
pub fn funding_transaction_signed<L: Deref>(
7783-
&mut self, witnesses: Vec<Witness>, logger: &L,
7784-
) -> Result<Option<msgs::TxSignatures>, APIError>
7785-
where
7786-
L::Target: Logger,
7787-
{
7788-
self.verify_interactive_tx_signatures(&witnesses);
7789-
if let Some(ref mut signing_session) = self.interactive_tx_signing_session {
7790-
let logger = WithChannelContext::from(logger, &self.context, None);
7791-
if let Some(holder_tx_signatures) = signing_session
7792-
.provide_holder_witnesses(self.context.channel_id, witnesses)
7793-
.map_err(|err| APIError::APIMisuseError { err })?
7794-
{
7795-
if self.is_awaiting_initial_mon_persist() {
7796-
log_debug!(
7797-
logger,
7798-
"Not sending tx_signatures: a monitor update is in progress."
7799-
);
7800-
return Ok(None);
7801-
}
7802-
return Ok(Some(holder_tx_signatures));
7803-
} else {
7804-
return Ok(None);
7805-
}
7806-
} else {
7807-
return Err(APIError::APIMisuseError {
7783+
pub fn funding_transaction_signed(
7784+
&mut self, witnesses: Vec<Witness>,
7785+
) -> Result<Option<msgs::TxSignatures>, APIError> {
7786+
self.interactive_tx_signing_session
7787+
.as_mut()
7788+
.ok_or_else(|| APIError::APIMisuseError {
78087789
err: format!(
78097790
"Channel with id {} not expecting funding signatures",
78107791
self.context.channel_id
78117792
),
7812-
});
7813-
}
7793+
})
7794+
.and_then(|signing_session| {
7795+
signing_session
7796+
.verify_interactive_tx_signatures(&self.context.secp_ctx, &witnesses)?;
7797+
signing_session
7798+
.provide_holder_witnesses(self.context.channel_id, witnesses)
7799+
.map_err(|err| APIError::APIMisuseError { err })
7800+
})
78147801
}
78157802

78167803
#[rustfmt::skip]

lightning/src/ln/channelmanager.rs

Lines changed: 22 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -5901,19 +5901,32 @@ where
59015901
/// counterparty's signature(s) the funding transaction will automatically be broadcast via the
59025902
/// [`BroadcasterInterface`] provided when this `ChannelManager` was constructed.
59035903
///
5904-
/// `SIGHASH_ALL` MUST be used for all signatures when providing signatures.
5905-
///
5906-
/// <div class="warning">
5907-
/// WARNING: LDK makes no attempt to prevent the counterparty from using non-standard inputs which
5908-
/// will prevent the funding transaction from being relayed on the bitcoin network and hence being
5909-
/// confirmed.
5910-
/// </div>
5904+
/// `SIGHASH_ALL` MUST be used for all signatures when providing signatures, otherwise your
5905+
/// funds can be held hostage!
5906+
///
5907+
/// LDK checks the following:
5908+
/// * Each input spends an output that is one of P2WPKH, P2WSH, or P2TR.
5909+
/// These were already checked by LDK when the inputs to be contributed were provided.
5910+
/// * All signatures use the `SIGHASH_ALL` sighash type.
5911+
/// * P2WPKH and P2TR key path spends are valid (verifies signatures)
5912+
///
5913+
/// NOTE:
5914+
/// * When checking P2WSH spends, LDK tries to decode 70-72 byte witness elements as ECDSA
5915+
/// signatures with a sighash flag. If the internal DER-decoding fails, then LDK just
5916+
/// assumes it wasn't a signature and carries with checks. If the element can be decoded
5917+
/// as an ECDSA signature, the the sighash flag must be `SIGHASH_ALL`.
5918+
/// * When checking P2TR script-path spends, LDK assumes all elements of exactly 65 bytes
5919+
/// with the last byte matching any valid sighash flag byte are schnorr signatures and checks
5920+
/// that the sighash type is `SIGHASH_ALL`. If the last byte is not any valid sighash flag, the
5921+
/// element is assumed not to be a signature and is ignored. Elements of 64 bytes are not
5922+
/// checked because if they were schnorr signatures then they would implicitly be `SIGHASH_DEFAULT`
5923+
/// which is an alias of `SIGHASH_ALL`.
59115924
///
59125925
/// Returns [`ChannelUnavailable`] when a channel is not found or an incorrect
59135926
/// `counterparty_node_id` is provided.
59145927
///
59155928
/// Returns [`APIMisuseError`] when a channel is not in a state where it is expecting funding
5916-
/// signatures.
5929+
/// signatures or if any of the checks described above fail.
59175930
///
59185931
/// [`ChannelUnavailable`]: APIError::ChannelUnavailable
59195932
/// [`APIMisuseError`]: APIError::APIMisuseError
@@ -5943,9 +5956,7 @@ where
59435956
match peer_state.channel_by_id.get_mut(channel_id) {
59445957
Some(channel) => match channel.as_funded_mut() {
59455958
Some(chan) => {
5946-
if let Some(tx_signatures) =
5947-
chan.funding_transaction_signed(witnesses, &self.logger)?
5948-
{
5959+
if let Some(tx_signatures) = chan.funding_transaction_signed(witnesses)? {
59495960
peer_state.pending_msg_events.push(MessageSendEvent::SendTxSignatures {
59505961
node_id: *counterparty_node_id,
59515962
msg: tx_signatures,

0 commit comments

Comments
 (0)