Uploaded image for project: 'Sling'
  1. Sling
  2. SLING-3443

Parameter based redirection in FormAuthenticationHandler should not handle absolute urls

    Details

      Description

      Suppose your login url is: http://blah/blah?resource=http://www.google.com

      Then after login succeeds, user would be redirected to http://www.google.com

      Will be submitting a pull request for this.

        Issue Links

          Activity

          Hide
          githubbot ASF GitHub Bot added a comment -

          GitHub user bond- opened a pull request:

          https://github.com/apache/sling/pull/12

          SLING-3443: Parameter based redirection vulnerability in FormAuthenticationHandler

          FormAuthenticationHandler didn't url encode the parameter(Authenticator.LOGIN_RESOURCE) before redirection. This leads the attacker to use this parameter to redirect to a different domain. This may also help in phishing attacks.

          This was initially spotted in one of our applications which use org.apache.sling:org.apache.sling.auth.form:1.0.2

          This pull request fixes the vulnerability.

          You can merge this pull request into a Git repository by running:

          $ git pull https://github.com/bond-/sling sling-3443

          Alternatively you can review and apply these changes as the patch at:

          https://github.com/apache/sling/pull/12.patch

          To close this pull request, make a commit to your master/trunk branch
          with (at least) the following in the commit message:

          This closes #12


          commit d1531735762e7423404f1e304dfc4c483f9556de
          Author: Raviteja Lokineni <raviteja.lokineni@gmail.com>
          Date: 2014-03-08T12:24:52Z

          SLING-3443: Parameter based redirection vulnerability in FormAuthenticationHandler


          Show
          githubbot ASF GitHub Bot added a comment - GitHub user bond- opened a pull request: https://github.com/apache/sling/pull/12 SLING-3443 : Parameter based redirection vulnerability in FormAuthenticationHandler FormAuthenticationHandler didn't url encode the parameter( Authenticator.LOGIN_RESOURCE ) before redirection. This leads the attacker to use this parameter to redirect to a different domain. This may also help in phishing attacks. This was initially spotted in one of our applications which use org.apache.sling:org.apache.sling.auth.form:1.0.2 This pull request fixes the vulnerability. You can merge this pull request into a Git repository by running: $ git pull https://github.com/bond-/sling sling-3443 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/sling/pull/12.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #12 commit d1531735762e7423404f1e304dfc4c483f9556de Author: Raviteja Lokineni <raviteja.lokineni@gmail.com> Date: 2014-03-08T12:24:52Z SLING-3443 : Parameter based redirection vulnerability in FormAuthenticationHandler
          Hide
          bond_ Raviteja Lokineni added a comment -

          Hi Carsten Ziegeler,

          This seems to be related to SLING-3141. Please look at the pull request whenever you find sometime.

          Thanks in advance.

          Show
          bond_ Raviteja Lokineni added a comment - Hi Carsten Ziegeler , This seems to be related to SLING-3141 . Please look at the pull request whenever you find sometime. Thanks in advance.
          Hide
          cziegeler Carsten Ziegeler added a comment -

          Thanks for your patch, I've applied a slightly modified version which also replaces the usage of deprecated API.
          The basic difference is that not AuthUtil.sendRedirect is used to do the redirect as this one always at least adds the login resource parameter which I think is wrong as the authentication was successful. The original code did not any parameter to the redirect, so I think we should keep it like this

          Show
          cziegeler Carsten Ziegeler added a comment - Thanks for your patch, I've applied a slightly modified version which also replaces the usage of deprecated API. The basic difference is that not AuthUtil.sendRedirect is used to do the redirect as this one always at least adds the login resource parameter which I think is wrong as the authentication was successful. The original code did not any parameter to the redirect, so I think we should keep it like this
          Hide
          bond_ Raviteja Lokineni added a comment -

          Yeah and I also think that the new code that has been added should be covered by tests. It's the best way to make sure that we don't hit any regressions.

          Show
          bond_ Raviteja Lokineni added a comment - Yeah and I also think that the new code that has been added should be covered by tests. It's the best way to make sure that we don't hit any regressions.
          Hide
          cziegeler Carsten Ziegeler added a comment -

          Yepp, I agree - I'Ve added your test and adapted it

          Show
          cziegeler Carsten Ziegeler added a comment - Yepp, I agree - I'Ve added your test and adapted it
          Hide
          bond_ Raviteja Lokineni added a comment -

          Just a suggestion. I think you might want to fix your build on: https://travis-ci.org/apache/sling/

          Is there someother CI that is in use?

          Show
          bond_ Raviteja Lokineni added a comment - Just a suggestion. I think you might want to fix your build on: https://travis-ci.org/apache/sling/ Is there someother CI that is in use?
          Hide
          githubbot ASF GitHub Bot added a comment -

          Github user bond- closed the pull request at:

          https://github.com/apache/sling/pull/12

          Show
          githubbot ASF GitHub Bot added a comment - Github user bond- closed the pull request at: https://github.com/apache/sling/pull/12

            People

            • Assignee:
              cziegeler Carsten Ziegeler
              Reporter:
              bond_ Raviteja Lokineni
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Time Tracking

                Estimated:
                Original Estimate - 48h
                48h
                Remaining:
                Remaining Estimate - 48h
                48h
                Logged:
                Time Spent - Not Specified
                Not Specified

                  Development