From cbcf5258dc6febc64e31ba88264571028cbb548c Mon Sep 17 00:00:00 2001
From: Danno Ferrin <danno.ferrin@gmail.com>
Date: Fri, 27 Aug 2021 08:08:20 -0600
Subject: [PATCH] Use native SECP256K1 and SECP256R1 for signature
 normalization (#2683)

Use native SECP256K1 and SEPC256R1 for signature normalization

Signed-off-by: Danno Ferrin <danno.ferrin@gmail.com>
---
 .../hyperledger/besu/crypto/SECP256K1.java    | 22 +++++++++++++++++--
 .../hyperledger/besu/crypto/SECP256R1.java    | 12 ++++++++++
 .../besu/crypto/SECP256R1Test.java            | 11 +++++++---
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256K1.java b/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256K1.java
index d64498d777..732a25db25 100644
--- a/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256K1.java
+++ b/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256K1.java
@@ -21,6 +21,7 @@ import org.hyperledger.besu.nativelib.secp256k1.LibSecp256k1.secp256k1_ecdsa_rec
 import org.hyperledger.besu.nativelib.secp256k1.LibSecp256k1.secp256k1_ecdsa_signature;
 import org.hyperledger.besu.nativelib.secp256k1.LibSecp256k1.secp256k1_pubkey;
 
+import java.math.BigInteger;
 import java.nio.ByteBuffer;
 import java.util.Optional;
 
@@ -117,7 +118,12 @@ public class SECP256K1 extends AbstractSECP256 {
   public Optional<SECPPublicKey> recoverPublicKeyFromSignature(
       final Bytes32 dataHash, final SECPSignature signature) {
     if (useNative) {
-      return recoverFromSignatureNative(dataHash, signature);
+      Optional<SECPPublicKey> result = recoverFromSignatureNative(dataHash, signature);
+      if (result.isEmpty()) {
+        throw new IllegalArgumentException("Could not recover public key");
+      } else {
+        return result;
+      }
     } else {
       return super.recoverPublicKeyFromSignature(dataHash, signature);
     }
@@ -184,6 +190,18 @@ public class SECP256K1 extends AbstractSECP256 {
         != 0;
   }
 
+  @Override
+  protected BigInteger recoverFromSignature(
+      final int recId, final BigInteger r, final BigInteger s, final Bytes32 dataHash) {
+    if (useNative) {
+      return recoverFromSignatureNative(dataHash, new SECPSignature(r, s, (byte) recId))
+          .map(key -> new BigInteger(1, key.getEncoded()))
+          .orElse(null);
+    } else {
+      return super.recoverFromSignature(recId, r, s, dataHash);
+    }
+  }
+
   private Optional<SECPPublicKey> recoverFromSignatureNative(
       final Bytes32 dataHash, final SECPSignature signature) {
 
@@ -205,7 +223,7 @@ public class SECP256K1 extends AbstractSECP256 {
     if (LibSecp256k1.secp256k1_ecdsa_recover(
             LibSecp256k1.CONTEXT, newPubKey, parsedSignature, dataHash.toArrayUnsafe())
         == 0) {
-      throw new IllegalArgumentException("Could not recover public key");
+      return Optional.empty();
     }
 
     // parse the key
diff --git a/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256R1.java b/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256R1.java
index 8109fdb0f7..39e744e33a 100644
--- a/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256R1.java
+++ b/crypto/src/main/java/org/hyperledger/besu/crypto/SECP256R1.java
@@ -102,6 +102,18 @@ public class SECP256R1 extends AbstractSECP256 {
         nativeSignature.getV());
   }
 
+  @Override
+  protected BigInteger recoverFromSignature(
+      final int recId, final BigInteger r, final BigInteger s, final Bytes32 dataHash) {
+    if (useNative) {
+      return recoverPublicKeyFromSignatureNative(dataHash, new SECPSignature(r, s, (byte) recId))
+          .map(key -> new BigInteger(1, key.getEncoded()))
+          .orElse(null);
+    } else {
+      return super.recoverFromSignature(recId, r, s, dataHash);
+    }
+  }
+
   private Optional<SECPPublicKey> recoverPublicKeyFromSignatureNative(
       final Bytes32 dataHash, final SECPSignature signature) {
     byte[] recoveredKey;
diff --git a/crypto/src/test/java/org/hyperledger/besu/crypto/SECP256R1Test.java b/crypto/src/test/java/org/hyperledger/besu/crypto/SECP256R1Test.java
index 22315f7adf..6de42dcbd5 100644
--- a/crypto/src/test/java/org/hyperledger/besu/crypto/SECP256R1Test.java
+++ b/crypto/src/test/java/org/hyperledger/besu/crypto/SECP256R1Test.java
@@ -142,19 +142,24 @@ public class SECP256R1Test {
   }
 
   @Test
-  public void signatureGenerationAndVerification() {
+  public void signatureGenerationVerificationAndPubKeyRecovery() {
     signTestVectors.forEach(
         signTestVector -> {
           final SECPPrivateKey privateKey =
               secp256R1.createPrivateKey(new BigInteger(signTestVector.getPrivateKey(), 16));
-          final SECPPublicKey publicKey =
-              secp256R1.createPublicKey(new BigInteger(signTestVector.getPublicKey(), 16));
+          final BigInteger publicKeyBigInt = new BigInteger(signTestVector.getPublicKey(), 16);
+          final SECPPublicKey publicKey = secp256R1.createPublicKey(publicKeyBigInt);
           final KeyPair keyPair = secp256R1.createKeyPair(privateKey);
 
           final Bytes32 dataHash = keccak256(Bytes.wrap(signTestVector.getData().getBytes(UTF_8)));
 
           final SECPSignature signature = secp256R1.sign(dataHash, keyPair);
           assertThat(secp256R1.verify(dataHash, signature, publicKey)).isTrue();
+
+          final BigInteger recoveredPubKeyBigInt =
+              secp256R1.recoverFromSignature(
+                  signature.getRecId(), signature.getR(), signature.getS(), dataHash);
+          assertThat(recoveredPubKeyBigInt).isEqualTo(recoveredPubKeyBigInt);
         });
   }