[CVE-2019-12437] Cross Site Request Forgery (CSRF) Protection Bypass …

…in GraphQL
silverstripe · Jun 10, 2019 · db28f30 · db28f30
1 parent c58711c
commit db28f30
Show file tree

Hide file tree

Showing 3 changed files with 88 additions and 1 deletion.
diff --git a/README.md b/README.md
@@ -1918,6 +1918,9 @@ resources are accessed.
 The `MemberAuthenticator` class is configured as the default option for authentication,
 and will attempt to use the current CMS `Member` session for authentication context.
 
+**If you are using the default session-based authentication, please be sure that you have
+the [CSRF Middleware](#csrf-tokens-required-for-mutations) enabled. (It is by default).**
+
 ### HTTP basic authentication
 
 Silverstripe has built in support for [HTTP basic authentication](https://en.wikipedia.org/wiki/Basic_access_authentication).
@@ -1927,6 +1930,9 @@ authenticator because GraphQL needs to use the successfully authenticated member
 for CMS permission filtering, whereas the global `BasicAuth` does not log the
 member in or use it for model security.
 
+When using HTTP basic authentication, you can feel free to remove the [CSRF Middleware](#csrf-tokens-required-for-mutations),
+as it just adds unnecessary overhead to the request.
+
 #### In GraphiQL
 
 If you want to add basic authentication support to your GraphQL requests you can
@@ -1993,12 +1999,19 @@ the `SecurityToken` API, using `SecurityToken::inst()->getValue()`.
 
 Queries do not require CSRF tokens.
 
+### Disabling CSRF protection (for token-based authentication only)
+
+If you are using HTTP basic authentication or a token-based system like OAuth or [JWT](https://github.com/Firesphere/silverstripe-graphql-jwt),
+you will want to remove the CSRF protection, as it just adds unnecessary overhead. You can do this by setting
+the middleware to `false`.
+
 ```yaml
   SilverStripe\GraphQL\Manager:
     properties:
       Middlewares:
         CSRFMiddleware: false
 ```
+
 ## Cross-Origin Resource Sharing (CORS)
 
 By default [CORS](https://developer.mozilla.org/en-US/docs/Web/HTTP/Access_control_CORS) is disabled in the GraphQL Server. This can be easily enabled via YAML:

diff --git a/src/Middleware/CSRFMiddleware.php b/src/Middleware/CSRFMiddleware.php
@@ -2,7 +2,11 @@
 
 namespace SilverStripe\GraphQL\Middleware;
 
+use GraphQL\Language\Parser;
+use GraphQL\Language\Source;
 use GraphQL\Schema;
+use GraphQL\Language\AST\NodeKind;
+use SilverStripe\GraphQL\Manager;
 use SilverStripe\GraphQL\Middleware\QueryMiddleware;
 use SilverStripe\Security\SecurityToken;
 use Exception;
@@ -11,7 +15,7 @@ class CSRFMiddleware implements QueryMiddleware
 {
     public function process(Schema $schema, $query, $context, $params, callable $next)
     {
-        if (preg_match('/^\s*mutation/', $query)) {
+        if ($this->isMutation($query)) {
             if (empty($context['token'])) {
                 throw new Exception('Mutations must provide a CSRF token in the X-CSRF-TOKEN header');
             }
@@ -24,4 +28,39 @@ public function process(Schema $schema, $query, $context, $params, callable $nex
 
         return $next($schema, $query, $context, $params);
     }
+
+    /**
+     * @param string $query
+     * @return bool
+     */
+    protected function isMutation($query)
+    {
+        // Simple string matching as a first check to prevent unnecessary static analysis
+        if (stristr($query, Manager::MUTATION_ROOT) === false) {
+            return false;
+        }
+
+        // If "mutation" is the first expression in the query, then it's a mutation.
+        if (preg_match('/^\s*' . preg_quote(Manager::MUTATION_ROOT, '/') . '/', $query)) {
+            return true;
+        }
+
+        // Otherwise, bring in the big guns.
+        $document = Parser::parse(new Source($query ?: 'GraphQL'));
+        $defs = $document->definitions;
+        foreach ($defs as $statement) {
+            $options = [
+                NodeKind::OPERATION_DEFINITION,
+                NodeKind::OPERATION_TYPE_DEFINITION
+            ];
+            if (!in_array($statement->kind, $options, true)) {
+                continue;
+            }
+            if ($statement->operation === Manager::MUTATION_ROOT) {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }
diff --git a/tests/Middleware/CSRFMiddlewareTest.php b/tests/Middleware/CSRFMiddlewareTest.php
@@ -30,7 +30,42 @@ public function testItThrowsIfNoTokenIsProvided()
             ' mutation someMutation { tester }'
         );
         $this->assertNotEquals('resolved', $result);
+        $result = $this->simulateMiddlewareProcess(
+            new CSRFMiddleware(),
+            ' mutation someMutation { tester }'
+        );
+        $this->assertNotEquals('resolved', $result);
+        $graphql = <<<GRAPHQL
+mutation MyMutation(\$SomeArg:string!) {
+    someMutation(Foo:\$SomeArg) {
+        tester
     }
+}
+GRAPHQL;
+
+        $result = $this->simulateMiddlewareProcess(
+            new CSRFMiddleware(),
+            $graphql
+        );
+        $this->assertNotEquals('resolved', $result);
+        $graphql = <<<GRAPHQL
+fragment myFragment on File {
+    id
+    width
+}
+mutation someMutation {
+        tester
+    }
+}
+GRAPHQL;
+
+        $result = $this->simulateMiddlewareProcess(
+            new CSRFMiddleware(),
+            $graphql
+        );
+        $this->assertNotEquals('resolved', $result);
+    }
+
 
     public function testItThrowsIfTokenIsInvalid()
     {