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

SSL error during _conditional_error will crash worker thread #641

Open
1 of 3 tasks
kasium opened this issue Sep 1, 2023 · 2 comments
Open
1 of 3 tasks

SSL error during _conditional_error will crash worker thread #641

kasium opened this issue Sep 1, 2023 · 2 comments
Labels
bug Something is broken triage

Comments

@kasium
Copy link
Contributor

kasium commented Sep 1, 2023

❓ I'm submitting a ...

  • 🐞 bug report
  • 🐣 feature request
  • ❓ question about the decisions made in the repository

🐞 Describe the bug. What is the current behavior?
If a communication fails, cheroot will catch all exceptions and execute self._conditional_error(req, '500 Internal Server Error'). If in this code any other exception beside FatalSSLAlert and NoSSLError is raised, the worker calling communicate will die.

❓ What is the motivation / use case for changing the behavior?
Better exception handling

πŸ’‘ To Reproduce
no way to reproduce this by purpose

πŸ’‘ Expected behavior
Thread does not crash

πŸ“‹ Details

SSLEOFError: EOF occurred in violation of protocol (_ssl.c:2423)
  File "cheroot/server.py", line 1267, in communicate
    req.parse_request()
  File "cheroot/server.py", line 702, in parse_request
    success = self.read_request_line()
  File "cheroot/server.py", line 761, in read_request_line
    self.simple_response(
  File "cheroot/server.py", line 1111, in simple_response
    self.conn.wfile.write(EMPTY.join(buf))
  File "cheroot/makefile.py", line 438, in write
    res = super().write(val, *args, **kwargs)
  File "cheroot/makefile.py", line 36, in write
    self._flush_unlocked()
  File "cheroot/makefile.py", line 45, in _flush_unlocked
    n = self.raw.write(bytes(self._write_buf))
  File "socket.py", line 724, in write
    return self._sock.send(b)
  File "ssl.py", line 1210, in send
    return self._sslobj.write(data)
SSLEOFError: EOF occurred in violation of protocol (_ssl.c:2423)
  File "cheroot/workers/threadpool.py", line 125, in run
    keep_conn_open = conn.communicate()
  File "cheroot/server.py", line 1296, in communicate
    self._conditional_error(req, '500 Internal Server Error')
  File "cheroot/server.py", line 1339, in _conditional_error
    req.simple_response(response)
  File "cheroot/server.py", line 1111, in simple_response
    self.conn.wfile.write(EMPTY.join(buf))
  File "cheroot/makefile.py", line 438, in write
    res = super().write(val, *args, **kwargs)
  File "cheroot/makefile.py", line 36, in write
    self._flush_unlocked()
  File "cheroot/makefile.py", line 45, in _flush_unlocked
    n = self.raw.write(bytes(self._write_buf))
  File "socket.py", line 724, in write
    return self._sock.send(b)
  File "ssl.py", line 1210, in send
    return self._sslobj.write(data)

πŸ“‹ Environment

  • Cheroot version: 8.4.1
  • Python version: 3.11.0
  • OS: Linux
  • Browser: all
@kasium kasium added bug Something is broken triage labels Sep 1, 2023
@webknjaz
Copy link
Member

webknjaz commented Sep 1, 2023

@jdavis74
Copy link

jdavis74 commented Sep 20, 2023

I am seeing this error when our Cheroot based server undergoes a Nessus scan. Attached are the exceptions encountered during the scan and a patch (taken against Cheroot 9.0.0) that at least prevents it from locking up. I am in no way suggesting that my patch is a proper fix!

cheroot-exc.txt

cheroot-nessuscrash.patch

webknjaz added a commit that referenced this issue Mar 31, 2024
A DoS would happen in many situations, including TLS errors and
attempts to close the underlying sockets erroring out.

This patch aims to prevent a situation when the worker threads are
killed by arbitrary exceptions that bubble up to their entry point
layers that aren't handled properly or at all.

PR #649

Fixes #358
Fixes #354

Ref #310
Ref #346
Ref #375
Ref #599
Ref #641

Resolves #365
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken triage
Projects
None yet
Development

No branches or pull requests

3 participants