https://httpoxy.org/

It is possible to set the HTTP_PROXY in CGI scripts by passing the Proxy header. If the script is a Python script and downloads files, urllib will happily use the attacker-supplied proxy to make requests.

This should be mitigated like it is in Perl (since 2001), Ruby, and libraries like curl.

See also: bug against python-requests https://github.com/kennethreitz/requests/issues/3422

I suspect this won’t help on OSes like Windows where environment variable names are case-insensitive (correct me if I am wrong).

Regardless, it may be worth making the change. It would be nice to also add test case(s). And I wonder if it would be appropriate to add a notice to the documentation saying that uppercase HTTP_PROXY is ignored if REQUEST_METHOD exists.

I am willing to work on documentation and tests if there is an interest in the patch.

On Windows, if REQUEST_METHOD is set, it is probably safe to assume that HTTP_* variables come from the web server: setting this variable is not the way we set a proxy there, so ignoring this dubious variable is probably fine.

I like this patch a great deal, I'll happily review it with docs and tests.

Ok, so I've taken a preliminary look at this patch. It looks good to me! I have one question: right now the patch as written will blow away not just HTTP_PROXY, but also any other mixed-case spelling of that name (e.g. HtTp_PrOxY) in a CGI environment.

That's *probably* not a concern: I think in practice such a spelling is almost never used. However, I wanted to draw it out explicitly: we should probably include a code comment that indicates that we know that it's a side effect of the code, and that we don't care.

For what it's worth, we should also consider commenting with a note regarding the CVE number assigned to Python. We may want to consider getting a CVE number for this specific fix as well, though I'd need to chat to someone in the PSRT at this point to get an idea of what they think.

Good work!

Thanks for this patch. The CVE number assigned to python -  CVE-2016-1000110.

There is redundancy in /Doc/library/urllib.request.rst change where the same paragraph is repeated twice. See if you can have it at a single location as a `Note` and reference it.

- Added CVE number
- Link to full note on getproxies() doc
- Improved comment on uppercase (lowercase will be preferred to mIxED_case too)

I think I misunderstood the Windows situation. Now I understand Windows has no lower-case variable names, so this patch would stop accepting any HTTP_PROXY variable there (in CGI mode). But that is okay by me.

I agree the mixed-case scenario is not worth worrying too much about. The normal scenario is all lowercase (http_proxy), and I think all-uppercase (HTTP_PROXY) is only supported for compatibility with some older browsers or OSes (can’t remember the details). However, since we already document “a case-insensitive approach”, perhaps it needs tweaking somehow. Perhaps it would be more correct to say, in CGI mode:

* Only lowercase _proxy suffix is accepted (stricter than just ignoring uppercase)
* No variable is accepted where names must be uppercase, i.e. Windows. As I understand it, you cannot have a lowercase http_proxy variable there.

Also, I think the “note” additions should be indented under the getproxies() etc headings. (Or drop the markup and make it an ordinary sentence or paragraph. “Note that” is also redundant IMO.)

The conversation seems to have stalled. Rémi, are you still working on the patch? Should someone take over?

I was away for a bit, I will make the requested changes tonight.

Here it goes
- Clarified that _proxy suffix should be lowercase
- Indented ..note: blocks under function/class

The patch looks good to me. I am checking this in.

For 3.3, 3.4 it seems reasonable to backport changes from issue26804 and then apply this patch. I will do this today.

New changeset 95b09ccc8a3e by Senthil Kumaran in branch '3.3':
Prevent HTTPoxy attack (CVE-2016-1000110)
https://hg.python.org/cpython/rev/95b09ccc8a3e

New changeset 3c19023c9fec by Senthil Kumaran in branch '3.4':
[merge from 3.3] Prevent HTTPoxy attack (CVE-2016-1000110)
https://hg.python.org/cpython/rev/3c19023c9fec

New changeset a0ac52ed8f79 by Senthil Kumaran in branch '3.5':
[merge from 3.4] - Prevent HTTPoxy attack (CVE-2016-1000110)
https://hg.python.org/cpython/rev/a0ac52ed8f79

New changeset 6c2e2de5ab8e by Senthil Kumaran in branch 'default':
[merge from 3.5] - Prevent HTTPoxy attack (CVE-2016-1000110)
https://hg.python.org/cpython/rev/6c2e2de5ab8e

This is also committed in 2.7 branch in ba915d561667.

This is committed in all active versions(2.7, 3.5, 3.6) and also versions which receive security updates (3.3 and 3.4).

This issue is resolved. Thank you for the patch, Rémi.


(In msg271688, I pondered if I need to backport a behavior change from issue26804 which will allow lower cased proxies, but then, I decided against it as it will introduce unnecessary changes to this security fix releases).

> (In msg271688, I pondered if I need to backport a behavior change from issue26804 which will allow lower cased proxies, but then, I decided against it as it will introduce unnecessary changes to this security fix releases).

Hmm, Senthil, while I understand, that you want to avoid unnecessary changes, doesn't this result in non deterministic behaviour of proxy handling without my patch? 

+       header. If you need to use an HTTP proxy in a CGI environment, either use
+       ``ProxyHandler`` explicitly, or make sure the variable name is in
+       lowercase (or at least the ``_proxy`` suffix).

Without 26804, this fix works by chance only for 3.3 and 3.4, since it depends on os.environ dictionary order, which is non deterministic by definition. 26804 resolves this by making sure, a lower case _proxy var has a higher priority over the other variants.

Hi Hans-Peter,

In 3.3 (95b09ccc8a3e) and 3.4 (3c19023c9fec) the change completely removes any variant of http_proxy if REQUEST_METHOD is set.  The only way to have http based proxy in cgi environment by using ProxyHandler method. This is solution introduced for the security fix.

If I backport your patch from issue26804, I imagined that we will be introducing a new feature for other environment variables like NO_PROXY, which folks might be prepared for in the security fix release. That was my concern in not making the other change.  Hope this reasoning helps.