silverstripe / cwp-core

CWP basic compatibility module
BSD 3-Clause "New" or "Revised" License
3 stars 12 forks source link

Direct REMOTE_ADDR usage makes assumptions about infrastructure configuration #99

Open chillu opened 3 years ago

chillu commented 3 years ago

https://github.com/silverstripe/cwp-core/blob/2/src/Control/CwpBasicAuthMiddleware.php#L82

$_SERVER[REMOTE_ADDR] should not be used directly, we've got HTTPRequest->getIP() for this purpose. Specifically, the HTTPRequest IP can be optionally overwritten with proxy IPs instead (through trusted proxy IPs and X-Forwarded-For headers). This works in other parts of framework.

When this code was written, it was reasonable to make assumptions about the infrastructure configuration. In the only valid WAF proxy available on CWP (Imperva), this handling of proxy IPs happens transparently in the infrastructure. This recipe might evolve beyond CWP though (e.g. porting code over, renaming the module), at which point it'll need to deal with other infrastructure proxy layers - and use the wrong information.

This isn't a security issue as such (doesn't allow bypass of the IP allowlist), instead it'll lock out everyone when infrastructure proxies are configured differently (IP allowlist will never match).