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 Access-Control-Allow-Origin to response header #1362

Closed

Conversation

christian-bromann
Copy link
Member

@christian-bromann christian-bromann commented Nov 19, 2018

It should be irrelevant from which origin the webdriver request was made. Not having this header set currently prevents e.g. browser to run webdriver tests which I know is an unusual use case but definitely something that can be from value. In fact the reason I bring this up is because of a conversation I had with @jlipps at Node+JS Interactive in Vancouver this year. We are working on making WebdriverIO capable to run in the browser.

I am happy to enhance my header PR (web-platform-tests/wpt#14074) in case this is considered to get accepted. Cheers!


Preview | Diff

@jlipps
Copy link
Contributor

jlipps commented Nov 19, 2018

Thanks @christian-bromann! Based on your comment I'm obviously a 👍 for this idea. I was working on a web-based client implementation, and it works running against Appium (because this response header is set), but not against Selenium. It seemed reasonable that the spec should allow access from any source as a matter of principle, but I'm open if the spec committee prefers to leave this up to the implementers (in which case I think it should be implemented in Selenium at the very least).

@jgraham
Copy link
Member

jgraham commented Nov 19, 2018

Hmm, so maybe I need to refresh my CORS knowledge, but if I happen to visit evil.com in a browser when I have a webdriver server running on localhost, it can potentially send requests that allow it to create a session, read the session id, and start controlling another browser instance? This seems like a bad idea without a clearer understanding of the security implications.

@jlipps
Copy link
Contributor

jlipps commented Nov 19, 2018

@jgraham what it means AFAICT is that any host which could already start a session using CURL against your webdriver server could now do the same thing with a browser. It doesn't grant network access to the webdriver server to any host which didn't have it previously. I can't see how it would be a security risk though I am unimaginative when it comes to those things :-)

@jlipps
Copy link
Contributor

jlipps commented Nov 19, 2018

@jgraham oh I missed your point. Yes, the scenario you envision could happen. evil.com's javascript could start sessions on an unauthenticated webdriver server your browser has network access to (including localhost).

@jlipps
Copy link
Contributor

jlipps commented Nov 19, 2018

Hmmm, maybe this is a reason to leave it up to implementations, so they can hide it behind a server flag that requires users to know what they are doing to turn it on? I do think you should be able to run webdriver sessions from the web, but agree with you there are potential security implications I hadn't considered.

@christian-bromann
Copy link
Member Author

@jlipps @jgraham we could define a hostname that would be granted to access the driver, e.g.

Access-Control-Allow-Origin: webdriver.local

so in order for you to run tests in the browser you need to set this hostname in /etc/hosts

@andreastt
Copy link
Member

It is recommended in the security appendix that the HTTPD only accepts connections from loopback devices:

The default setting for this might be to limit connections to the IPv4 localhost CIDR range 127.0.0.0/8 and the IPv6 localhost address ::1.

If you then have an HTTPD on your local system and evil.com somehow intercepts the WebDriver session ID, even if Access-Control-Allow-Origin: * is set, the evil remote would not be able to reach the HTTPD unless it had been specifically configured to accept non-loopback connections?

Am I misunderstanding this?

@jgraham
Copy link
Member

jgraham commented Nov 20, 2018

The question is, what happens if evil.com contains some javascript code like

fetch("http://localhost:4444/session", {method: "POST", mode:"cors", body:"{}"})
  .then(resp => resp.json())
  .then(body => body.sessionId);

or something. If that works then there isn't a nonlocal connection, but a website could take arbitary webdriver actions.

Copy link
Member

@andreastt andreastt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the clarification @jgraham. That makes sense. So evil.com loads a document that contains fetch requests to the localhost, so the connection would come from the loopback device circumventing the non-loopback connection security recommendation.

I think further security review is needed here. I do see the usefulness for a local document to be able to call out of the document to the currently attached WebDriver session, however. I have also had that use case in the past.

@jgraham
Copy link
Member

jgraham commented Nov 20, 2018

My strong first preference would be to do nothing here. The sites that want this kidn of access can probably come up with some bespoke solution (i.e. a local executable to act as a proxy).

If we nevertheless do something, I would want some mechanism that creates explicit consent for sessions originating from a domain e.g. some mechanism where the local user passes the domain into the driver, gets back a token, and the domain has to provide that token in the new session request in order to get access to the response bodies. Even then, I would expect geckodriver to disable a bunch of (Firefox-specific) features in that case, like the ability to set a profile (and I would certainly want any mechanism here to go through rigourous security review).

@jgraham
Copy link
Member

jgraham commented Nov 20, 2018

For an example of an unsolved problem: what if the site isn't https. Then you can leak the secret token and be subject to MITM attacks.

@jlipps
Copy link
Contributor

jlipps commented Nov 20, 2018

It seems like there are bigger issues here than @christian-bromann and I originally thought. I can buy an argument that enabling CORS shouldn't be part of this spec, but should be the responsibility of the most proximate remote end (a grid service of some kind, which would enable protection independently via access tokens, or Selenium itself, via an --allow-cors server flag that turns the header on).

I mostly just care that there is a way for users to opt into a mode where servers they run can receive connections from scripts running inside browsers. If the Selenium project is open to adding something like the flag I mentioned, then at least there is a path forward to this (since all the other drivers can be used behind Selenium).

FWIW I also think @christian-bromann's suggestion of only allowing connections to webdriver.local makes sense, because it requires users to proactively opt in to the security hazard by updating their /etc/hosts in order to make it work--which they would presumably only do after understanding the security implications documented alongside the hostname description. However, from a user experience point of view, this is potentially somewhat more convoluted than simply using a server flag or a vendor-provided proxy.

@shs96c can you comment on whether Selenium would accept a PR that allows users to opt into a server mode that allows CORS (along with the risks entailed)?

Copy link
Contributor

@jlipps jlipps left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(changing my review based on conversation)

jlipps added a commit to appium/appium-base-driver that referenced this pull request Nov 21, 2018
It turns out blindly allowing CORS could be a security hazard (who knew)
See w3c/webdriver#1362 for the conversation that led to this realization.
This change makes the cross domain middleware conditional (defaulting to the current behavior).
This way Appium can pass in 'false' based on a flag, for better server security.
dpgraham pushed a commit to appium/appium-base-driver that referenced this pull request Nov 22, 2018
* make allowing CORS conditional
It turns out blindly allowing CORS could be a security hazard (who knew)
See w3c/webdriver#1362 for the conversation that led to this realization.
This change makes the cross domain middleware conditional (defaulting to the current behavior).
This way Appium can pass in 'false' based on a flag, for better server security.

* actually include function parameter we can use from appium
@AutomatedTester
Copy link
Contributor

I am going to close this and if we ever have a meaningful mechanism that covers all the concerns in this thread that we create a new PR.

@jlipps
Copy link
Contributor

jlipps commented Jan 31, 2019

Makes sense, I think selenium/appium adding opt-in modes for this is better than having it be part of the spec anyway.

@christian-bromann christian-bromann deleted the cb-allow-origin branch February 22, 2019 11:02
dpgraham pushed a commit to dpgraham/appium-monorepo that referenced this pull request Mar 2, 2019
* make allowing CORS conditional
It turns out blindly allowing CORS could be a security hazard (who knew)
See w3c/webdriver#1362 for the conversation that led to this realization.
This change makes the cross domain middleware conditional (defaulting to the current behavior).
This way Appium can pass in 'false' based on a flag, for better server security.

* actually include function parameter we can use from appium
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.

5 participants