lnwire: enforce bolt 4 rules for onion message final-hop payloads#10948
lnwire: enforce bolt 4 rules for onion message final-hop payloads#10948bitromortac wants to merge 4 commits into
Conversation
When decoding an onion message payload, the loop that forwards unrecognized final hop TLVs to higher layers skipped any entry with a zero-length value. DecodeWithParsedTypesP2P marks a recognized type with a nil map entry but records the raw bytes for an unknown type, and an unknown odd TLV with an empty value is valid. Keying the skip off a length check therefore dropped such a TLV instead of passing it through. Test the recognized-type skip against a nil entry so a valid unknown odd zero-length TLV is preserved.
BOLT 4 requires the final node to ignore an onion message whose onionmsg_tlv contains an unknown even type, since even types are "must understand". The TLV stream decoder does not enforce this on its own: its parsed-type map collects unknown types of either parity, so an even type such as 70 would otherwise be accepted as a final hop payload. Reject any unknown even type during decode, regardless of its range. The check runs before the final hop range skip so unknown even types below type 64 are rejected as well.
BOLT 4 requires the final node to ignore an onion message whose onionmsg_tlv contains more than one payload field, where payload fields are the tlv types reserved for the final hop (type 64 and above). Decode previously accumulated every such field it found, so a payload bundling invoice_request, invoice, and invoice_error together was accepted. Reject the payload when more than one final hop field is present. Every entry collected in FinalHopTLVs is in the final hop range, so its count is the number of payload fields. The round-trip test for multiple fields becomes a rejection test, and the property test now draws at most one payload field.
github-actions
Bot
added
the
severity-critical
🔴 PR Severity: CRITICAL
🔴 Critical (1 file)
🟢 Low (2 files)
AnalysisThis PR changes No bump conditions apply: only 2 non-test/non-generated files changed (well under the 20-file threshold) and ~55 non-test lines changed (well under 500), and only a single critical package ( To override, add a |
vctt94
left a comment
vctt94
left a comment
There was a problem hiding this comment.
LGTM!
This resolves the onion-message payload comments: unknown even TLVs now fail regardless of range, unknown odd zero-length final-hop TLVs are preserved via the nil check, and multiple final-hop payload fields are rejected after collecting both known final-hop namespaces and unknown odd final-hop TLVs.
great job!
4 participants
While reviewing #10789 (h/t to @vctt94), three BOLT 4 compliance gaps in onion message decoding came up but were deferred as out of scope.
This PR forces that we only accept a single final hop payload type and that we correctly handle even/odd acceptance. We could in principle also refactor
OnionMessagePayloadto only carry a single final hop TLV, but that would need further consumer changes.