Check arguments passed to secp256k1 methods (#94)

* Check arguments passed to secp256k1 methods Illegal arguments will trigger an internal callback that prints to stderr and calls abort. We already check arguments in our JNI and kotlin native code but had missed 2 checks (recid in ecdsaRecover, empty arrays in pubkeyCombine). * Implement the same "tweak" checks in the native code and JNI code The native code was missing checks on the "tweak" size (which must be 32 bytes)
ACINQ · Dec 13, 2023 · f242b4f · f242b4f
1 parent 161da89
commit f242b4f
Show file tree

Hide file tree

Showing 4 changed files with 41 additions and 1 deletion.
diff --git a/build.gradle.kts b/build.gradle.kts
@@ -22,7 +22,7 @@ buildscript {
 
 allprojects {
     group = "fr.acinq.secp256k1"
-    version = "0.11.0"
+    version = "0.12.0-SNAPSHOT"
 
     repositories {
         google()

diff --git a/jni/c/src/fr_acinq_secp256k1_Secp256k1CFunctions.c b/jni/c/src/fr_acinq_secp256k1_Secp256k1CFunctions.c
@@ -517,6 +517,7 @@ JNIEXPORT jbyteArray JNICALL Java_fr_acinq_secp256k1_Secp256k1CFunctions_secp256
     if (jpubkeys == NULL) return NULL;
 
     count = (*penv)->GetArrayLength(penv, jpubkeys);
+    CHECKRESULT(count < 1, "pubkey array cannot be empty")
     pubkeys = calloc(count, sizeof(secp256k1_pubkey*));
 
     for(i = 0; i < count; i++) {
@@ -600,6 +601,7 @@ JNIEXPORT jbyteArray JNICALL Java_fr_acinq_secp256k1_Secp256k1CFunctions_secp256
     if (jctx == 0) return NULL;
     if (jsig == NULL) return NULL;
     if (jmsg == NULL) return NULL;
+    CHECKRESULT(recid < 0 || recid > 3, "invalid recovery id");
 
     sigSize = (*penv)->GetArrayLength(penv, jsig);
     int sigFormat = GetSignatureFormat(sigSize);

diff --git a/src/nativeMain/kotlin/fr/acinq/secp256k1/Secp256k1Native.kt b/src/nativeMain/kotlin/fr/acinq/secp256k1/Secp256k1Native.kt
@@ -125,6 +125,7 @@ public object Secp256k1Native : Secp256k1 {
 
     public override fun privKeyTweakAdd(privkey: ByteArray, tweak: ByteArray): ByteArray {
         require(privkey.size == 32)
+        require(tweak.size == 32)
         memScoped {
             val added = privkey.copyOf()
             val natAdd = toNat(added)
@@ -136,6 +137,7 @@ public object Secp256k1Native : Secp256k1 {
 
     public override fun privKeyTweakMul(privkey: ByteArray, tweak: ByteArray): ByteArray {
         require(privkey.size == 32)
+        require(tweak.size == 32)
         memScoped {
             val multiplied = privkey.copyOf()
             val natMul = toNat(multiplied)
@@ -156,6 +158,7 @@ public object Secp256k1Native : Secp256k1 {
 
     public override fun pubKeyTweakAdd(pubkey: ByteArray, tweak: ByteArray): ByteArray {
         require(pubkey.size == 33 || pubkey.size == 65)
+        require(tweak.size == 32)
         memScoped {
             val nPubkey = allocPublicKey(pubkey)
             val nTweak = toNat(tweak)
@@ -166,6 +169,7 @@ public object Secp256k1Native : Secp256k1 {
 
     public override fun pubKeyTweakMul(pubkey: ByteArray, tweak: ByteArray): ByteArray {
         require(pubkey.size == 33 || pubkey.size == 65)
+        require(tweak.size == 32)
         memScoped {
             val nPubkey = allocPublicKey(pubkey)
             val nTweak = toNat(tweak)
@@ -175,6 +179,7 @@ public object Secp256k1Native : Secp256k1 {
     }
 
     public override fun pubKeyCombine(pubkeys: Array<ByteArray>): ByteArray {
+        require(pubkeys.isNotEmpty())
         pubkeys.forEach { require(it.size == 33 || it.size == 65) }
         memScoped {
             val nPubkeys = pubkeys.map { allocPublicKey(it).ptr }
@@ -199,6 +204,7 @@ public object Secp256k1Native : Secp256k1 {
     public override fun ecdsaRecover(sig: ByteArray, message: ByteArray, recid: Int): ByteArray {
         require(sig.size == 64)
         require(message.size == 32)
+        require(recid in 0..3)
         memScoped {
             val nSig = toNat(sig)
             val rSig = alloc<secp256k1_ecdsa_recoverable_signature>()

diff --git a/tests/src/commonTest/kotlin/fr/acinq/secp256k1/Secp256k1Test.kt b/tests/src/commonTest/kotlin/fr/acinq/secp256k1/Secp256k1Test.kt
@@ -352,6 +352,38 @@ class Secp256k1Test {
         }
     }
 
+    @Test
+    fun testInvalidArguments() {
+        assertFails {
+            Secp256k1.pubkeyCreate(ByteArray(32))
+        }
+        assertFails {
+            Secp256k1.pubkeyCreate(Hex.decode("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
+        }
+        assertFails {
+            Secp256k1.pubkeyParse(ByteArray(33))
+        }
+        assertFails {
+            Secp256k1.pubkeyParse(Hex.decode("03ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
+        }
+        assertFails {
+            Secp256k1.pubKeyCombine(arrayOf())
+        }
+        assertFails {
+            Secp256k1.pubKeyCombine(arrayOf(ByteArray(0)))
+        }
+        assertFails {
+            Secp256k1.signSchnorr(ByteArray(0), Hex.decode("0101010101010101010101010101010101010101010101010101010101010101"), null)
+        }
+        assertFails {
+            Secp256k1.ecdsaRecover(
+                Hex.decode("01010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101"),
+                Hex.decode("0202020202020202020202020202020202020202020202020202020202020202"),
+                -1
+            )
+        }
+    }
+
     @Test
     fun fuzzEcdsaSignVerify() {
         val random = Random.Default