From 91ced6f6408eb0ef314a59acc8f24045cda5b56c Mon Sep 17 00:00:00 2001 From: Jan-Hendrik Willms Date: Wed, 7 Aug 2024 16:33:41 +0200 Subject: improve oauth1 request validation, fixes #4463 --- db/migrations/6.0.13_add_oauth1_nonces_table.php | 27 +++++++ lib/classes/OAuth1.php | 96 ++++++++++++++++-------- tests/unit/lib/classes/OAuth1Test.php | 19 ++++- 3 files changed, 107 insertions(+), 35 deletions(-) create mode 100644 db/migrations/6.0.13_add_oauth1_nonces_table.php diff --git a/db/migrations/6.0.13_add_oauth1_nonces_table.php b/db/migrations/6.0.13_add_oauth1_nonces_table.php new file mode 100644 index 0000000..a20ffb0 --- /dev/null +++ b/db/migrations/6.0.13_add_oauth1_nonces_table.php @@ -0,0 +1,27 @@ +exec($query); + } + + protected function down() + { + $query = "DROP TABLE `oauth1_nonces`"; + DBManager::get()->exec($query); + } +}; diff --git a/lib/classes/OAuth1.php b/lib/classes/OAuth1.php index 1695f9f..413bd63 100644 --- a/lib/classes/OAuth1.php +++ b/lib/classes/OAuth1.php @@ -1,6 +1,7 @@ 0) { - throw new RuntimeException('Missing oauth parameters ' . implode(', ', $missing)); + if (!self::checkNonce($parameters['oauth_nonce'], $parameters['oauth_timestamp'])) { + return false; } + return self::verifySignature($request, $consumerSecret, $tokenSecret); + } + + /** + * Verifies an oauth request. + * + * @throws RuntimeException if any necessary oauth parameter is missing + */ + public static function verifySignature( + Request $request, + string $consumerSecret, + string $tokenSecret + ): bool { + $parameters = self::extractParameters($request); + $signatureToVerify = $parameters['oauth_signature']; unset($parameters['oauth_signature']); @@ -80,9 +91,19 @@ final class OAuth1 /** * Extracts the oauth parameters either from the Authorization header or * from the query string. + * + * @throws RuntimeException if any necessary oauth parameter is missing */ - public static function extractParameters(Request $request): array - { + public static function extractParameters( + Request $request, + array $required = [ + 'oauth_consumer_key', + 'oauth_nonce', + 'oauth_signature', + 'oauth_signature_method', + 'oauth_timestamp', + ] + ): array { $parameters = $request->getQueryParams(); $header = $request->getHeaderLine('Authorization'); @@ -93,10 +114,15 @@ final class OAuth1 foreach ($chunks as $chunk) { [$key, $value] = explode('=', $chunk, 2); $value = trim($value, '"'); - $parameters[$key] = self::urldecode($value); + $parameters[$key] = rawurldecode($value); } } + $missing = array_diff($required, array_keys($parameters)); + if (count($missing) > 0) { + throw new RuntimeException('Missing oauth parameters ' . implode(', ', $missing)); + } + return $parameters; } @@ -118,7 +144,7 @@ final class OAuth1 ksort($parameters); return implode('&', array_map( - self::urlencode(...), + rawurlencode(...), [ strtoupper($request->getMethod()), (string) $request->getUri()->withQuery(''), @@ -136,9 +162,9 @@ final class OAuth1 { $method = strtolower($method); return match ($method) { - 'hmac-sha1', 'sha1' => base64_encode(hash_hmac('sha1', $text, $key, true)), - 'hmac-sha256', 'sha256' => base64_encode(hash_hmac('sha256', $text, $key, true)), - 'hmac-sha512', 'sha512' => base64_encode(hash_hmac('sha512', $text, $key, true)), + 'hmac-sha1' => base64_encode(hash_hmac('sha1', $text, $key, true)), + 'hmac-sha256' => base64_encode(hash_hmac('sha256', $text, $key, true)), + 'hmac-sha512' => base64_encode(hash_hmac('sha512', $text, $key, true)), 'plaintext' => $key, @@ -147,21 +173,29 @@ final class OAuth1 } /** - * Urlencodes a given input + * Checks whether the combination of nonce and timestamp has already been + * used. If not, the combination will be stored for future checks. */ - public static function urldecode(string $input): string + public static function checkNonce(string $nonce, int $timestamp): bool { - return rawurldecode($input); - } + // Remove all outdated entries from nonces table + $query = "DELETE FROM `oauth1_nonces` + WHERE `timestamp` < NOW() - INTERVAL 5 MINUTE"; + DBManager::get()->exec($query); + + // Query if the combination of nonce and timestamp has already been used + $query = "SELECT 1 + FROM `oauth1_nonces` + WHERE `timestamp` = FROM_UNIXTIME(?) + AND `nonce` = ?"; + if (DBManager::get()->fetchColumn($query, [$nonce, $timestamp])) { + return false; + } - /** - * Urldecodes a given input - */ - public static function urlencode(string $input): string - { - $encoded = rawurlencode($input); - return str_starts_with($encoded, '/%7E') - ? str_replace('/%7E', '/~', $encoded) - : $encoded; + // Store combination of nonce and timestamp + $query = "INSERT INTO `oauth1_nonces` VALUES (FROM_UNIXTIME(?), ?)"; + DBManager::get()->execute($query, [$timestamp, $nonce]); + + return true; } } diff --git a/tests/unit/lib/classes/OAuth1Test.php b/tests/unit/lib/classes/OAuth1Test.php index a3fa8bd..f31340b 100644 --- a/tests/unit/lib/classes/OAuth1Test.php +++ b/tests/unit/lib/classes/OAuth1Test.php @@ -40,12 +40,12 @@ final class OAuth1Test extends \Codeception\Test\Unit } /** - * @covers OAuth1::verifyRequest + * @covers OAuth1::verifySignature */ public function testVerifyingARequest(): void { $this->assertTrue( - OAuth1::verifyRequest( + OAuth1::verifySignature( $this->getDefaultTestRequest(['oauth_signature' => 'tR3+Ty81lMeYAr/Fid0kMTYa/WM=']), 'kd94hf93k423kf44', 'pfkkdhi9sl3r4s00' @@ -54,7 +54,7 @@ final class OAuth1Test extends \Codeception\Test\Unit } /** - * @covers OAuth1::verifyRequest + * @covers OAuth1::verifySignature * @covers OAuth1::extractParameters */ public function testVerifyingARequestFromAuthorizationHeader(): void @@ -75,7 +75,7 @@ final class OAuth1Test extends \Codeception\Test\Unit ); $this->assertTrue( - OAuth1::verifyRequest( + OAuth1::verifySignature( $request, 'kd94hf93k423kf44', 'pfkkdhi9sl3r4s00' @@ -83,6 +83,17 @@ final class OAuth1Test extends \Codeception\Test\Unit ); } + public function testOutdatedTimestampOfRequest(): void + { + $this->assertFalse( + OAuth1::verifyRequest( + $this->getDefaultTestRequest(['oauth_signature' => 'tR3+Ty81lMeYAr/Fid0kMTYa/WM=']), + 'kd94hf93k423kf44', + 'pfkkdhi9sl3r4s00' + ) + ); + } + private function getTestRequest(): ServerRequestInterface { $psr17Factory = new Psr17Factory(); -- cgit v1.0