From bf1883db538c97b076801a60677733816cb3cfb7 Mon Sep 17 00:00:00 2001 From: Rikki Schulte Date: Sat, 6 Jun 2020 21:17:58 -0400 Subject: [PATCH] Merge pull request from GHSA-4852-vrh7-28rf * fix: security vulnerability allowing XSS reflection with user input * fix: update docs, replace santize-html and sanitizeUrl with xss module --- README.md | 118 ++++++++++------ SECURITY.md | 132 ++++++++++++++++++ packages/graphql-playground-html/README.md | 2 + packages/graphql-playground-html/package.json | 4 +- .../src/render-playground-page.ts | 71 +++++++--- .../README.md | 15 ++ .../examples/basic/index.js | 35 +++-- .../README.md | 57 ++++++++ .../README.md | 16 +++ .../README.md | 46 ++++++ .../examples/basic/README.md | 5 + packages/graphql-playground-react/README.md | 23 +-- scripts/build.sh | 2 +- yarn.lock | 13 ++ 14 files changed, 447 insertions(+), 92 deletions(-) create mode 100644 SECURITY.md create mode 100644 packages/graphql-playground-middleware-hapi/README.md create mode 100644 packages/graphql-playground-middleware-lambda/README.md diff --git a/README.md b/README.md index 49d098e1d..3dd18e51c 100644 --- a/README.md +++ b/README.md @@ -1,6 +1,8 @@ +> **SECURITY WARNING:** This `graphql-playground-html` and [all of it's middleware dependents](#impacted-packages) in this repository **had a severe XSS Reflection attack vulnerability to unsanitized user input** until version `graphql-playground-html@1.6.20`. Impacted are any and all **user-defined** input to `renderPlaygroundPage()`, `koaPlayground()`,`expressPlayground()`, `koaPlayground()`, or `lambdaPlayground()`. If you used static values, such as `graphql-playground-electron` does in [it's webpack config](https://github.com/prisma-labs/graphql-playground/blob/master/packages/graphql-playground-electron/webpack.config.build.js#L16), you were not vulnerable to the attack. [More Details](./SECURITY.md) +

-[![npm version](https://badge.fury.io/js/graphql-playground-react.svg)](https://badge.fury.io/js/graphql-playground-react) +[![npm version](https://badge.fury.io/js/graphql-playground-react.svg)](https://badge.fury.io/js/graphql-playground-react) [![prisma-labs](https://circleci.com/gh/prisma-labs/graphql-playground.svg?style=shield)](https://circleci.com/gh/prisma-labs/graphql-playground) **Future of this repository**: see [the announcement issue](https://github.com/prisma-labs/graphql-playground/issues/1143) for details. @@ -25,6 +27,32 @@ $ brew cask install graphql-playground - ⚙ GraphQL Config support with multiple Projects & Endpoints - 🚥 Apollo Tracing support +## Security Details + +**NOTE: only _unsanitized user input_ to the functions in these packages is vulnerable** to the recently reported XSS Reflection attack. + +### Impact + +The only reason this vulnerability exists is because we are using template strings in `renderPlaygroundPage()` with potentially user defined variables. This allows an attacker to inject html and javascript into a page on execution. + +Common examples may be user-defined path parameters, query string, unsanitized UI provided values in database, etc that are used to build template strings or passed directly to a `renderPlaygroundPage()` or the matching middleware function equivalent. + +### Impacted Packages + +**All versions of these packages are impacted until the ones specified below**, which are now safe for user defined input: + +- `graphql-playground-html`: **☔ safe** @ `1.6.20` +- `graphql-playground-express` **☔ safe** @ `1.7.15` +- `graphql-playground-koa` **☔ safe** @ `1.6.14` +- `graphql-playground-hapi` **☔ safe** @ `1.6.12` +- `graphql-playground-lambda` **☔ safe** @ `1.7.16` +- `graphql-playground-electron` has always been **☔ safe** from XSS attacks! This is because configuration is statically defined [it's webpack config](https://github.com/prisma-labs/graphql-playground/blob/master/packages/graphql-playground-electron/webpack.config.build.js#L16) +- `graphql-playground-react` is safe because it does not use `renderPlaygroundPage()` anywhere, and thus is not susceptible to template string XSS reflection attacks. + +### More Information + +See the [security docs](./SECURITY.md) for more details on how your implementation might be impacted by this vulnerability. It contains safe examples, unsafe examples, workarounds, and more details. + ## FAQ ### How is this different from [GraphiQL](https://github.com/graphql/graphiql)? @@ -122,12 +150,12 @@ interface ISettings { ```ts interface Tab { - endpoint: string - query: string - name?: string - variables?: string - responses?: string[] - headers?: { [key: string]: string } + endpoint: string + query: string + name?: string + variables?: string + responses?: string[] + headers?: { [key: string]: string } } ``` @@ -169,8 +197,8 @@ Including Fonts (`1.`) ```html ``` @@ -183,10 +211,10 @@ import { Provider } from 'react-redux' import { Playground, store } from 'graphql-playground-react' ReactDOM.render( - - - , - document.body, + + + , + document.body, ) ``` @@ -232,18 +260,18 @@ import lambdaPlayground from 'graphql-playground-middleware-lambda' // const lambdaPlayground = require('graphql-playground-middleware-lambda').default exports.graphqlHandler = function graphqlHandler(event, context, callback) { - function callbackFilter(error, output) { - // eslint-disable-next-line no-param-reassign - output.headers['Access-Control-Allow-Origin'] = '*' - callback(error, output) - } - - const handler = graphqlLambda({ schema: myGraphQLSchema }) - return handler(event, context, callbackFilter) + function callbackFilter(error, output) { + // eslint-disable-next-line no-param-reassign + output.headers['Access-Control-Allow-Origin'] = '*' + callback(error, output) + } + + const handler = graphqlLambda({ schema: myGraphQLSchema }) + return handler(event, context, callbackFilter) } exports.playgroundHandler = lambdaPlayground({ - endpoint: '/dev/graphql', + endpoint: '/dev/graphql', }) ``` @@ -267,6 +295,10 @@ functions: cors: true ``` +#### Security Issue + +There is an [XSS Reflection Vulnerability](./SECURITY.md) when using these middlewares with unsanitized user input before + ## Development ```sh @@ -285,27 +317,27 @@ These are the available options: ```ts export interface EditorColours { - property: string - comment: string - punctuation: string - keyword: string - def: string - qualifier: string - attribute: string - number: string - string: string - builtin: string - string2: string - variable: string - meta: string - atom: string - ws: string - selection: string - cursorColor: string - editorBackground: string - resultBackground: string - leftDrawerBackground: string - rightDrawerBackground: string + property: string + comment: string + punctuation: string + keyword: string + def: string + qualifier: string + attribute: string + number: string + string: string + builtin: string + string2: string + variable: string + meta: string + atom: string + ws: string + selection: string + cursorColor: string + editorBackground: string + resultBackground: string + leftDrawerBackground: string + rightDrawerBackground: string } ``` diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 000000000..381633e25 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,132 @@ +# Known Vulnerabilities + +## XSS Reflection Vulnerability + +the origin of the vulnerability is in `renderPlaygroundPage`, found in `graphql-playground-html` + +### Impact + +When using + +- `renderPlaygroundPage()`, +- `koaPlayground()` +- `expressPlayground()` +- `koaPlayground()` +- `lambdaPlayground()` +- any downstream dependents that use these functions + +without sanitization of user input, your application is vulnerable to an XSS Reflecton Attack. This is a serious vulnerability that could allow for exfiltration of data or user credentials, or to disrupt systems. + +### Impacted Packages + +**All versions of these packages are impacted until those specified below**, which are now safe for user defined input: + +- `graphql-playground-html`: **☔ safe** @ `1.6.20` +- `graphql-playground-express` **☔ safe** @ `1.7.15` +- `graphql-playground-koa` **☔ safe** @ `1.6.14` +- `graphql-playground-hapi` **☔ safe** @ `1.6.12` +- `graphql-playground-lambda` **☔ safe** @ `1.7.16` + +### Static input was always safe + +These examples are safe for _all versions_ **because input is static** + +with `express` and `renderPlaygroundPage`: + +```js +app.get('/playground', (req) => { + res.html( + renderPlaygroundPage({ + endpoint: `/our/graphql`, + }), + ) + next() +}) +``` + +with `expressPlayground`: + +```js +// params +app.get('/playground', (req) => + expressPlayground({ + endpoint: `/our/graphql`, + settings: { 'editor.theme': req.query.darkMode ? 'dark' : 'light' }, + }), +) +``` + +with `koaPlayground`: + +```js +const koa = require('koa') +const koaRouter = require('koa-router') +const koaPlayground = require('graphql-playground-middleware-koa') + +const app = new koa() +const router = new koaRouter() + +router.all('/playground', koaPlayground({ endpoint: '/graphql' })) +``` + +### Vulnerable Examples + +Here are some examples where the vulnerability would be present before the patch, because of unfiltered user input + +```js +const express = require('express') +const expressPlayground = require('graphql-playground-middleware-express') + .default + +const app = express() + +app.use(express.json()) + +// params +app.get('/playground/:id', (req) => + expressPlayground({ + endpoint: `/our/graphql/${req.params.id}`, + }), +) + +// params +app.get('/playground', (req) => + expressPlayground({ + endpoint: `/our/graphql`, + // any settings that are unsanitized user input, not just `endpoint` + settings: { 'editor.fontFamily': req.query.font }, + }), +) +``` + +### Workaround + +To fix this issue without the update, you can sanitize however you want. + +We suggest using [`xss`](https://www.npmjs.com/package/xss) (what we use for our own fix) + +For example, with `graphql-playground-middleware-express`: + +```js +const express = require('express') +const { filterXSS } = require('xss') +const expressPlayground = require('graphql-playground-middleware-express') + .default + + +const app = express() + +const filter = (val) => filterXSS(val, { + whitelist: [], + stripIgnoreTag: true, + stripIgnoreTagBody: ['script'] +}) + +// simple example +app.get('/playground/:id', (req) => + expressPlayground({ endpoint: `/graphql/${filter(req.params.id)}` }) + +// advanced params +app.get('/playground', (req) => + expressPlayground(JSON.parse(filter(JSON.stringify(req.query)))) +``` diff --git a/packages/graphql-playground-html/README.md b/packages/graphql-playground-html/README.md index 66d3d35b6..bd27fddf4 100644 --- a/packages/graphql-playground-html/README.md +++ b/packages/graphql-playground-html/README.md @@ -1,5 +1,7 @@ # graphql-playground-html +> **SECURITY WARNING:** This package and all of it's dependendents had a severe XSS Reflection attack vulnerability until version `1.6.20` of this package. You must sanitize any and all user input values to `renderPlaygroundPage()` values. If you used static values in your middlewares, including ours, you were not vulnerable to the attack. + This package is being used by the GraphQL Playground middlewares. For local development, you can `yarn link` this package, then use `yarn link graphql-playground-html` in the diff --git a/packages/graphql-playground-html/package.json b/packages/graphql-playground-html/package.json index 4aae26c65..0818d9410 100644 --- a/packages/graphql-playground-html/package.json +++ b/packages/graphql-playground-html/package.json @@ -33,5 +33,7 @@ "typescript": { "definition": "dist/index.d.ts" }, - "dependencies": {} + "dependencies": { + "xss": "^1.0.6" + } } diff --git a/packages/graphql-playground-html/src/render-playground-page.ts b/packages/graphql-playground-html/src/render-playground-page.ts index db066eac6..41370733d 100644 --- a/packages/graphql-playground-html/src/render-playground-page.ts +++ b/packages/graphql-playground-html/src/render-playground-page.ts @@ -1,3 +1,5 @@ +import { filterXSS } from 'xss'; + import getLoadingMarkup from './get-loading-markup' export interface MiddlewareOptions { @@ -72,20 +74,38 @@ export interface Tab { headers?: { [key: string]: string } } +const filter = (val) => { + return filterXSS(val, { + // @ts-ignore + whiteList: [], + stripIgnoreTag: true, + stripIgnoreTagBody: ["script"] + }) +} + + const loading = getLoadingMarkup() -const getCdnMarkup = ({ version, cdnUrl = '//cdn.jsdelivr.net/npm', faviconUrl }) => ` - - ${typeof faviconUrl === 'string' ? `` : ''} - ${faviconUrl === undefined ? `` : ''} - -` +const getCdnMarkup = ({ version, cdnUrl = '//cdn.jsdelivr.net/npm', faviconUrl }) => { + const buildCDNUrl = (packageName: string, suffix: string) => filter(`${cdnUrl}/${packageName}/${version ? `@${version}/` : ''}${suffix}` || '') + return ` + + ${typeof faviconUrl === 'string' ? `` : ''} + ${faviconUrl === undefined ? `` : ''} + +`} + + +const renderConfig = (config) => { + return filterXSS(`
${JSON.stringify(config)}
`, { + whiteList: { div: ['id'] }, + }) +} export function renderPlaygroundPage(options: RenderPageOptions) { const extendedOptions: any = { @@ -94,7 +114,7 @@ export function renderPlaygroundPage(options: RenderPageOptions) { } // for compatibility if ((options as any).subscriptionsEndpoint) { - extendedOptions.subscriptionEndpoint = (options as any).subscriptionsEndpoint + extendedOptions.subscriptionEndpoint = filter((options as any).subscriptionsEndpoint || '') } if (options.config) { extendedOptions.configString = JSON.stringify(options.config, null, 2) @@ -105,6 +125,9 @@ export function renderPlaygroundPage(options: RenderPageOptions) { `WARNING: You didn't provide an endpoint and don't have a .graphqlconfig. Make sure you have at least one of them.`, ) } + else if (extendedOptions.endpoint) { + extendedOptions.endpoint = filter(extendedOptions.endpoint || '') + } return ` @@ -115,9 +138,9 @@ export function renderPlaygroundPage(options: RenderPageOptions) { ${extendedOptions.title || 'GraphQL Playground'} ${ - extendedOptions.env === 'react' || extendedOptions.env === 'electron' - ? '' - : getCdnMarkup(extendedOptions) + extendedOptions.env === 'react' || extendedOptions.env === 'electron' + ? '' + : getCdnMarkup(extendedOptions) } @@ -168,6 +191,7 @@ export function renderPlaygroundPage(options: RenderPageOptions) { } ${loading.container} + ${renderConfig(extendedOptions)}
diff --git a/packages/graphql-playground-middleware-express/README.md b/packages/graphql-playground-middleware-express/README.md index bd6cbba9a..addc0690c 100644 --- a/packages/graphql-playground-middleware-express/README.md +++ b/packages/graphql-playground-middleware-express/README.md @@ -1,6 +1,7 @@ # graphql-playground-middleware-express > Express middleware to expose an endpoint for the GraphQL Playground IDE +> **SECURITY NOTE**: All versions of `graphql-playground-express` until `1.7.15` or later have a security vulnerability when unsanitized user input is used while invoking `expressPlayground()`. [Read more below](#security-notes) ## Installation @@ -29,3 +30,17 @@ const app = express() app.get('/playground', expressPlayground({ endpoint: '/graphql' })) ``` + +## Security Notes + +All versions before `1.7.15` were vulnerable to user-defined input to `expressPlayground()`. Read more in [the security notes](https://github.com/prisma/graphql-playground/tree/master/SECURITY.md) + +### Security Upgrade Steps + +To fix the issue, you can upgrade to `1.6.12` or later. If you aren't able to upgrade, see the security notes for a workaround. + +**yarn:** +`yarn add graphql-playground-express@^1.7.15` + +**npm:** +`npm install --save graphql-playground-express@^1.7.15` diff --git a/packages/graphql-playground-middleware-express/examples/basic/index.js b/packages/graphql-playground-middleware-express/examples/basic/index.js index c39209ee8..ac3c89dfe 100644 --- a/packages/graphql-playground-middleware-express/examples/basic/index.js +++ b/packages/graphql-playground-middleware-express/examples/basic/index.js @@ -1,6 +1,6 @@ -const express = require("express"); -const { ApolloServer, gql } = require("apollo-server-express"); -const expressPlayground = require("../../dist/index").default; +const express = require('express') +const { ApolloServer, gql } = require('apollo-server-express') +const expressPlayground = require('../../dist/index').default const typeDefs = gql` type Query { @@ -9,22 +9,27 @@ const typeDefs = gql` schema { query: Query } -`; +` const resolvers = { Query: { - hello: () => "world" - } -}; + hello: () => 'world', + }, +} -const PORT = 4000; +const PORT = 4000 -const server = new ApolloServer({ typeDefs, resolvers }); -const app = express(); -server.applyMiddleware({ app }); +const server = new ApolloServer({ typeDefs, resolvers }) +const app = express() +server.applyMiddleware({ app }) -app.get("/playground", expressPlayground({ endpoint: "/graphql" })); -app.listen(PORT); +app.get( + '/playground', + expressPlayground({ + endpoint: '/graphql/