-
Notifications
You must be signed in to change notification settings - Fork 197
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 some security checks when handling a HTTP connection. #1634
base: master
Are you sure you want to change the base?
Conversation
The BiDi version of this is w3c/webdriver-bidi#155 |
1817c64
to
ae4770e
Compare
ae4770e
to
68f0fb1
Compare
<li><p><p>If <var>request</var> has an <a>Origin header</a>, | ||
let <var>origin</var> be the value of that header. Otherwise | ||
let <var>origin</var> be null.</p></li> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When WebDriver server is run in a Docker container, the Host header
is always different from the host of the HTTP server
. Current spec includes this specific to "or to another host the implementation has been configured to allow.", but maybe it would make sense to extract it into the specific step, so that the need of such a configuration will be more explicit?
<li><p>Let <var>implementation allowed hosts rule</var> be the configured by the implementation rule determines if the given host allowed or not. Otherwise let <var>implementation allowed hosts rule</var> be the rule matching nothing.</p></li> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's hard to be very specific here because the details of what's required depend a lot on the setup you want to support. For example it's possible to support the docker setup by making the host of the HTTP server the same as the host of the container. But it's also possible to support it by e.g. binding to an IP address, in which case the server just has to know it should accept requests with the specific host header that the conatiner will have. So I've tried to write some general guidance in the security section which suggests you probably want to provide some configuration options to support this kind of scenario, but without saying too much about exactly what they need to be.
<a>host</a> of the HTTP server or to another <a>host</a> the | ||
implementation has been configured to allow.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<a>host</a> of the HTTP server or to another <a>host</a> the | |
implementation has been configured to allow.</p></li> | |
<a>host</a> of the HTTP server or | |
does not match the <var>implementation allowed hosts rule</var>.</p></li> |
<p class=note>Rejecting connections with unexpected values in | ||
the <a>Origin header</a> is necessary to prevent untrusted websites | ||
from establishing a WebDriver session.</p></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe add note about Docker and some other edge cases here?
The Browser Testing and Tools Working Group just discussed The full IRC log of that discussion<jgraham> Topic: Host/Origin checks<jgraham> github: https://github.com//pull/1634 <sadym> q+ <jgraham> jgraham: Allowing HTTP/WebSocket connections comes with some security concerns eg. if you don't check origin header then websites might be able to start a session. The spec didn't cover this before, but the PR adds some checks in when establishing a connection. <jgraham> ack sadym <jgraham> sadym: There is a use case with docker, because the host header for the container is different to the actual host. We need to account for this in the spec. <jgraham> jgraham: Agreed, we should allow implementations to allow an implemtation-defined list of hosts to handle these cases. <jgraham> jgraham: Also need to perform the checks on the WebSocker upgrade request for BiDi. |
68f0fb1
to
b3adb90
Compare
I've updated this a bit; I think it now covers all the points in the linked issues apart from:
In any case I think if there's more to do for those points we should consider them in their own issues / PRs and limit this one to just the header validation requirements for accepting incomping requests. |
b3adb90
to
70f3a0b
Compare
Check the Host and Origin headers for the incoming connection to verify the connection is allowed. The language is intended to allow the specific behaviour to be largely implementation defined, whilst recommending a default behaviour that prevents CSRF-type attacks (reject host headers that aren't an IP address or the server hostname, reject any requests with an origin header). Hopefully adding this text will ensure that implementations consider the security issues accepting a connection, even though it's not possible to give precise requirements that apply to all implementations.
This avoids confusion with null as a string value of the Origin header
This prevents e.g. websites creating forms that can start a local WebDriver session in a browser, even if there's no Origin header
This doesn't add normative requirements, but encourages implementors to ship with secure defaults, whilst mentioning that there are real-world use cases which require remote ends to accept connections coming from somewhere other than localhost.
70f3a0b
to
a26a826
Compare
@sadym-chromium in case you have some available time soon maybe you could have a look at this PR again and if your comments have been addressed or clarified? Thanks. |
a <a>domain</a> identical to the | ||
<a>host</a> of the HTTP server or to another <a>host</a> the | ||
implementation has been configured to allow.</p></li> | ||
<li><p>The <code>port</code> part of <var>host</var> is present |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe verifying the port of the Host header adds any significant security (especially with respect to DNS rebinding attacks) (?). It is probably not necessary to require this verification.
Check the Host and Origin headers for the incoming connection to
verify the connection is allowed.
The language is intended to allow the specific behaviour to be largely
implementation defined, whilst recommending a default
behaviour that prevents CSRF-type attacks (reject host headers that
aren't an IP address or the server hostname, reject any requests with
an origin header).
Hopefully adding this text will ensure that implementations consider
the security issues accepting a connection, even though it's not
possible to give precise requirements that apply to all
implementations.
Preview | Diff
Preview | Diff