From f8d201a605e697f73d2e03234555b817b58d6027 Mon Sep 17 00:00:00 2001 From: Conner Fromknecht Date: Tue, 13 Apr 2021 11:47:25 -0700 Subject: [PATCH] channeldb/invoice: only allow pay-addr lookup w/ no query hash Without this check, it's possible to send a probe HTLC w/ a valid payment address but invalid payment hash that gets settled. Because there was no assertion that the HTLC's payment hash matches the invoice, the link would fail when it receives an invalid preimage for the HTLC on its commitment. --- channeldb/invoice_test.go | 30 ++++++++++++++++++++++++++++++ channeldb/invoices.go | 9 ++++++++- 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/channeldb/invoice_test.go b/channeldb/invoice_test.go index 61806d39..8f0f2848 100644 --- a/channeldb/invoice_test.go +++ b/channeldb/invoice_test.go @@ -365,6 +365,36 @@ func TestAddDuplicateKeysendPayAddr(t *testing.T) { require.Equal(t, invoice2, &dbInv2) } +// TestFailInvoiceLookupMPPPayAddrOnly asserts that looking up a MPP invoice +// that matches _only_ by payment address fails with ErrInvoiceNotFound. This +// ensures that the HTLC's payment hash always matches the payment hash in the +// returned invoice. +func TestFailInvoiceLookupMPPPayAddrOnly(t *testing.T) { + db, cleanUp, err := MakeTestDB() + defer cleanUp() + require.NoError(t, err) + + // Create and insert a random invoice. + invoice, err := randInvoice(1000) + require.NoError(t, err) + + payHash := invoice.Terms.PaymentPreimage.Hash() + payAddr := invoice.Terms.PaymentAddr + _, err = db.AddInvoice(invoice, payHash) + require.NoError(t, err) + + // Modify the queried payment hash to be invalid. + payHash[0] ^= 0x01 + + // Lookup the invoice by (invalid) payment hash and payment address. The + // lookup should fail since we require the payment hash to match for + // legacy/MPP invoices, as this guarantees that the preimage is valid + // for the given HTLC. + ref := InvoiceRefByHashAndAddr(payHash, payAddr) + _, err = db.LookupInvoice(ref) + require.Equal(t, ErrInvoiceNotFound, err) +} + // TestInvRefEquivocation asserts that retrieving or updating an invoice using // an equivocating InvoiceRef results in ErrInvRefEquivocation. func TestInvRefEquivocation(t *testing.T) { diff --git a/channeldb/invoices.go b/channeldb/invoices.go index fbec1a39..24d80b3e 100644 --- a/channeldb/invoices.go +++ b/channeldb/invoices.go @@ -991,7 +991,14 @@ func fetchInvoiceNumByRef(invoiceIndex, payAddrIndex, setIDIndex kvdb.RBucket, return invoiceNumByAddr, nil // Return invoices by payment addr only. - case invoiceNumByAddr != nil: + // + // NOTE: We constrain this lookup to only apply if the invoice ref does + // not contain a payment hash. Legacy and MPP payments depend on the + // payment hash index to enforce that the HTLCs payment hash matches the + // payment hash for the invoice, without this check we would + // inadvertently assume the invoice contains the correct preimage for + // the HTLC, which we only enforce via the lookup by the invoice index. + case invoiceNumByAddr != nil && payHash == nil: return invoiceNumByAddr, nil // If we were only able to reference the invoice by hash, return the