Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Security concerns over IP replacement done in rt-config.php #24

Open
gagan0123 opened this issue Apr 29, 2021 · 2 comments
Open

Security concerns over IP replacement done in rt-config.php #24

gagan0123 opened this issue Apr 29, 2021 · 2 comments
Assignees
Labels
priority/high something that should be prioritised when taking up next issues

Comments

@gagan0123
Copy link
Member

Bug Report

Current Behavior
Assumption: While creating rt-config.php it was assumed that if HTTP_CF_CONNECTING_IP header is there, it means the request is coming from Cloudflare and we replace REMOTE_ADDR and HTTP_X_REAL_IP with HTTP_CF_CONNECTING_IP.

Though true, most of the time, this assumption can lead to security risks, because, if the site is not behind Cloudflare, and a user simply edits his/her headers, to make it look like it is, then there's no check for its validity.

There can be any number of plugins in the site, that might be using the values of HTTP_X_REAL_IP or REMOTE_ADDR without checking if its even an IP address or not, and since, anyone can change that value, we can insert anything in DB(in case that plugin is saving the data in DB) 😈

Expected behavior/code
Before replacing the values, it must be validated that the request is coming from Cloudflare's IP and even after that, it must be validated that the header actually contains an IP address.
Something like this:
https://github.com/cloudflare/cf-ip-rewrite/blob/master/src/CloudFlare/IpRewrite.php#L48-L87

Steps to reproduce the bug
On any client site, that was built using fork of this repo, which is not using Cloudflare, and has a plugin that stores HTTP_X_REAL_IP or REMOTE_ADDR without validating(many plugins do that unfortunately, since no one expects someone will replace these values without validation).

Eg:
In this site, I can fool this function, that validates IP address from a pool of zombaio_ips
https://github.com/rtCamp/freebiemom.com/blob/2261a62713eb6201d152bc83118a7e5825122fcd/wp-content/plugins/mycred/addons/buy-creds/gateways/zombaio.php#L91

Even Sucuri is taking in REMOTE_ADDR, but its validation is useless since, we've set the value to whatever user wanted.
https://github.com/rtCamp/greaterkashmir.com/blob/df214c786f5f6895d352522a245ccebfd29a7627/plugins/sucuri-scanner/src/base.lib.php#L508

This even allows anyone to bypass any security measure any plugin would have took to ensure, that the request came from Cloudflare or not, for instance sucuri scanner, redirection plugin, defender security etc...

Possible Solution
Use something like this:
https://github.com/cloudflare/cf-ip-rewrite/blob/master/src/CloudFlare/IpRewrite.php#L48-L87

instead of direct replacement of IPs, in global $_SERVER variable.

@dhsathiya dhsathiya added the priority/high something that should be prioritised when taking up next issues label Apr 29, 2021
@pradeep910
Copy link

@gagan0123 I think we can check server variables using filter_input, so even if someone tries to add malicious code / string it can be sanitized with FILTER_SANITIZE_STRING.

https://github.com/rtCamp/wordpress-skeleton/blob/main/rt-config/rt-config.php#L18-L21

@gagan0123
Copy link
Member Author

@pradeep910 filter_input with FILTER_VALIDATE_IP can help a bit validating that its an IP, but it does not rectify the issue, because the user will still be able to mask or manipulate his/her IP address, which is undesirable/still a security concern.
So if we need to replace that value in Super Global variable, we need to ensure that the request really came from Cloudflare, and not someone else.

So, either

  1. Do not replace and simply remove those lines.
  2. Ensure the request originated from Cloudflare.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/high something that should be prioritised when taking up next issues
Projects
None yet
Development

No branches or pull requests

4 participants