sphinx: add strictAttribution flag for onion error decryption

We now add a flag to the decryption function which controls the way
we're carrying out the decryption with regards to the attribution data.
This is primarily done for graceful backwards compatibility, in order to
prevent nodes upgrading to attributable failures from blaming their
first hop peers, which would very fast blacklist their channels and
cause failed payments. As an initial step, we'd want the senders to
perform the non-strict decryption, eventually switching to the strict
decryption once adoption is sufficient.

Author: GeorgeTsagk

attr_error_test.go

@@ -69,7 +69,9 @@ func TestAttributableOnionFailure(t *testing.T) {
 
 	// Emulate that sender node receive the failure message and trying to
 	// unwrap it, by applying obfuscation and checking the hmac.
-	decryptedError, err := deobfuscator.DecryptError(legacyData, attrData)
+	decryptedError, err := deobfuscator.DecryptError(
+		legacyData, attrData, true,
+	)
 	require.NoError(t, err)
 
 	// We should understand the node from which error have been received.
@@ -144,7 +146,9 @@ func TestOnionFailureCorruption(t *testing.T) {
 
 	// Emulate that sender node receive the failure message and trying to
 	// unwrap it, by applying obfuscation and checking the hmac.
-	decryptedError, err := deobfuscator.DecryptError(legacyData, attrData)
+	decryptedError, err := deobfuscator.DecryptError(
+		legacyData, attrData, true,
+	)
 	require.NoError(t, err)
 
 	// Assert that the second hop is correctly identified as the error
@@ -247,7 +251,9 @@ func TestAttributableFailureSpecVector(t *testing.T) {
 
 	// Emulate that sender node receives the failure message and trying to
 	// unwrap it, by applying obfuscation and checking the hmac.
-	decryptedError, err := deobfuscator.DecryptError(legacyData, attrData)
+	decryptedError, err := deobfuscator.DecryptError(
+		legacyData, attrData, true,
+	)
 	require.NoError(t, err)
 
 	// Check that message have been properly de-obfuscated.
@@ -291,7 +297,9 @@ func TestAttributableOnionFailureZeroesMessage(t *testing.T) {
 	// unwrap it, by applying obfuscation and checking the hmac.
 	obfuscatedData := make([]byte, 20000)
 
-	decryptedError, err := deobfuscator.DecryptError(obfuscatedData, nil)
+	decryptedError, err := deobfuscator.DecryptError(
+		obfuscatedData, nil, true,
+	)
 	require.NoError(t, err)
 
 	require.Equal(t, 1, decryptedError.SenderIdx)
@@ -317,7 +325,9 @@ func TestAttributableOnionFailureShortMessage(t *testing.T) {
 	obfuscatedData := make([]byte, deobfuscator.hmacsAndPayloadsLen()-1)
 	failureMsg := bytes.Repeat([]byte{1}, minOnionErrorLength)
 
-	decryptedError, err := deobfuscator.DecryptError(failureMsg, obfuscatedData)
+	decryptedError, err := deobfuscator.DecryptError(
+		failureMsg, obfuscatedData, true,
+	)
 	require.NoError(t, err)
 
 	require.Equal(t, 1, decryptedError.SenderIdx)

crypto.go

@@ -340,17 +340,40 @@ const minOnionErrorLength = minPaddedOnionErrorLength + sha256.Size
 // returned that contains the decrypted error message and information of the
 // error sender. We also report the hold times in ms for each hop on the error
 // path.
+//
+// The strictAttribution flag controls the behavior of the decryption logic
+// surrounding the presence of attribution data:
+//
+//   - If set, then the first node with bad attribution data will be blamed
+//     immediately.
+//
+//   - If unset, decryption continues optimistically until a successful error
+//     message decryption occurs, regardless of attribution data validity. Hold
+//     times are still extracted for nodes that provided valid attribution data.
 func (o *OnionErrorDecrypter) DecryptError(encryptedData []byte,
-	attrData []byte) (*DecryptedError, error) {
+	attrData []byte, strictAttribution bool) (*DecryptedError, error) {
 
-	// Ensure the error message and attribution data length is as expected.
-	if len(encryptedData) < minOnionErrorLength ||
-		len(attrData) < o.hmacsAndPayloadsLen() {
+	// Ensure the error message field is present and has the correct length.
+	if len(encryptedData) < minOnionErrorLength {
+		return nil, fmt.Errorf("invalid error length: "+
+			"expected at least %v got %v", minOnionErrorLength,
+			len(encryptedData))
+	}
 
-		return &DecryptedError{
-			Sender:    o.circuit.PaymentPath[0],
-			SenderIdx: 1,
-		}, nil
+	validAttr := true
+
+	// If we're decrypting with strict attribution, we need to have the
+	// correct attribution data present too. If strictAttribution is set
+	// then we immediately blame the first hop.
+	if len(attrData) < o.hmacsAndPayloadsLen() {
+		if strictAttribution {
+			return &DecryptedError{
+				Sender:    o.circuit.PaymentPath[0],
+				SenderIdx: 1,
+			}, nil
+		} else {
+			validAttr = false
+		}
 	}
 
 	sharedSecrets, _, err := generateSharedSecrets(
@@ -393,49 +416,69 @@ func (o *OnionErrorDecrypter) DecryptError(encryptedData []byte,
 
 		// With the shared secret, we'll now strip off a layer of
 		// encryption from the encrypted failure and attribution
-		// data.
+		// data. This needs to be done before parsing the attribution
+		// data, as the attribution data HMACs commit to it.
 		failData = onionEncrypt(AMMAG, &sharedSecret, failData)
-		attrData = onionEncrypt(AMMAG_EXT, &sharedSecret, attrData)
-
-		payloads := o.payloads(attrData)
-		hmacs := o.hmacs(attrData)
 
-		// Let's calculate the HMAC we expect for the corresponding
-		// payloads.
-		position := o.hopCount - i - 1
-		expectedAttrHmac := o.calculateHmac(
-			sharedSecret, position, failData, payloads, hmacs,
-		)
-
-		// Let's retrieve the actual HMAC from the correct position in
-		// the HMACs array.
-		actualAttrHmac := hmacs[i*o.hmacSize : (i+1)*o.hmacSize]
-
-		// If the hmac does not match up, exit with a nil message. This
-		// is not done for the dummy iterations.
-		if !bytes.Equal(actualAttrHmac, expectedAttrHmac) &&
-			sender == 0 && i < len(o.circuit.PaymentPath) {
-
-			sender = i + 1
-			msg = nil
+		// If the attribution data are valid then do another round of
+		// attribution data decryption.
+		if validAttr {
+			attrData = onionEncrypt(
+				AMMAG_EXT, &sharedSecret, attrData,
+			)
+
+			payloads := o.payloads(attrData)
+			hmacs := o.hmacs(attrData)
+
+			// Let's calculate the HMAC we expect for the
+			// corresponding payloads.
+			position := o.hopCount - i - 1
+			expectedAttrHmac := o.calculateHmac(
+				sharedSecret, position, failData, payloads,
+				hmacs,
+			)
+
+			// Let's retrieve the actual HMAC from the correct
+			// position in the HMACs array.
+			actualAttrHmac := hmacs[i*o.hmacSize : (i+1)*o.hmacSize]
+
+			// If the hmac does not match up, exit with a nil
+			// message. This is not done for the dummy iterations.
+			if !bytes.Equal(actualAttrHmac, expectedAttrHmac) &&
+				sender == 0 && i < len(o.circuit.PaymentPath) {
+
+				switch strictAttribution {
+				case true:
+					sender = i + 1
+					msg = nil
+
+				case false:
+					// Flag the attribution data as invalid
+					// from this point onwards. This will
+					// prevent the loop from trying to
+					// extract anything from the attribution
+					// data.
+					validAttr = false
+				}
+			}
+
+			// Extract the payload and exit with a nil message if it
+			// is invalid.
+			holdTime := o.extractPayload(payloads)
+			if sender == 0 && validAttr {
+				// Store hold time reported by this node.
+				hopPayloads = append(hopPayloads, holdTime)
+
+				// Update the message.
+				msg = failData[sha256.Size:]
+			}
+
+			// Shift payloads and hmacs to the left to prepare for
+			// the next iteration.
+			o.shiftPayloadsLeft(payloads)
+			o.shiftHmacsLeft(hmacs)
 		}
 
-		// Extract the payload and exit with a nil message if it is
-		// invalid.
-		holdTime := o.extractPayload(payloads)
-		if sender == 0 {
-			// Store hold time reported by this node.
-			hopPayloads = append(hopPayloads, holdTime)
-
-			// Update the message.
-			msg = failData[sha256.Size:]
-		}
-
-		// Shift payloads and hmacs to the left to prepare for the next
-		// iteration.
-		o.shiftPayloadsLeft(payloads)
-		o.shiftHmacsLeft(hmacs)
-
 		// Next, we'll need to separate the failure data, from the MAC
 		// itself so we can reconstruct and verify it.
 		expectedMac := failData[:sha256.Size]

obfuscation_test.go

@@ -42,7 +42,7 @@ func TestOnionFailure(t *testing.T) {
 
 	// Emulate the situation when last hop creates the onion failure
 	// message and send it back.
-	legacyData, attrData, err := obfuscator.EncryptError(
+	legacyData, _, err := obfuscator.EncryptError(
 		true, failureData, nil, 0,
 	)
 	require.NoError(t, err)
@@ -55,8 +55,8 @@ func TestOnionFailure(t *testing.T) {
 			sharedSecrets[i], attributableErrorTestStructure,
 		)
 
-		legacyData, attrData, err = obfuscator.EncryptError(
-			false, legacyData, attrData, 1,
+		legacyData, _, err = obfuscator.EncryptError(
+			false, legacyData, nil, 1,
 		)
 		require.NoError(t, err)
 	}
@@ -69,7 +69,9 @@ func TestOnionFailure(t *testing.T) {
 
 	// Emulate that sender node receive the failure message and trying to
 	// unwrap it, by applying obfuscation and checking the hmac.
-	decryptedError, err := deobfuscator.DecryptError(legacyData, attrData)
+	decryptedError, err := deobfuscator.DecryptError(
+		legacyData, nil, false,
+	)
 	if err != nil {
 		t.Fatalf("unable to de-obfuscate the onion failure: %v", err)
 	}
@@ -278,7 +280,9 @@ func TestOnionFailureSpecVector(t *testing.T) {
 
 	// Emulate that sender node receives the failure message and trying to
 	// unwrap it, by applying obfuscation and checking the hmac.
-	decryptedError, err := deobfuscator.DecryptError(legacyData, attrData)
+	decryptedError, err := deobfuscator.DecryptError(
+		legacyData, attrData, false,
+	)
 	if err != nil {
 		t.Fatalf("unable to de-obfuscate the onion failure: %v", err)
 	}