gh-142533: Document CRLF injection vulnerability in http.server and wsgiref modules#143395
Conversation
… and wsgiref modules
|
Hi, according to the Dev Guide, the document only changes don't need a news entry file. |
Doc/library/http.server.rst
Outdated
| This method does not reject input containing CRLF sequences allowing the | ||
| possibility of CRLF injection, where a single method call can inject | ||
| multiple arbitrary headers. |
There was a problem hiding this comment.
| This method does not reject input containing CRLF sequences allowing the | |
| possibility of CRLF injection, where a single method call can inject | |
| multiple arbitrary headers. | |
| This method does not reject input containing CRLF sequences. |
Only mention the possibility of CRLF injection in the security consideration section.
There was a problem hiding this comment.
Thank you for the suggestion. Addressed for both modules
Doc/library/wsgiref.rst
Outdated
| This method does not reject input containing CRLF sequences allowing the | ||
| possibility of CRLF injection, where a single method call can inject | ||
| multiple arbitrary headers. |
Doc/library/wsgiref.rst
Outdated
| Security considerations | ||
| ----------------------- |
There was a problem hiding this comment.
Please format this the same as we did for Http.server, that is: add a label and enough blank lines.
|
@picnixz thank you for the suggestions. I believe this is addressed now. |
|
cc @SethMichaelLarson @gpshead @serhiy-storchaka: Do you want to double check this change? |
|
Thank you for review approval to both. The first GitHub profile mentioned in the comment above says in their bio we likely meant @sethmlarson so tagging them here. |
hugovk
added
needs backport to 3.13
vstinner
dismissed
their stale review
wsgiref was modified in the meanwhile to reject control characters including newline characters.
Doc/library/wsgiref.rst
Outdated
|
|
||
| Content-Disposition: attachment; filename="bud.gif" | ||
|
|
||
| This method does not reject input containing CRLF sequences. |
There was a problem hiding this comment.
This is no longer true, please check the update wsgiref code.
There was a problem hiding this comment.
Indeed, the vulnerability was addressed while this change was in review. Thank you.
I removed my wsgiref.rst changes, while leaving http.server.rst change in place as it remains relevant.
…cumentation since the issue was recently fixed
Doc/library/http.server.rst
Outdated
| requests, this makes it possible for files outside of the specified directory | ||
| to be served. | ||
|
|
||
| The :meth:`BaseHTTPRequestHandler.send_header` method assumes sanitized input |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Thanks @tadejmagajna for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…er doc (pythonGH-143395) (cherry picked from commit 617f4cc) Co-authored-by: Tadej Magajna <tmagajna@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org>
|
GH-148020 is a backport of this pull request to the 3.14 branch. |
…er doc (pythonGH-143395) (cherry picked from commit 617f4cc) Co-authored-by: Tadej Magajna <tmagajna@gmail.com> Co-authored-by: Victor Stinner <vstinner@python.org>
|
GH-148021 is a backport of this pull request to the 3.13 branch. |
|
Since wsgiref was modified recently to reject control characters, I hesitated to propose changing http.server to reject also control characters such as CRLF. But Serhiy is right, http.server users are responsible to pass data encoded for HTTP and so wrap long lines with CRLF: #142533 (comment). So it's better to document the vulnerability rather than fixing it. http.server has now a long history, it's late to change how data is encoded without breaking existing code. |
|
PR merged, thanks for your contribution. I backported the doc change to 3.13 and 3.14 branches. |
5 participants
This change documents the CRLF injection vulnerability for http headers in
http.serverandwsgirefmodules.Initial report in #142533 focused on
http.serveronly, though further discussion suggested also addressing a closely related vulnerability inwsgirefreferenced in related issues #55880 and #72964.After discussing #142605, we pivoted from a direct fix to a documentation update because a fix would disrupt users who rely on using the vulnerability for non-malicious purposes.
The change documents the low-level vulnerability (i.e. absence of checking for CRLF) in mehod-specific sections while describing the high level implications (i.e. assuming sanitized input) under the "Security considerations" section.
http.server#142533📚 Documentation preview 📚: https://cpython-previews--143395.org.readthedocs.build/