Fix hop payload bounds#74
Conversation
Summary of ChangesHello @erickcestari, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a critical fix to how onion packet payloads are processed, specifically preventing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request addresses a potential vulnerability in unwrapPacket where a malformed packet could cause the decoder to read beyond the intended routing information boundary. The fix correctly restricts the data passed to DecodeHopPayload by slicing the buffer to the appropriate length. This is a solid and important correction. The addition of a specific test case, TestUnwrapPacketBeyondRoutingInfoBoundary, is excellent as it directly verifies the fix against the described vulnerability. I've also noted the correction of typos in other test function names. My review includes a couple of minor suggestions for the new test code to align with the repository's style guide concerning line length.
a8eb498 to
3fad506
Compare
|
Maybe we should also change |
3fad506 to
ace6bdb
Compare
Updated! I have created a new constant |
IIUC it still results in the malformed packet being rejected? Or which inconsistencies are you referring to here? |
| // instructions. | ||
| hopPayload, err := DecodeHopPayload( | ||
| bytes.NewReader(hopInfo), tlvPayloadOnly, | ||
| bytes.NewReader(hopInfo[:routingInfoLen]), tlvPayloadOnly, |
There was a problem hiding this comment.
👍
We should restrict to the actual length of the payload, rather than trusting the sender to accurately encode the length within the payload.
The MAC check of the next hop should fail, but then the HTLC fails one hop later than it should actually.
There was a problem hiding this comment.
Not sure if I follow. That would require to already peak bytes from the payload, which is what is happening in DecodeHopPayload. Are you suggesting to lift that logic out of that func?
There was a problem hiding this comment.
What I mean was that before we assumed the length encoded and the actual length of the packet were the same thing. This PR fixes that.
There was a problem hiding this comment.
sphinx_test_diff.txt
Onion messaging is throwing a spanner in the works. Onion messages allow for all kinds of payload sizes, also way bigger than 1300 bytes. As such this fix will not work. (See also my other comments for more details) I've created a diff file that I'll share here that updates one of the unit tests to make a huge payload that shouldn't err out but currently does because of the added check.
| // instructions. | ||
| hopPayload, err := DecodeHopPayload( | ||
| bytes.NewReader(hopInfo), tlvPayloadOnly, | ||
| bytes.NewReader(hopInfo[:routingInfoLen]), tlvPayloadOnly, |
There was a problem hiding this comment.
Not sure if I follow. That would require to already peak bytes from the payload, which is what is happening in DecodeHopPayload. Are you suggesting to lift that logic out of that func?
As you said, the packet would be forwarded to the next hop, where it would be rejected as invalid due to its invalid |
ace6bdb to
cac3ec8
Compare
The unwrapPacket function passes the full hopInfo buffer to DecodeHopPayload for parsing. However, this buffer contains both the actual routing information and zero-padding used for XOR decryption. The routing info occupies the first portion of the buffer (determined by the routingInfoLen parameter), while the remainder is padding. When a malformed packet contains an oversized payload length field, the decoder could read beyond the routing info boundary into the padding area. This creates parsing inconsistencies with other implementations that correctly enforce size boundaries. This commit constrains DecodeHopPayload to read only from the routing info portion by slicing hopInfo to routingInfoLen bytes. This fix is compatible with both: - HTLC onion packets (update_add_htlc): 1300-byte routing info - Onion messages: variable size up to ~32KB routing info The boundary is determined dynamically by the routingInfoLen parameter passed to unwrapPacket, so the fix works correctly regardless of the onion packet type being processed. A malformed packet that attempts to read beyond its routing info boundary will now fail with an EOF error during payload decoding, rather than silently reading from the padding area.
Rename TestSphinxNodeRelpay* to TestSphinxNodeReplay*.
cac3ec8 to
93404fe
Compare
Yeah, I understood that. My comment was on @Roasbeef earlier comment. Sorry for the confusion. |
|
@erickcestari So with the current solution, a malformed packet could still look into the bytes of the 32-byte HMAC, correct? |
Yes, I only saw it after I commented 😅.
No. If a malformed payload tries to read into the For example, with a 1300-byte boundary: if a payload claims 1280 bytes, after reading the 3-byte length prefix + 1280-byte payload, only 17 bytes remain. Not enough for the 32-byte HMAC read. Am I missing something? |
|
@gijswijs: review reminder |
gijswijs
left a comment
There was a problem hiding this comment.
No. If a malformed payload tries to read into the
HMACarea, the subsequent 32-byteHMACread will fail withio.ErrUnexpectedEOFdue to insufficient remaining bytes.
Clear. But before it fails it will read potentially some bytes of the HMAC area into the payload, here:
https://github.com/erickcestari/lightning-onion/blob/93404feef6d7b8c1768b7cc661186b8b838dc8f5/payload.go#L127-L132
Or, if the payloadSize is already too big for reading the payload you get an io.ErrUnexpectedEOF there.
But we mean the basically the same thing.
gijswijs
left a comment
There was a problem hiding this comment.
Could you apply the diff I've linked to in this comment?
It extends the testing of payloads that exceed 1300 bytes. It's a variant of the earlier diff I created and it will catch a regression to the earlier fix you provided. After that everything seems good to go.
- Refactor newOnionMessageRoute to accept customizable last hop message and return BlindedPath for better test flexibility - Expand TestPaymentPathTotalPayloadSizeExceeds1300 to test both single-hop with large payload and multi-hop routes exceeding 1300 bytes - Add packet processing validation to verify proper handling of oversized payloads - Add t.Parallel() to replay tests for concurrent execution Co-authored-by: gijswijs <gijswijs@users.noreply.github.com>
Added! 🫡 |
Pull Request Test Coverage Report for Build 20800496391Details
💛 - Coveralls |
The
unwrapPacketfunction passes the fullhopInfobuffer toDecodeHopPayloadfor parsing. However, this buffer contains both the actual routing information and zero-padding used for XOR decryption.The routing info occupies the first portion of the buffer (determined by the
routingInfoLenparameter), while the remainder is padding.When a malformed packet contains an oversized payload length field, the decoder could read beyond the routing info boundary into the padding area.
This PR constrains
DecodeHopPayloadto read only from the routing info portion by slicinghopInfotoroutingInfoLenbytes. This fix is compatible with both:update_add_htlc): 1300-byte routing infoThe boundary is determined dynamically by the
routingInfoLenparameter passed tounwrapPacket, so the fix works correctly regardless of the onion packet type being processed.A malformed packet that attempts to read beyond its routing info boundary will now fail with an EOF error during payload decoding, rather than silently reading from the padding area.
Found through differential fuzzing (bitcoinfuzz) where Core Lightning and rust-lightning rejected the malformed onion packet.