Skip to content

Various followups to #3917 #3941

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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 4 additions & 15 deletions lightning/src/blinded_path/message.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,27 +543,20 @@ impl_writeable_tlv_based_enum!(MessageContext,
{3, DNSResolver} => (),
);

// NOTE:
// Several TLV fields (`nonce`, `hmac`, etc.) were removed in LDK v0.2
// following the introduction of `ReceiveAuthKey`-based authentication for
// inbound `BlindedMessagePath`s. These fields are now commented out and
// their `type` values must not be reused unless support for LDK v0.2
// and earlier is fully dropped.
//
// For context-specific removals, see the commented-out fields within each enum variant.
// Note: Several TLV fields (`nonce`, `hmac`, etc.) were removed in LDK v0.2 following the
// introduction of `ReceiveAuthKey`-based authentication for inbound `BlindedMessagePath`s. Because
// we do not support receiving to those contexts anymore (they will fail the `ReceiveAuthKey`-based
// authentication checks), we can reuse those fields here.
impl_writeable_tlv_based_enum!(OffersContext,
(0, InvoiceRequest) => {
(0, nonce, required),
},
(1, OutboundPayment) => {
(0, payment_id, required),
(1, nonce, required),
// Removed: (2, hmac, option)
},
(2, InboundPayment) => {
(0, payment_hash, required),
// Removed: (1, nonce, required),
// Removed: (2, hmac, required)
},
(3, StaticInvoiceRequested) => {
(0, recipient_id, required),
Expand All @@ -575,12 +568,8 @@ impl_writeable_tlv_based_enum!(OffersContext,
impl_writeable_tlv_based_enum!(AsyncPaymentsContext,
(0, OutboundPayment) => {
(0, payment_id, required),
// Removed: (2, nonce, required),
// Removed: (4, hmac, required),
},
(1, InboundPayment) => {
// Removed: (0, nonce, required),
// Removed: (2, hmac, required),
(4, path_absolute_expiry, required),
},
(2, OfferPaths) => {
Expand Down
14 changes: 12 additions & 2 deletions lightning/src/crypto/streams.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,10 @@ impl<'a, T: Writeable> Writeable for ChaChaPolyWriteAdapter<'a, T> {

Ok(())
}

fn serialized_length(&self) -> usize {
self.writeable.serialized_length() + 16
}
}

/// Encrypts the provided plaintext with the given key using ChaCha20Poly1305 in the modified
Expand Down Expand Up @@ -124,7 +128,10 @@ impl<T: Readable> LengthReadableArgs<([u8; 32], [u8; 32])> for ChaChaDualPolyRea
ChaChaDualPolyReader { chacha: &mut chacha, poly: &mut mac, read_len: 0, read: s };

let readable: T = Readable::read(&mut chacha_stream)?;
chacha_stream.read.eat_remaining()?;
while chacha_stream.read.bytes_remain() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused by this change. eat_remaining will consume the remaining bytes and error if we didn't read all of the bytes in the initial call to read, which seems correct to me. Doesn't any bytes remaining indicate a buggy node, since they incorrectly encoded the length we were supposed to read in the LengthLimitedReader? If we're reading multiple objects, seems like the underlying Readable should be on a tuple and still not have any bytes remaining.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eat_remaining will read those bytes directly, though, rather than going through the chacha_stream. Note that we call eat_remaining on chacha_stream.read not on chacha_stream. As a result, any bytes which get read via that will not be included in the HMAC, which seems wrong.

let mut buf = [0; 256];
chacha_stream.read(&mut buf)?;
}

let read_len = chacha_stream.read_len;

Expand Down Expand Up @@ -199,7 +206,10 @@ impl<T: Readable> LengthReadableArgs<[u8; 32]> for ChaChaPolyReadAdapter<T> {
let s = FixedLengthReader::new(r, decrypted_len);
let mut chacha_stream = ChaChaPolyReader { chacha: &mut chacha, read: s };
let readable: T = Readable::read(&mut chacha_stream)?;
chacha_stream.read.eat_remaining()?;
while chacha_stream.read.bytes_remain() {
let mut buf = [0; 256];
chacha_stream.read(&mut buf)?;
}

let mut tag = [0 as u8; 16];
r.read_exact(&mut tag)?;
Expand Down
5 changes: 2 additions & 3 deletions lightning/src/offers/flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -543,13 +543,12 @@ where

/// Verifies the provided [`AsyncPaymentsContext`] for an inbound [`HeldHtlcAvailable`] message.
///
/// The context is verified using the `nonce` and `hmac` values, and ensures that the context
/// has not expired based on `path_absolute_expiry`.
/// Because blinded path contexts are verified as a part of onion message processing, this only
/// validates that the context is not yet expired based on `path_absolute_expiry`.
///
/// # Errors
///
/// Returns `Err(())` if:
/// - The HMAC verification fails for inbound context.
/// - The inbound payment context has expired.
#[cfg(async_payments)]
pub fn verify_inbound_async_payment_context(
Expand Down
2 changes: 1 addition & 1 deletion lightning/src/offers/signer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ const WITH_ENCRYPTED_PAYMENT_ID_HMAC_INPUT: &[u8; 16] = &[4; 16];
// `OffersContext`, but were removed in LDK v0.2 with the introduction of `ReceiveAuthKey`-based
// authentication.
// Their corresponding values (`[5; 16]` and `[7; 16]`) are now reserved and must not
// be reused to preserve backward compatibility.
// be reused to ensure type confusion attacks are impossible.
//
// Reserved HMAC_INPUT values — do not reuse:
//
Expand Down
Loading