Page MenuHomePhabricator

Case sensitive handling of HTTP headers like "X-Forwarded-for"
Closed, ResolvedPublic

Description

Author: wegge

Description:
According to RFC 2616 section 4.2, headers like X-Forwarded-for are case insensitive:

Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive.

However, ProxyTools.php looks for a specific case, that is "X-Forwarded-For", which means that edits coming through a proxy not using this specific case of the header will be registered as coming from the proxy itself, no matter the content of $wgSquidServers.


Version: 1.13.x
Severity: major
URL: http://www.faqs.org/rfcs/rfc2616.html

Details

Reference
bz13281

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:03 PM
bzimport set Reference to bz13281.
bzimport added a subscriber: Unknown Object (MLST).

The important part of our checks is to ensure that a malicious client that sends a header like "X_Forwarded_For" can't override the *real* "X-Forwarded-For" header. This is a problem with how HTTP headers from the server get stuck into the $_SERVER array (which follows the CGI spec, folding '-' and '_'.)

In theory, any intermediate proxy server should merge all legitimate case variations into a single one, so it should indeed be safe to do a case-insensitive check on the final apache_request_headers() array.

WebRequest::getHeader() appears to be doing this check correctly. If WebRequest is safe to use at ProxyTools time, the sensible thing would be to go ahead and just change the ProxyTools bits to use that function. If not, the two should be merged to a sane shared backend function.

wegge wrote:

Using WebRequest::getHeader would be unsafe in this case, as it gives no indication whether it's the case-folded header it's returning. Thus a header:

Client-ip: 127.0.0.1
X_Forwarded_For: spoof

Could in theory return the spoofed value. The obvious choice is to handle this in a single place, where getHeader() surely is the logical choice.

For now I've added the appropriate strtoupper() calls in wfGetForwardedFor() and wfGetAgent().