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.

Sign up for GitHub

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add some security checks when handling a HTTP connection. #1634

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
887be0d
Add some security checks when handling a HTTP connection.
jgraham Dec 3, 2021
5e8999a
Use undefined rather than null as the defaut header value.
jgraham Mar 18, 2022
e07e6ec
Disallow CORS-safelisted Content-Type headers.
jgraham Mar 18, 2022
a26a826
Add some extra detail about allowing external network connections.
jgraham Mar 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 93 additions & 14 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,8 +428,72 @@ <h3>Processing model</h3>
received data, according to the requirements of [[RFC7230]]. If it
is not possible to construct a complete <a>HTTP request</a>,
the <a>remote end</a> must either close the <a>connection</a>,
return an HTTP response with status code 500, or return
an <a>error</a> with <a>error code</a> <a>unknown error</a>.
return an HTTP response with status code 500, or <a>send an
error</a> with <a>error code</a> <a>unknown error</a>, and then
jump to step 1.

<li><p>If <var>request</var> has a <a>Host header</a>,
let <var>host</var> be the value of that header. Otherwise
let <var>host</var> be undefined.</p></li>

<li><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 undefined.</p></li>

<li><p>If <var>request</var> has an <a>Content-Type header</a>,
let <var>content-type</var> be the value of that header. Otherwise
let <var>content-type</var> be undefined.</p></li>

Copy link
Contributor

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?

Suggested change
<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>

Copy link
Member Author

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.

<li><p>If any of the following conditions hold:
<ul>
<li><p><var>host</var> is undefined.</p></li>
<li><p><var>host</var> doesn't match the <code>Host</code>
grammar [[RFC7230]]</p></li>
<li><p>The result of <a>host parsing</a> the <code>uri-host</code>
part of <var>host</var> is not an <a>IP address</a>,
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>
Comment on lines +455 to +456
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<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>

<li><p>The <code>port</code> part of <var>host</var> is present
Copy link

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.

but doesn't match the port of the HTTP server.</p></li>
<li><p>The <code>port</code> part of <var>host</var> is not
present, and the port of the HTTP server doesn't match the
default port for the request's scheme.</p></li>
<li><p>The implementation wants to reject connections
with <var>host</var> as the <a>Host header</a>.</p></li>
</ul>
<p>Then <a>send an error</a> with <a>error code</a> <a>unknown
error</a>, and jump to step 1.</p>

<p class="note">Rejecting connections with unexpected values in the
<a>Host header</a> prevents DNS rebinding attacks. Implementations
can opt to provide more stringent controls where appropriate, for
example only accepting connections when the <var>host</var> value
corresponds to a loopback interface [[RFC5735]]. Further guidance
for implementors is given in the <a href="#security">security</a>
section.</p>
</li>

<li><p>If <var>origin</var> is not undefined and is not identical to
an <a>Origin header</a> value that the implementation has been
configured to allow, then stop running these steps and act as if the
requested service is not available.</p>

<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>
Comment on lines +482 to +484
Copy link
Contributor

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?


<li><p>If <var>content-type</var> is not undefined, and
("<code>content-type</code>", <var>content-type</var>) is a
[=CORS-safelisted request-header=], or otherwise if the value
of <var>content-type</var> is not a <a>Content-Type header</a> the
implementation allows, then stop running these steps and act as if
the requested service is not available.</p>

<p class=note>This provides an additional layer of defence against
requests originating from untrusted websites. Implementations can
choose to implement this by only accepting requests with the
"<code>application/json</code>" Content-Type header.</p></li>

<li><p>Let <var>request match</var> be the result of the algorithm
to <a>match a request</a> with <var>request</var>’s
Expand Down Expand Up @@ -10433,18 +10497,18 @@ <h2>Security</h2>
and that WebDriver remains disabled
in publicly consumed versions of the user agent.

<p>To prevent arbitrary machines on the network
from connecting and creating <a>sessions</a>,
it is suggested that only connections from
loopback devices are allowed by default.

<p>The <a>remote end</a> can include
a configuration option to limit
the accepted IP range allowed to connect and make requests.
The default setting for this might be
to limit connections to the IPv4 localhost
CIDR range <code>127.0.0.0/8</code>
and the IPv6 localhost address <code>::1</code>. [[RFC4632]]
<p>To prevent arbitrary machines on the network from connecting and
creating <a>sessions</a>, it is suggested that only connections from
loopback devices are allowed by default. However, testing setups
commonly put the <a>remote end</a> and <a>local end</a> on different
network hosts. Users deploying such a setup are encouraged to
restrict access to the remote end to the greatest extent possible,
either by restricting network connections to trusted hosts (e.g. in
the case of a lab setting, or the remote end running in a containers
on the same bridged network), or by routing all connections through
an <a>intermediary node</a> that provides authorization and
authentication. <a>Remote end</a> implementors are encouraged to
provide minimal, opt-in, configuration to support these scenarios.

<p>It is also suggested that user agents
make an effort to visually distinguish
Expand Down Expand Up @@ -10958,6 +11022,21 @@ <h2>Index</h2>
it is supposed that the implementation supports the relevant subsets of
[[RFC7230]], [[RFC7231]], [[RFC7232]], [[RFC7234]], and [[RFC7235]].

<dd><p>The following terms are defined in the Web Origin Concept specification: [[RFC6454]]
<ul>
<!-- Origin header --> <li><dfn><a href="https://datatracker.ietf.org/doc/html/rfc6454#section-7">Origin header</a></dfn>
</ul>

<dd><p>The following terms are defined in the Hypertext Transfer Protocol (HTTP/1.1): Message Syntax and Routing specification: [[RFC7230]]
<ul>
<!-- Host header --> <li><dfn><a href="https://datatracker.ietf.org/doc/html/rfc7230#section-5.4">Host header</a></dfn>
</ul>

<dd><p>The following terms are defined in the Hypertext Transfer Protocol (HTTP/1.1): Semantics and Content specification: [[RFC7231]]
<ul>
<!-- Content-Type header --> <li><dfn><a href="https://datatracker.ietf.org/doc/html/rfc7231#section-3.1.1.5">Content-Type header</a></dfn>
</ul>

<dd><p>The following terms are defined in the Cookie specification: [[RFC6265]]
<ul>
<!-- Compute cookie-string --> <li><dfn><a href=https://tools.ietf.org/html/rfc6265#section-5.4>Compute <code>cookie-string</code></a></dfn>
Expand Down