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

[Security] Mitigate DNS rebinding flaw #3046

Merged
merged 2 commits into from
Jul 30, 2019
Merged

Conversation

feross
Copy link
Contributor

@feross feross commented Jul 30, 2019

Fix: brave/brave-browser#5460

Submitter Checklist:

Test Plan:

  1. Go to https://webtorrent.io/free-torrents
  2. Select Sintel (magnet link)
  3. Start the download
  4. Hover your mouse over the download link (the downward-facing arrow) and observe the link's port number. It should be something like e.g. 58630.
  5. Run the following command in Terminal (tested on Mac): cat <(echo -en 'GET / HTTP/1.1\r\nHost: attacker.com\r\n\r\n') - | nc localhost 58630. Note: be sure to replace 58630 in the command with the actual port number you observed in Step 4.
  6. The command should hang without outputting anything to Terminal at all. If you see an HTTP response or HTML, then this is broken. Nothing should be output from the command.

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@feross feross added security feature/webtorrent Label for webtorrent related issues labels Jul 30, 2019
@feross feross added this to the 0.70.x - Nightly milestone Jul 30, 2019
@feross feross requested review from diracdeltas and yrliou July 30, 2019 20:43
@feross feross self-assigned this Jul 30, 2019
yrliou
yrliou previously approved these changes Jul 30, 2019
@feross
Copy link
Contributor Author

feross commented Jul 30, 2019

The commit that we're pinning to now is here: brave/webtorrent@423f1fa

diracdeltas
diracdeltas previously approved these changes Jul 30, 2019
@feross feross dismissed stale reviews from diracdeltas and yrliou via 8936a16 July 30, 2019 21:22
@feross feross force-pushed the security-dns-rebinding branch from 8feb18f to 8936a16 Compare July 30, 2019 21:22
@feross
Copy link
Contributor Author

feross commented Jul 30, 2019

Forgot to update package-lock.json so this will need review again.

@feross feross requested review from diracdeltas and yrliou July 30, 2019 21:23
diracdeltas
diracdeltas previously approved these changes Jul 30, 2019
@feross
Copy link
Contributor Author

feross commented Jul 30, 2019

I'm getting a travis failure that I'm not sure is my fault. Any ideas?

npm ERR! code ENOAUDIT
npm ERR! audit Your configured registry (https://registry.npmjs.org/) may not support audit requests, or the audit endpoint may be temporarily unavailable.
npm ERR! audit The server said: Invalid package tree, run  npm install  to rebuild your package-lock.json

@yrliou
Copy link
Member

yrliou commented Jul 30, 2019

I'm getting a travis failure that I'm not sure is my fault. Any ideas?

npm ERR! code ENOAUDIT
npm ERR! audit Your configured registry (https://registry.npmjs.org/) may not support audit requests, or the audit endpoint may be temporarily unavailable.
npm ERR! audit The server said: Invalid package tree, run  npm install  to rebuild your package-lock.json

As I shared through DM, this might be fixed if you remove node_modules and reinstall.

@feross
Copy link
Contributor Author

feross commented Jul 30, 2019

Yep, thanks. That was the fix. Looks like npm gets confused by the fact that we're pinning to the forks of torrent-discovery and bittorrent-tracker while the webtorrent package also depends on its own versions of these, and so it messes up the package-lock.json depending on when install is run. :/

This annoyance should hopefully be fixed by: brave/brave-browser#856

@feross feross merged commit 6188cf7 into master Jul 30, 2019
@feross feross deleted the security-dns-rebinding branch July 30, 2019 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/webtorrent Label for webtorrent related issues security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security] Mitigate DNS rebinding flaw
3 participants