Merge pull request #864 from lcobucci/fix-ecdsa-key-size-validation

Fix ecdsa key size validation

src/Signer/CannotSignPayload.php

@@ -10,6 +10,6 @@ final class CannotSignPayload extends InvalidArgumentException implements Except
 {
     public static function errorHappened(string $error): self
     {
-        return new self('There was an error while creating the signature: ' . $error);
+        return new self('There was an error while creating the signature:' . $error);
     }
 }

src/Signer/Ecdsa.php

@@ -26,33 +26,42 @@ final public function sign(string $payload, Key $key): string
     {
         return $this->converter->fromAsn1(
             $this->createSignature($key->contents(), $key->passphrase(), $payload),
-            $this->keyLength()
+            $this->pointLength()
         );
     }
 
     final public function verify(string $expected, string $payload, Key $key): bool
     {
         return $this->verifySignature(
-            $this->converter->toAsn1($expected, $this->keyLength()),
+            $this->converter->toAsn1($expected, $this->pointLength()),
             $payload,
             $key->contents()
         );
     }
 
-    final public function keyType(): int
+    /** {@inheritdoc} */
+    final protected function guardAgainstIncompatibleKey(int $type, int $lengthInBits): void
     {
-        return OPENSSL_KEYTYPE_EC;
-    }
+        if ($type !== OPENSSL_KEYTYPE_EC) {
+            throw InvalidKeyProvided::incompatibleKeyType(
+                self::KEY_TYPE_MAP[OPENSSL_KEYTYPE_EC],
+                self::KEY_TYPE_MAP[$type],
+            );
+        }
 
-    final public function minimumBitsLengthForKey(): int
-    {
-        return 224;
+        $expectedKeyLength = $this->expectedKeyLength();
+
+        if ($lengthInBits !== $expectedKeyLength) {
+            throw InvalidKeyProvided::incompatibleKeyLength($expectedKeyLength, $lengthInBits);
+        }
     }
 
+    abstract public function expectedKeyLength(): int;
+
     /**
      * Returns the length of each point in the signature, so that we can calculate and verify R and S points properly
      *
      * @internal
      */
-    abstract public function keyLength(): int;
+    abstract public function pointLength(): int;
 }

src/Signer/Ecdsa/Sha256.php

@@ -19,8 +19,13 @@ public function algorithm(): int
         return OPENSSL_ALGO_SHA256;
     }
 
-    public function keyLength(): int
+    public function pointLength(): int
     {
         return 64;
     }
+
+    public function expectedKeyLength(): int
+    {
+        return 256;
+    }
 }

src/Signer/Ecdsa/Sha384.php

@@ -19,8 +19,13 @@ public function algorithm(): int
         return OPENSSL_ALGO_SHA384;
     }
 
-    public function keyLength(): int
+    public function pointLength(): int
     {
         return 96;
     }
+
+    public function expectedKeyLength(): int
+    {
+        return 384;
+    }
 }

src/Signer/Ecdsa/Sha512.php

@@ -19,8 +19,13 @@ public function algorithm(): int
         return OPENSSL_ALGO_SHA512;
     }
 
-    public function keyLength(): int
+    public function pointLength(): int
     {
         return 132;
     }
+
+    public function expectedKeyLength(): int
+    {
+        return 521;
+    }
 }

src/Signer/Ecdsa/UnsafeSha256.php

@@ -20,7 +20,7 @@ public function algorithm(): int
         return OPENSSL_ALGO_SHA256;
     }
 
-    public function keyLength(): int
+    public function pointLength(): int
     {
         return 64;
     }

src/Signer/Ecdsa/UnsafeSha384.php

@@ -20,7 +20,7 @@ public function algorithm(): int
         return OPENSSL_ALGO_SHA384;
     }
 
-    public function keyLength(): int
+    public function pointLength(): int
     {
         return 96;
     }

src/Signer/Ecdsa/UnsafeSha512.php

@@ -20,7 +20,7 @@ public function algorithm(): int
         return OPENSSL_ALGO_SHA512;
     }
 
-    public function keyLength(): int
+    public function pointLength(): int
     {
         return 132;
     }

src/Signer/InvalidKeyProvided.php

@@ -10,12 +10,23 @@ final class InvalidKeyProvided extends InvalidArgumentException implements Excep
 {
     public static function cannotBeParsed(string $details): self
     {
-        return new self('It was not possible to parse your key, reason: ' . $details);
+        return new self('It was not possible to parse your key, reason:' . $details);
     }
 
-    public static function incompatibleKey(): self
+    public static function incompatibleKeyType(string $expectedType, string $actualType): self
     {
-        return new self('This key is not compatible with this signer');
+        return new self(
+            'The type of the provided key is not "' . $expectedType
+            . '", "' . $actualType . '" provided'
+        );
+    }
+
+    public static function incompatibleKeyLength(int $expectedLength, int $actualLength): self
+    {
+        return new self(
+            'The length of the provided key is different than ' . $expectedLength . ' bits, '
+            . $actualLength . ' bits provided'
+        );
     }
 
     public static function cannotBeEmpty(): self

src/Signer/OpenSSL.php

@@ -11,7 +11,6 @@
 use function is_array;
 use function is_bool;
 use function is_int;
-use function is_string;
 use function openssl_error_string;
 use function openssl_free_key;
 use function openssl_pkey_get_details;
@@ -20,8 +19,21 @@
 use function openssl_sign;
 use function openssl_verify;
 
+use const OPENSSL_KEYTYPE_DH;
+use const OPENSSL_KEYTYPE_DSA;
+use const OPENSSL_KEYTYPE_EC;
+use const OPENSSL_KEYTYPE_RSA;
+use const PHP_EOL;
+
 abstract class OpenSSL implements Signer
 {
+    protected const KEY_TYPE_MAP = [
+        OPENSSL_KEYTYPE_RSA => 'RSA',
+        OPENSSL_KEYTYPE_DSA => 'DSA',
+        OPENSSL_KEYTYPE_DH => 'DH',
+        OPENSSL_KEYTYPE_EC => 'EC',
+    ];
+
     /**
      * @throws CannotSignPayload
      * @throws InvalidKeyProvided
@@ -37,10 +49,7 @@ final protected function createSignature(
             $signature = '';
 
             if (! openssl_sign($payload, $signature, $key, $this->algorithm())) {
-                $error = openssl_error_string();
-                assert(is_string($error));
-
-                throw CannotSignPayload::errorHappened($error);
+                throw CannotSignPayload::errorHappened($this->fullOpenSSLErrorString());
             }
 
             return $signature;
@@ -49,9 +58,6 @@ final protected function createSignature(
         }
     }
 
-    /** @return positive-int */
-    abstract public function minimumBitsLengthForKey(): int;
-
     /**
      * @return resource|OpenSSLAsymmetricKey
      *
@@ -101,26 +107,34 @@ private function getPublicKey(string $pem)
     private function validateKey($key): void
     {
         if (is_bool($key)) {
-            $error = openssl_error_string();
-            assert(is_string($error));
-
-            throw InvalidKeyProvided::cannotBeParsed($error);
+            throw InvalidKeyProvided::cannotBeParsed($this->fullOpenSSLErrorString());
         }
 
         $details = openssl_pkey_get_details($key);
         assert(is_array($details));
 
-        if (! array_key_exists('key', $details) || $details['type'] !== $this->keyType()) {
-            throw InvalidKeyProvided::incompatibleKey();
-        }
-
         assert(array_key_exists('bits', $details));
         assert(is_int($details['bits']));
-        if ($details['bits'] < $this->minimumBitsLengthForKey()) {
-            throw InvalidKeyProvided::tooShort($this->minimumBitsLengthForKey(), $details['bits']);
+        assert(array_key_exists('type', $details));
+        assert(is_int($details['type']));
+
+        $this->guardAgainstIncompatibleKey($details['type'], $details['bits']);
+    }
+
+    private function fullOpenSSLErrorString(): string
+    {
+        $error = '';
+
+        while ($msg = openssl_error_string()) {
+            $error .= PHP_EOL . '* ' . $msg;
         }
+
+        return $error;
     }
 
+    /** @throws InvalidKeyProvided */
+    abstract protected function guardAgainstIncompatibleKey(int $type, int $lengthInBits): void;
+
     /** @param resource|OpenSSLAsymmetricKey $key */
     private function freeKey($key): void
     {
@@ -131,13 +145,6 @@ private function freeKey($key): void
         openssl_free_key($key); // Deprecated and no longer necessary as of PHP >= 8.0
     }
 
-    /**
-     * Returns the type of key to be used to create/verify the signature (using OpenSSL constants)
-     *
-     * @internal
-     */
-    abstract public function keyType(): int;
-
     /**
      * Returns which algorithm to be used to create/verify the signature (using OpenSSL constants)
      *

src/Signer/Rsa.php

@@ -7,6 +7,8 @@
 
 abstract class Rsa extends OpenSSL
 {
+    private const MINIMUM_KEY_LENGTH = 2048;
+
     final public function sign(string $payload, Key $key): string
     {
         return $this->createSignature($key->contents(), $key->passphrase(), $payload);
@@ -17,13 +19,17 @@ final public function verify(string $expected, string $payload, Key $key): bool
         return $this->verifySignature($expected, $payload, $key->contents());
     }
 
-    final public function keyType(): int
+    final protected function guardAgainstIncompatibleKey(int $type, int $lengthInBits): void
     {
-        return OPENSSL_KEYTYPE_RSA;
-    }
+        if ($type !== OPENSSL_KEYTYPE_RSA) {
+            throw InvalidKeyProvided::incompatibleKeyType(
+                self::KEY_TYPE_MAP[OPENSSL_KEYTYPE_RSA],
+                self::KEY_TYPE_MAP[$type],
+            );
+        }
 
-    final public function minimumBitsLengthForKey(): int
-    {
-        return 2048;
+        if ($lengthInBits < self::MINIMUM_KEY_LENGTH) {
+            throw InvalidKeyProvided::tooShort(self::MINIMUM_KEY_LENGTH, $lengthInBits);
+        }
     }
 }

src/Signer/UnsafeEcdsa.php

@@ -27,33 +27,34 @@ final public function sign(string $payload, Key $key): string
     {
         return $this->converter->fromAsn1(
             $this->createSignature($key->contents(), $key->passphrase(), $payload),
-            $this->keyLength()
+            $this->pointLength()
         );
     }
 
     final public function verify(string $expected, string $payload, Key $key): bool
     {
         return $this->verifySignature(
-            $this->converter->toAsn1($expected, $this->keyLength()),
+            $this->converter->toAsn1($expected, $this->pointLength()),
             $payload,
             $key->contents()
         );
     }
 
-    final public function keyType(): int
+    // phpcs:ignore SlevomatCodingStandard.Functions.UnusedParameter.UnusedParameter
+    final protected function guardAgainstIncompatibleKey(int $type, int $lengthInBits): void
     {
-        return OPENSSL_KEYTYPE_EC;
-    }
-
-    final public function minimumBitsLengthForKey(): int
-    {
-        return 1;
+        if ($type !== OPENSSL_KEYTYPE_EC) {
+            throw InvalidKeyProvided::incompatibleKeyType(
+                self::KEY_TYPE_MAP[OPENSSL_KEYTYPE_EC],
+                self::KEY_TYPE_MAP[$type],
+            );
+        }
     }
 
     /**
      * Returns the length of each point in the signature, so that we can calculate and verify R and S points properly
      *
      * @internal
      */
-    abstract public function keyLength(): int;
+    abstract public function pointLength(): int;
 }

src/Signer/UnsafeRsa.php

@@ -18,13 +18,14 @@ final public function verify(string $expected, string $payload, Key $key): bool
         return $this->verifySignature($expected, $payload, $key->contents());
     }
 
-    final public function keyType(): int
+    // phpcs:ignore SlevomatCodingStandard.Functions.UnusedParameter.UnusedParameter
+    final protected function guardAgainstIncompatibleKey(int $type, int $lengthInBits): void
     {
-        return OPENSSL_KEYTYPE_RSA;
-    }
-
-    final public function minimumBitsLengthForKey(): int
-    {
-        return 1;
+        if ($type !== OPENSSL_KEYTYPE_RSA) {
+            throw InvalidKeyProvided::incompatibleKeyType(
+                self::KEY_TYPE_MAP[OPENSSL_KEYTYPE_RSA],
+                self::KEY_TYPE_MAP[$type],
+            );
+        }
     }
 }

test/_keys/Keys.php

@@ -41,8 +41,6 @@ public static function createEcdsaKeys(): void
             'private_ec512'  => Key\InMemory::file(__DIR__ . '/ecdsa/private_ec512.key'),
             'public_ec512'   => Key\InMemory::file(__DIR__ . '/ecdsa/public_ec512.key'),
             'public2_ec512'  => Key\InMemory::file(__DIR__ . '/ecdsa/public2_ec512.key'),
-            'private_short'  => Key\InMemory::file(__DIR__ . '/ecdsa/private_ec160.key'),
-            'public_short'   => Key\InMemory::file(__DIR__ . '/ecdsa/public_ec160.key'),
         ];
     }