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)
Bit-Wasp · Dec 11, 2019 · 09f4144 · 09f4144
1 parent ac08142
commit 09f4144
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 11 deletions.
diff --git a/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php b/src/Crypto/EcAdapter/Impl/Secp256k1/Signature/CompactSignature.php
@@ -33,31 +33,34 @@ 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();
             },
             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
@@ -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
@@ -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));
     }
 }