Update IP-based security configurations to more sane defaults (now that CORS + CSRF are enabled)

[tdonohue]

References

• Fixes Default Proxy settings should automatically use CORS settings #2984 (by defaulting the proxies.trusted.ipranges setting to the IP address of dspace.ui.url, determined dynamically). This change also fixes Unable to login/create session when dspace 7 angular is in production mode dspace-angular#1036
• Fixes JWT Authentication Token(s) should not store IP address by default #2938 (by removing jwt.*.token.include.ip settings)

Description

This PR includes the following changes:

• proxies.trusted.ipranges - This setting has now been commented out by default. When commented out, the ONLY trusted IP is localhost (127.0.0.1), and (possibly) the IP address of dspace.ui.url (see next configuration). Therefore, this setting should ONLY be used or changed if you need to add additional trusted proxies.
• proxies.trusted.include_ui_ip - This is a new configuration which decides whether to automatically trust the UI as a proxy. When enabled (which is default), the backend determines the IP address of dspace.ui.url and adds it to the list of trusted proxies.
• jwt.*.token.include.ip - This setting has been removed entirely. These settings allowed you to store your IP address in your JWT auth token, therefore disabling token reuse across IP addresses. However, this has the side-effect of forcing you to login again if your IP changes (and if you have a non-static IP, it's possible your IP could change during your session.
  • When authenticating via the Angular UI, your token is already protected by HTTPS + CORS + CSRF.
    • Authentication is already blocked from untrustworthy origins by CORS. See the new testCannotAuthenticateFromUntrustedOrigin() test to AuthenticationRestControllerIT, alongside the existing test testCannotReuseTokenFromUntrustedOrigin().
    • CSRF provides extra security here too, as to successfully perform authenticated actions, you must (1) be coming from a trusted origin and (2) have a valid JWT and (3) have a valid CSRF token.
  • When authenticating via the REST API (without a browser), CORS has little effect (as an Origin can be faked in that scenario)... However, you are still kept safe by HTTPS and CSRF.
• Building tests for the above changes required upgrading Mockito & switching to using mockito-inline in order to use the new support for mocking static methods.
• Finally, I copied the rest.cors.allowed-origins setting over into our local.cfg.EXAMPLE to make that setting more prominent.

Instructions for Reviewers

• Review the code changes, including new/updated Tests
• Verify basic actions like authentication, editing, etc still work
• Optionally, test that Unable to login/create session when dspace 7 angular is in production mode dspace-angular#1036 is fixed.

[tomdesair]

Since I was the developer who orginally implemented the security measures, let me provide a bit more context on why they where added and why they are different than CORS and CSRF.

The proxies.trusted.ipranges was added to prevent IP address spoofing at the API layer. If I write a script that sends an API request with an X-Forwarded-For header containing a fake IP, DSpace will assume the fake IP to be the client IP if you're not checking the list of trusted proxies. More information can be found here: https://portswigger.net/kb/issues/00400110_spoofable-client-ip-address

The jwt.*.token.include.ip option is there to prevent session hijacking (https://owasp.org/www-community/attacks/Session_hijacking_attack). A recommended way to limit the session hijacking possibilities is to link the original client IP to the session (thus in DSpace the JWT session token), see https://cheatsheetseries.owasp.org/cheatsheets/Session_Management_Cheat_Sheet.html#binding-the-session-id-to-other-user-properties

Note that both measures are not related to CSRF: https://owasp.org/www-community/attacks/csrf . Trusted proxies or ip-locked session tokens will not prevent a user from clicking on an maliciously-crafted link.

CORS is a pure browser-based security measure, see https://owasp.org/www-community/attacks/CORS_OriginHeaderScrutiny:

The risk here is that a web client can put any value into the Origin request HTTP header in order to force web application to provide it the target resource content. In the case of a Browser web client, the header value is managed by the browser but another “web client” can be used (like Curl/Wget/Burp suite/…) to change/override the “Origin” header value. For this reason it is not recommended to use the Origin header to authenticate requests as coming from your site.

So for non-browser based API access (malicious scripts), CORS does not offer any protection.

Since I'm not active anymore, I don't have all the context on why this PR was created so I will leave it up to the community if the above two security measures are still relevant in the current DSpace implementation. But I wanted to make sure that everyone understands with which purpose they were originally added.

[tdonohue]

@tomdesair : I appreciate your feedback here. However, both of these settings have become problematic in early user testing of DSpace 7, which is why we are looking to remove them:

• proxies.trusted.ipranges has caused issues cause it MUST be configured with the IP address of all known/trusted clients, or else authentication is blocked from clients. This includes blocking the UI by default, unless you configure this with the public IP of the UI. This configuration was only used for things like logs & statistics and had nothing to do with any code related to security/authentication/authorization. So if someone manages to "fake" their IP in DSpace's stats or logs, it's not as big of a deal as causing out-of-the-box authentication issues with all clients. (UPDATE: I reconsidered this statement below)
• jwt.*.token.include.ip has caused issues when proxies are in use.. If your IP address actually changes during your session (because of a proxy or because you have a non-static IP), you lose your entire session. While I understand Session Hijacking is the reason for this configuration, it'd only be possible to "hijack" a DSpace session if you can (1) fake a trusted Origin, (2) steal a user's active JWT and (3) steal that same user's active, unique CSRF Token (which is separate from the JWT). Stealing the JWT alone will not allow you to hijack a session... previously (when you built this feature) it was possible to hijack the session just by stealing/leaking the JWT.

[mwoodiupui]

If it is decided that proxies.trusted.ipranges must be kept, can we just automatically add the address of the UI server (derived from dspace.ui.url) to that list?

If we find no good solution to the problems with jwt.*.token.include.ip, a work-around for session hijacking would be to enforce HTTPS for all sessions to protect session secrets. Probably this should be a simple recommendation, not baked into the code.

[tdonohue]

As I noted in today's meeting, overnight I rethought my response to @tomdesair 's kind feedback.

I still think we can remove jwt.*.token.include.ip safely, as HTTPS + CSRF Token + Auth Token should sufficiently protect us here. And CORS helps as well for browser-based requests (only).

However, we may want to keep proxies.trusted.ipranges and configure it to have a more "sane" default (like @mwoodiupui says, it should default to the Angular UI's IP address, determined dynamically, and perhaps 127.0.0.1). The reason to keep this is to avoid spamming of the DSpace Statistics with fake IPs.

Therefore, I'm temporarily flagging this as WIP until I rework that configuration.

[tdonohue]

@mwoodiupui and @abollini : I've removed the work in progress label as this PR is now ready for review.

I've updated the description of the PR to describe the new implementation.

• I restored the proxies.trusted.ipranges setting, configuring it to now default to the IP address of dspace.ui.url
• I kept the commit to delete the jwt.*.token.include.ip setting... as previously noted, our JWT is protected already via HTTPS + CSRF (plus CORS when using a web browser), so protection based on IP-address seems unnecessary. Plus, when this setting is enabled, you would need to re-login anytime your IP changes (which can occur if you are on a non-static IP).

[benbosman]

Hi Tim,

This PR looks good in general, but I'd prefer a few small change to reduce the odds of mistakes being made with the setup. I've included the details inline

[abollini]

hi @tdonohue the code looks good to me. I agree with the suggestion from @benbosman , please apply them before to merge this PR. I have only another concern about the use of mockito-inline, we could avoid it in this case just introducing a new utility class not final/static

[tdonohue]

@benbosman and @abollini : This PR is now ready for re-review. The test failures I mentioned in the meeting were temporary failures. (Completely unrelated to the changes in this PR, apparently a few of our Statistics ITs can fail if the tests run at the exact time when the month changes...and by random chance, the tests yesterday ran while the date changed from March 31 -> April 1. I reran all tests today and they all succeeded.)

In any case, I've addressed all your prior feedback (see comments above), so I think this should be rather easy to review.

@benbosman : As mentioned in today's meeting, the changes in this PR (around determining trusted IPs) might also be useful in determining when we accept a GET to the /api/authn/shortlivedtokens endpoint as you already documented in DSpace/RestContract#159

[benbosman]

@tdonohue I've tested this implementation, and noticed a problem depending on the setup

I was able to create a setup with:

• A call performed by Angular
• Apache configured using mod_http (adding a proxy header)

This caused getClientIp(String remoteIp, String xForwardedForHeaderValue) to receive:

• remoteIp: 127.0.0.1 (from Apache connecting to REST)
• xForwardedForHeaderValue: containing my ip, the server's ip from the Angular proxy

The trustedProxies contains 127.0.0.1 and the server's ip, but getXForwardedForIpValue() has returned the server's ip, loosing my ip

This is due to a missing trim

Can you adjust this, and also add a test?

[tdonohue]

Thanks @benbosman for your review. I've updated this PR again based on your feedback (see also my answers to your comments).

[benbosman]

Thanks @tdonohue

I've reviewed the code and tested again.

I was able to identify it did see my IP correctly, even when using Apache with mod_http, and Angular.
When requesting the main page "/", Angular did proxy the request to Apache (using Angular's public IP), which proxied the request to tomcat (using localhost). This all worked correctly.

When trying to spoof my IP by adding another "X-Forwarded-For" in Curl (also including a cookie to ensure this code is called:

curl 'https://dspace7-test.atmire.com/' --header "X-Forwarded-For: 192.168.0.2"

• org.dspace.service.impl.ClientInfoServiceImpl#getXForwardedForIpValue will first read my IP as 192.168.0.2
• Hereafter it will read it as my real IP
• Hereafter it will read the Angular IP, but skip it (keeping my real IP)
• This will return my actual IP

I also couldn't spoof it by:

• Using the Angular server's IP in the X-Forwarded-For header
• Using multiple IPs in the X-Forwarded-For header
• Using a combination of a spoofed IP and the Angular server's IP in the X-Forwarded-For header

[abollini]

code and tests look good thanks @tdonohue