Proxy headers, what if a reverse proxy uses one header lower in the array than a header a client might spoof?

[Sander-Kastelein]

What would happen when the following would be done:

I have WHIP setup with a custom-reverse-proxy which would use a header called: "HTTP_X_FORWARDED" for the clients real ip-address.

So let's say BOB sends a normal HTTP request from the ip-address of 1.1.1.1 through the reverse proxy (which is white-listed, and the PROXY_HEADERS mask is enabled). The revere proxy would pass along the 1.1.1.1 fine, and then WHIP would see it and correctly tell the ip-address is 1.1.1.1

Now Bob sends an extra header with the following: "HTTP_CLIENT_IP: 2.2.2.2" Would WHIP not see HTTP_CLIENT_IP: 2.2.2.2 before it saw "HTTP_X_FORWARDED: 1.1.1.1" and therefor assume (wrongly) that the client's ip-address is 2.2.2.2?

If so, a possible fix could be to parse all the headers in the PROXY_HEADERS and check if they're all equal, if not the request is probably malicious, and WHIP should throw an exception.

[jdpanderson]

Your concern is valid, though I think it would be best addressed with a documentation update.

The proxy headers method is for out-of-the-box support for headers used by many proxy servers. I think the custom headers method is better suited to your use case. (Is there a difference in behavior between the two methods that prevents you from using the custom headers method over the proxy headers method?)

Otherwise, the issue you've identified is a pitfall that we don't want anyone to encounter accidentally. I think a clear statement about this in the top-level documentation would be the best way to address this problem.

Do you believe this to be a satisfactory solution?

[Sander-Kastelein]

A better way would probably be to remove the reverse proxy configuration constants, and make it so that they need to be configured manually, and then add documentation on how to configure the lib for some popular reverse-proxies such as cloudflare and nginx.

[ckdarby]

Otherwise, the issue you've identified is a pitfall that we don't want anyone to encounter accidentally. I think a clear statement about this in the top-level documentation would be the best way to address this problem.

I think this is the best approach.

[jdpanderson]

@Sander-Kastelein, it sounds like custom headers does exactly what you're asking for. It behaves like proxy headers, except it has to be configured manually.

use Vectorface\Whip\Whip;

$whip = new Whip(
    Whip::CUSTOM_HEADERS,
    ['HTTP_X_MYHEADER' => ['1.2.3.4']] // Whitelist your proxy
);
$whip->addCustomHeader('HTTP_X_MYHEADER');
$ip = $whip->getValidIpAddress();

// or for testing
$source = ['REMOTE_ADDR' => '1.2.3.4', 'HTTP_X_MYHEADER' => '4.3.2.1'];
$ip = $whip->getValidIpAddress($source);
assert($ip === '4.3.2.1');

I agree with you about the ordering issue, in that the proxy headers method is not suitable for use in scenarios where the IP address needs to be trusted. (E.g. in a security context.)

I still believe that the proper solution is to make the purpose of the two methods obvious in the main documentation, and to point out the security implications the proxy headers method.

@ckdarby, thank you for your input. I'm glad you're in agreement.

[jdpanderson]

I've added documentation that clearly exposes this issue, and highlights Whip's intended use when behind a trusted proxy.

Relevant commit: 35a644c6694b7de2f3d93d2058ea200b9c6f6690