Merge pull request #3470 from cfromknecht/invalid-onion-payload

htlcswitch+lnwire: invalid onion payload

htlcswitch/hop/payload.go

@@ -5,21 +5,56 @@ import (
 	"fmt"
 	"io"
 
-	"github.com/lightningnetwork/lightning-onion"
+	sphinx "github.com/lightningnetwork/lightning-onion"
 	"github.com/lightningnetwork/lnd/lnwire"
 	"github.com/lightningnetwork/lnd/record"
 	"github.com/lightningnetwork/lnd/tlv"
 )
 
+// PayloadViolation is an enum encapsulating the possible invalid payload
+// violations that can occur when processing or validating a payload.
+type PayloadViolation byte
+
+const (
+	// OmittedViolation indicates that a type was expected to be found the
+	// payload but was absent.
+	OmittedViolation PayloadViolation = iota
+
+	// IncludedViolation indicates that a type was expected to be omitted
+	// from the payload but was present.
+	IncludedViolation
+
+	// RequiredViolation indicates that an unknown even type was found in
+	// the payload that we could not process.
+	RequiredViolation
+)
+
+// String returns a human-readable description of the violation as a verb.
+func (v PayloadViolation) String() string {
+	switch v {
+	case OmittedViolation:
+		return "omitted"
+
+	case IncludedViolation:
+		return "included"
+
+	case RequiredViolation:
+		return "required"
+
+	default:
+		return "unknown violation"
+	}
+}
+
 // ErrInvalidPayload is an error returned when a parsed onion payload either
 // included or omitted incorrect records for a particular hop type.
 type ErrInvalidPayload struct {
 	// Type the record's type that cause the violation.
 	Type tlv.Type
 
-	// Ommitted if true, signals that the sender did not include the record.
-	// Otherwise, the sender included the record when it shouldn't have.
-	Omitted bool
+	// Violation is an enum indicating the type of violation detected in
+	// processing Type.
+	Violation PayloadViolation
 
 	// FinalHop if true, indicates that the violation is for the final hop
 	// in the route (identified by next hop id), otherwise the violation is
@@ -33,13 +68,9 @@ func (e ErrInvalidPayload) Error() string {
 	if e.FinalHop {
 		hopType = "final"
 	}
-	violation := "included"
-	if e.Omitted {
-		violation = "omitted"
-	}
 
-	return fmt.Sprintf("onion payload for %s hop %s record with type %d",
-		hopType, violation, e.Type)
+	return fmt.Sprintf("onion payload for %s hop %v record with type %d",
+		hopType, e.Violation, e.Type)
 }
 
 // Payload encapsulates all information delivered to a hop in an onion payload.
@@ -87,13 +118,34 @@ func NewPayloadFromReader(r io.Reader) (*Payload, error) {
 
 	parsedTypes, err := tlvStream.DecodeWithParsedTypes(r)
 	if err != nil {
+		// Promote any required type failures into ErrInvalidPayload.
+		if e, required := err.(tlv.ErrUnknownRequiredType); required {
+			// If the parser returned an unknown required type
+			// failure, we'll first check that the payload is
+			// properly formed according to our known set of
+			// constraints. If an error is discovered, this
+			// overrides the required type failure.
+			nextHop := lnwire.NewShortChanIDFromInt(cid)
+			err = ValidateParsedPayloadTypes(parsedTypes, nextHop)
+			if err != nil {
+				return nil, err
+			}
+
+			// Otherwise the known constraints were applied
+			// successfully, report the invalid type failure
+			// returned by the parser.
+			return nil, ErrInvalidPayload{
+				Type:      tlv.Type(e),
+				Violation: RequiredViolation,
+				FinalHop:  nextHop == Exit,
+			}
+		}
 		return nil, err
 	}
 
-	nextHop := lnwire.NewShortChanIDFromInt(cid)
-
 	// Validate whether the sender properly included or omitted tlv records
 	// in accordance with BOLT 04.
+	nextHop := lnwire.NewShortChanIDFromInt(cid)
 	err = ValidateParsedPayloadTypes(parsedTypes, nextHop)
 	if err != nil {
 		return nil, err
@@ -133,17 +185,17 @@ func ValidateParsedPayloadTypes(parsedTypes tlv.TypeSet,
 	// All hops must include an amount to forward.
 	case !hasAmt:
 		return ErrInvalidPayload{
-			Type:     record.AmtOnionType,
-			Omitted:  true,
-			FinalHop: isFinalHop,
+			Type:      record.AmtOnionType,
+			Violation: OmittedViolation,
+			FinalHop:  isFinalHop,
 		}
 
 	// All hops must include a cltv expiry.
 	case !hasLockTime:
 		return ErrInvalidPayload{
-			Type:     record.LockTimeOnionType,
-			Omitted:  true,
-			FinalHop: isFinalHop,
+			Type:      record.LockTimeOnionType,
+			Violation: OmittedViolation,
+			FinalHop:  isFinalHop,
 		}
 
 	// The exit hop should omit the next hop id. If nextHop != Exit, the
@@ -151,9 +203,9 @@ func ValidateParsedPayloadTypes(parsedTypes tlv.TypeSet,
 	// inclusion at intermediate hops directly.
 	case isFinalHop && hasNextHop:
 		return ErrInvalidPayload{
-			Type:     record.NextHopOnionType,
-			Omitted:  false,
-			FinalHop: true,
+			Type:      record.NextHopOnionType,
+			Violation: IncludedViolation,
+			FinalHop:  true,
 		}
 	}
 

htlcswitch/hop/payload_test.go

@@ -16,13 +16,23 @@ type decodePayloadTest struct {
 }
 
 var decodePayloadTests = []decodePayloadTest{
+	{
+		name:    "final hop valid",
+		payload: []byte{0x02, 0x00, 0x04, 0x00},
+	},
+	{
+		name: "intermediate hop valid",
+		payload: []byte{0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x01, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		},
+	},
 	{
 		name:    "final hop no amount",
 		payload: []byte{0x04, 0x00},
 		expErr: hop.ErrInvalidPayload{
-			Type:     record.AmtOnionType,
-			Omitted:  true,
-			FinalHop: true,
+			Type:      record.AmtOnionType,
+			Violation: hop.OmittedViolation,
+			FinalHop:  true,
 		},
 	},
 	{
@@ -31,18 +41,18 @@ var decodePayloadTests = []decodePayloadTest{
 			0x00, 0x00, 0x00, 0x00,
 		},
 		expErr: hop.ErrInvalidPayload{
-			Type:     record.AmtOnionType,
-			Omitted:  true,
-			FinalHop: false,
+			Type:      record.AmtOnionType,
+			Violation: hop.OmittedViolation,
+			FinalHop:  false,
 		},
 	},
 	{
 		name:    "final hop no expiry",
 		payload: []byte{0x02, 0x00},
 		expErr: hop.ErrInvalidPayload{
-			Type:     record.LockTimeOnionType,
-			Omitted:  true,
-			FinalHop: true,
+			Type:      record.LockTimeOnionType,
+			Violation: hop.OmittedViolation,
+			FinalHop:  true,
 		},
 	},
 	{
@@ -51,9 +61,9 @@ var decodePayloadTests = []decodePayloadTest{
 			0x00, 0x00, 0x00, 0x00,
 		},
 		expErr: hop.ErrInvalidPayload{
-			Type:     record.LockTimeOnionType,
-			Omitted:  true,
-			FinalHop: false,
+			Type:      record.LockTimeOnionType,
+			Violation: hop.OmittedViolation,
+			FinalHop:  false,
 		},
 	},
 	{
@@ -62,9 +72,60 @@ var decodePayloadTests = []decodePayloadTest{
 			0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 		},
 		expErr: hop.ErrInvalidPayload{
-			Type:     record.NextHopOnionType,
-			Omitted:  false,
-			FinalHop: true,
+			Type:      record.NextHopOnionType,
+			Violation: hop.IncludedViolation,
+			FinalHop:  true,
+		},
+	},
+	{
+		name:    "required type after omitted hop id",
+		payload: []byte{0x02, 0x00, 0x04, 0x00, 0x08, 0x00},
+		expErr: hop.ErrInvalidPayload{
+			Type:      8,
+			Violation: hop.RequiredViolation,
+			FinalHop:  true,
+		},
+	},
+	{
+		name: "required type after included hop id",
+		payload: []byte{0x02, 0x00, 0x04, 0x00, 0x06, 0x08, 0x01, 0x00,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x08, 0x00,
+		},
+		expErr: hop.ErrInvalidPayload{
+			Type:      8,
+			Violation: hop.RequiredViolation,
+			FinalHop:  false,
+		},
+	},
+	{
+		name:    "required type zero final hop",
+		payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00},
+		expErr: hop.ErrInvalidPayload{
+			Type:      0,
+			Violation: hop.RequiredViolation,
+			FinalHop:  true,
+		},
+	},
+	{
+		name: "required type zero final hop zero sid",
+		payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x08,
+			0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		},
+		expErr: hop.ErrInvalidPayload{
+			Type:      6,
+			Violation: hop.IncludedViolation,
+			FinalHop:  true,
+		},
+	},
+	{
+		name: "required type zero intermediate hop",
+		payload: []byte{0x00, 0x00, 0x02, 0x00, 0x04, 0x00, 0x06, 0x08,
+			0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+		},
+		expErr: hop.ErrInvalidPayload{
+			Type:      0,
+			Violation: hop.RequiredViolation,
+			FinalHop:  false,
 		},
 	},
 }

htlcswitch/link.go

@@ -2645,12 +2645,23 @@ func (l *channelLink) processRemoteAdds(fwdPkg *channeldb.FwdPkg,
 		fwdInfo, err := chanIterator.ForwardingInstructions()
 		if err != nil {
 			// If we're unable to process the onion payload, or we
-			// we received malformed TLV stream, then we should
-			// send an error back to the caller so the HTLC can be
-			// canceled.
+			// received invalid onion payload failure, then we
+			// should send an error back to the caller so the HTLC
+			// can be canceled.
+			var failedType uint64
+			if e, ok := err.(hop.ErrInvalidPayload); ok {
+				failedType = uint64(e.Type)
+			}
+
+			// TODO: currently none of the test unit infrastructure
+			// is setup to handle TLV payloads, so testing this
+			// would require implementing a separate mock iterator
+			// for TLV payloads that also supports injecting invalid
+			// payloads. Deferring this non-trival effort till a
+			// later date
 			l.sendHTLCError(
 				pd.HtlcIndex,
-				lnwire.NewInvalidOnionVersion(onionBlob[:]),
+				lnwire.NewInvalidOnionPayload(failedType, 0),
 				obfuscator, pd.SourceRef,
 			)
 			needUpdate = true