Prevent Signature swapping within our APIs

Since the updated Schnorr API makes use of the same Signature class as ECDSA, we need to make sure signature swapping at our API layer is caught and rejected.

This isn't a cryptographic mitigation. It is not meant to guard against algorithm confusion attacks.

This is only intended to prevent a trivial kind of developer mistake.

Author: paragonie-security

src/Crypto/Signature/SchnorrSigner.php

@@ -6,6 +6,7 @@
 use Exception;
 use GMP;
 use InvalidArgumentException;
+use Mdanter\Ecc\Exception\IncorrectAlgorithmException;
 use Mdanter\Ecc\Util\BinaryString;
 use Mdanter\Ecc\Crypto\Key\{
     PrivateKeyInterface,
@@ -49,7 +50,7 @@ public function signWithKey(
         // Deconstruct as a Signature object:
         $r = gmp_init(BinaryString::substring($results['signature'], 0, $l), 16);
         $s = gmp_init(BinaryString::substring($results['signature'], $l), 16);
-        return new Signature($r, $s);
+        return new Signature($r, $s, Signature::TYPE_SCHNORR);
     }
 
     /**
@@ -63,6 +64,9 @@ public function verifyWithKey(
         Signature $signature,
         string $message
     ): bool {
+        if (!(hash_equals(Signature::TYPE_SCHNORR, $signature->getSignatureType()))) {
+            throw new IncorrectAlgorithmException('This is not a Schnorr signature');
+        }
         $ptX = gmp_strval($key->getPoint()->getX(), 16);
         $x = str_pad($ptX, 64, '0', STR_PAD_LEFT);
         $serialized = $this->formatSignature($key, $signature);

src/Crypto/Signature/Signature.php

@@ -3,6 +3,8 @@
 
 namespace Mdanter\Ecc\Crypto\Signature;
 
+use GMP;
+
 /**
  * *********************************************************************
  * Copyright (C) 2012 Matyas Danter
@@ -32,34 +34,45 @@
  */
 class Signature implements SignatureInterface
 {
+    const TYPE_ECDSA = 'ecdsa';
+
+    const TYPE_SCHNORR = 'schnorr';
+
     /**
-     * @var \GMP
+     * @var GMP
      */
     private $r;
 
     /**
      *
-     * @var \GMP
+     * @var GMP
      */
     private $s;
 
+    /**
+     * @var string $sigType
+     */
+    private $sigType;
+
     /**
      * Initialize a new instance with values
      *
-     * @param \GMP $r
-     * @param \GMP $s
+     * @param GMP $r
+     * @param GMP $s
+     * @param string $signatureType "ecdsa" or "schnorr"
      */
-    public function __construct(\GMP $r, \GMP $s)
+    public function __construct(GMP $r, GMP $s, string $signatureType = self::TYPE_ECDSA)
     {
         $this->r = $r;
         $this->s = $s;
+        $this->sigType = $signatureType;
     }
 
     /**
      * {@inheritDoc}
      * @see SignatureInterface::getR
      */
-    public function getR(): \GMP
+    public function getR(): GMP
     {
         return $this->r;
     }
@@ -68,8 +81,16 @@ public function getR(): \GMP
      * {@inheritDoc}
      * @see SignatureInterface::getS
      */
-    public function getS(): \GMP
+    public function getS(): GMP
     {
         return $this->s;
     }
+
+    /**
+     * @return string
+     */
+    public function getSignatureType(): string
+    {
+        return $this->sigType;
+    }
 }

src/Crypto/Signature/SignatureInterface.php

@@ -45,4 +45,11 @@ public function getR(): \GMP;
      * @return \GMP
      */
     public function getS(): \GMP;
+
+    /**
+     * Returns "ecdsa" or "schnorr" depending on the signature type.
+     *
+     * @return string
+     */
+    public function getSignatureType(): string;
 }

src/Crypto/Signature/Signer.php

@@ -10,6 +10,7 @@
 use Mdanter\Ecc\Curves\NamedCurveFp;
 use Mdanter\Ecc\Curves\NistCurve;
 use Mdanter\Ecc\Curves\SecgCurve;
+use Mdanter\Ecc\Exception\IncorrectAlgorithmException;
 use Mdanter\Ecc\Math\ConstantTimeMath;
 use Mdanter\Ecc\Math\GmpMathInterface;
 use Mdanter\Ecc\Crypto\Key\PrivateKeyInterface;
@@ -211,6 +212,11 @@ public function verify(PublicKeyInterface $key, SignatureInterface $signature, G
         $r = $signature->getR();
         $s = $signature->getS();
 
+        // Make sure this isn't a Schnorr signature:
+        if (!(hash_equals(Signature::TYPE_ECDSA, $signature->getSignatureType()))) {
+            throw new IncorrectAlgorithmException('This is not an ECDSA signature');
+        }
+
         $math = $this->adapter;
         /** @var GMP $one */
         $one = gmp_init(1, 10);

src/Exception/IncorrectAlgorithmException.php

@@ -0,0 +1,8 @@
+<?php
+
+namespace Mdanter\Ecc\Exception;
+
+class IncorrectAlgorithmException extends \RuntimeException
+{
+
+}

tests/unit/Crypto/Signature/SchnorrSignerTest.php

@@ -7,9 +7,14 @@
 use Exception;
 use Mdanter\Ecc\Crypto\Key\PrivateKey;
 use Mdanter\Ecc\Crypto\Signature\SchnorrSigner;
+use Mdanter\Ecc\Crypto\Signature\Signature;
+use Mdanter\Ecc\Crypto\Signature\Signer;
+use Mdanter\Ecc\Crypto\Signature\SignHasher;
 use Mdanter\Ecc\Curves\SecureCurveFactory;
+use Mdanter\Ecc\Exception\IncorrectAlgorithmException;
 use Mdanter\Ecc\Exception\InsecureCurveException;
 use Mdanter\Ecc\Math\ConstantTimeMath;
+use Mdanter\Ecc\Math\GmpMath;
 use Mdanter\Ecc\Tests\AbstractTestCase;
 
 /**
@@ -99,6 +104,30 @@ public function testSchnorrVerificationAndSigning(
 
         // Ensure the same verification result occurs:
         self::assertSame($signer->formatSignature($pkObject, $signResult2), $signResult['signature']);
+
+        // First, we make a fake "ECDSA" signature out of this Schnorr signature and ensure it's not accepted:
+        $ecdsaSig = new Signature($signResult2->getR(), $signResult2->getS());
+
+        $thrown = false;
+        try {
+            $signer->verifyWithKey($pkObject, $ecdsaSig, $message);
+        } catch (IncorrectAlgorithmException $ex) {
+            $thrown = true;
+        }
+        self::assertTrue($thrown, 'ECDSA / Schnorr signatures can be swapped');
+
+        // Next, we try the inverse attack: Feeding a Schnorr sig into the ECDSA class:
+        $math = new GmpMath();
+        $ecdsaSigner = new Signer($math, true);
+        $hash = (new SignHasher('sha256', $math))->makeHash($message, $generator);
+
+        $thrown = false;
+        try {
+            $ecdsaSigner->verify($pkObject, $signResult2, $hash);
+        } catch (IncorrectAlgorithmException $ex) {
+            $thrown = true;
+        }
+        self::assertTrue($thrown, 'ECDSA / Schnorr signatures can be swapped');
     }
 
     /**

tests/unit/Crypto/Signature/SignatureTest.php

@@ -15,5 +15,6 @@ public function testInstance()
         $signature = new Signature($r, $s);
         $this->assertSame($r, $signature->getR());
         $this->assertSame($s, $signature->getS());
+        $this->assertSame(Signature::TYPE_ECDSA, $signature->getSignatureType());
     }
 }