Repository: thephpleague/oauth2-server (php)

Issue:
Change CryptKey to use PHPSecLib for key validation
As the title says :)

Pull request:
Validate key with openssl
Added key validation using phpseclib. RSA and EC keys are supported. In the future, I would like to make separate types for Public and Private keys to check this aspect as well.

Fixes #1214



Patch:
diff --git a/src/CryptKey.php b/src/CryptKey.php
index e2077f70..be33ce80 100644
--- a/src/CryptKey.php
+++ b/src/CryptKey.php
@@ -16,9 +16,12 @@ use RuntimeException;
 
 class CryptKey
 {
+    /** @deprecated left for backward compatibility check */
     const RSA_KEY_PATTERN =
         '/^(-----BEGIN (RSA )?(PUBLIC|PRIVATE) KEY-----)\R.*(-----END (RSA )?(PUBLIC|PRIVATE) KEY-----)\R?$/s';
 
+    private const FILE_PREFIX = 'file://';
+
     /**
      * @var string
      */
@@ -36,36 +39,43 @@ class CryptKey
      */
     public function __construct($keyPath, $passPhrase = null, $keyPermissionsCheck = true)
     {
-        if ($rsaMatch = \preg_match(static::RSA_KEY_PATTERN, $keyPath)) {
-            $keyPath = $this->saveKeyToFile($keyPath);
-        } elseif ($rsaMatch === false) {
-            throw new \RuntimeException(
-                \sprintf('PCRE error [%d] encountered during key match attempt', \preg_last_error())
-            );
-        }
+        $this->passPhrase = $passPhrase;
 
-        if (\strpos($keyPath, 'file://') !== 0) {
-            $keyPath = 'file://' . $keyPath;
-        }
+        if (\is_file($keyPath)) {
+            if (\strpos($keyPath, self::FILE_PREFIX) !== 0) {
+                $keyPath = self::FILE_PREFIX . $keyPath;
+            }
 
-        if (!\file_exists($keyPath) || !\is_readable($keyPath)) {
-            throw new LogicException(\sprintf('Key path "%s" does not exist or is not readable', $keyPath));
+            if (!\is_readable($keyPath)) {
+                throw new LogicException(\sprintf('Key path "%s" does not exist or is not readable', $keyPath));
+            }
+            $isFileKey = true;
+            $contents = \file_get_contents($keyPath);
+            $this->keyPath = $keyPath;
+        } else {
+            $isFileKey = false;
+            $contents = $keyPath;
+            $this->keyPath = $this->saveKeyToFile($keyPath);
         }
 
         if ($keyPermissionsCheck === true) {
             // Verify the permissions of the key
-            $keyPathPerms = \decoct(\fileperms($keyPath) & 0777);
+            $keyPathPerms = \decoct(\fileperms($this->keyPath) & 0777);
             if (\in_array($keyPathPerms, ['400', '440', '600', '640', '660'], true) === false) {
-                \trigger_error(\sprintf(
-                    'Key file "%s" permissions are not correct, recommend changing to 600 or 660 instead of %s',
-                    $keyPath,
-                    $keyPathPerms
-                ), E_USER_NOTICE);
+                \trigger_error(
+                    \sprintf(
+                        'Key file "%s" permissions are not correct, recommend changing to 600 or 660 instead of %s',
+                        $this->keyPath,
+                        $keyPathPerms
+                    ),
+                    E_USER_NOTICE
+                );
             }
         }
 
-        $this->keyPath = $keyPath;
-        $this->passPhrase = $passPhrase;
+        if (!$this->isValidKey($contents, $this->passPhrase ?? '')) {
+            throw new LogicException('Unable to read key' . ($isFileKey ? " from file $keyPath" : ''));
+        }
     }
 
     /**
@@ -81,7 +91,7 @@ class CryptKey
         $keyPath = $tmpDir . '/' . \sha1($key) . '.key';
 
         if (\file_exists($keyPath)) {
-            return 'file://' . $keyPath;
+            return self::FILE_PREFIX . $keyPath;
         }
 
         if (\file_put_contents($keyPath, $key) === false) {
@@ -96,7 +106,30 @@ class CryptKey
             // @codeCoverageIgnoreEnd
         }
 
-        return 'file://' . $keyPath;
+        return self::FILE_PREFIX . $keyPath;
+    }
+
+    /**
+     * Validate key contents.
+     *
+     * @param string $contents
+     * @param string $passPhrase
+     *
+     * @return bool
+     */
+    private function isValidKey($contents, $passPhrase)
+    {
+        $pkey = \openssl_pkey_get_private($contents, $passPhrase) ?: \openssl_pkey_get_public($contents);
+        if ($pkey === false) {
+            return false;
+        }
+        $details = \openssl_pkey_get_details($pkey);
+
+        return $details !== false && \in_array(
+            $details['type'] ?? -1,
+            [OPENSSL_KEYTYPE_RSA, OPENSSL_KEYTYPE_EC],
+            true
+        );
     }
 
     /**


Test patch:
diff --git a/tests/Utils/CryptKeyTest.php b/tests/Utils/CryptKeyTest.php
index 667daaf2..8514a9a3 100644
--- a/tests/Utils/CryptKeyTest.php
+++ b/tests/Utils/CryptKeyTest.php
@@ -33,10 +33,7 @@ class CryptKeyTest extends TestCase
 
         $key = new CryptKey($keyContent);
 
-        $this->assertEquals(
-            'file://' . \sys_get_temp_dir() . '/' . \sha1($keyContent) . '.key',
-            $key->getKeyPath()
-        );
+        $this->assertEquals(self::generateKeyPath($keyContent), $key->getKeyPath());
 
         $keyContent = \file_get_contents(__DIR__ . '/../Stubs/private.key.crlf');
 
@@ -46,24 +43,92 @@ class CryptKeyTest extends TestCase
 
         $key = new CryptKey($keyContent);
 
-        $this->assertEquals(
-            'file://' . \sys_get_temp_dir() . '/' . \sha1($keyContent) . '.key',
-            $key->getKeyPath()
-        );
+        $this->assertEquals(self::generateKeyPath($keyContent), $key->getKeyPath());
+    }
+
+    public function testUnsupportedKeyType()
+    {
+        $this->expectException(\LogicException::class);
+        $this->expectExceptionMessage('Unable to read key');
+
+        try {
+            // Create the keypair
+            $res = \openssl_pkey_new([
+                'digest_alg' => 'sha512',
+                'private_key_bits' => 2048,
+                'private_key_type' => OPENSSL_KEYTYPE_DSA,
+            ]);
+            // Get private key
+            \openssl_pkey_export($res, $keyContent, 'mystrongpassword');
+            $path = self::generateKeyPath($keyContent);
+
+            new CryptKey($keyContent, 'mystrongpassword');
+        } finally {
+            if (isset($path)) {
+                @\unlink($path);
+            }
+        }
+    }
+
+    public function testECKeyType()
+    {
+        try {
+            // Create the keypair
+            $res = \openssl_pkey_new([
+                'digest_alg' => 'sha512',
+                'curve_name' => 'prime256v1',
+                'private_key_type' => OPENSSL_KEYTYPE_EC,
+            ]);
+            // Get private key
+            \openssl_pkey_export($res, $keyContent, 'mystrongpassword');
+
+            $key = new CryptKey($keyContent, 'mystrongpassword');
+            $path = self::generateKeyPath($keyContent);
+
+            $this->assertEquals($path, $key->getKeyPath());
+            $this->assertEquals('mystrongpassword', $key->getPassPhrase());
+        } catch (\Throwable $e) {
+            $this->fail('The EC key was not created');
+        } finally {
+            if (isset($path)) {
+                @\unlink($path);
+            }
+        }
+    }
+
+    public function testRSAKeyType()
+    {
+        try {
+            // Create the keypair
+            $res = \openssl_pkey_new([
+                 'digest_alg' => 'sha512',
+                 'private_key_bits' => 2048,
+                 'private_key_type' => OPENSSL_KEYTYPE_RSA,
+            ]);
+            // Get private key
+            \openssl_pkey_export($res, $keyContent, 'mystrongpassword');
+
+            $key = new CryptKey($keyContent, 'mystrongpassword');
+            $path = self::generateKeyPath($keyContent);
+
+            $this->assertEquals($path, $key->getKeyPath());
+            $this->assertEquals('mystrongpassword', $key->getPassPhrase());
+        } catch (\Throwable $e) {
+            $this->fail('The RSA key was not created');
+        } finally {
+            if (isset($path)) {
+                @\unlink($path);
+            }
+        }
     }
 
     /**
-     * Test whether we get a RuntimeException if a PCRE error is encountered.
+     * @param string $keyContent
      *
-     * @link https://www.php.net/manual/en/function.preg-last-error.php
+     * @return string
      */
-    public function testPcreErrorExceptions()
+    private static function generateKeyPath($keyContent)
     {
-        $this->expectException(\RuntimeException::class);
-        $this->expectExceptionMessageMatches('/^PCRE error/');
-
-        new class('foobar foobar foobar') extends CryptKey {
-            const RSA_KEY_PATTERN = '/(?:\D+|<\d+>)*[!?]/';
-        };
+        return 'file://' . \sys_get_temp_dir() . '/' . \sha1($keyContent) . '.key';
     }
 }