From 252dd0c4c701293cbada2ce7dfa61b6c93fb46fe Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Pawe=C5=82=20=C5=9Al=C4=85ski?= <pawel.slaski@iohk.io>
Date: Fri, 27 Nov 2020 15:00:27 +0100
Subject: [PATCH] Fix - made public key from signature recover method safer

---
 .../iohk/ethereum/crypto/ECDSASignature.scala | 42 ++++++++++---------
 .../ethereum/crypto/ECDSASignatureSpec.scala  |  8 ++++
 2 files changed, 31 insertions(+), 19 deletions(-)

diff --git a/src/main/scala/io/iohk/ethereum/crypto/ECDSASignature.scala b/src/main/scala/io/iohk/ethereum/crypto/ECDSASignature.scala
index 94a0097eb9..913c5e2ced 100644
--- a/src/main/scala/io/iohk/ethereum/crypto/ECDSASignature.scala
+++ b/src/main/scala/io/iohk/ethereum/crypto/ECDSASignature.scala
@@ -9,6 +9,8 @@ import org.bouncycastle.crypto.params.ECPublicKeyParameters
 import org.bouncycastle.crypto.signers.{ECDSASigner, HMacDSAKCalculator}
 import org.bouncycastle.math.ec.{ECCurve, ECPoint}
 
+import scala.util.Try
+
 object ECDSASignature {
 
   val SLength = 32
@@ -107,26 +109,28 @@ object ECDSASignature {
       chainId: Option[Byte],
       messageHash: Array[Byte]
   ): Option[Array[Byte]] = {
-    val order = curve.getCurve.getOrder
-    //ignore case when x = r + order because it is negligibly improbable
-    //says: https://github.com/paritytech/rust-secp256k1/blob/f998f9a8c18227af200f0f7fdadf8a6560d391ff/depend/secp256k1/src/ecdsa_impl.h#L282
-    val xCoordinate = r
-    val curveFp = curve.getCurve.asInstanceOf[ECCurve.Fp]
-    val prime = curveFp.getQ
-
-    getRecoveredPointSign(recId, chainId).flatMap { recovery =>
-      if (xCoordinate.compareTo(prime) < 0) {
-        val R = constructPoint(xCoordinate, recovery)
-        if (R.multiply(order).isInfinity) {
-          val e = BigInt(1, messageHash)
-          val rInv = r.modInverse(order)
-          //Q = r^(-1)(sR - eG)
-          val q = R.multiply(s.bigInteger).subtract(curve.getG.multiply(e.bigInteger)).multiply(rInv.bigInteger)
-          //byte 0 of encoded ECC point indicates that it is uncompressed point, it is part of bouncycastle encoding
-          Some(q.getEncoded(false).tail)
+    Try {
+      val order = curve.getCurve.getOrder
+      //ignore case when x = r + order because it is negligibly improbable
+      //says: https://github.com/paritytech/rust-secp256k1/blob/f998f9a8c18227af200f0f7fdadf8a6560d391ff/depend/secp256k1/src/ecdsa_impl.h#L282
+      val xCoordinate = r
+      val curveFp = curve.getCurve.asInstanceOf[ECCurve.Fp]
+      val prime = curveFp.getQ
+
+      getRecoveredPointSign(recId, chainId).flatMap { recovery =>
+        if (xCoordinate.compareTo(prime) < 0) {
+          val R = constructPoint(xCoordinate, recovery)
+          if (R.multiply(order).isInfinity) {
+            val e = BigInt(1, messageHash)
+            val rInv = r.modInverse(order)
+            //Q = r^(-1)(sR - eG)
+            val q = R.multiply(s.bigInteger).subtract(curve.getG.multiply(e.bigInteger)).multiply(rInv.bigInteger)
+            //byte 0 of encoded ECC point indicates that it is uncompressed point, it is part of bouncycastle encoding
+            Some(q.getEncoded(false).tail)
+          } else None
         } else None
-      } else None
-    }
+      }
+    }.toOption.flatten
   }
 
   private def constructPoint(xCoordinate: BigInt, recId: Int): ECPoint = {
diff --git a/src/test/scala/io/iohk/ethereum/crypto/ECDSASignatureSpec.scala b/src/test/scala/io/iohk/ethereum/crypto/ECDSASignatureSpec.scala
index 30e2eeb04c..ed29ed72e0 100644
--- a/src/test/scala/io/iohk/ethereum/crypto/ECDSASignatureSpec.scala
+++ b/src/test/scala/io/iohk/ethereum/crypto/ECDSASignatureSpec.scala
@@ -2,6 +2,7 @@ package io.iohk.ethereum.crypto
 
 import akka.util.ByteString
 import io.iohk.ethereum.nodebuilder.SecureRandomBuilder
+import io.iohk.ethereum.utils.ByteStringUtils
 import org.scalacheck.Arbitrary
 import org.scalacheck.Arbitrary.arbitrary
 import org.bouncycastle.crypto.params.ECPublicKeyParameters
@@ -35,6 +36,13 @@ class ECDSASignatureSpec extends AnyFlatSpec with Matchers with ScalaCheckProper
     sig.publicKey(bytesToSign).isEmpty shouldBe true
   }
 
+  it should "fail public key recover without throwing when signature is bad (Invalid point compression)" in {
+    val sig = ECDSASignature(ByteStringUtils.string2hash("149a2046f51f5d043633664d76eef4f99cdba8e53851dcda57224dfe8770f98a"), ByteStringUtils.string2hash("a8898478e9aae9fadb71c7ab5451d47d2efa4199fc26ecc1da62ce8fb77e06f1"), 28.toByte)
+    val messageHash = ByteStringUtils.string2hash("a1ede9cdf0b6fe37a384b265dce6b74a7464f11799dcee022f628450a19cf4eb")
+
+    sig.publicKey(messageHash).isEmpty shouldBe true
+  }
+
   it should "sign message and recover public key" in {
     forAll(arbitrary[Array[Byte]], Arbitrary.arbitrary[Unit].map(_ => generateKeyPair(secureRandom))) {
       (message, keys) =>