feat/195: trust REMOTE_ADDR header only

[Sidsector9]

Description of the Change

This PR proposes a solution to prevent IP spoofing by trusting only the REMOTE_ADDR header.
It adds 2 new filters to filter trusted proxies and; headers that the trusted proxies use to forward client IP information.

Alternate Designs

TBD.

Possible Drawbacks

This will be a breaking change.

Verification Process

Checklist:

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my change.
• All new and existing tests passed.

Applicable Issues

Closes #195

Changelog

Security - New filters - rsa_trusted_proxies and rsa_trusted_headers have been added to help prevent IP spoofing attacks.

Credits

@Sidsector9, @dkotter, @peterwilsoncc, @marcS0H, @DanielRuf.

[jeffpaul]

@Sidsector9 @dkotter what else is needed to get this PR open for review (and then merged and ready for release)?

[dkotter]

what else is needed to get this PR open for review (and then merged and ready for release)?

I've left some comments on things I think should be changed, though I haven't tested this out yet.

I do think it's still worth discussing on if this is something we want to fix and if this is the best way to fix it. My concern still stands that this is a backwards compatibility break and I'm not super comfortable with that. I like this new approach of handling things as it is more secure but in essence this will potentially break (meaning not work as expected) any site that is running behind some sort of proxy (like a CDN). I almost like the idea of having this be opt in behavior, either through a filter or a new setting. So by default we keep things as-is and if individual sites want better security, they can turn this new approach on.

[jeffpaul]

New opt-in filter/setting seems like a good approach as well, in no way looking to introduce a breaking change without a user taking an educated action to trigger that change.

[jeffpaul]

@Sidsector9 checking back in to see if you're able to get this ready for review/merge?

[Sidsector9]

@jeffpaul I'll go through Darin's comments and then estimate how much time it'll take.

[Sidsector9]

@dkotter I've responded to your comments and refactored the logic a bit and updated the comments with more details. The code reflects the below flowchart.

If this gets approved, then I'll proceed with adding this as an opt-in feature.

[dkotter]

Thanks for all the work here @Sidsector9 (as well as continuing to remind me this needed my attention). I've made a few minor changes, mostly around maintaining backwards compatibility with the existing list of headers we support by default.

For information sake, the approach here (or at least the approach I was looking at, don't want to speak for Sid) was based on how Symfony handles proxies (see docs, code).

Here's an overview of how this should be working now:

1. We get the IP address from the REMOTE_ADDR header. If that doesn't exist, we return early with an empty string
2. A new filter is run that can be used to set the trusted IP addresses of a proxy. By default this returns an empty array, so user action is needed here to set this up.
3. If any value is added via the filter, we then validate the REMOTE_ADDR value matches the value of the trusted proxy IP. If it doesn't match, we return early with an empty string
4. If it does match or no trusted proxy IP addresses were added, we then get a list of trusted HTTP headers we want to look at. By default this is the exact same as it is now but a filter is added around this that can be used to add or remove trusted headers. The recommendation would be for any sites not behind a proxy, use this new filter to remove all headers (return an empty array)
5. We then add the REMOTE_ADDR header to this list so that header will always be checked
6. All of those headers are then iterated through and as soon as we find a match, we return that

There are two things I'd love some feedback on here (cc/ @jeffpaul @peterwilsoncc):

1. This new functionality is all behind filters, so code-level changes are needed to take advantage of this. We could add new settings to our admin UI to handle this but I'd recommend either not doing that at all or doing that in a later iteration (concern with cluttering that UI too much and also want to get this PR out sooner)
2. The changes we've made were done with backwards compatibility in mind. This means out of the box, things aren't any more secure than they are now, the new filters will need to be utilized to take advantage of that. I'm on the fence on this one but we could do a major version bump here and change this so we're more secure by default and you'd have to use these new filters to get back to where we are now. That impacts all existing users but does force all users to be more secure

[dkotter]

Note (mostly for myself so I don't forget) I see two unit tests are failing here. At first glance I think this isn't a regression in the code changes here but actually highlights a potential bug that is now fixed.

If you look at the current code that determines the IP address, we set the $ip variable to an empty string here. We then iterate through all our headers and set each of those to the variable $ip here. But we only return that value if it passes the filter_var check here. And then at the very end of this function we return the $ip variable.

So if none of the IP addresses validate, instead of returning an empty string we return the last value that was stored in $ip, which will be the last header that we process (should always be REMOTE_ADDR). In the case of our tests, the IP address is 127.0.0.1, which doesn't pass the filter_var check. In the current code, it still ends up returning that value but in the changes we've made here, it will return an empty string. I think that's the right approach but maybe we should always return the IP address stored in REMOTE_ADDR? In real world usage, that should always validate and shouldn't be a problem, but just need to decide for the sake of those tests.

EDIT: I've left the above for posterity but I've gone ahead and made sure we try and always have a default IP that we return (this will be the REMOTE_ADDR header). I can still see this being a bug but I can also see it being intended behavior and it is how it works now. This fixes the tests

[peterwilsoncc]

A few notes inline.

It would be nice if the tests used data providers rather than multiple tests in a single block, but not a blocker.

[peterwilsoncc]

The empty() prevents developers disallowing any proxies. I'd expect the following to prevent proxy access:

add_filter( 'rsa_trusted_proxies', '__return_empty_array' );

It might be worht checking with has_filter() but I could be convinced otherwise.

[dkotter]

Just want to make sure I'm following correctly. You're suggesting we replace the empty check with a has_filter check, allowing someone to use this filter to return an empty array, which would then pass our check but because there's no proxies to iterate over at that point, we would just return an empty string which should in essence block access to the site?

Guess I'm just wondering what the use case here would be for this, seems like this could be achieved already using the settings in the admin. I'm not opposed to making this change just having a hard time coming up with a scenario where this would be helpful.

[peterwilsoncc]

It's possible I am misunderstanding the purpose of the code. Let me do some testing during the day and get back to you. If needs be I will provide a test to clarify my thinking.

[peterwilsoncc]

I had myself totally confused when I read this code yesterday. I see what it's doing and it all makes sense to me.

[Sidsector9]

@jeffpaul this is approved. As discussed yesterday in 1:1, this can go with 7.3.2.

[dkotter]

Note that I've updated the readmes with information on how to utilize these new filters

[jeffpaul]

@Sidsector9 can you please update the changelog entry in the PR description (I added the credit line folks), thanks!