bolt11: incorrect parsing due to tagged fields conversion #3693
Comments
|
I made a simple test to demonstrate the parsing discrepancy: #[test]
fn test_raw_data_base32_roundtrip() {
use bech32::Fe32;
// These are the expected Fe32 values that should round-trip correctly
// The critical difference is in the handling of tag 5 (Features) with empty payload
let expected_values: Vec<u8> = vec![
0, 0, 4, 8, 23, 5, 0, 1, 1, 20, 16, 0, 24, 0, 1, 18, 1, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
0, 0, 0, 0, 24, 0, 1, 18, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 6, 0,
0, 16, 0, 0, 0, 0, 0, 5, 0, 1, 0, 13, 0, 0,
];
// Convert to Fe32 for processing
let values_input: Vec<Fe32> = expected_values
.iter()
.map(|&v| <Fe32 as TryFrom<u8>>::try_from(v).expect("Value out of range"))
.collect();
// Round-trip through the parser
let raw_data = RawDataPart::from_base32(&values_input).unwrap();
let actual_output = raw_data.fe_iter().collect::<Vec<_>>();
// The test fails because the parser doesn't correctly handle empty Features field
assert_eq!(values_input, actual_output, "Failed to correctly round-trip BOLT11 data");
}Result: |
vincenzopalazzo
added a commit
to vincenzopalazzo/rust-lightning
that referenced
this issue
Apr 3, 2025
This is a reproduction of the issue that I found in the lightning-invoice crate. The issue is described in details in [1]. [1] lightningdevkit#3693 Suggested-by: @erickcestari Signed-off-by: Vincenzo Palazzo <[email protected]>
|
Nice, thanks for doing differential fuzzing. So the invoice here has an all-0s length-1 (5-bits) features, which obviously no one should be writing (though really no idea how you were fuzzing LDK with this, given it fails to parse for two reasons - missing payment secret and missing the payment-secret feature bit). We currently fail to round-trip serialize this for a sighash because we strip excess 0-bytes from features after we read them. Sadly, we have to do this to prevent a corollary bug which would have the same impact. We store features as a list of bytes, and thus once we load them we lose the number-of-bits information stored in the invoice itself. We currently always only write out the field elements we need, skipping any 0-FEs that we don't actually need (as in this case). Even if we didn't do that, there's no way for us to store that we need one FE as storing the one zero byte would result in us writing two FEs to get 10 bits. IMO this should be a spec amendment to make this invoice invalid, rather than us go store the features length information separately just to fix round-trip for invoice generators that include extra unnecessary FEs.. |
TheBlueMatt
added a commit
to TheBlueMatt/lightning-rfc
that referenced
this issue
Apr 3, 2025
When decoding BOLT 11 invoices, LDK has always read them into fields, parsing what it can. When parsing features, this loses the information on the number of field elements which were used to encode the features in the invoice itself. When we then go to calculate a hash of the invoice for signature validation/key recovery, we go re-serialize the invoice. At this point, any excess field elements used to encode features will be lost and the invoice's hash will be different from what the original encoder intended. Luckily, this doesn't appear to have ever happened in practice. This was, in fact, only found by @erickcestari, @brunoerg, and @morehouse when doing differential fuzzing. Because this hasn't happened and it breaks a straightforward way to handle BOLT 11 parsing, there's no reason to retain it, so instead here we simply forbid non-minimally-encoded features in BOLT 11 invoices. See lightningdevkit/rust-lightning#3693 for the specific example generated by fuzzing.
TheBlueMatt
added a commit
to TheBlueMatt/lightning-rfc
that referenced
this issue
Apr 7, 2025
When decoding BOLT 11 invoices, LDK has always read them into fields, parsing what it can. When parsing features, this loses the information on the number of field elements which were used to encode the features in the invoice itself. When we then go to calculate a hash of the invoice for signature validation/key recovery, we go re-serialize the invoice. At this point, any excess field elements used to encode features will be lost and the invoice's hash will be different from what the original encoder intended. Luckily, this doesn't appear to have ever happened in practice. This was, in fact, only found by @erickcestari, @brunoerg, and @morehouse when doing differential fuzzing. Because this hasn't happened and it breaks a straightforward way to handle BOLT 11 parsing, there's no reason to retain it, so instead here we simply forbid non-minimally-encoded features in BOLT 11 invoices. See lightningdevkit/rust-lightning#3693 for the specific example generated by fuzzing.
TheBlueMatt
added a commit
to TheBlueMatt/lightning-rfc
that referenced
this issue
Apr 7, 2025
When decoding BOLT 11 invoices, LDK has always read them into fields, parsing what it can. When parsing features, CLTV deltas, and expiry times, this loses the information on the number of field elements which were used to encode the features in the invoice itself. When we then go to calculate a hash of the invoice for signature validation/key recovery, we go re-serialize the invoice. At this point, any excess field elements used to encode fields will be lost and the invoice's hash will be different from what the original encoder intended. Luckily, this doesn't appear to have ever happened in practice. This was, in fact, only found by @erickcestari, @brunoerg, and @morehouse when doing differential fuzzing. Because this hasn't happened and it breaks a straightforward way to handle BOLT 11 parsing, there's no reason to retain it, so instead here we simply forbid non-minimally-encoded features in BOLT 11 invoices. See lightningdevkit/rust-lightning#3693 for the specific example generated by fuzzing.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
In a joint work with @erickcestari and @morehouse on differential fuzzing of Lightning Network implementations, we discovered a parsing discrepancy (BOLT11). Our testing revealed that rust-lightning incorrectly identifies the payee when processing the following invoice:
lnbc1qqygh9qpp5sqcqpjpqqqqqqqqqqqqqqcqpjqqqqqqqqqqqqqqqqqqxqqsqqqqq9qpqdqqqqqqqqqqqqqpjpqqlqqqqqqqqqqqqqqcqpjqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqqlqqqqqqqqqqqqqqqqqqqqqqqyzg3dy.03ce9acfb825b3ae1065cec8a3b27a4987faa4b3f4d2e0be64750bd70e13f800de03ce9acfb825b3ae1065cec8a3b27a4987faa4b3f4d2e0be64750bd70e13f800de0271ee1a7baa96c8dedf414fc6edb9c07d081a39e6ba1fb4c9d8a0e365d9d0065aBy debugging both LND and rust-lightning, we noticed a mismatch in the data part, as shown in the following table. When converting the tagged fields to 5-bit format, only the end changes because the payment hash and its bytes come first. The issue occurs when parsing the Features tag and Description tag.
In the format, the tag comes first, followed by len/32 and len%32, and then the payload. The tag for Features is 5 and is being interpreted as length 0, which creates a difference at the end of the data. The Features as [5, 0, 0] which should be [5, 0, 1, 0]. It appears that the payload is getting truncated when the tag is unknown.
We just a quick test and with the following modification to
write_tagged_fieldit returns the correct payee for the provided invoice:The text was updated successfully, but these errors were encountered: