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

Add hostname option to mitigate DNS rebinding #1260

Merged
merged 2 commits into from Mar 2, 2018

Conversation

@diracdeltas
Copy link
Contributor

diracdeltas commented Jan 12, 2018

This adds the hostname option to allow the server to validate the Host header of incoming requests to prevent DNS rebinding attacks. Needed for brave/browser-laptop#12616.

For more context, see https://bugs.chromium.org/p/project-zero/issues/detail?id=1447.

This adds the `hostname` opt to allow the server to validate the `Host` header of incoming requests to prevent DNS rebinding attacks. Needed for brave/browser-laptop#12616.
@diracdeltas

This comment has been minimized.

Copy link
Contributor Author

diracdeltas commented Jan 12, 2018

lazy manual test:

  1. in Brave, or another webtorrent client, find the code that calls createServer and specify origin: * to pretend that same-origin policy doesn't exist. (this is done to simulate DNS rebinding.) open https://webtorrent.io/torrents/sintel.torrent in Brave or whatever client you're using.
  2. click the 'start download' button and inspect the page to figure out the origin of the torrent server, ex http://localhost:$PORT
  3. serve the following HTML file on a localhost server and then load it in your browser:
<div id='result'></div>
<script>
    const PORT = $PORT // replace this with $PORT number from step 2
    const serverUrl = `http://localhost:${PORT}/0`
    fetch(serverUrl).then((response) => {
      response.text().then((text) => result.innerText = text)
    })
</script>
  1. the page should display a bunch of text from the torrent file
  2. now go back to the source code that calls createServer and add hostname: 'localhost' to the createServer options.
  3. repeat steps 1-3
  4. this time the page should not load a bunch of text from the torrent file
@diracdeltas

This comment has been minimized.

Copy link
Contributor Author

diracdeltas commented Jan 12, 2018

credit goes to @taviso for finding and posting transmission/transmission#468

@diracdeltas

This comment has been minimized.

Copy link
Contributor Author

diracdeltas commented Jan 16, 2018

@feross any thoughts on this approach?

// header that does not match the origin of the torrent server to prevent
// DNS rebinding attacks.
if (opts.hostname) {
if (req.headers.host !== `${opts.hostname}:${server.address().port}`) {

This comment has been minimized.

Copy link
@feross

feross Jan 18, 2018

Member

What would Brave set the hostname option to? If it would be set to chrome-extension://xxx, then I fear this won't work since it's going to append the torrent server's port to that and reject anything that doesn't match that.

I think we can remove the code that adds the port. Correct me if I'm wrong.

This comment has been minimized.

Copy link
@diracdeltas

diracdeltas Jan 18, 2018

Author Contributor

@feross actually it would set it to localhost. The expected host is localhost:$RANDOM_PORT, as assigned in https://github.com/dcposch/webtorrent-remote/blob/3e99afb9a58787afb68f780da0a8f3174aa6cf0e/server.js#L203

note that the Host header is the origin of the server that the client is trying to contact

Copy link
Member

feross left a comment

I think we can safely merge this option with origin, which keeps things simpler for the user and increases the likelihood that they'll configure both these options correctly.

Brave's goal is to ensure that content is only served to chrome-extension://xxx, even in the presence of DNS rebinding attacks. I propose that we change the origin option so that setting it to chrome-extension://xxx has the following effects:

  • Server refuses to reply to any request where the Host header is not set to chrome-extension://xxx.

  • When the Origin header is present (indicating a cross-origin request) and the value of that header is chrome-extension://xxx the server will add CORS headers to the response. This tells the user's browser to allow the request.

How does this sound?

@feross

This comment has been minimized.

Copy link
Member

feross commented Jan 18, 2018

One last thought -- the origin option currently has three possible values:

  1. '*' - allow all cross-origin requests
  2. 'example.com' - only allow cross-origin requests from example.com
  3. false - no cross-origin requests allowed

(Same origin requests are always allowed, with all the options above)

Given the presence of DNS rebinding, it doesn't make sense to offer the third option, since cross-origin requests are effectively allowed, even when that option is set.

Proposal

Let's remove the special handling when the origin option is false.

If we merge the hostname option into the origin option as I proposed above, that leaves the following possible values for origin:

  1. '*' - allow all requests from any origin
  2. 'example.com' - allow requests from example.com only

When { origin: '*' } is set:

[This is the default value when the user does not set an origin.]

  • Server will not check the Host header.

  • When the Origin header is present (indicating a cross-origin request) and set to ANY VALUE the server will add CORS headers to the response. This tells the user's browser to allow the request.

When { origin: 'example.com' } is set:

  • Server refuses to reply to any request where the Host header is not set to example.com.

  • When the Origin header is present (indicating a cross-origin request) and the value of that header is example.com the server will add CORS headers to the response. This tells the user's browser to allow the request.

Thoughts?

@diracdeltas LMK your thoughts.

@diracdeltas

This comment has been minimized.

Copy link
Contributor Author

diracdeltas commented Jan 18, 2018

@feross It actually can't be merged with origin AFAICT since these are mitigating two unrelated attack vectors:

  • the origin parameter specifies the Access-Control-Allow-Origin HTTP response header returned by the server on localhost:$RANDOM_PORT. this is used for the server to whitelist what other origins can make requests to it. This prevents attacker.com from making an XHR request to localhost:$RANDOM_PORT.
  • the host parameter specifies the Host HTTP request header which the server will accept from a client. This is somewhat confusing because you'd think that if a browser is making a request to localhost:44494 (for instance), then the Host header would be always set to localhost:44494. However the key trick of DNS rebinding is forcing the client to visit localhost:44494 by binding it to attacker.com:44494 - in that case, the Host header seen by the torrent server will be attacker.com:44494 instead of localhost:44494. This PR lets the server detect when this is happening and block those requests.

So in the case of Brave, we would set the origin param to chrome-extension://$BRAVE_EXTENSION_ID (since this is the origin of the page which is making requests to the webtorrent localhost server), and we would set the host param to http://localhost:$RANDOM_PORT (the origin of the webtorrent localhost server itself).

@feross
feross approved these changes Mar 2, 2018
@feross feross merged commit e7d6e4f into webtorrent:master Mar 2, 2018
3 checks passed
3 checks passed
Node Security No known vulnerabilities found
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@feross

This comment has been minimized.

Copy link
Member

feross commented Mar 2, 2018

Sorry – I lost track of this PR.

Merged and released as 0.98.24.

@lock lock bot locked as resolved and limited conversation to collaborators May 31, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.