From cba803b856fd9a4d9b12aa181cad68403b01365a Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Mon, 29 Apr 2019 21:21:21 -0700 Subject: [PATCH 1/9] build: update to latest lightning-onion version --- go.mod | 3 +-- go.sum | 7 +++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index 9e3c50a4..63a78f28 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,6 @@ require ( github.com/NebulousLabs/fastrand v0.0.0-20180208210444-3cf7173006a0 // indirect github.com/NebulousLabs/go-upnp v0.0.0-20180202185039-29b680b06c82 github.com/Yawning/aez v0.0.0-20180114000226-4dad034d9db2 - github.com/aead/chacha20 v0.0.0-20180709150244-8b13a72661da // indirect github.com/btcsuite/btcd v0.0.0-20190426011420-63f50db2f70a github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d @@ -29,7 +28,7 @@ require ( github.com/juju/version v0.0.0-20180108022336-b64dbd566305 // indirect github.com/kkdai/bstream v0.0.0-20181106074824-b3251f7901ec github.com/lightninglabs/neutrino v0.0.0-20190426010803-a655679fe131 - github.com/lightningnetwork/lightning-onion v0.0.0-20180605012408-ac4d9da8f1d6 + github.com/lightningnetwork/lightning-onion v0.0.0-20190430041606-751fb4dd8b72 github.com/lightningnetwork/lnd/queue v1.0.1 github.com/lightningnetwork/lnd/ticker v1.0.0 github.com/ltcsuite/ltcd v0.0.0-20190101042124-f37f8bf35796 diff --git a/go.sum b/go.sum index 13e80430..4391d91a 100644 --- a/go.sum +++ b/go.sum @@ -14,6 +14,7 @@ github.com/aead/siphash v1.0.1/go.mod h1:Nywa3cDsYNNK3gaciGTWPwHt0wlpNV15vwmswBA github.com/boltdb/bolt v1.3.1 h1:JQmyP4ZBrce+ZQu0dY660FMfatumYDLun9hBCUVIkF4= github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx27Ps= github.com/btcsuite/btcd v0.0.0-20180823030728-d81d8877b8f3/go.mod h1:Dmm/EzmjnCiweXmzRIAiUWCInVmPgjkzgv5k4tVyXiQ= +github.com/btcsuite/btcd v0.0.0-20181130015935-7d2daa5bfef2/go.mod h1:Jr9bmNVGZ7TH2Ux1QuP0ec+yGgh0gE9FIlkzQiI5bR0= github.com/btcsuite/btcd v0.0.0-20190213025234-306aecffea32 h1:qkOC5Gd33k54tobS36cXdAzJbeHaduLtnLQQwNoIi78= github.com/btcsuite/btcd v0.0.0-20190213025234-306aecffea32/go.mod h1:DrZx5ec/dmnfpw9KyYoQyYo7d0KEvTkk/5M/vbZjAr8= github.com/btcsuite/btcd v0.0.0-20190426011420-63f50db2f70a h1:wH5Nq2kt+BdVJhVyYZD54lHQpz95mog0Z7r1h638TY4= @@ -110,8 +111,8 @@ github.com/lightninglabs/neutrino v0.0.0-20190313035638-e1ad4c33fb18 h1:lxD7RgKY github.com/lightninglabs/neutrino v0.0.0-20190313035638-e1ad4c33fb18/go.mod h1:v6tz6jbuAubTrRpX8ke2KH9sJxml8KlPQTKgo9mAp1Q= github.com/lightninglabs/neutrino v0.0.0-20190426010803-a655679fe131 h1:1qKraSAbJFxd2BUHrxFEswNRav749pt4P37Ez8avbAA= github.com/lightninglabs/neutrino v0.0.0-20190426010803-a655679fe131/go.mod h1:/XWY/6/btfsknUpLPV8vvIZyhod61zYaUJiE8HxsFUs= -github.com/lightningnetwork/lightning-onion v0.0.0-20180605012408-ac4d9da8f1d6 h1:ONLGrYJVQdbtP6CE/ff1KNWZtygRGEh12RzonTiCzPs= -github.com/lightningnetwork/lightning-onion v0.0.0-20180605012408-ac4d9da8f1d6/go.mod h1:8EgEt4a/NUOVQd+3kk6n9aZCJ1Ssj96Pb6lCrci+6oc= +github.com/lightningnetwork/lightning-onion v0.0.0-20190430041606-751fb4dd8b72 h1:KgmypyQfJnEf2vhwboKCtTp4mHxIcLeXPBPWDbPuzFQ= +github.com/lightningnetwork/lightning-onion v0.0.0-20190430041606-751fb4dd8b72/go.mod h1:Sooe/CoCqa85JxqHV+IBR2HW+6t2Cv+36awSmoccswM= github.com/ltcsuite/ltcd v0.0.0-20190101042124-f37f8bf35796 h1:sjOGyegMIhvgfq5oaue6Td+hxZuf3tDC8lAPrFldqFw= github.com/ltcsuite/ltcd v0.0.0-20190101042124-f37f8bf35796/go.mod h1:3p7ZTf9V1sNPI5H8P3NkTFF4LuwMdPl2DodF60qAKqY= github.com/ltcsuite/ltcutil v0.0.0-20181217130922-17f3b04680b6/go.mod h1:8Vg/LTOO0KYa/vlHWJ6XZAevPQThGH5sufO0Hrou/lA= @@ -134,6 +135,7 @@ go.etcd.io/bbolt v1.3.0/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU= go.etcd.io/bbolt v1.3.2 h1:Z/90sZLPOeCy2PwprqkFa25PdkusRzaj9P8zm/KNyvk= go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU= golang.org/x/crypto v0.0.0-20170930174604-9419663f5a44/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= +golang.org/x/crypto v0.0.0-20190103213133-ff983b9c42bc/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/crypto v0.0.0-20190211182817-74369b46fc67 h1:ng3VDlRp5/DHpSWl02R4rM9I+8M2rhmsuLwAMmkLQWE= golang.org/x/crypto v0.0.0-20190211182817-74369b46fc67/go.mod h1:6SG95UA2DQfeDnfUPMdvaQW0Q7yPrPDi9nlGo2tz2b4= golang.org/x/exp v0.0.0-20190121172915-509febef88a4/go.mod h1:CJ0aWSM057203Lf6IL+f9T1iT9GByDxfZKAQTCR3kQA= @@ -156,6 +158,7 @@ golang.org/x/sync v0.0.0-20181221193216-37e7f081c4d4/go.mod h1:RxMgew5VJxzue5/jJ golang.org/x/sys v0.0.0-20180821140842-3b58ed4ad339/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20180909124046-d0be0721c37e/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= +golang.org/x/sys v0.0.0-20190102155601-82a175fd1598/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/sys v0.0.0-20190209173611-3b5209105503 h1:5SvYFrOM3W8Mexn9/oA44Ji7vhXAZQ9hiP+1Q/DMrWg= golang.org/x/sys v0.0.0-20190209173611-3b5209105503/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg= From bab957382fc6a1c351340d79cc7cf5fb0ef430ab Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Thu, 10 Jan 2019 20:03:07 -0800 Subject: [PATCH 2/9] router: convert Route.ToHopPayloads() to Route.ToSphinxPath() In this commit, we update the process that we use to generate a sphinx packet to send our onion routed HTLC. Due to recent changes in the `sphinx` package we use, we now need to use a new PaymentPath struct. As a result, it no longer makes sense to split up the nodes in a route and their per hop paylods as they're now in the same struct. All tests have been updated accordingly. --- routing/pathfind_test.go | 17 +++++---- routing/route/route.go | 80 +++++++++++++++++++++++----------------- routing/router.go | 33 +++++++---------- 3 files changed, 70 insertions(+), 60 deletions(-) diff --git a/routing/pathfind_test.go b/routing/pathfind_test.go index 3865dc2b..3a8e1868 100644 --- a/routing/pathfind_test.go +++ b/routing/pathfind_test.go @@ -807,19 +807,22 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc // Next, we'll assert that the "next hop" field in each route payload // properly points to the channel ID that the HTLC should be forwarded // along. - hopPayloads := route.ToHopPayloads() - if len(hopPayloads) != expectedHopCount { + sphinxPath, err := route.ToSphinxPath() + if err != nil { + t.Fatalf("unable to make sphinx path: %v", err) + } + if sphinxPath.TrueRouteLength() != expectedHopCount { t.Fatalf("incorrect number of hop payloads: expected %v, got %v", - expectedHopCount, len(hopPayloads)) + expectedHopCount, sphinxPath.TrueRouteLength()) } // Hops should point to the next hop for i := 0; i < len(expectedHops)-1; i++ { var expectedHop [8]byte binary.BigEndian.PutUint64(expectedHop[:], route.Hops[i+1].ChannelID) - if !bytes.Equal(hopPayloads[i].NextAddress[:], expectedHop[:]) { + if !bytes.Equal(sphinxPath[i].HopData.NextAddress[:], expectedHop[:]) { t.Fatalf("first hop has incorrect next hop: expected %x, got %x", - expectedHop[:], hopPayloads[i].NextAddress) + expectedHop[:], sphinxPath[i].HopData.NextAddress) } } @@ -827,9 +830,9 @@ func testBasicGraphPathFindingCase(t *testing.T, graphInstance *testGraphInstanc // to indicate it's the exit hop. var exitHop [8]byte lastHopIndex := len(expectedHops) - 1 - if !bytes.Equal(hopPayloads[lastHopIndex].NextAddress[:], exitHop[:]) { + if !bytes.Equal(sphinxPath[lastHopIndex].HopData.NextAddress[:], exitHop[:]) { t.Fatalf("first hop has incorrect next hop: expected %x, got %x", - exitHop[:], hopPayloads[lastHopIndex].NextAddress) + exitHop[:], sphinxPath[lastHopIndex].HopData.NextAddress) } var expectedTotalFee lnwire.MilliSatoshi diff --git a/routing/route/route.go b/routing/route/route.go index fb0a046d..6aaa4e49 100644 --- a/routing/route/route.go +++ b/routing/route/route.go @@ -104,40 +104,6 @@ func (r *Route) HopFee(hopIndex int) lnwire.MilliSatoshi { return incomingAmt - r.Hops[hopIndex].AmtToForward } -// ToHopPayloads converts a complete route into the series of per-hop payloads -// that is to be encoded within each HTLC using an opaque Sphinx packet. -func (r *Route) ToHopPayloads() []sphinx.HopData { - hopPayloads := make([]sphinx.HopData, len(r.Hops)) - - // For each hop encoded within the route, we'll convert the hop struct - // to the matching per-hop payload struct as used by the sphinx - // package. - for i, hop := range r.Hops { - hopPayloads[i] = sphinx.HopData{ - // TODO(roasbeef): properly set realm, make sphinx type - // an enum actually? - Realm: 0, - ForwardAmount: uint64(hop.AmtToForward), - OutgoingCltv: hop.OutgoingTimeLock, - } - - // As a base case, the next hop is set to all zeroes in order - // to indicate that the "last hop" as no further hops after it. - nextHop := uint64(0) - - // If we aren't on the last hop, then we set the "next address" - // field to be the channel that directly follows it. - if i != len(r.Hops)-1 { - nextHop = r.Hops[i+1].ChannelID - } - - binary.BigEndian.PutUint64(hopPayloads[i].NextAddress[:], - nextHop) - } - - return hopPayloads -} - // NewRouteFromHops creates a new Route structure from the minimally required // information to perform the payment. It infers fee amounts and populates the // node, chan and prev/next hop maps. @@ -163,3 +129,49 @@ func NewRouteFromHops(amtToSend lnwire.MilliSatoshi, timeLock uint32, return route, nil } + +// ToSphinxPath converts a complete route into a sphinx PaymentPath that +// contains the per-hop paylods used to encoding the HTLC routing data for each +// hop in the route. +func (r *Route) ToSphinxPath() (*sphinx.PaymentPath, error) { + var path sphinx.PaymentPath + + // For each hop encoded within the route, we'll convert the hop struct + // to an OnionHop with matching per-hop payload within the path as used + // by the sphinx package. + for i, hop := range r.Hops { + pub, err := btcec.ParsePubKey( + hop.PubKeyBytes[:], btcec.S256(), + ) + if err != nil { + return nil, err + } + + path[i] = sphinx.OnionHop{ + NodePub: *pub, + HopData: sphinx.HopData{ + // TODO(roasbeef): properly set realm, make + // sphinx type an enum actually? + Realm: [1]byte{0}, + ForwardAmount: uint64(hop.AmtToForward), + OutgoingCltv: hop.OutgoingTimeLock, + }, + } + + // As a base case, the next hop is set to all zeroes in order + // to indicate that the "last hop" as no further hops after it. + nextHop := uint64(0) + + // If we aren't on the last hop, then we set the "next address" + // field to be the channel that directly follows it. + if i != len(r.Hops)-1 { + nextHop = r.Hops[i+1].ChannelID + } + + binary.BigEndian.PutUint64( + path[i].HopData.NextAddress[:], nextHop, + ) + } + + return &path, nil +} diff --git a/routing/router.go b/routing/router.go index 3afb8c5e..de393212 100644 --- a/routing/router.go +++ b/routing/router.go @@ -1467,30 +1467,25 @@ func generateSphinxPacket(rt *route.Route, paymentHash []byte) ([]byte, return nil, nil, route.ErrNoRouteHopsProvided } - // First obtain all the public keys along the route which are contained - // in each hop. - nodes := make([]*btcec.PublicKey, len(rt.Hops)) - for i, hop := range rt.Hops { - pub, err := btcec.ParsePubKey(hop.PubKeyBytes[:], - btcec.S256()) - if err != nil { - return nil, nil, err - } - - nodes[i] = pub + // Now that we know we have an actual route, we'll map the route into a + // sphinx payument path which includes per-hop paylods for each hop + // that give each node within the route the necessary information + // (fees, CLTV value, etc) to properly forward the payment. + sphinxPath, err := rt.ToSphinxPath() + if err != nil { + return nil, nil, err } - // Next we generate the per-hop payload which gives each node within - // the route the necessary information (fees, CLTV value, etc) to - // properly forward the payment. - hopPayloads := rt.ToHopPayloads() - log.Tracef("Constructed per-hop payloads for payment_hash=%x: %v", paymentHash[:], newLogClosure(func() string { - return spew.Sdump(hopPayloads) + return spew.Sdump(sphinxPath[:sphinxPath.TrueRouteLength()]) }), ) + // Generate a new random session key to ensure that we don't trigger + // any replay. + // + // TODO(roasbeef): add more sources of randomness? sessionKey, err := btcec.NewPrivateKey(btcec.S256()) if err != nil { return nil, nil, err @@ -1499,7 +1494,7 @@ func generateSphinxPacket(rt *route.Route, paymentHash []byte) ([]byte, // Next generate the onion routing packet which allows us to perform // privacy preserving source routing across the network. sphinxPacket, err := sphinx.NewOnionPacket( - nodes, sessionKey, hopPayloads, paymentHash, + sphinxPath, sessionKey, paymentHash, ) if err != nil { return nil, nil, err @@ -1523,7 +1518,7 @@ func generateSphinxPacket(rt *route.Route, paymentHash []byte) ([]byte, return onionBlob.Bytes(), &sphinx.Circuit{ SessionKey: sessionKey, - PaymentPath: nodes, + PaymentPath: sphinxPath.NodeKeys(), }, nil } From 558c8ca2a858ae98ae0af98b8fcf1e1a14c22ffa Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 30 Apr 2019 18:12:31 -0700 Subject: [PATCH 3/9] lnwire: export failureMessageLength constant --- lnwire/onion_error.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/lnwire/onion_error.go b/lnwire/onion_error.go index dbc20777..7e04cc95 100644 --- a/lnwire/onion_error.go +++ b/lnwire/onion_error.go @@ -26,9 +26,9 @@ type FailureMessage interface { Error() string } -// failureMessageLength is the size of the failure message plus the size of +// FailureMessageLength is the size of the failure message plus the size of // padding. The FailureMessage message should always be EXACTLY this size. -const failureMessageLength = 256 +const FailureMessageLength = 256 const ( // FlagBadOnion error flag describes an unparsable, encrypted by @@ -1101,7 +1101,7 @@ func DecodeFailure(r io.Reader, pver uint32) (FailureMessage, error) { if err := ReadElement(r, &failureLength); err != nil { return nil, fmt.Errorf("unable to read error len: %v", err) } - if failureLength > failureMessageLength { + if failureLength > FailureMessageLength { return nil, fmt.Errorf("failure message is too "+ "long: %v", failureLength) } @@ -1170,14 +1170,14 @@ func EncodeFailure(w io.Writer, failure FailureMessage, pver uint32) error { // The combined size of this message must be below the max allowed // failure message length. failureMessage := failureMessageBuffer.Bytes() - if len(failureMessage) > failureMessageLength { + if len(failureMessage) > FailureMessageLength { return fmt.Errorf("failure message exceed max "+ "available size: %v", len(failureMessage)) } // Finally, we'll add some padding in order to ensure that all failure // messages are fixed size. - pad := make([]byte, failureMessageLength-len(failureMessage)) + pad := make([]byte, FailureMessageLength-len(failureMessage)) return WriteElements(w, uint16(len(failureMessage)), From 649e080da6e433801eb2a8c04cc20c3c5a523dad Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 30 Apr 2019 18:20:23 -0700 Subject: [PATCH 4/9] htlcswitch: for UpdateFailMalformedHTLC packets mark fail as needing conversion In this commit, we start the first phase of fixing an existing bug within the switch. As is, we don't properly convert `UpdateFailMalformedHTLC` to regular `UpdateFailHTLC` messages that are fully encrypted. When we receive a `UpdateFailMalformedHTLC` today, we'll convert it into a regular fail message by simply encoding the failure message raw. This failure message doesn't have a MAC yet, so if we sent it backwards, then the destination wouldn't be able to decrypt it. We can recognize this type of failure as it'll be the same size as the raw failure message max size, but it has 4 extra bytes for the encoding. When we come across this message, we'll mark is as needing conversion so the switch can take care of it. --- htlcswitch/link.go | 16 +++++++++++++++- htlcswitch/packet.go | 8 ++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/htlcswitch/link.go b/htlcswitch/link.go index 79a37f00..f84c4b63 100644 --- a/htlcswitch/link.go +++ b/htlcswitch/link.go @@ -2369,10 +2369,24 @@ func (l *channelLink) processRemoteSettleFails(fwdPkg *channeldb.FwdPkg, outgoingHTLCID: pd.ParentIndex, destRef: pd.DestRef, htlc: &lnwire.UpdateFailHTLC{ - Reason: lnwire.OpaqueReason(pd.FailReason), + Reason: lnwire.OpaqueReason( + pd.FailReason, + ), }, } + // If the failure message lacks an HMAC (but includes + // the 4 bytes for encoding the message and padding + // lengths, then this means that we received it as an + // UpdateFailMalformedHTLC. As a result, we'll signal + // that we need to convert this error within the switch + // to an actual error, by encrypting it as if we were + // the originating hop. + convertedErrorSize := lnwire.FailureMessageLength + 4 + if len(pd.FailReason) == convertedErrorSize { + failPacket.convertedError = true + } + // Add the packet to the batch to be forwarded, and // notify the overflow queue that a spare spot has been // freed up within the commitment state. diff --git a/htlcswitch/packet.go b/htlcswitch/packet.go index 41e364a9..363a71dd 100644 --- a/htlcswitch/packet.go +++ b/htlcswitch/packet.go @@ -61,6 +61,14 @@ type htlcPacket struct { // encrypted with any shared secret. localFailure bool + // convertedError is set to true if this is an HTLC fail that was + // created using an UpdateFailMalformedHTLC from the remote party. If + // this is true, then when forwarding this failure packet, we'll need + // to wrap it as if we were the first hop if it's a multi-hop HTLC. If + // it's a direct HTLC, then we'll decode the error as no encryption has + // taken place. + convertedError bool + // hasSource is set to true if the incomingChanID and incomingHTLCID // fields of a forwarded fail packet are already set and do not need to // be looked up in the circuit map. From c67ca0a3292d391834cb66d8b0d3c679958e9202 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 30 Apr 2019 18:22:10 -0700 Subject: [PATCH 5/9] htlcswitch: add new EncryptMalformedError method to ErrorEncrypter In this commit, we add a new method to the ErrorEncrypter interface: `EncryptMalformedError`. This takes a raw error (no encryption or MAC), and encrypts it as if we were the originator of this error. This will be used by the switch to convert malformed fail errors to regular fully encrypted errors. --- htlcswitch/failure.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/htlcswitch/failure.go b/htlcswitch/failure.go index d9edb145..a17517bb 100644 --- a/htlcswitch/failure.go +++ b/htlcswitch/failure.go @@ -87,6 +87,13 @@ type ErrorEncrypter interface { // slightly, in that it computes a proper MAC over the error. EncryptFirstHop(lnwire.FailureMessage) (lnwire.OpaqueReason, error) + // EncryptMalformedError is similar to EncryptFirstHop (it adds the + // MAC), but it accepts an opaque failure reason rather than a failure + // message. This method is used when we receive an + // UpdateFailMalformedHTLC from the remote peer and then need to + // convert that into a proper error from only the raw bytes. + EncryptMalformedError(lnwire.OpaqueReason) lnwire.OpaqueReason + // IntermediateEncrypt wraps an already encrypted opaque reason error // in an additional layer of onion encryption. This process repeats // until the error arrives at the source of the payment. @@ -153,6 +160,17 @@ func (s *SphinxErrorEncrypter) EncryptFirstHop(failure lnwire.FailureMessage) (l return s.EncryptError(true, b.Bytes()), nil } +// EncryptMalformedError is similar to EncryptFirstHop (it adds the MAC), but +// it accepts an opaque failure reason rather than a failure message. This +// method is used when we receive an UpdateFailMalformedHTLC from the remote +// peer and then need to convert that into an proper error from only the raw +// bytes. +// +// NOTE: Part of the ErrorEncrypter interface. +func (s *SphinxErrorEncrypter) EncryptMalformedError(reason lnwire.OpaqueReason) lnwire.OpaqueReason { + return s.EncryptError(true, reason) +} + // IntermediateEncrypt wraps an already encrypted opaque reason error in an // additional layer of onion encryption. This process repeats until the error // arrives at the source of the payment. We re-encrypt the message on the From cdc4aca40f4be75a3ed7a6b37e5c1c84b1b85ecf Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 30 Apr 2019 18:24:52 -0700 Subject: [PATCH 6/9] htlcswitch: properly handle direct link malformed HTLC failures In this commit, we fix a bug that caused us to be unable to properly handle malformed HTLC failures from our direct link. Before this commit, we would attempt to decrypt it and fail since it wasn't well formed. In this commit, if its an error for a local payment, and it needed to be converted, then we'll decode it w/o decrypting since it's already plaintext. --- htlcswitch/switch.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index f48cbdc9..3e1ad107 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -939,7 +939,7 @@ func (s *Switch) parseFailedPayment(payment *pendingPayment, pkt *htlcPacket, // The payment never cleared the link, so we don't need to // decrypt the error, simply decode it them report back to the // user. - case pkt.localFailure: + case pkt.localFailure || pkt.convertedError: var userErr string r := bytes.NewReader(htlc.Reason) failureMsg, err := lnwire.DecodeFailure(r, 0) From be63c7d286bdcbbc386528c13eeb1460e94bfc1b Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 30 Apr 2019 18:27:11 -0700 Subject: [PATCH 7/9] htlcswitch: properly convert multi-hop malformed HTLC failures In this commit, we now properly convert multi-hop malformed HTLC failures. Before this commit, we wouldn't properly add a layer of encryption to these errors meaning that the destination would fail to decrypt the error as it was actually plaintext. To remedy this, we'll now check if we need to convert an error, and if so we'll encrypt it as if it we were the source of the error (the true source is our direct channel peer). --- htlcswitch/switch.go | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/htlcswitch/switch.go b/htlcswitch/switch.go index 3e1ad107..988c33dd 100644 --- a/htlcswitch/switch.go +++ b/htlcswitch/switch.go @@ -1155,13 +1155,12 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { fail, isFail := htlc.(*lnwire.UpdateFailHTLC) if isFail && !packet.hasSource { switch { + // No message to encrypt, locally sourced payment. case circuit.ErrorEncrypter == nil: - // No message to encrypt, locally sourced - // payment. + // If this is a resolution message, then we'll need to + // encrypt it as it's actually internally sourced. case packet.isResolution: - // If this is a resolution message, then we'll need to encrypt - // it as it's actually internally sourced. var err error // TODO(roasbeef): don't need to pass actually? failure := &lnwire.FailPermanentChannelFailure{} @@ -1174,6 +1173,25 @@ func (s *Switch) handlePacketForward(packet *htlcPacket) error { log.Error(err) } + // Alternatively, if the remote party send us an + // UpdateFailMalformedHTLC, then we'll need to convert + // this into a proper well formatted onion error as + // there's no HMAC currently. + case packet.convertedError: + log.Infof("Converting malformed HTLC error "+ + "for circuit for Circuit(%x: "+ + "(%s, %d) <-> (%s, %d))", packet.circuit.PaymentHash, + packet.incomingChanID, packet.incomingHTLCID, + packet.outgoingChanID, packet.outgoingHTLCID) + + fail.Reason = circuit.ErrorEncrypter.EncryptMalformedError( + fail.Reason, + ) + if err != nil { + err = fmt.Errorf("unable to obfuscate "+ + "error: %v", err) + log.Error(err) + } default: // Otherwise, it's a forwarded error, so we'll perform a // wrapper encryption as normal. From 56c969c9119dfe74c21bf0b7d592531c206cf514 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 30 Apr 2019 18:28:39 -0700 Subject: [PATCH 8/9] htlcswitch: add new TestUpdateFailMalformedHTLCErrorConversion test In this commit, we add a new test to ensure that we're able to properly convert malformed HTLC errors that are sourced from multiple hops away, or our direct channel peers. In order to test this effectively, we force the onion decryptors of various peers to always fail which will trigger the malformed HTLC logic. --- htlcswitch/mock.go | 9 +++++ htlcswitch/switch_test.go | 73 +++++++++++++++++++++++++++++++++++++++ htlcswitch/test_utils.go | 22 +++++++----- 3 files changed, 96 insertions(+), 8 deletions(-) diff --git a/htlcswitch/mock.go b/htlcswitch/mock.go index a4d8f1bb..f1d7dc47 100644 --- a/htlcswitch/mock.go +++ b/htlcswitch/mock.go @@ -368,7 +368,10 @@ func (o *mockObfuscator) EncryptFirstHop(failure lnwire.FailureMessage) ( func (o *mockObfuscator) IntermediateEncrypt(reason lnwire.OpaqueReason) lnwire.OpaqueReason { return reason +} +func (o *mockObfuscator) EncryptMalformedError(reason lnwire.OpaqueReason) lnwire.OpaqueReason { + return reason } // mockDeobfuscator mock implementation of the failure deobfuscator which @@ -400,6 +403,8 @@ type mockIteratorDecoder struct { mu sync.RWMutex responses map[[32]byte][]DecodeHopIteratorResponse + + decodeFail bool } func newMockIteratorDecoder() *mockIteratorDecoder { @@ -451,6 +456,10 @@ func (p *mockIteratorDecoder) DecodeHopIterators(id []byte, req.OnionReader, req.RHash, req.IncomingCltv, ) + if p.decodeFail { + failcode = lnwire.CodeTemporaryChannelFailure + } + resp := DecodeHopIteratorResponse{ HopIterator: iterator, FailCode: failcode, diff --git a/htlcswitch/switch_test.go b/htlcswitch/switch_test.go index 59b0e4ed..0aa435b0 100644 --- a/htlcswitch/switch_test.go +++ b/htlcswitch/switch_test.go @@ -2033,3 +2033,76 @@ func TestMultiHopPaymentForwardingEvents(t *testing.T) { } } } + +// TestUpdateFailMalformedHTLCErrorConversion tests that we're able to properly +// convert malformed HTLC errors that originate at the direct link, as well as +// during multi-hop HTLC forwarding. +func TestUpdateFailMalformedHTLCErrorConversion(t *testing.T) { + t.Parallel() + + // First, we'll create our traditional three hop network. + channels, cleanUp, _, err := createClusterChannels( + btcutil.SatoshiPerBitcoin*3, btcutil.SatoshiPerBitcoin*5, + ) + if err != nil { + t.Fatalf("unable to create channel: %v", err) + } + defer cleanUp() + + n := newThreeHopNetwork( + t, channels.aliceToBob, channels.bobToAlice, + channels.bobToCarol, channels.carolToBob, testStartingHeight, + ) + if err := n.start(); err != nil { + t.Fatalf("unable to start three hop network: %v", err) + } + + assertPaymentFailure := func(t *testing.T) { + // With the decoder modified, we'll now attempt to send a + // payment from Alice to carol. + finalAmt := lnwire.NewMSatFromSatoshis(100000) + htlcAmt, totalTimelock, hops := generateHops( + finalAmt, testStartingHeight, n.firstBobChannelLink, + n.carolChannelLink, + ) + firstHop := n.firstBobChannelLink.ShortChanID() + _, err = makePayment( + n.aliceServer, n.carolServer, firstHop, hops, finalAmt, + htlcAmt, totalTimelock, + ).Wait(30 * time.Second) + + // The payment should fail as Carol is unable to decode the + // onion blob sent to her. + if err == nil { + t.Fatalf("unable to send payment: %v", err) + } + + fwdingErr := err.(*ForwardingError) + failureMsg := fwdingErr.FailureMessage + if _, ok := failureMsg.(*lnwire.FailTemporaryChannelFailure); !ok { + t.Fatalf("expected temp chan failure instead got: %v", + fwdingErr.FailureMessage) + } + } + + t.Run("multi-hop error conversion", func(t *testing.T) { + // Now that we have our network up, we'll modify the hop + // iterator for the Bob <-> Carol channel to fail to decode in + // order to simulate either a replay attack or an issue + // decoding the onion. + n.carolOnionDecoder.decodeFail = true + + assertPaymentFailure(t) + }) + + t.Run("direct channel error conversion", func(t *testing.T) { + // Similar to the above test case, we'll now make the Alice <-> + // Bob link always fail to decode an onion. This differs from + // the above test case in that there's no encryption on the + // error at all since Alice will directly receive a + // UpdateFailMalformedHTLC message. + n.bobOnionDecoder.decodeFail = true + + assertPaymentFailure(t) + }) +} diff --git a/htlcswitch/test_utils.go b/htlcswitch/test_utils.go index 89762d75..2ebc1e38 100644 --- a/htlcswitch/test_utils.go +++ b/htlcswitch/test_utils.go @@ -589,15 +589,18 @@ func generateRoute(hops ...ForwardingInfo) ([lnwire.OnionPacketSize]byte, error) // threeHopNetwork is used for managing the created cluster of 3 hops. type threeHopNetwork struct { - aliceServer *mockServer - aliceChannelLink *channelLink + aliceServer *mockServer + aliceChannelLink *channelLink + aliceOnionDecoder *mockIteratorDecoder bobServer *mockServer firstBobChannelLink *channelLink secondBobChannelLink *channelLink + bobOnionDecoder *mockIteratorDecoder - carolServer *mockServer - carolChannelLink *channelLink + carolServer *mockServer + carolChannelLink *channelLink + carolOnionDecoder *mockIteratorDecoder hopNetwork } @@ -948,15 +951,18 @@ func newThreeHopNetwork(t testing.TB, aliceChannel, firstBobChannel, } return &threeHopNetwork{ - aliceServer: aliceServer, - aliceChannelLink: aliceChannelLink.(*channelLink), + aliceServer: aliceServer, + aliceChannelLink: aliceChannelLink.(*channelLink), + aliceOnionDecoder: aliceDecoder, bobServer: bobServer, firstBobChannelLink: firstBobChannelLink.(*channelLink), secondBobChannelLink: secondBobChannelLink.(*channelLink), + bobOnionDecoder: bobDecoder, - carolServer: carolServer, - carolChannelLink: carolChannelLink.(*channelLink), + carolServer: carolServer, + carolChannelLink: carolChannelLink.(*channelLink), + carolOnionDecoder: carolDecoder, hopNetwork: *hopNetwork, } From 2f2a907b58807c2611f2ad6c8fbf127ebe2138c8 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 30 Apr 2019 18:30:24 -0700 Subject: [PATCH 9/9] test: update itest to no longer expect incorrect message for sphinx replay test In this commit, we update the itests to expect the correct message for the sphinx replay test. Before the fixes in the prior commits, we expected the wrong error since we were actually unable to decrypt these converted malformed HTLC errors. Now, we'll properly return a parse able error, so we assert against that error instead of the failure to decode an error. --- lnd_test.go | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lnd_test.go b/lnd_test.go index 51fe50ed..91f2584c 100644 --- a/lnd_test.go +++ b/lnd_test.go @@ -3522,13 +3522,10 @@ func testSphinxReplayPersistence(net *lntest.NetworkHarness, t *harnessTest) { // Construct the response we expect after sending a duplicate packet // that fails due to sphinx replay detection. - replayErr := fmt.Sprintf("unable to route payment to destination: "+ - "TemporaryChannelFailure: unable to de-obfuscate onion failure, "+ - "htlc with hash(%x): unable to retrieve onion failure", - invoiceResp.RHash) - - if resp.PaymentError != replayErr { - t.Fatalf("received payment error: %v", resp.PaymentError) + replayErr := "TemporaryChannelFailure" + if !strings.Contains(resp.PaymentError, replayErr) { + t.Fatalf("received payment error: %v, expected %v", + resp.PaymentError, replayErr) } // Since the payment failed, the balance should still be left