gh-144370: Disallow usage of control characters in statuses in Lib/wsgiref/handlers.py for security#144371
gh-144370: Disallow usage of control characters in statuses in Lib/wsgiref/handlers.py for security#144371benediktjohannes wants to merge 53 commits intopython:mainfrom
Lib/wsgiref/handlers.py for security#144371Conversation
|
Important note: This PR needs backport to 3.10, 3.11, 3.12, 3.13 and 3.14 please, thanks! 👍 (I'll probably add these tomorrow if there are no concerns.) |
|
@gpshead or @StanFromIreland / @ZeroIntensity please review, thanks! |
|
What is different from #144118? |
|
I would suggest that you directly comment on Seth's PR instead. |
|
@picnixz I guess that this was a misunderstanding, this PR is about a completely different thing that is changes. This is not a duplicate of the other PR because the other one is about adding HTAB again to Lib/wsgiref/headers.py, but this one is about Lib/wsgiref/handlers.py, not headers.py. This is something completely different because the other one only adds one character to be allowed, but this one reguards disallowing several characters in handlers.py (and not headers.py) which wasn‘t mentioned at all in the other PR because this concerns completely different aspects like status and some changes in debug mode in handlers.py and not headers.py. Please reopen. Thanks! |
|
@picnixz I guess, I know why this seemed to be a duplicate as I described the „differences“ to the other PR and then you thought that this was about some changes that I suggested for the other PR, but this is not what this PR is about because this is about something completely different in another location, so please reopen. Thanks! |
|
And I think that is would not be a very good idea if we combined these changes in another PR because the first one was already merged two weeks ago and the other one is only about adding allowed phrases again while this one is about the opposit (disallowing phrases) with the only thing in common that this one does not disallow one thing which will be added in the new PR again, but both other PRs concern another location which makes this not even a duplication in that way, so please reopen. Thanks! |
|
Thank you very much! |
vstinner
left a comment
vstinner
left a comment
There was a problem hiding this comment.
You should write tests for theses changes.
Co-authored-by: Victor Stinner <vstinner@python.org>
|
Hi @vstinner thank you very much for your review! I really like your suggestions and I'll make some commits in order to get them in the PR soon, thank you very much! |
Co-authored-by: Victor Stinner <vstinner@python.org>
|
(Added the "re" again because it's included in vstinner's regex and also in sethmlarson's PR so if this will be used in both cases then it's consistent in that way and it would take too much time to change this there as well and the suggestions by seth and vstinner were both with the "re", so I've added it) |
|
I'm not quite sure about the status whether it should be handled as a value or a name, so please correct me if I'm mistaken inthis case (or even none of both? but I don't think that) |
Tests are basically already included by seth's PR, but I can of course add similar ones to handlers as well. |
Please add new tests on handlers. |
|
@vstinner please re-review, thanks! (I'm still not quite sure about it because I haven't tested it (also the tests are not tested) or is this automatically done by the lint check?) |
|
(but even if this is done by the lint check automatically, I'm not quite sure about the handling of the status) |
|
and thanks for the review by the way 👍 |
|
Ahh, this seems actually to be checked automatically by the Tests based on what I've seen! Nice! But I'm not quite sure about the correct classification of the status nevertheless, so please have a look there, thanks! |
|
Ah, seems like it's because of the ` where on one side is one and on the other are two |
|
It should only have one ` I guess (the other PRs also only include one always, so I think that this is correct) |
|
I've changed the news a little bit to "to prevent injections." because it's not only header, but also status injections and thank you very much for your review @vstinner and thanks for the suggestions, both are good! 👍 |
|
And please also have a look at the comment above concerning the handling of the status and the status message because of HTAB, thanks! @vstinner |
|
Edit: I've added @sethmlarson to ACKS |
Inserting newlines in a status is mostly useful to inject a HTTP header, no? |
Mostly (probably) yes, but I thought there may be edge cases where it‘s used differently and my message is a little bit more universally, but I‘d agree changing it to your version if you want me to do so. |
|
I've now implemented your suggested changes (_convert_string_type() now stays unchanged) and I've added a security warning that headers_class must be used in the line above (or at least another check checking for those chars) because otherwise the header name and value in debug mode are injectable because they use _convert_string_type() without the check of C0 control characters. So now the double security check is removed because it's already safe because they're checked via headers_class and I added a warning so that this isn't changed in order to prevent further issues with this. 👍 Thanks for your review @vstinner and the suggestions! Please also have a look at the comments above, e.g. concerning HTAB and so on, thank you! And by the way, I've added a "are" to the message because it was missing. |
benediktjohannes
changed the title
Lib/wsgiref/handlers.py for securityLib/wsgiref/handlers.py for security
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
… also in this case
|
Thanks @vstinner for the suggestions! Please also have a look at all the messages above (if you haven't already, e.g. when it comes to HTAB (whether we should use the chars allowed in names or values)) and thank you for your reviews! I hope that everything is fine now! 👍 |
5 participants
This PR is opened as public as the primary described CVE is already publicated and this only contains one certain point that has to be changed in code in Python and refers to issue #144370 (gh-144370).
In Lib/wsgiref/handlers.py at 260 (def _convert_string_type(self, value, title):) it is necessary to add the same fix as in #143916 requested as well (credits to @sethmlarson 👍). This references #143917 and #144118 and already includes the latest fix for the inclusion of HTAB, additions to this in form of tests will automatically be added by merging #144118.
There are some differences to wsgiref.headers.Headers that I made because I chose the default status of "name" to be "True" instead of false as "True" includes one more disallowed character for safety in future use cases if not defined otherwise, but I also added "name=True" even if not necessary in use cases just for safety for another case (if the default case was changed in order to produce similarity to wsgiref.headers.Headers while I'd recommend to change wsgiref.headers.Headers). I've also made an addition to the raised "ValueError" including "values" (without a further description of the very exact in- and exclusions like HTAB because this doesn't seem to be user-sided relevant to me) and I've used a slightly shorter internal form for the variables (which can be changed if wanted, but it's just style).
Important note: I was / am not quite sure about these changes as they were not tested, but I'm highly confident that this should be correct as it (mostly) bases on changes which were already made in other PRs (or at least in one other PR because this is the only merged one (but this also refers to a "non-merged" PR, so has to be concretized in that way)). And another important aspect is that I was not quite sure about the "status" (whether my classification is correct here as a "non-name value" when considering the handling of the characters, so please correct me if I'm mistaken (and I wasn't quite sure as well at the question whether potentially there should be added some different disallowed characters for this or some disallowed be removed, so please correct me if I'm mistaken)). Please also correct me if I'm mistaken in general anywhere in this PR (maybe this isn't even necessary for this case and I'm completely mistaken because this wasn't tested, but I'm pretty confident just from looking at the code).