Vulnerable code suggested on OWASP?

Question

Session Hijacking Prevention

It is good practice to bind sessions to IP addresses, that would prevent most session hijacking scenarios (but not all), however some users might use anonymity tools (such as TOR) and they would have problems with your service.

To implement this, simply store the client IP in the session first time it is created, and enforce it to be the same afterwards. The code snippet below returns client IP address:

$IP = (getenv ( "HTTP_X_FORWARDED_FOR" )) ? getenv ( "HTTP_X_FORWARDED_FOR" ) : getenv ( "REMOTE_ADDR" );

Source: https://www.owasp.org/index.php/PHP_Security_Cheat_Sheet#Session_Hijacking_Prevention

Seems to me this code is vulnerable to IP spoofing! Right?

Edit: AFIK, X_FORWARDED_FOR is an HTTP header that is very easy for clients to set to anything they want.

Answer 1

This code allows an attacker who knows the victim's X-Forwarded-For: header and the victim's session ID to login as that user. If the victim doesn't have an X-Forwarded-For: header, the attacker can put the victim's IP address in his header and the code will use that value as his legitimate IP address instead of his actual IP address. This is the vulnerability.

• In a network-sniffing scenario, the attacker gets both of these values at the same time, hence adding this code does not protect against network sniffing.

• In an XSS scenario, where the attacker injects javascript to steal the user's cookie, this is usually done by causing the victim's browser to make a request to an attacker-controlled web server with the cookie value appended as a GET parameter. The X-Forwarded-For: header of the victim would be supplied to the attacker in that request.

I haven't yet been able to think of any scenario where an attacker could obtain a victim's cookie but not be able to obtain the value of a legitimate X-Forwarded-For: header or, if no header was present, the victim's IP address.

So the answer is yes, as written, this code does nothing to improve security. It should trust the directly connected IP address only.

---

Note: If your web application server is behind a reverse web proxy (such as nginx, Varnish, HAProxy, mod_proxy, Amazon ELB and many others) that always adds the connecting IP address to an X-Forwarded-For header and makes a new connection to your web application server with its own IP address, you can "trust" part of the value of this header.

Modules such as mod_rpaf or mod_remoteip for Apache can be configured with a list of trusted proxy IP addresses and will replace the value of REMOTE_ADDR with the trusted part of the X-Forwarded-For header for connections coming from those addresses. This REMOTE_ADDR will be available in PHP as $_SERVER['REMOTE_ADDR] and will be logged in your Apache log files as the true connecting IP address.

If you don't do this (or the equivalent if you are not using Apache) then you will only ever see connections from one IP address, your reverse proxy server, and therefore all your sessions will contain the same IP address.

Answer 2

I'd say you're right! They don't seem to have kept in mind that the HTTP header X-FORWARDED-FOR might return '; DROP TABLE users;--.

They suggest this in order to also lock sessions for people behind proxies, and name Tor as example. This is sort of stupid; Tor never forwards the original IP for obvious reasons. Moreover, it doesn't (shouldn't?) even look at the packet itself; it merely forwards it until it's out of the network. What's the use of a proxy if the original IP is forwarded anyway? Just lock it to the remote address, that's the only safe bet. Even if you were to validate the x-forwarded-for header as valid IPv4 or IPv6 address, it might still contain an arbitrary IP.

Below the example code on that page, they do think of local IPs and IPv6 (even though the IPv6 examples contain invalid addresses):

Keep in mind that in local environments, a valid IP is not returned, and usually the string :::1 or :::127 might pop up, thus adapt your IP checking logic.

But they should also have thought about that the header can contain arbitrary data. Nice find!

Since it's a wiki, I guess anyone with an account could edit the page. If not, you should contact them. This is indeed bad practice, and many people might even insert $IP unescaped into a database. It's usually not possible to spoof the REMOTE_ADDR environmental variable, so it should be safe under normal circumstances (though I wouldn't insert it unescaped even if that were the case), but the X-forwarded-for header breaks it all here.

Answer 3

This code is a security control, and as such, it must be evaluated against the threat it was intended to control. In this case, there is no special access being granted based on IP address. Instead, IP address is being used as an additional constraint on an already strong authorization mechanism (HTTP Cookies).

It is true that an attacker could spoof a user's IP through the X-Forwarded-For header and bypass this additional check, but that means that the attacker must not only know the user's session ID (cookie), but also the user's IP address. This makes exploitation of session-stealing XSS more difficult.

Consider the other ways this control could have been approached. First, if the control were not in place at all, then any session-stealing attack (XSS, traffic sniffing, SSL MITM, etc) would result in a fully-usable session for the attacker, with no extra work.

Instead, if the control were implemented without consulting the X-Forwarded-For header, but only HTTP_REMOTE_ADDR, then an attacker who is behind the same proxy (in a school or business, for instance) would not need any knowledge of the control or the user's internal IP to have a fully-usable session, since all outgoing HTTP sessions would appear to come from the same IP (that of the proxy).

By adding the check for X-Forwarded-For, the control requires an attacker to have knowledge of the user's IP address, and the ability to spoof it in the X-Forwarded-For header (not every attacker is completely unconstrained, so there is a reduction in risk. Not an elimination of risk, since we must assume there are some unconstrained attackers.)

One way to possibly strengthen this control would be to keep an extra bit of information: The source of the last IP address that was used. For instance, if the session were first granted and associated with an IP that came from HTTP_REMOTE_ADDR, the control could require that future traffic on that session must come from an IP gleaned from HTTP_REMOTE_ADDR, not X-Forwarded-For. This eliminates some chance for spoofing. The gains for implementing the check in reverse (preferring X-Forwarded-For over HTTP_REMOTE_ADDR) are probably not worth the effort, since it is much harder to spoof a real network address than an HTTP header.