Skip to content

DefaultRedirectStrategy vulnerable to open redirect phishing attacks using protocol relative paths (double slash) #4197

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

Open
louisburton opened this issue Feb 1, 2017 · 4 comments
Labels
status: waiting-for-triage An issue we've not yet triaged

Comments

@louisburton
Copy link

Summary

If a org.springframework.security.web.DefaultRedirectStrategy is configured to be context relative, it does not prevent against protocol relative URLs. The configuration would suggest protection against phishing attacks, but this is not the case if it can take you to another domain.

This was mentioned here with a suggested solution by Wojciech Gizynski. If worried as to the suggestion, protection against only use cases that start with // might be lower risk:
#2405 (comment)

Actual Behavior

A query parameter such as ?redirect=//phishing.com will redirect you to another domain despite the strategy being configured as context relative.

Expected Behavior

A query parameter such as ?redirect=//phishing.com will redirect you to a context relative location if the strategy has been configured as context relative.

Configuration

  <bean id="simpleUrlAuthenticationSuccessHandler"
        class="org.springframework.security.web.authentication.SimpleUrlAuthenticationSuccessHandler">
    <property name="targetUrlParameter" value="continue"/>
    <property name="defaultTargetUrl" value="/"/>
    <property name="redirectStrategy">
      <bean class="org.springframework.security.web.DefaultRedirectStrategy">
        <property name="contextRelative" value="true"/>
      </bean>
    </property>
  </bean>

Version

4.2.1.RELEASE

@mpalourdio
Copy link
Contributor

Wouldn't this fix solve this problem too ?

#4142

@louisburton
Copy link
Author

Hi @mpalourdio - thanks for responding quickly.
I personally don't feel the current PR for #4142 resolves the issue and prefer your later suggestion of throwing an exception.

The issue is worded from the perspective of it being done by mistake rather than for phishing, and leans to tolerance. However, as a redirect class from a security library, I think it should lean to caution, and assume that people setting contextRelative are focused on security enforcement rather than URL preference.
I feel that if you are setting it to be contextRelative I expect it to be enforcing that a contextRelative path is returned. I feel it is why it doesn't currently return the absolute path if you have configured it to be context relative.

@mpalourdio
Copy link
Contributor

mpalourdio commented Feb 6, 2017

I agree that the default behaviour in this library should be very defensive in most cases. Maybe @rwinch can give a feedback on those 2 issues.

I must say that at first sight, the default behavior of DefaultRedirectStrategy#calculateRedirectUrl was not very obvious to me.

That's why too I wouldn't see a problem to throw an exception in the case of #4142

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label May 7, 2019
@barrelmonkey
Copy link

barrelmonkey commented Nov 4, 2019

I appreciate this is quite old, but it remains open and I wanted to comment on our recent pen test findings.

the calculateRedirectUrl method strips out the scheme and then looks for the contextPath.
For applications that are running at base it finds '/', removes this, and then returns the rest of the url.

This presents a vulnerability that can be exploited by attackers added more '/' characters in front of their url.

For example the following would all correctly keep you within the main application after running through this method

https://www.mywebsite.com?redirect=https://google.com (strips https:// then has no '/' left for context, so url is blank)
https://www.mywebsite.com?redirect=https://google.com/search (strips https:// then finds the latter '/' so returned url is 'search'
https://www.mywebsite.com?redirect=//google.com (finds '/' as context so url returned is path relative '/google.com')
https://www.mywebsite.com?redirect=///google.com (finds '/' as context then returns '//google.com' which is a full external redirect)

For the time being we are cleaning the url before it goes through to the calculateRedirectUrl method, by not allowing more than 1 consecutive '/'character, but I think this should be part of this security method.

To add further complexity this can be exploited by adding more '/' characters in the middle of the redirect url.

For example
https://www.mywebsite.com?redirect=https://google.com/https://anothergoogle.com would redirect me to https://anothergoogle.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: waiting-for-triage An issue we've not yet triaged
Projects
None yet
Development

No branches or pull requests

4 participants