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

louisburton commented 7 years ago

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: https://github.com/spring-projects/spring-security/issues/2405#issuecomment-180600671

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 commented 7 years ago

Wouldn't this fix solve this problem too ?

https://github.com/spring-projects/spring-security/pull/4142

louisburton commented 7 years ago

Hi @mpalourdio - thanks for responding quickly. I personally don't feel the current PR for https://github.com/spring-projects/spring-security/pull/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 commented 7 years ago

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

barrelmonkey commented 4 years ago

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

spring-projects / spring-security