BOLT 12 deserialization fuzzers #1977

jkczyz · 2023-01-20T22:56:17Z

Add fuzzers for BOLT 12 message deserialization

bech32-encoded string parsing and TLV stream deserialization for Offer and Refund
TLV stream deserialization for Invoice and InvoiceRequest

Also makes the offers module public.

Based on #1926.

codecov-commenter · 2023-01-20T23:14:49Z

Codecov Report

Base: 90.89% // Head: 91.37% // Increases project coverage by +0.47% 🎉

Coverage data is based on head (9c2a3d0) compared to base (8ecd7c3).
Patch coverage: 88.97% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1977      +/-   ##
==========================================
+ Coverage   90.89%   91.37%   +0.47%     
==========================================
  Files          99      104       +5     
  Lines       52570    63898   +11328     
  Branches    52570    63898   +11328     
==========================================
+ Hits        47785    58386   +10601     
- Misses       4785     5512     +727

Impacted Files	Coverage Δ
lightning/src/lib.rs	`71.42% <ø> (-28.58%)`	⬇️
lightning/src/routing/gossip.rs	`91.49% <ø> (-0.39%)`	⬇️
lightning/src/util/ser_macros.rs	`89.85% <0.00%> (+3.12%)`	⬆️
lightning/src/offers/offer.rs	`93.63% <75.00%> (+0.63%)`	⬆️
lightning/src/offers/refund.rs	`97.25% <85.71%> (+1.20%)`	⬆️
lightning/src/offers/parse.rs	`94.00% <86.95%> (ø)`
lightning/src/offers/invoice.rs	`93.17% <89.70%> (-1.63%)`	⬇️
lightning/src/offers/invoice_request.rs	`96.79% <95.65%> (+0.55%)`	⬆️
lightning/src/offers/payer.rs	`100.00% <100.00%> (ø)`
... and 72 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

fuzz/src/invoice_request_deser.rs

TheBlueMatt · 2023-01-31T00:22:35Z

Needs rebase, but otherwise LGTM. Do you want to target this/making the offer structs public in 114 or do you want to wait until early in the 115 cycle?

jkczyz · 2023-01-31T02:22:38Z

Needs rebase, but otherwise LGTM. Do you want to target this/making the offer structs public in 114 or do you want to wait until early in the 115 cycle?

I'd lean towards making them public for 114. Would let others play around with the API awhile.

jkczyz · 2023-01-31T02:48:11Z

Rebased and squashed. Also enabled previously ignored doc tests.

TheBlueMatt · 2023-01-31T18:02:30Z

Looks like CI is sad on the refund having two different call semantics thing. Thinking more about it, I think we need to undo that - one issue is that we could have two downstream dependencies, one which sets the std flag, and one which does not, all built in the same project. This will cause the one without the std flag to have a compilation error because it expects the no-std call semantics, but the std feature got applied globally.

jkczyz · 2023-01-31T20:42:20Z

Fixed the docs and made respond_with_no_std versions available regardless of feature flag. Seemed the simplest way to reuse code and not need to repeat the docs.

TheBlueMatt

This basically LGTM, will find some time to run the fuzzers.

lightning/src/offers/invoice.rs

TheBlueMatt · 2023-02-01T05:12:34Z

lightning/src/offers/invoice.rs

+	/// path.
+	pub cltv_expiry_delta: u16,
+
+	/// The minimum HTLC value (in millisatoshi) that is acceptable to all channel peers on the


This should specify if its an amount as received by the final node or an amount as received by the introduction point.

Hmm... the blinded path is treated as one hop with regard to path finding. So you can think of this being the minimum amount the final node is willing to accept over the "hop" from the introduction point, which would mean that it must be acceptable to all peers on the path, no? (e.g., if the final hop is 1 msat and one of the previous hops is 10 msats, this value would need to be 10 msats. i.e., it's the maximum of all minimums)

Maybe I don't follow your question though.

The difference is the fees - if the min hop at the end if 10msats, then the minimum at the introduction point is 10+fee msats.

Gotcha. Adjusted the comment to note fees along the blinded path need to be accounted for.

"accounting for fees along the path" still doesn't tell me if its the amount as seen by the introduction point, or the amount as seen by the recipient.

Note that the same applies to htlc_maximum_msat as well.

IIUC, the previous hop should be for the amount seen by the introduction point since that is the hop to it. So this must therefore be for the amount seen by the final node.

Updated. Let me know if I misunderstood or if you have a different wording in mind.

TheBlueMatt

One further comment on the fuzz tests themselves. Gonna give them some time today.

TheBlueMatt · 2023-02-02T21:34:27Z

fuzz/src/invoice_request_deser.rs

+			fee_proportional_millionths: 1_000,
+			cltv_expiry_delta: 42,
+			htlc_minimum_msat: 100,
+			htlc_maximum_msat: 1_000_000_000_000,


I'm afraid of overflows based on the blinded pay infos, let's somehow read these amounts in from the fuzzer input, maybe just read the numbers as the first N bytes of input and pass them to build_response? Same goes for refunds.

Not sure I'm following how that would find an overflow. We are only writing these here.

Ah, I was kinda getting ahead of myself. So my worry is that the data in the blinded paths is, effectively, pull from untrusted sources (the network graph) and from there we'll incorporate it into the invoice data. That said, its not really the BlindedPayInfo that's the problem, its the code that creates the BlindedPayInfo that is. Once we have utilities to build a BlindedPayInfo from the network graph, we should update the code here to use them with some kind of fuzz-produced network graph. Until then, this is fine.

TheBlueMatt · 2023-02-03T01:20:30Z

I'd lean towards making them public for 114. Would let others play around with the API awhile.

I guess I don't really understand this - we can't really do anything with the BOLT12 datastructures yet - we can't send or accept to blinded paths, and don't have utilities in place to send or handle the messages from onion messages. Is there some specific aspect of the API you anticipate people using in its current state? Otherwise I'd just assume keep it private until its usable, maybe even make the pub form cfg(fuzzing)

jkczyz · 2023-02-03T14:47:05Z

I guess I don't really understand this - we can't really do anything with the BOLT12 datastructures yet - we can't send or accept to blinded paths, and don't have utilities in place to send or handle the messages from onion messages. Is there some specific aspect of the API you anticipate people using in its current state? Otherwise I'd just assume keep it private until its usable, maybe even make the pub form cfg(fuzzing)

Folks from VLS have been interested in experimenting with parsing and signature verification. Could maybe put it behind a feature if you prefer?

TheBlueMatt · 2023-02-03T17:53:07Z

Folks from VLS have been interested in experimenting with parsing and signature verification.

Ah! I'd totally forgot about that. Yep, that sounds great, then!

arik-so

Fuzz code seems perfectly fine to me.

arik-so · 2023-02-03T19:45:09Z

lightning/src/offers/invoice.rs

@@ -45,7 +45,12 @@
 //!
 //! // Invoice for the "offer to be paid" flow.
 //! InvoiceRequest::try_from(bytes)?
-//!     .respond_with(payment_paths, payment_hash)?
+#![cfg_attr(feature = "std", doc = r#"


what does r# do?

Raw string literal using one #. It can use zero or more of them to avoid closing the literal early. But looks like I don't need to use a raw string literal at all here. Was copying this from a longer example. Removed.

Both Refund::respond_with and InvoiceRequest::respond_with take a created_at since the Unix epoch Duration in no-std. However, this can cause problems if two downstream dependencies want to use the lightning crate with different feature flags set. Instead, define respond_with_no_std versions of each method in addition to a respond_with version in std.

This is needed in order to fuzz test BOLT 12 message deserialization.

TheBlueMatt · 2023-02-15T21:52:36Z

This appears to fix it:

-                       let expected_amount_msats = offer_amount_msats * quantity.unwrap_or(1);
+                       let expected_amount_msats = offer_amount_msats.checked_mul(quantity.unwrap_or(1)).ok_or(SemanticError::InvalidAmount)?;

jkczyz · 2023-02-15T22:48:58Z

This appears to fix it:

-                       let expected_amount_msats = offer_amount_msats * quantity.unwrap_or(1);
+                       let expected_amount_msats = offer_amount_msats.checked_mul(quantity.unwrap_or(1)).ok_or(SemanticError::InvalidAmount)?;

Thanks! Added the fix and a couple tests.

jkczyz · 2023-02-15T22:53:35Z

There may be a similar overflow possible in InvoiceBuilder. Will fix in a little bit.

TheBlueMatt · 2023-02-17T20:06:29Z

This input causes invoice_request_deser to fail with an invalid signature generation.

000008000a06000000000d00162103414141414141414141414141414141 41414141414f4f4f4f4ff5000000405d2458210306030079798312797979 7974f07ef00af0f0f000f0016b057df0f00000f09d00f040620000000000 000000000000000000000000000000000000000000000000000006030079 7983127979797974f07ef00af0f0f000f0016b057df0f00000f09d00fd02 f500fd02fd00

jkczyz · 2023-02-19T02:34:09Z

This input causes invoice_request_deser to fail with an invalid signature generation.

000008000a06000000000d00162103414141414141414141414141414141 41414141414f4f4f4f4ff5000000405d2458210306030079798312797979 7974f07ef00af0f0f000f0016b057df0f00000f09d00f040620000000000 000000000000000000000000000000000000000000000000000006030079 7983127979797974f07ef00af0f0f000f0016b057df0f00000f09d00fd02 f500fd02fd00

Oh, right. When creating the Invoice, we sign using a static key, but it needs to match the randomly generated one set in OfferContents::signing_pubkey. I suppose we can disable the verify_schnorr check in fuzzing when signing? That way, we still exercise the signature code but just don't check that the signature corresponds to the signing pubkey.

TheBlueMatt · 2023-02-20T03:34:27Z

Can we just skip the unwrap on the sign result? That way we still do the sign (and it can still pass, sometimes), but doesn't need to.

TheBlueMatt · 2023-02-21T17:07:14Z

Actually, on second thought, can we still do the unwrap but check that the result is ok/err based on if the signing pubkey matches?

jkczyz · 2023-02-21T18:57:18Z

Can we just skip the unwrap on the sign result? That way we still do the sign (and it can still pass, sometimes), but doesn't need to.

I suppose so, but we'd be skipping the remaining code in UnsignedInvoice::sign in order to call something we know will almost certainly fail. The skipped code isn't doing much now, but possibly could in the future.

Actually, on second thought, can we still do the unwrap but check that the result is ok/err based on if the signing pubkey matches?

Hmm... the only time it will be ok is if we randomly find the signing pubkey associated with the hardcoded private key used in sign.

TheBlueMatt · 2023-02-21T19:19:00Z

Hmm... the only time it will be ok is if we randomly find the signing pubkey associated with the hardcoded private key used in sign.

The fuzzer should have no problem making 32 bytes of the input match something that's hard-coded, as long as the hard-coded thing is a "nice" pattern.

Offer and Refund derive Debug, Clone, and PartialEq. For consistency, derive these traits for InvoiceRequest and Invoice as well.

TheBlueMatt · 2023-02-23T00:18:35Z

Can you provide a fuzz test case which hits the "fix amount overflow in invoice building" fix? Otherwise this LGTM.

jkczyz · 2023-02-23T16:41:29Z

Can you provide a fuzz test case which hits the "fix amount overflow in invoice building" fix? Otherwise this LGTM.

Building an Invoice from the following InvoiceRequest would hit it, but we actually fail to parse the InvoiceRequest -- the test added in that commit needed build_unchecked to create it. So I believe the code is actually unreachable.

// hex-encoded invoice_request
"00200101010101010101010101010101010101010101010101010101010101010101080203e80a03666f6f1400162102bb58b5feca505c74edc000d8282fc556e51a1024fc8e7d7e56c6f887c5c8d5f25608ffffffffffffffff5821035be5e9478209674a96e60f1f037f6176540fd001fa1d64694770c56a7709c42cf040666c8601f9eaae3cb70d26b9f9e699173d6f38cda2ea929a18ee07fb894db7b5d3f4a35be410835d38cbe6e0c52cf27bee78b4b3b4761fa11a52e24abe88ad89"

TheBlueMatt · 2023-02-23T23:03:32Z

fuzz/src/invoice_request_deser.rs

+					.sign::<_, Infallible>(
+						|digest| Ok(secp_ctx.sign_schnorr_no_aux_rand(digest, &keys))
+					)
+					.unwrap_err();


This unwrap is reachable - the fuzzer signatures for the odd pubkey and even pubkey (ie beginning with 2/3) with the same following 32 bytes is identical, so we can actually have two different key but the same signature.

Changed to check either parity, IIUC.

An invoice request is serialized as a TLV stream and encoded as bytes. Add a fuzz test that parses the TLV stream and deserializes the underlying InvoiceRequest. Then compare the original bytes with those obtained by re-serializing the InvoiceRequest.

An invoice is serialized as a TLV stream and encoded as bytes. Add a fuzz test that parses the TLV stream and deserializes the underlying Invoice. Then compare the original bytes with those obtained by re-serializing the Invoice.

In order to fuzz test Bech32Encode parsing independent of the underlying message deserialization, the trait needs to be exposed. Conditionally expose it only for fuzzing.

Fuzz testing bech32 decoding along with deserializing the underlying message can result in overly exhaustive searches. Instead, the message deserializations are now fuzzed separately. Add fuzzing for bech32 decoding.

An overflow can occur when multiplying the offer amount by the requested quantity when checking if the given amount is enough. Return an error instead of overflowing.

An overflow can occur when multiplying the offer amount by the requested quantity when no amount is given in the request. Return an error instead of overflowing.

alecchendev

Mainly took a look at the fuzzers and things seemed to make sense to me 👍

alecchendev · 2023-02-24T02:23:33Z

fuzz/src/invoice_request_deser.rs

+				unsigned_invoice
+					.sign::<_, Infallible>(
+						|digest| Ok(secp_ctx.sign_schnorr_no_aux_rand(digest, &keys))
+					)
+					.unwrap()
+					.write(&mut buffer)
+					.unwrap();


I'm wondering why is making sure it's possible to sign (here and in the others) important to test with fuzzing? I figure signing wouldn't really change much between invoices built from different requests?

Also on this section, could someone elaborate on what this means?

The fuzzer should have no problem making 32 bytes of the input match something that's hard-coded, as long as the hard-coded thing is a "nice" pattern.

I'm wondering why is making sure it's possible to sign (here and in the others) important to test with fuzzing? I figure signing wouldn't really change much between invoices built from different requests?

It's probably not super critical, but it will exercise message-specific serialization code and use the result to form a message digest to sign. The latter uses more generic TlvStream code for iterating through the bytes, which expects well-formed input.

Also on this section, could someone elaborate on what this means?

The fuzzer should have no problem making 32 bytes of the input match something that's hard-coded, as long as the hard-coded thing is a "nice" pattern.

I believe this pertains to how the fuzzer explores different code branches, but @TheBlueMatt can provide more thorough explanation.

Yea, the fuzzers may pass "random" input into tests, but its not actually random, instead its a defined set of mutations/"nice patterns" (all 0s, all 1, etc, etc) made to the existing set of tests, focusing on tests that had good code coverage.

It's probably not super critical, but it will exercise message-specific serialization code and use the result to form a message digest to sign. The latter uses more generic TlvStream code for iterating through the bytes, which expects well-formed input.

Ah gotcha

but its not actually random, instead its a defined set of mutations/"nice patterns" (all 0s, all 1, etc, etc) made to the existing set of tests, focusing on tests that had good code coverage.

Oh woah cool I'll have to look more into this

arik-so · 2023-02-24T17:33:15Z

lightning/src/offers/invoice_request.rs

+	/// Creates an [`Invoice`] for the request with the given required fields and using the
+	/// [`Duration`] since [`std::time::SystemTime::UNIX_EPOCH`] as the creation time.
+	///
+	/// See [`InvoiceRequest::respond_with_no_std`] for further details where the aforementioned


It's been a while since I last checked this PR, so I hope I'm not relitigating something that's already been discussed, but I am wondering:

Do we wanna hide respond_with behind std, or do we wanna modify respond_with_no_std to make its timestamp argument an Option, so respond_with would always be available, but depending on the std setting, would or would not check for expiry?

This isn't checking for expiry; it's constructing the required invoice creation time. We conditionally check that the underlying offer has not expired when calling InvoiceBuilder::build, though.

TheBlueMatt · 2023-02-25T18:18:24Z

CC Bitcoin Optech - this made the BOLT12 structures public! We can't yet send or receive blinded paths so we don't have BOLT12 support, but with this our users can parse offers/invoice_requests/invoices/etc.

TheBlueMatt reviewed Jan 21, 2023

View reviewed changes

fuzz/src/invoice_request_deser.rs Outdated Show resolved Hide resolved

fuzz/src/invoice_request_deser.rs Show resolved Hide resolved

jkczyz force-pushed the 2023-01-offers-fuzz branch from 85e31a1 to e12b0a0 Compare January 24, 2023 04:25

jkczyz force-pushed the 2023-01-offers-fuzz branch from e12b0a0 to c098b53 Compare January 31, 2023 02:47

jkczyz force-pushed the 2023-01-offers-fuzz branch from c098b53 to 83e75b6 Compare January 31, 2023 20:40

jkczyz force-pushed the 2023-01-offers-fuzz branch 2 times, most recently from 593ff88 to 09da717 Compare February 1, 2023 05:13

TheBlueMatt reviewed Feb 1, 2023

View reviewed changes

jkczyz force-pushed the 2023-01-offers-fuzz branch from 09da717 to b7501d8 Compare February 1, 2023 18:29

TheBlueMatt reviewed Feb 2, 2023

View reviewed changes

jkczyz force-pushed the 2023-01-offers-fuzz branch from b7501d8 to 0353e71 Compare February 2, 2023 23:35

TheBlueMatt added the Seeking Code Review label Feb 3, 2023

arik-so reviewed Feb 3, 2023

View reviewed changes

jkczyz added 4 commits February 3, 2023 15:14

Fix BOLT 12 invoice doctests to work with no-std

3302f25

Add test for requesting invoice from expired offer

6388c9a

Make offers module public

153d831

This is needed in order to fuzz test BOLT 12 message deserialization.

jkczyz force-pushed the 2023-01-offers-fuzz branch from 0353e71 to 7e9cc03 Compare February 3, 2023 21:24

TheBlueMatt removed the Seeking Code Review label Feb 3, 2023

TheBlueMatt assigned TheBlueMatt and arik-so Feb 3, 2023

arik-so previously approved these changes Feb 4, 2023

View reviewed changes

jkczyz force-pushed the 2023-01-offers-fuzz branch from c41ba4d to 2fd0cbb Compare February 16, 2023 02:18

Derive traits for InvoiceRequest and Invoice

9a65709

Offer and Refund derive Debug, Clone, and PartialEq. For consistency, derive these traits for InvoiceRequest and Invoice as well.

jkczyz force-pushed the 2023-01-offers-fuzz branch from 2fd0cbb to 822c8dd Compare February 21, 2023 20:18

TheBlueMatt reviewed Feb 23, 2023

View reviewed changes

jkczyz added 6 commits February 23, 2023 18:24

Fuzz test for parsing Invoice

16168d4

An invoice is serialized as a TLV stream and encoded as bytes. Add a fuzz test that parses the TLV stream and deserializes the underlying Invoice. Then compare the original bytes with those obtained by re-serializing the Invoice.

Expose Bech32Encode trait for fuzzing

56a01de

In order to fuzz test Bech32Encode parsing independent of the underlying message deserialization, the trait needs to be exposed. Conditionally expose it only for fuzzing.

Fuzz test for bech32 decoding

3d41df0

Fuzz testing bech32 decoding along with deserializing the underlying message can result in overly exhaustive searches. Instead, the message deserializations are now fuzzed separately. Add fuzzing for bech32 decoding.

Fix amount overflow in Offer parsing and building

32ed69a

An overflow can occur when multiplying the offer amount by the requested quantity when checking if the given amount is enough. Return an error instead of overflowing.

Fix amount overflow in Invoice building

9c2a3d0

An overflow can occur when multiplying the offer amount by the requested quantity when no amount is given in the request. Return an error instead of overflowing.

jkczyz force-pushed the 2023-01-offers-fuzz branch from 822c8dd to 9c2a3d0 Compare February 24, 2023 00:26

TheBlueMatt approved these changes Feb 24, 2023

View reviewed changes

alecchendev reviewed Feb 24, 2023

View reviewed changes

arik-so reviewed Feb 24, 2023

View reviewed changes

arik-so approved these changes Feb 25, 2023

View reviewed changes

TheBlueMatt merged commit f71daed into lightningdevkit:main Feb 25, 2023

TheBlueMatt added the Optech Make Me Famous label Feb 25, 2023

jkczyz mentioned this pull request May 10, 2023

BOLT12 Offers Tracking Issue #1970

Open

59 tasks

BOLT 12 deserialization fuzzers #1977

BOLT 12 deserialization fuzzers #1977

Conversation

jkczyz commented Jan 20, 2023

codecov-commenter commented Jan 20, 2023 • edited Loading

Codecov Report

TheBlueMatt commented Jan 31, 2023

jkczyz commented Jan 31, 2023

jkczyz commented Jan 31, 2023

TheBlueMatt commented Jan 31, 2023

jkczyz commented Jan 31, 2023

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 3, 2023

jkczyz commented Feb 3, 2023

TheBlueMatt commented Feb 3, 2023

arik-so left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 15, 2023

jkczyz commented Feb 15, 2023

jkczyz commented Feb 15, 2023

TheBlueMatt commented Feb 17, 2023

jkczyz commented Feb 19, 2023

TheBlueMatt commented Feb 20, 2023

TheBlueMatt commented Feb 21, 2023

jkczyz commented Feb 21, 2023

TheBlueMatt commented Feb 21, 2023

TheBlueMatt commented Feb 23, 2023

jkczyz commented Feb 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alecchendev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt commented Feb 25, 2023

codecov-commenter commented Jan 20, 2023 •

edited

Loading