From f6d5976ec4608e51184b0db1ee5b9e9a99d2501c Mon Sep 17 00:00:00 2001 From: Guy Sartorelli <36352093+GuySartorelli@users.noreply.github.com> Date: Mon, 16 Oct 2023 13:45:17 +1300 Subject: [PATCH] [CVE-2023-40180] Add protection against recursive queries (#558) --- _config/schema-global.yml | 2 + src/Controller.php | 6 +- src/QueryHandler/QueryHandler.php | 58 +++- tests/Fake/FakeSiteTree.php | 8 + tests/Schema/IntegrationTest.php | 264 +++++++++++++++++- .../_testCustomComplexityLimit/config.yml | 1 + .../_testCustomComplexityLimit/models.yml | 3 + tests/Schema/_testCustomDepthLimit/config.yml | 7 + tests/Schema/_testCustomDepthLimit/models.yml | 3 + tests/Schema/_testCustomNodeLimit/config.yml | 1 + tests/Schema/_testCustomNodeLimit/models.yml | 3 + .../Schema/_testDefaultDepthLimit/config.yml | 5 + .../Schema/_testDefaultDepthLimit/models.yml | 3 + tests/Schema/_testDefaultNodeLimit/models.yml | 3 + .../_testGlobalRuleNotRemoved/config.yml | 5 + .../_testGlobalRuleNotRemoved/models.yml | 3 + tests/Schema/fixtures.yml | 1 + 17 files changed, 362 insertions(+), 14 deletions(-) create mode 100644 tests/Schema/_testCustomComplexityLimit/config.yml create mode 100644 tests/Schema/_testCustomComplexityLimit/models.yml create mode 100644 tests/Schema/_testCustomDepthLimit/config.yml create mode 100644 tests/Schema/_testCustomDepthLimit/models.yml create mode 100644 tests/Schema/_testCustomNodeLimit/config.yml create mode 100644 tests/Schema/_testCustomNodeLimit/models.yml create mode 100644 tests/Schema/_testDefaultDepthLimit/config.yml create mode 100644 tests/Schema/_testDefaultDepthLimit/models.yml create mode 100644 tests/Schema/_testDefaultNodeLimit/models.yml create mode 100644 tests/Schema/_testGlobalRuleNotRemoved/config.yml create mode 100644 tests/Schema/_testGlobalRuleNotRemoved/models.yml diff --git a/_config/schema-global.yml b/_config/schema-global.yml index 2869aba09..8c73913f1 100644 --- a/_config/schema-global.yml +++ b/_config/schema-global.yml @@ -10,6 +10,8 @@ SilverStripe\GraphQL\Schema\Schema: valueParser: 'SilverStripe\GraphQL\Schema\Resolver\JSONResolver::parseValue' literalParser: 'SilverStripe\GraphQL\Schema\Resolver\JSONResolver::parseLiteral' config: + max_query_depth: 15 + max_query_nodes: 500 resolverStrategy: 'SilverStripe\GraphQL\Schema\Resolver\DefaultResolverStrategy::getResolverMethod' defaultResolver: 'SilverStripe\GraphQL\Schema\Resolver\DefaultResolver::defaultFieldResolver' modelCreators: diff --git a/src/Controller.php b/src/Controller.php index 571c9e50b..084c16387 100644 --- a/src/Controller.php +++ b/src/Controller.php @@ -30,6 +30,7 @@ use SilverStripe\Security\Permission; use SilverStripe\Versioned\Versioned; use BadMethodCallException; +use SilverStripe\Core\ClassInfo; /** * Top level controller for handling graphql requests. @@ -113,8 +114,11 @@ public function index(HTTPRequest $request): HTTPResponse } $handler = $this->getQueryHandler(); $this->applyContext($handler); - $queryDocument = Parser::parse(new Source($query)); $ctx = $handler->getContext(); + if (ClassInfo::hasMethod($handler, 'validateQueryBeforeParsing')) { + $handler->validateQueryBeforeParsing($query, $ctx); + } + $queryDocument = Parser::parse(new Source($query)); $result = $handler->query($graphqlSchema, $query, $variables); // Fire an eventYou diff --git a/src/QueryHandler/QueryHandler.php b/src/QueryHandler/QueryHandler.php index 989b0723a..046205f5e 100644 --- a/src/QueryHandler/QueryHandler.php +++ b/src/QueryHandler/QueryHandler.php @@ -5,14 +5,20 @@ use GraphQL\Error\Error; use GraphQL\Error\SyntaxError; +use GraphQL\Error\UserError; use GraphQL\Executor\ExecutionResult; use GraphQL\GraphQL; use GraphQL\Language\AST\DocumentNode; use GraphQL\Language\AST\NodeKind; +use GraphQL\Language\Lexer; use GraphQL\Language\Parser; use GraphQL\Language\Source; use GraphQL\Language\SourceLocation; +use GraphQL\Language\Token; use GraphQL\Type\Schema as GraphQLSchema; +use GraphQL\Validator\DocumentValidator; +use GraphQL\Validator\Rules\QueryComplexity; +use GraphQL\Validator\Rules\QueryDepth; use SilverStripe\Control\Director; use SilverStripe\Core\Config\Configurable; use SilverStripe\Core\Extensible; @@ -24,6 +30,7 @@ use SilverStripe\GraphQL\PersistedQuery\PersistedQueryProvider; use SilverStripe\GraphQL\Schema\Interfaces\ContextProvider; use SilverStripe\GraphQL\Schema\Schema; +use SilverStripe\GraphQL\Schema\SchemaConfig; use SilverStripe\ORM\ValidationException; /** @@ -99,12 +106,61 @@ public function queryAndReturnResult(GraphQLSchema $schema, $query, ?array $vars } $context = $this->getContext(); $last = function ($schema, $query, $context, $vars) { - return GraphQL::executeQuery($schema, $query, null, $context, $vars); + if (is_string($query)) { + $this->validateQueryBeforeParsing($query, $context); + } + + $validationRules = DocumentValidator::allRules(); + if (isset($context[SchemaConfigProvider::KEY])) { + /** @var SchemaConfig $config */ + $config = $context[SchemaConfigProvider::KEY]; + $maxDepth = $config->get('max_query_depth'); + $maxComplexity = $config->get('max_query_complexity'); + if ($maxDepth) { + $validationRules[QueryDepth::class] = new QueryDepth($maxDepth); + } + if ($maxComplexity) { + $validationRules[QueryComplexity::class] = new QueryComplexity($maxComplexity); + } + } + return GraphQL::executeQuery($schema, $query, null, $context, $vars, null, null, $validationRules); }; return $this->callMiddleware($schema, $query, $context, $vars ?? [], $last); } + /** + * Validate a query before parsing it in case there are issues we can catch early. + */ + public function validateQueryBeforeParsing(string $query, array $context): void + { + if (!isset($context[SchemaConfigProvider::KEY])) { + return; + } + + /** @var SchemaConfig $config */ + $config = $context[SchemaConfigProvider::KEY]; + $maxNodes = $config->get('max_query_nodes'); + if (!$maxNodes) { + return; + } + + $lexer = new Lexer(new Source($query)); + $numNodes = 0; + + // Check how many nodes there are in this query + do { + $next = $lexer->advance(); + if ($next->kind === Token::NAME) { + $numNodes++; + } + } while ($next->kind !== Token::EOF && $numNodes <= $maxNodes); + + // Throw a UserError if there are too many nodes + if ($numNodes > $maxNodes) { + throw new UserError("GraphQL query body must not be longer than $maxNodes nodes."); + } + } /** * get query from persisted id, return null if not found diff --git a/tests/Fake/FakeSiteTree.php b/tests/Fake/FakeSiteTree.php index af4ddbd65..087fff07a 100644 --- a/tests/Fake/FakeSiteTree.php +++ b/tests/Fake/FakeSiteTree.php @@ -15,6 +15,14 @@ class FakeSiteTree extends DataObject implements TestOnly 'Content' => 'HTMLText' ]; + private static $has_one = [ + 'Parent' => self::class, + ]; + + private static $has_many = [ + 'Children' => self::class, + ]; + private static $extensions = [ Versioned::class, ]; diff --git a/tests/Schema/IntegrationTest.php b/tests/Schema/IntegrationTest.php index c8dc2491a..fede9d3d5 100644 --- a/tests/Schema/IntegrationTest.php +++ b/tests/Schema/IntegrationTest.php @@ -36,10 +36,16 @@ use Symfony\Component\Filesystem\Filesystem; use GraphQL\Type\Schema as GraphQLSchema; use Exception; +use GraphQL\Error\Error as GraphQLError; +use GraphQL\Validator\DocumentValidator; +use GraphQL\Validator\Rules\CustomValidationRule; +use GraphQL\Validator\Rules\QueryComplexity; +use GraphQL\Validator\Rules\QueryDepth; +use GraphQL\Validator\ValidationContext; +use ReflectionProperty; class IntegrationTest extends SapphireTest { - protected static $extra_dataobjects = [ FakePage::class, DataObjectFake::class, @@ -1197,6 +1203,220 @@ public function testHtaccess(): void ); } + /** + * @dataProvider provideDefaultDepthLimit + */ + public function testDefaultDepthLimit(int $queryDepth, int $limit) + { + // This global rule should be ignored. + DocumentValidator::addRule(new QueryDepth(1)); + + try { + $schema = $this->createSchema(new TestSchemaBuilder(['_' . __FUNCTION__])); + $this->runDepthLimitTest($queryDepth, $limit, $schema); + } finally { + $this->removeDocumentValidatorRule(QueryDepth::class); + } + } + + public function provideDefaultDepthLimit() + { + return $this->createProviderForComplexityOrDepth(15); + } + + /** + * @dataProvider provideCustomDepthLimit + */ + public function testCustomDepthLimit(int $queryDepth, int $limit) + { + // This global rule should be ignored. + DocumentValidator::addRule(new QueryDepth(1)); + + try { + $schema = $this->createSchema(new TestSchemaBuilder(['_' . __FUNCTION__])); + $this->runDepthLimitTest($queryDepth, $limit, $schema); + } finally { + $this->removeDocumentValidatorRule(QueryDepth::class); + } + } + + public function provideCustomDepthLimit() + { + return $this->createProviderForComplexityOrDepth(25); + } + + /** + * @dataProvider provideCustomComplexityLimit + */ + public function testCustomComplexityLimit(int $queryComplexity, int $limit) + { + // This global rule should be ignored. + DocumentValidator::addRule(new QueryComplexity(1)); + + try { + $schema = $this->createSchema(new TestSchemaBuilder(['_' . __FUNCTION__])); + $this->runComplexityLimitTest($queryComplexity, $limit, $schema); + } finally { + $this->removeDocumentValidatorRule(QueryComplexity::class); + } + } + + public function provideCustomComplexityLimit() + { + return $this->createProviderForComplexityOrDepth(10); + } + + /** + * @dataProvider provideDefaultNodeLimit + */ + public function testDefaultNodeLimit(int $numNodes, int $limit) + { + $schema = $this->createSchema(new TestSchemaBuilder(['_' . __FUNCTION__])); + $this->runNodeLimitTest($numNodes, $limit, $schema); + } + + public function provideDefaultNodeLimit() + { + return $this->createProviderForComplexityOrDepth(500); + } + + /** + * @dataProvider provideCustomNodeLimit + */ + public function testCustomNodeLimit(int $numNodes, int $limit) + { + $schema = $this->createSchema(new TestSchemaBuilder(['_' . __FUNCTION__])); + $this->runNodeLimitTest($numNodes, $limit, $schema); + } + + public function provideCustomNodeLimit() + { + return $this->createProviderForComplexityOrDepth(200); + } + + public function testGlobalRuleNotRemoved() + { + // This global rule should NOT be ignored. + DocumentValidator::addRule(new CustomValidationRule('never-passes', function (ValidationContext $context) { + $context->reportError(new GraphQLError('This is the custom rule')); + return []; + })); + + try { + $schema = $this->createSchema(new TestSchemaBuilder(['_' . __FUNCTION__])); + $result = $this->querySchema($schema, $this->craftRecursiveQuery(15)); + $this->assertFailure($result); + $this->assertErrorMatchingRegex($result, '/^This is the custom rule$/'); + } finally { + $this->removeDocumentValidatorRule('never-passes'); + } + } + + private function removeDocumentValidatorRule(string $ruleName): void + { + $reflectionRules = new ReflectionProperty(DocumentValidator::class, 'rules'); + $reflectionRules->setAccessible(true); + $rules = $reflectionRules->getValue(); + unset($rules[$ruleName]); + $reflectionRules->setValue($rules); + } + + private function createProviderForComplexityOrDepth(int $limit): array + { + return [ + 'far less than limit' => [1, $limit], + 'one less than limit' => [$limit - 1, $limit], + 'exactly at the limit' => [$limit, $limit], + 'one more than limit' => [$limit + 1, $limit], + 'far more than limit' => [$limit + 25, $limit], + ]; + } + + private function runDepthLimitTest(int $queryDepth, int $maxDepth, Schema $schema): void + { + $result = $this->querySchema($schema, $this->craftRecursiveQuery($queryDepth)); + if ($queryDepth > $maxDepth) { + $this->assertFailure($result); + $this->assertErrorMatchingRegex($result, '/^Max query depth should be ' . $maxDepth . ' but got ' . $queryDepth . '\.$/'); + } else { + // Note that the depth limit is based on the depth of the QUERY, not of the RESULTS, so all we really care about + // is that the query was successful, not what the results were. + $this->assertSuccess($result); + } + } + + private function runComplexityLimitTest(int $queryComplexity, int $maxComplexity, Schema $schema): void + { + $result = $this->querySchema($schema, $this->craftComplexQuery($queryComplexity)); + if ($queryComplexity > $maxComplexity) { + $this->assertFailure($result); + $this->assertErrorMatchingRegex($result, '/^Max query complexity should be ' . $maxComplexity . ' but got ' . $queryComplexity . '\.$/'); + } else { + // Note that the complexity limit is based on the complexity of the QUERY, not of the RESULTS, so all we really care about + // is that the query was successful, not what the results were. + $this->assertSuccess($result); + } + } + + private function runNodeLimitTest(int $queryNodeCount, int $maxNodes, Schema $schema): void + { + $result = $this->querySchema($schema, $this->craftComplexQuery($queryNodeCount - 1)); + if ($queryNodeCount > $maxNodes) { + $this->assertFailure($result); + $this->assertErrorMatchingRegex($result, '/^GraphQL query body must not be longer than ' . $maxNodes . ' nodes\.$/'); + } else { + // Note that the complexity limit is based on the complexity of the QUERY, not of the RESULTS, so all we really care about + // is that the query was successful, not what the results were. + $this->assertSuccess($result); + } + } + + private function craftRecursiveQuery(int $queryDepth): string + { + $query = 'query{ readFakeSiteTrees { nodes {'; + + for ($i = 0; $i < $queryDepth; $i++) { + if ($i % 3 === 0) { + $query .= 'id title'; + } elseif ($i % 3 === 1) { + $query .= ' parent {'; + } elseif ($i % 3 === 2) { + if ($i === $queryDepth - 1) { + $query .= 'id title'; + } else { + $query .= 'id title children { nodes {'; + } + } + } + + $endsWith = strrpos($query, 'id title') === strlen($query) - strlen('id title'); + $query .= $endsWith ? '' : 'id title'; + // Add all of the closing brackets + $numChars = array_count_values(str_split($query)); + for ($i = 0; $i < $numChars['{']; $i++) { + $query .= '}'; + } + + return $query; + } + + private function craftComplexQuery(int $queryComplexity): string + { + $query = 'query{ readOneFakeSiteTree { id'; + + // skip the first two complexity, because those are taken up by "readOneFakeSiteTree { id" above + for ($i = 0; $i < $queryComplexity - 2; $i++) { + $query .= ' id'; + } + // Add all of the closing brackets + $numChars = array_count_values(str_split($query)); + for ($i = 0; $i < $numChars['{']; $i++) { + $query .= '}'; + } + + return $query; + } + /** * @param TestSchemaBuilder $factory * @return Schema @@ -1254,18 +1474,14 @@ private function clean() private function assertSuccess(array $result) { $errors = $result['errors'] ?? []; - if (!empty($errors)) { - $this->fail('Failed to assert successful query. Got errors: ' . json_encode($errors, JSON_PRETTY_PRINT)); - } + $this->assertEmpty($errors, 'Failed to assert successful query. Got errors: ' . json_encode($errors, JSON_PRETTY_PRINT)); $error = $result['error'] ?? null; - if ($error) { - $this->fail('Failed to assert successful query. Got error: ' . $error); - } + $this->assertFalse((bool) $error, 'Failed to assert successful query. Got error: ' . $error); } private function assertFailure(array $result) { - $errors = $result['errors'] ?? []; + $errors = $result['errors'] ?? $result['error'] ?? []; if (empty($errors)) { $this->fail('Failed to assert that query was not successful'); } @@ -1273,14 +1489,38 @@ private function assertFailure(array $result) private function assertMissingField(array $result, string $fieldName) { + $this->assertErrorMatchingRegex( + $result, + '/^Cannot query field "' . $fieldName . '"/', + 'Failed to assert that result was missing field "' . $fieldName . '"' + ); + } + + private function assertErrorMatchingRegex( + array $result, + string $errorRegex, + string $message = 'Failed to assert that expected error was present.' + ) { $errors = $result['errors'] ?? []; + if (isset($result['error'])) { + $errors[] = ['message' => $result['error']]; + } + $errorMessages = []; + $foundError = false; foreach ($errors as $error) { - if (preg_match('/^Cannot query field "' . $fieldName . '"/', $error['message'] ?? '')) { - return; + if (!isset($error['message'])) { + continue; } + if (preg_match($errorRegex, $error['message'])) { + $foundError = true; + break; + } + $errorMessages[] = '"' . $error['message'] . '"'; } - - $this->fail('Failed to assert that result was missing field "' . $fieldName . '"'); + $this->assertTrue( + $foundError, + $message . ' Regex was: ' . $errorRegex . ', Errors were: ' . implode(', ', $errorMessages) + ); } private function assertResults(array $expected, array $actual) diff --git a/tests/Schema/_testCustomComplexityLimit/config.yml b/tests/Schema/_testCustomComplexityLimit/config.yml new file mode 100644 index 000000000..953230f4b --- /dev/null +++ b/tests/Schema/_testCustomComplexityLimit/config.yml @@ -0,0 +1 @@ +max_query_complexity: 10 diff --git a/tests/Schema/_testCustomComplexityLimit/models.yml b/tests/Schema/_testCustomComplexityLimit/models.yml new file mode 100644 index 000000000..18ff2d22e --- /dev/null +++ b/tests/Schema/_testCustomComplexityLimit/models.yml @@ -0,0 +1,3 @@ +SilverStripe\GraphQL\Tests\Fake\FakeSiteTree: + fields: '*' + operations: '*' diff --git a/tests/Schema/_testCustomDepthLimit/config.yml b/tests/Schema/_testCustomDepthLimit/config.yml new file mode 100644 index 000000000..abdba25bb --- /dev/null +++ b/tests/Schema/_testCustomDepthLimit/config.yml @@ -0,0 +1,7 @@ +# this just makes it easier to craft the recursive query programatically +modelConfig: + DataObject: + nested_query_plugins: + paginateList: true + +max_query_depth: 25 diff --git a/tests/Schema/_testCustomDepthLimit/models.yml b/tests/Schema/_testCustomDepthLimit/models.yml new file mode 100644 index 000000000..18ff2d22e --- /dev/null +++ b/tests/Schema/_testCustomDepthLimit/models.yml @@ -0,0 +1,3 @@ +SilverStripe\GraphQL\Tests\Fake\FakeSiteTree: + fields: '*' + operations: '*' diff --git a/tests/Schema/_testCustomNodeLimit/config.yml b/tests/Schema/_testCustomNodeLimit/config.yml new file mode 100644 index 000000000..ea1184a4e --- /dev/null +++ b/tests/Schema/_testCustomNodeLimit/config.yml @@ -0,0 +1 @@ +max_query_nodes: 200 diff --git a/tests/Schema/_testCustomNodeLimit/models.yml b/tests/Schema/_testCustomNodeLimit/models.yml new file mode 100644 index 000000000..18ff2d22e --- /dev/null +++ b/tests/Schema/_testCustomNodeLimit/models.yml @@ -0,0 +1,3 @@ +SilverStripe\GraphQL\Tests\Fake\FakeSiteTree: + fields: '*' + operations: '*' diff --git a/tests/Schema/_testDefaultDepthLimit/config.yml b/tests/Schema/_testDefaultDepthLimit/config.yml new file mode 100644 index 000000000..98f4298c9 --- /dev/null +++ b/tests/Schema/_testDefaultDepthLimit/config.yml @@ -0,0 +1,5 @@ +# this just makes it easier to craft the recursive query programatically +modelConfig: + DataObject: + nested_query_plugins: + paginateList: true diff --git a/tests/Schema/_testDefaultDepthLimit/models.yml b/tests/Schema/_testDefaultDepthLimit/models.yml new file mode 100644 index 000000000..18ff2d22e --- /dev/null +++ b/tests/Schema/_testDefaultDepthLimit/models.yml @@ -0,0 +1,3 @@ +SilverStripe\GraphQL\Tests\Fake\FakeSiteTree: + fields: '*' + operations: '*' diff --git a/tests/Schema/_testDefaultNodeLimit/models.yml b/tests/Schema/_testDefaultNodeLimit/models.yml new file mode 100644 index 000000000..18ff2d22e --- /dev/null +++ b/tests/Schema/_testDefaultNodeLimit/models.yml @@ -0,0 +1,3 @@ +SilverStripe\GraphQL\Tests\Fake\FakeSiteTree: + fields: '*' + operations: '*' diff --git a/tests/Schema/_testGlobalRuleNotRemoved/config.yml b/tests/Schema/_testGlobalRuleNotRemoved/config.yml new file mode 100644 index 000000000..98f4298c9 --- /dev/null +++ b/tests/Schema/_testGlobalRuleNotRemoved/config.yml @@ -0,0 +1,5 @@ +# this just makes it easier to craft the recursive query programatically +modelConfig: + DataObject: + nested_query_plugins: + paginateList: true diff --git a/tests/Schema/_testGlobalRuleNotRemoved/models.yml b/tests/Schema/_testGlobalRuleNotRemoved/models.yml new file mode 100644 index 000000000..18ff2d22e --- /dev/null +++ b/tests/Schema/_testGlobalRuleNotRemoved/models.yml @@ -0,0 +1,3 @@ +SilverStripe\GraphQL\Tests\Fake\FakeSiteTree: + fields: '*' + operations: '*' diff --git a/tests/Schema/fixtures.yml b/tests/Schema/fixtures.yml index c44b5e9fa..a0c6276d6 100644 --- a/tests/Schema/fixtures.yml +++ b/tests/Schema/fixtures.yml @@ -3,6 +3,7 @@ SilverStripe\Security\Member: FirstName: Author1 author2: FirstName: Author2 + SilverStripe\GraphQL\Tests\Fake\FakeProductPage: productPage1: Title: This is product page one