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

Jump to bottom

Remove race condition when creating new HTTPChannel #435

Merged
merged 6 commits into from
Jun 8, 2024
Merged

Remove race condition when creating new HTTPChannel #435

merged 6 commits into from
Jun 8, 2024

Conversation

digitalresistor
Copy link
Member

When we accept() a connection we get the remote address and we pass it to HTTPChannel, then in wasyncore.dispatcher we were calling getpeername() again to get the same remote address that we already had.

This would cause an issue if the remote client had sent data, but closed the connection before getpeername() because the wasyncore.dispatcher would set the channel to no longer being connected.

Further down the line this would cause issues whereby handle_write would loop over and over because it would mark itself as selectable but the socket would never actually get torn down.

Closes #418
Supersedes #419

@digitalresistor
HTTPChannel is always created from accept, explicitly set self.connec…
03cc640 
…ted to True
@digitalresistor
Assume socket is not connected when passed to wasyncore.dispatcher
840aebc 
No longer call getpeername() on the remote socket either, as it is not
necessary for any of the places where waitress requires that self.addr
in a subclass of the dispatcher needs it.

This removes a race condition when setting up a HTTPChannel where we
accepted the socket, and know the remote address, yet call getpeername()
again which would have the unintended side effect of potentially setting
self.connected to False because the remote has already shut down part of
the socket.

This issue was uncovered in #418, where the server would go into a hard
loop because self.connected was used in various parts of the code base.
@digitalresistor
Remove test for getpeername()
86c680d
@digitalresistor
Don't exit handle_write early -- even if socket is not connected
8cba302 
Calling handle_close() multiple times does not hurt anything, and is
safe.
@digitalresistor
Remove code not used by waitress from vendored asyncore
63678e6
@digitalresistor digitalresistor force-pushed the bugfix/remove-race-condition branch 2 times, most recently from ef3833d to ff7e3c1 Compare March 3, 2024 23:54
@digitalresistor
When closing the socket, set it to None
9d99c89 
This avoids calling close() twice on the same socket if self.close() or
self.handle_close() is called multiple times
@digitalresistor digitalresistor mentioned this pull request Mar 4, 2024
Closed
@djay
Copy link

djay commented Apr 26, 2024

@digitalresistor it's been running for a few weeks with no problem. the pentests still happen and we haven't see any problems.

@digitalresistor digitalresistor merged commit 1ae4e89 into main Jun 8, 2024
29 checks passed
@digitalresistor digitalresistor deleted the bugfix/remove-race-condition branch June 8, 2024 21:51
@digitalresistor digitalresistor mentioned this pull request Jun 8, 2024
Closed
5 tasks
@vondele
Copy link

vondele commented Jul 7, 2024

great, find and thanks for fixing this. We have been running a server at https://tests.stockfishchess.org/tests that almost reads like the description of the testcase in #418 (comment) and have seen this happen multiple times in the past, if the load on the server was high (with dropped connections by nginx / nginx reboots), almost on a daily basis (most recently discussed as official-stockfish/fishtest#2094 ... but we had no real clue).

We'd be happy to upgrade waitress to an eventual 3.0.1 as mentioned in 3495674 . Is there a timeline for that (also in light of the green CI)?

nicopicchio pushed a commit to uktrade/jml that referenced this pull request Oct 29, 2024
@dependabot
Bump waitress from 2.1.2 to 3.0.1 (#479)
25906bc 
Bumps [waitress](https://github.com/Pylons/waitress) from 2.1.2 to
3.0.1.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/Pylons/waitress/releases">waitress's
releases</a>.</em></p>
<blockquote>
<h2>v3.0.0</h2>
<h1>3.0.0 (2024-02-04)</h1>
<ul>
<li>
<p>Rename &quot;master&quot; git branch to &quot;main&quot;</p>
</li>
<li>
<p>Fix a bug that would appear on macOS whereby if we accept() a socket
that is
already gone, setting socket options would fail and take down the
server. See
<a
href="https://github.com/Pylons/waitress/pull/399">Pylons/waitress#399</a></p>
</li>
<li>
<p>Fixed testing of vendored asyncore code to not rely on particular
naming for
errno's. See <a
href="https://github.com/Pylons/waitress/pull/397">Pylons/waitress#397</a></p>
</li>
<li>
<p>HTTP Request methods and versions are now validated to meet the HTTP
standards thereby dropping invalid requests on the floor. See
<a
href="https://github.com/Pylons/waitress/pull/423">Pylons/waitress#423</a></p>
</li>
<li>
<p>No longer close the connection when sending a HEAD request response.
See
<a
href="https://github.com/Pylons/waitress/pull/428">Pylons/waitress#428</a></p>
</li>
<li>
<p>Always attempt to send the Connection: close response header when we
are
going to close the connection to let the remote know in more instances.
<a
href="https://github.com/Pylons/waitress/pull/429">Pylons/waitress#429</a></p>
</li>
<li>
<p>Python 3.7 is no longer supported. Add support for Python 3.11, 3.12
and
PyPy 3.9, 3.10. See <a
href="https://github.com/Pylons/waitress/pull/412">Pylons/waitress#412</a></p>
</li>
<li>
<p>Document that trusted_proxy may be set to a wildcard value to trust
all
proxies. See <a
href="https://github.com/Pylons/waitress/pull/431">Pylons/waitress#431</a></p>
</li>
</ul>
<h2>Updated Defaults</h2>
<ul>
<li>clear_untrusted_proxy_headers is set to True by default. See
<a
href="https://github.com/Pylons/waitress/pull/370">Pylons/waitress#370</a></li>
</ul>
</blockquote>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/Pylons/waitress/blob/main/CHANGES.txt">waitress's
changelog</a>.</em></p>
<blockquote>
<h2>3.0.1 (2024-11-28)</h2>
<p>Security</p>
<pre><code>
- Fix a bug that would lead to Waitress busy looping on select() on a
half-open
socket due to a race condition that existed when creating a new
HTTPChannel.
  See Pylons/waitress#435,
  Pylons/waitress#418 and

GHSA-3f84-rpwh-47g6
<p>With thanks to Dylan Jay and Dieter Maurer for their extensive
debugging and<br />
helping track this down.</p>
<ul>
<li>
<p>No longer strip the header values before passing them to the WSGI
environ.<br />
See <a
href="https://github.com/Pylons/waitress/pull/434">Pylons/waitress#434</a>
and<br />
<a
href="https://github.com/Pylons/waitress/issues/432">Pylons/waitress#432</a></p>
</li>
<li>
<p>Fix a race condition in Waitress when
<code>channel_request_lookahead</code> is enabled<br />
that could lead to HTTP request smuggling.</p>
<p>See <a
href="https://github.com/Pylons/waitress/security/advisories/GHSA-9298-4cf8-g4wj">https://github.com/Pylons/waitress/security/advisories/GHSA-9298-4cf8-g4wj</a></p>
</li>
</ul>
<h2>3.0.0 (2024-02-04)</h2>
<ul>
<li>
<p>Rename &quot;master&quot; git branch to &quot;main&quot;</p>
</li>
<li>
<p>Fix a bug that would appear on macOS whereby if we accept() a socket
that is<br />
already gone, setting socket options would fail and take down the
server. See<br />
<a
href="https://github.com/Pylons/waitress/pull/399">Pylons/waitress#399</a></p>
</li>
<li>
<p>Fixed testing of vendored asyncore code to not rely on particular
naming for<br />
errno's. See <a
href="https://github.com/Pylons/waitress/pull/397">Pylons/waitress#397</a></p>
</li>
<li>
<p>HTTP Request methods and versions are now validated to meet the
HTTP<br />
standards thereby dropping invalid requests on the floor. See<br />
<a
href="https://github.com/Pylons/waitress/pull/423">Pylons/waitress#423</a></p>
</li>
<li>
<p>No longer close the connection when sending a HEAD request response.
See<br />
<a
href="https://github.com/Pylons/waitress/pull/428">Pylons/waitress#428</a></p>
</li>
<li>
<p>Always attempt to send the Connection: close response header when we
are<br />
going to close the connection to let the remote know in more
instances.<br />
<a
href="https://github.com/Pylons/waitress/pull/429">Pylons/waitress#429</a></p>
</li>
<li>
<p>Python 3.7 is no longer supported. Add support for Python 3.11, 3.12
and<br />
PyPy 3.9, 3.10. See <a
href="https://github.com/Pylons/waitress/pull/412">Pylons/waitress#412</a></p>
</li>
</ul>
<p>&lt;/tr&gt;&lt;/table&gt;<br />
</code></pre></p>
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/Pylons/waitress/commit/ae949bb428e50cf04152db56460f31c1e6d3a2a9"><code>ae949bb</code></a>
Ready for 3.0.1</li>
<li><a
href="https://github.com/Pylons/waitress/commit/e4359018537af376cf24bd13616d861e2fb76f65"><code>e435901</code></a>
Merge commit from fork</li>
<li><a
href="https://github.com/Pylons/waitress/commit/810a435f9e9e293bd3446a5ce2df86f59c4e7b1b"><code>810a435</code></a>
Add documentation for channel_request_lookahead</li>
<li><a
href="https://github.com/Pylons/waitress/commit/f4ba1c260cf17156b582c6252496213ddc96b591"><code>f4ba1c2</code></a>
Fix a race condition on recv_bytes boundary when request is invalid</li>
<li><a
href="https://github.com/Pylons/waitress/commit/7e7f11e61d358ab1cb853fcadf2b46b1f00f5993"><code>7e7f11e</code></a>
Add a new test to validate the lookahead race condition</li>
<li><a
href="https://github.com/Pylons/waitress/commit/6943dcf556610ece2ff3cddb39e59a05ef110661"><code>6943dcf</code></a>
Make DummySock() look more like an actual socket</li>
<li><a
href="https://github.com/Pylons/waitress/commit/fdd2ecfd325af2f419d91c62b2551e2c3922f686"><code>fdd2ecf</code></a>
Merge pull request <a
href="https://github.com/Pylons/waitress/issues/445">#445</a>
from Pylons/feature/support-py-3-13</li>
<li><a
href="https://github.com/Pylons/waitress/commit/dcd18e7b4b8e78e2abea8f286c23b0b9298bea9b"><code>dcd18e7</code></a>
Update exclude matrix</li>
<li><a
href="https://github.com/Pylons/waitress/commit/4633ea6d69d6b7eff5db91e263ea85f437026db0"><code>4633ea6</code></a>
Drop Python 3.8 and add Python 3.13</li>
<li><a
href="https://github.com/Pylons/waitress/commit/4584936eac5838b6d3b07e84a86874fa586ffe6e"><code>4584936</code></a>
Merge pull request <a
href="https://github.com/Pylons/waitress/issues/440">#440</a>
from Pylons/fix/ci</li>
<li>Additional commits viewable in <a
href="https://github.com/Pylons/waitress/compare/v2.1.2...v3.0.1">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=waitress&package-manager=pip&previous-version=2.1.2&new-version=3.0.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the
[Security Alerts page](https://github.com/uktrade/jml/network/alerts).

</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

100% cpu in mainthread due to not closing properly? (channel.connected == False)
3 participants
@digitalresistor @djay @vondele