bpo-35906: Avoid headers injections in urllib by matrixise · Pull Request #11768 · python/cpython (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation13 Commits3 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

matrixise

@matrixise

@matrixise

JulienPalard

@matrixise

eamanu

Choose a reason for hiding this comment

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

lgtm

@push0ebp

Sorry, I'm late. I'm this bug reporter.
I recommend you to encode URL, but for the previous versions, it will cause double encoding.
it may be better not to encode. after splitting lines, If it gets only the first line without remains lines, the availability may be broken. it may be OK? How about removing line breaks for preserving all of the lines. If you don't need to recommit against this commit necessarily, I also think that this commit may be OK.

@matrixise

@push0ebp

yes, but because the previous versions haven't encoded the URL, I think that urlopen() shouldn't encode the URL. it may avoid encoding(quote) two times in previous versions.
So, truncating remained lines will be OK?. How about remove CRLF for preserving the other lines?.
or raise the exception?
Actually, 3.4.6- versions which aren't be affected by this vulnerability raise the exception(no host ~~blabla).
try it with same payload.

https://bugs.python.org/issue35906#msg335005

@tomashek

Is this part of the accepted resolution of CVE-2019-9947? If so, what is blocking the merging of this PR? There has been no action for many weeks.

@mcepl

I would vote for accepting this solution. Could anybody tell me any perceivable legal URL containing \r\n combination?

@matrixise

Thanks Matěj but I am not an expert of the http lib. @orsenthil is the expert of this part of CPython

@Plazmaz

@mcepl There are no urls that are valid containing unencoded \r\n as far as I can tell. In cases where newlines are needed (such as in params) they should be url encoded

@mcepl

@mcepl There are no urls that are valid containing unencoded \r\n as far as I can tell. In cases where newlines are needed (such as in params) they should be url encoded

And if they are already not, that it is malformed URL and so it shouldn't be fixed but rejected. I am really turning towards OpenJDK has it right.

@Plazmaz

There's two important things here.

  1. This is still unpatched and easily exploitable.
  2. The best way to fix it is whitelisting vs blacklisting
    I suggest getting a solution out the door ASAP, then change the approach to whitelisting valid characters moving forwards in a second release

@mcepl

OK, I am leaning against this PR and closer to putting #2303 as a patch to all SUSE packages. I just have to torture @vstinner to explain what did he mean by #2303 (comment) . Does it mean that the solution should be somewhere lower in the stack (http.client?).

@vstinner

@csabella

Thank you for the patch. Based on the last message on this ticket, this is fixed in bpo-30458, so I'm closing this pull request. Please add a comment to bpo-30458 if you believe needs further discussion. Thanks!