bpo-35906: Avoid headers injections in urllib

[matrixise]

https://bugs.python.org/issue35906

[eamanu]

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 encode the URL?

[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]

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?).

https://bugs.python.org/issue30500 has been fixed differently and not directly related to https://bugs.python.org/issue30458

[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!