[CVE-2023-40180] Add protection against recursive queries (#558)

silverstripe · Oct 16, 2023 · f6d5976 · f6d5976
1 parent 1d360f2
commit f6d5976
Show file tree

Hide file tree

Showing 17 changed files with 362 additions and 14 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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,
     ];