"HTTPoxy", use of HTTP_PROXY flag supplied by attacker in CGI scripts

[remram44]

BPO	27568
Nosy	@jcea, @orsenthil, @encukou, @vadmium, @sigmavirus24, @Lukasa, @remram44
Files	python-2.7-httpoxy.patch python-3.5-httpoxy.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = 'https://github.com/orsenthil'
closed_at = <Date 2016-07-31.06:57:15.341>
created_at = <Date 2016-07-18.22:30:13.775>
labels = ['type-security', 'library']
title = '"HTTPoxy", use of HTTP_PROXY flag supplied by attacker in CGI scripts'
updated_at = <Date 2016-08-11.03:10:11.407>
user = 'https://github.com/remram44'

bugs.python.org fields:

activity = <Date 2016-08-11.03:10:11.407>
actor = 'jcea'
assignee = 'orsenthil'
closed = True
closed_date = <Date 2016-07-31.06:57:15.341>
closer = 'orsenthil'
components = ['Library (Lib)']
creation = <Date 2016-07-18.22:30:13.775>
creator = 'remram'
dependencies = []
files = ['43943', '43944']
hgrepos = []
issue_num = 27568
keywords = ['patch']
message_count = 17.0
messages = ['270795', '270800', '270811', '270819', '270840', '270844', '270854', '270901', '271617', '271624', '271658', '271687', '271688', '271725', '271726', '271893', '272104']
nosy_count = 9.0
nosy_names = ['jcea', 'orsenthil', 'frispete', 'petr.viktorin', 'python-dev', 'martin.panter', 'icordasc', 'Lukasa', 'remram']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'security'
url = 'https://bugs.python.org/issue27568'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4', 'Python 3.5', 'Python 3.6']

[remram44]

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

[vadmium]

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.

[remram44]

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.

[Lukasa]

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

[Lukasa]

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!

[orsenthil]

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.

[remram44]

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

[vadmium]

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

[encukou]

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

[remram44]

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

[remram44]

Here it goes

• Clarified that _proxy suffix should be lowercase
• Indented ..note: blocks under function/class