Various followups to #3917 #3941
Open
+19
−21
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We recently switched to `ReceiveAuthKey`-based blinded path authentication, removing various fields used to authenticate blinded paths from contexts. In doing so, we left a comment arguing that such fields should not have their TLV IDs reused, however there's no reason for that restriction. If we reuse old blinded paths with those fields set they'll fail the new authentication checks anyway, so the fields should never end up being seen in blinded paths we'd accept. This simply updates the comments.
We recently switched to `ReceiveAuthKey`-based blinded path authentication, removing various fields used to authenticate blinded paths from contexts. In doing so we forgot to update the documentation on `verify_inbound_async_payment_context` to note that it no longer does any cryptographic verification but instead only validates the expiry. Here we update that documentation.
We recently switched to `ReceiveAuthKey`-based blinded path authentication, removing various fields used to authenticate blinded paths from contexts. In doing so we removed no-longer-needed `HMAC_INPUT`s in offer metadata validation, and left a comment noting that previously used values should not be reused. That comment was slightly incorrect as it indicated some kind of "backward compatibility" concern, but of course we broke backwards compatibility when we stopped accepting the previous authentication scheme. Instead, here, we update the comment to note that what we're protecting against is a type confusion attack.
`ChaChaPolyWriteAdapter` is used to wrap `Writeable` objects in an AEAD before serializing them. This is great, except that we sometimes call `<T as ChaChaPolyWriteAdapter>::encode` which first calls `Writeable::serialized_length` to calculate the length to pre-allocate in a `Vec` before doing the actual write. Because `ChaChaPolyWriteAdapter` did not override `Writeable::serialized_length`, this led to doing the full cryptographic AEAD encryption and MAC twice, once to calculate the length and once to actually write the data. Instead, here, we override `<ChaChaPolyWriteAdapter as Writeable::serialized_length` to simply calculate the length we need.
`ChaCha[Dual]PolyReadAdapter` currently read the encrypted object using `Readable` through the ChaCha stream (including the Poly1305 HMAC), but then consume any remaining bytes directly. This results in any extra bytes not consumed by the desired type's `Readable` being ignored and not included in the HMAC check. This is likely not the desired behavior - if we get some data which has extra slack at the end we ignore, it should still be authenticated as the sender likely thinks that data has meaning and included it in their HMAC check. Luckily, I believe this is currently dead code - `ChaCha[Dual]PolyReadAdapter` are only used for TLV stream reads which consume the full underlying stream. However, if either is used for non-TLV-streams in the future, this may be important. Here we simply push any extra bytes read through the ChaCha20Poly1305 reader, ensuring extra data is included in the HMAC check.
|
👋 Thanks for assigning @valentinewallace as a reviewer! |
| @@ -128,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() { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
valentinewallace
approved these changes
Jul 18, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some comment/doc corrections, a bug in dead code, and a performance improvement.