lightningdevkit
bolt11: incorrect parsing due to tagged fields conversion
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.
| Implementation | Payee |
|---|---|
| LND | 03ce9acfb825b3ae1065cec8a3b27a4987faa4b3f4d2e0be64750bd70e13f800de |
| core-lightning | 03ce9acfb825b3ae1065cec8a3b27a4987faa4b3f4d2e0be64750bd70e13f800de |
| rust-lightning | 0271ee1a7baa96c8dedf414fc6edb9c07d081a39e6ba1fb4c9d8a0e365d9d0065a |
By 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.
| Implementation | Data part |
|---|---|
| LND | [0, 8, 139, 148, 1, 13, 32, 12, 0, 50, 8, 0, 0, 0, 0, 0, 0, 0, 0, 24, 0, 100, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 24, 0, 128, 0, 0, 0, 160, 8, 26, 0, 0] |
| rust-lightning | [0, 8, 139, 148, 1, 13, 32, 12, 0, 50, 8, 0, 0, 0, 0, 0, 0, 0, 0, 24, 0, 100, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 24, 0, 128, 0, 0, 0, 160, 3, 64] |
| rust-lightning (Fe32) | [Fe32(0), Fe32(0), Fe32(4), Fe32(8), Fe32(23), Fe32(5), Fe32(0), Fe32(1), Fe32(1), Fe32(20), Fe32(16), Fe32(0), Fe32(24), Fe32(0), Fe32(1), Fe32(18), Fe32(1), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(24), Fe32(0), Fe32(1), Fe32(18), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(6), Fe32(0), Fe32(0), Fe32(16), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(5), Fe32(0), Fe32(0), Fe32(13), Fe32(0), Fe32(0)] |
We just a quick test and with the following modification to write_tagged_field it returns the correct payee for the provided invoice:
fn write_tagged_field<'s, P>(
tag: u8, payload: &'s P,
) -> TaggedFieldIter<Box<dyn Iterator<Item = Fe32> + 's>>
where
P: Base32Iterable + Base32Len + ?Sized,
{
let len = payload.base32_len();
assert!(len < 1024, "Every tagged field data can be at most 1023 bytes long.");
// Special handling for feature bits (tag 5) with empty payload
if tag == 5 && len == 0 {
// Create a new payload iterator that includes an extra 0
let modified_payload: Box<dyn Iterator<Item = Fe32> + 's> = Box::new(
std::iter::once(Fe32::try_from(0).expect("< 32")).chain(payload.fe_iter()),
);
// Use len=1 for the modified payload
return [
Fe32::try_from(tag).expect("invalid tag, not in 0..32"),
Fe32::try_from(0).expect("< 32"), // len / 32 = 0
Fe32::try_from(1).expect("< 32"), // len % 32 = 1
]
.into_iter()
.chain(modified_payload);
}
// Normal case - unchanged
[
Fe32::try_from(tag).expect("invalid tag, not in 0..32"),
Fe32::try_from((len / 32) as u8).expect("< 32"),
Fe32::try_from((len % 32) as u8).expect("< 32"),
]
.into_iter()
.chain(payload.fe_iter())
}
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:
---- ser::test::test_raw_data_base32_roundtrip stdout ----
thread 'ser::test::test_raw_data_base32_roundtrip' panicked at lightning-invoice/src/ser.rs:533:9:
assertion `left == right` failed
left: [Fe32(0), Fe32(0), Fe32(4), Fe32(8), Fe32(23), Fe32(5), Fe32(0), Fe32(1), Fe32(1), Fe32(20), Fe32(16), Fe32(0), Fe32(24), Fe32(0), Fe32(1), Fe32(18), Fe32(1), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(24), Fe32(0), Fe32(1), Fe32(18), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(6), Fe32(0), Fe32(0), Fe32(16), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(5), Fe32(0), Fe32(1), Fe32(0), Fe32(13), Fe32(0), Fe32(0)]
right: [Fe32(0), Fe32(0), Fe32(4), Fe32(8), Fe32(23), Fe32(5), Fe32(0), Fe32(1), Fe32(1), Fe32(20), Fe32(16), Fe32(0), Fe32(24), Fe32(0), Fe32(1), Fe32(18), Fe32(1), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(24), Fe32(0), Fe32(1), Fe32(18), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(6), Fe32(0), Fe32(0), Fe32(16), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(0), Fe32(5), Fe32(0), Fe32(0), Fe32(13), Fe32(0), Fe32(0)]
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..
