From f242b4ffe8f2611d203dcb69c83c5fa1bc2fc909 Mon Sep 17 00:00:00 2001 From: Fabrice Drouin Date: Wed, 13 Dec 2023 13:42:14 +0100 Subject: [PATCH] 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) --- build.gradle.kts | 2 +- .../fr_acinq_secp256k1_Secp256k1CFunctions.c | 2 ++ .../fr/acinq/secp256k1/Secp256k1Native.kt | 6 ++++ .../fr/acinq/secp256k1/Secp256k1Test.kt | 32 +++++++++++++++++++ 4 files changed, 41 insertions(+), 1 deletion(-) diff --git a/build.gradle.kts b/build.gradle.kts index 537b2d9..75f594e 100644 --- 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 index 162a5c2..4bc7b0a 100644 --- 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 index 7ffe516..2f0db83 100644 --- 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 { + 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() diff --git a/tests/src/commonTest/kotlin/fr/acinq/secp256k1/Secp256k1Test.kt b/tests/src/commonTest/kotlin/fr/acinq/secp256k1/Secp256k1Test.kt index c36ef50..cf07ae1 100644 --- 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