From 1aab3eec0cc272824ba7a54666e98c1f14d6e34e Mon Sep 17 00:00:00 2001 From: Thomas Kerin Date: Wed, 11 Dec 2019 00:44:05 +0000 Subject: [PATCH 1/2] Fix #805: Incorrect tests of resource type --- .../Impl/Secp256k1/Adapter/EcAdapter.php | 2 +- .../Impl/Secp256k1/Key/PublicKey.php | 3 +- .../Secp256k1/Signature/CompactSignature.php | 4 +- .../Impl/Secp256k1/Signature/Signature.php | 4 +- .../Impl/Secp256k1/Adapter/EcAdapterTest.php | 37 +++++++++++++++++++ .../Impl/Secp256k1/Key/PublicKeyTest.php | 36 ++++++++++++++++++ .../Signature/CompactSignatureTest.php | 36 ++++++++++++++++++ .../Secp256k1/Signature/SignatureTest.php | 35 ++++++++++++++++++ 8 files changed, 148 insertions(+), 9 deletions(-) create mode 100644 tests/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapterTest.php create mode 100644 tests/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKeyTest.php create mode 100644 tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php create mode 100644 tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php diff --git a/src/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapter.php b/src/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapter.php index 334806d81..49494d53e 100644 --- a/src/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapter.php +++ b/src/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapter.php @@ -44,7 +44,7 @@ class EcAdapter implements EcAdapterInterface */ public function __construct(Math $math, GeneratorPoint $generator, $secp256k1_context_t) { - if (!is_resource($secp256k1_context_t) || !get_resource_type($secp256k1_context_t) === SECP256K1_TYPE_CONTEXT) { + if (!is_resource($secp256k1_context_t) || get_resource_type($secp256k1_context_t) !== SECP256K1_TYPE_CONTEXT) { throw new \InvalidArgumentException('Secp256k1: Must pass a secp256k1_context_t resource'); } diff --git a/src/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKey.php b/src/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKey.php index 649bf55af..4db7ab3d4 100644 --- a/src/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKey.php +++ b/src/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKey.php @@ -38,8 +38,7 @@ class PublicKey extends Key implements PublicKeyInterface */ public function __construct(EcAdapter $ecAdapter, $secp256k1_pubkey_t, bool $compressed = false) { - if (!is_resource($secp256k1_pubkey_t) || - !get_resource_type($secp256k1_pubkey_t) === SECP256K1_TYPE_PUBKEY) { + if (!is_resource($secp256k1_pubkey_t) || get_resource_type($secp256k1_pubkey_t) !== SECP256K1_TYPE_PUBKEY) { throw new \InvalidArgumentException('Secp256k1\Key\PublicKey expects ' . SECP256K1_TYPE_PUBKEY . ' resource'); } diff --git a/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php b/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php index 7a80c2219..e0bb0f09a 100644 --- a/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php +++ b/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php @@ -39,9 +39,7 @@ class CompactSignature extends Signature implements CompactSignatureInterface */ public function __construct(EcAdapter $ecAdapter, $secp256k1_ecdsa_signature_t, int $recid, bool $compressed) { - if (!is_resource($secp256k1_ecdsa_signature_t) - || SECP256K1_TYPE_RECOVERABLE_SIG !== get_resource_type($secp256k1_ecdsa_signature_t) - ) { + if (!is_resource($secp256k1_ecdsa_signature_t) || SECP256K1_TYPE_RECOVERABLE_SIG !== get_resource_type($secp256k1_ecdsa_signature_t)) { throw new \RuntimeException('CompactSignature: must pass recoverable signature resource'); } diff --git a/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/Signature.php b/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/Signature.php index b792414cc..e363cd05c 100644 --- a/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/Signature.php +++ b/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/Signature.php @@ -40,9 +40,7 @@ class Signature extends Serializable implements SignatureInterface */ public function __construct(EcAdapter $adapter, \GMP $r, \GMP $s, $secp256k1_ecdsa_signature_t) { - if (!is_resource($secp256k1_ecdsa_signature_t) || - !get_resource_type($secp256k1_ecdsa_signature_t) === SECP256K1_TYPE_SIG - ) { + if (!is_resource($secp256k1_ecdsa_signature_t) || get_resource_type($secp256k1_ecdsa_signature_t) !== SECP256K1_TYPE_SIG) { throw new \InvalidArgumentException('Secp256k1\Signature\Signature expects ' . SECP256K1_TYPE_SIG . ' resource'); } diff --git a/tests/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapterTest.php b/tests/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapterTest.php new file mode 100644 index 000000000..7ee63be90 --- /dev/null +++ b/tests/Crypto/EcAdapter/Impl/Secp256k1/Adapter/EcAdapterTest.php @@ -0,0 +1,37 @@ +markTestSkipped("secp256k1 not installed"); + } + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Secp256k1: Must pass a secp256k1_context_t resource"); + $this->callConstructor(""); + } + public function testContextWrongResource() + { + if (!function_exists('secp256k1_context_create')) { + $this->markTestSkipped("secp256k1 not installed"); + } + $ctx = gmp_init(1); + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage("Secp256k1: Must pass a secp256k1_context_t resource"); + $this->callConstructor($ctx); + } +} diff --git a/tests/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKeyTest.php b/tests/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKeyTest.php new file mode 100644 index 000000000..dd422086a --- /dev/null +++ b/tests/Crypto/EcAdapter/Impl/Secp256k1/Key/PublicKeyTest.php @@ -0,0 +1,36 @@ +markTestSkipped("secp256k1 not installed"); + } + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Secp256k1\Key\PublicKey expects ' . SECP256K1_TYPE_PUBKEY . ' resource'); + $this->callConstructor(""); + } + public function testWrongResourceType() + { + if (!function_exists('secp256k1_context_create')) { + $this->markTestSkipped("secp256k1 not installed"); + } + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Secp256k1\Key\PublicKey expects ' . SECP256K1_TYPE_PUBKEY . ' resource'); + $this->callConstructor(gmp_init(1)); + } +} diff --git a/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php new file mode 100644 index 000000000..4a0ae104f --- /dev/null +++ b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php @@ -0,0 +1,36 @@ +markTestSkipped("secp256k1 not installed"); + } + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); + $this->callConstructor(""); + } + public function testWrongResourceType() + { + if (!function_exists('secp256k1_context_create')) { + $this->markTestSkipped("secp256k1 not installed"); + } + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); + $this->callConstructor(gmp_init(1)); + } +} diff --git a/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php new file mode 100644 index 000000000..628b75a30 --- /dev/null +++ b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php @@ -0,0 +1,35 @@ +markTestSkipped("secp256k1 not installed"); + } + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); + $this->callConstructor(""); + } + public function testWrongResourceType() + { + if (!function_exists('secp256k1_context_create')) { + $this->markTestSkipped("secp256k1 not installed"); + } + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); + $this->callConstructor(gmp_init(1)); + } +} From 183e79313cec4d254ba736d6e25d437d52879d51 Mon Sep 17 00:00:00 2001 From: Thomas Kerin Date: Wed, 11 Dec 2019 01:20:20 +0000 Subject: [PATCH 2/2] Bugfix: invalid resource type passed in CompactSignature In the secp256k1 CompactSignature implementation we pass the recoverable signature instead of the signature.. This is a bug, although it does make us wonder what the goal of this design decision was (to extend Signature) --- .../Secp256k1/Signature/CompactSignature.php | 17 ++++++++++------- .../Signature/CompactSignatureTest.php | 4 ++-- .../Impl/Secp256k1/Signature/SignatureTest.php | 4 ++-- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php b/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php index e0bb0f09a..8146c4a4d 100644 --- a/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php +++ b/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php @@ -33,19 +33,19 @@ class CompactSignature extends Signature implements CompactSignatureInterface /** * @param EcAdapter $ecAdapter - * @param resource $secp256k1_ecdsa_signature_t + * @param resource $secp256k1_recoverable_signature_t * @param int $recid * @param bool $compressed */ - public function __construct(EcAdapter $ecAdapter, $secp256k1_ecdsa_signature_t, int $recid, bool $compressed) + public function __construct(EcAdapter $ecAdapter, $secp256k1_recoverable_signature_t, int $recid, bool $compressed) { - if (!is_resource($secp256k1_ecdsa_signature_t) || SECP256K1_TYPE_RECOVERABLE_SIG !== get_resource_type($secp256k1_ecdsa_signature_t)) { - throw new \RuntimeException('CompactSignature: must pass recoverable signature resource'); + if (!is_resource($secp256k1_recoverable_signature_t) || SECP256K1_TYPE_RECOVERABLE_SIG !== get_resource_type($secp256k1_recoverable_signature_t)) { + throw new \RuntimeException('CompactSignature: must pass recoverable signature resource, not ' . $secp256k1_recoverable_signature_t); } $ser = ''; $recidout = 0; - secp256k1_ecdsa_recoverable_signature_serialize_compact($ecAdapter->getContext(), $ser, $recidout, $secp256k1_ecdsa_signature_t); + secp256k1_ecdsa_recoverable_signature_serialize_compact($ecAdapter->getContext(), $ser, $recidout, $secp256k1_recoverable_signature_t); list ($r, $s) = array_map( function ($val) { return (new Buffer($val))->getGmp(); @@ -53,11 +53,14 @@ function ($val) { str_split($ser, 32) ); - $this->resource = $secp256k1_ecdsa_signature_t; + $this->resource = $secp256k1_recoverable_signature_t; $this->recid = $recid; $this->compressed = $compressed; $this->ecAdapter = $ecAdapter; - parent::__construct($ecAdapter, $r, $s, $secp256k1_ecdsa_signature_t); + + $sig_t = null; + secp256k1_ecdsa_recoverable_signature_convert($this->ecAdapter->getContext(), $sig_t, $this->resource); + parent::__construct($ecAdapter, $r, $s, $sig_t); } /** diff --git a/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php index 4a0ae104f..b1d27d371 100644 --- a/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php +++ b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignatureTest.php @@ -20,7 +20,7 @@ public function testNotResource() if (!function_exists('secp256k1_context_create')) { $this->markTestSkipped("secp256k1 not installed"); } - $this->expectException(\InvalidArgumentException::class); + $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); $this->callConstructor(""); } @@ -29,7 +29,7 @@ public function testWrongResourceType() if (!function_exists('secp256k1_context_create')) { $this->markTestSkipped("secp256k1 not installed"); } - $this->expectException(\InvalidArgumentException::class); + $this->expectException(\RuntimeException::class); $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); $this->callConstructor(gmp_init(1)); } diff --git a/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php index 628b75a30..a5a437188 100644 --- a/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php +++ b/tests/Crypto/EcAdapter/Impl/Secp256k1/Signature/SignatureTest.php @@ -20,7 +20,7 @@ public function testNotResource() $this->markTestSkipped("secp256k1 not installed"); } $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); + $this->expectExceptionMessage('Secp256k1\Signature\Signature expects ' . SECP256K1_TYPE_SIG . ' resource'); $this->callConstructor(""); } public function testWrongResourceType() @@ -29,7 +29,7 @@ public function testWrongResourceType() $this->markTestSkipped("secp256k1 not installed"); } $this->expectException(\InvalidArgumentException::class); - $this->expectExceptionMessage('CompactSignature: must pass recoverable signature resource'); + $this->expectExceptionMessage('Secp256k1\Signature\Signature expects ' . SECP256K1_TYPE_SIG . ' resource'); $this->callConstructor(gmp_init(1)); } }