2

My web application allows me to tell the login form to redirect to a certain page on the website after successfully logging in. For example, if the user goes to the url http://localhost/login.php?returnto=%2FconfirmEmail.php, then after completing the login process, they will automatically be redirected to http://localhost/confirmEmail.php.

After completing the login process, I used the following code to validate the url:

function isValidReturnURL( $input ) {
    if ( !is_string( $input ) ) { //Input is not a string
        return FALSE;
    }
    $cleanString = trim( urldecode( $input ) );
    if ( !strlen( $cleanString ) ) { //Input is an empty string
        return FALSE;
    }
    //Edit: I updated the following code (see below)
    $urlHost = parse_url( $cleanString, PHP_URL_HOST );
    if ( !empty($urlHost) ) { //All local urls will be relative
        return FALSE;
    }
    //End edit
    return TRUE;
}
if ( isValidReturnURL( $_GET["returnto"] ) && $continue ) {
    header("Location:" . urldecode( $_GET["returnto"] ) );
}

I'd like to know if this code is secure, or will it still be possible for an attacker to cause the page to redirect to an external website (or to other malicious locations)?

Update: Previously, my code to ensure that the links were local was as follows:

...
if ( $cleanString[0] !== '/' ) { //All local urls should start with '/'
    return FALSE;
}
...

As noted by various comments, this will consider protocol relative URLS (e.g. //google.com) to be valid. I therefore updated the code above to take that into consideration.

Improve this question
6
  • 2
    urlencode('//google.com') returns true Commented Aug 24, 2016 at 16:27
  • @JosephYoung Thank you. What about the updated code? Commented Aug 24, 2016 at 17:29
  • Found another exploit, posted it as an answer as it's a little longer Commented Aug 24, 2016 at 18:30
  • There is yet another bypass by exploiting the double-urldecode - returnto=http://google.com%2540evil.com. All GET/POST values are urldecoded() by the platform, calling urldecode() in PHP means you decode the value twice and this double-decode will change how the URL is parsed. Commented Aug 25, 2016 at 6:36
  • this is a very dangerous approach, as overtime new bypass techniques WILL be discovered. See my answer for a 100% secure solution... Commented Aug 25, 2016 at 11:35

5 Answers 5

Reset to default
2

Rather than validating whether the URL is bad, simply prepend the context path of your application.

  1. Verify that $_GET["returnto"] does not contain any line returns (\r or \n), because I'm not sure if the header function is performing this security check internally.

  2. Write the redirect location with the context path prepended

     header("Location: https://example.com/" . $_GET["returnto"] );

    So if http://badguy.net is passed, then the user will be safely redirected to https://example.com/http://badguy.net which is a harmless 404 page provided by your site.

  3. It is no longer necessary for you to verify that URLs are relative.

Note: I saw where you apperently made this mistake earlier. If you urldecode when acting on the variable, you should also urldecode when sanitizing/verifying whether it is valid. Similarly, if you do not urldecode when sanitizing, you should not urldecode when acting.

  1. Warning: Be sure you do not have any open redirect handlers. For example, some sites use a special URL such as https://example.com/go?url=http://google.com so that when a user clicks out you can log when they have done so.

    If you have such a feature, then you have to add a separate security check, otherwise the user could pass go?url=http://badguy.net (which is a relative URL), access the open redirect, and then the user is redirected to http://badguy.net.

Improve this answer
7
  • Not really an answer to OPs question, but this is definitely the correct solution. It also means that the redirect now follows the Location spec, which demands absolute URLs. And yes, PHP isn't vulnerable to HTTP response splitting or any other header injections in recent versions. Commented Aug 24, 2016 at 19:24
  • 1
    @tim See the new standard which permits relative URLs. Commented Aug 24, 2016 at 19:43
  • @GeorgeBailey would header("Location:http://" . $_SERVER["HTTP_HOST"] . '/' . ltrim(urldecode( $_GET["returnto"] ), "/") ); be safe? Commented Aug 24, 2016 at 19:59
  • 1
    @Inkbug, You should refrain from morphing your question. If it has been answered, accept the answer, and feel free to ask another more specific question. Also note that Security SE is a good site for providing guidelines on what security issues to solve and generally how to solve them. (i.e. I showed you how to solve the relative url problem most securely.) If you know what to do (i.e. remove \r and \n), but not sure how it should be written in the code (i.e. PHP), you should usually ask that on StackOverflow. Commented Aug 24, 2016 at 20:19
  • 1
    @rook That issue was fixed in 5.1.2 (released in Jan 2016). I have seen earlier PHP versions still being used in production, so unless you know exactly where your code will run I would still check for linebreaks. Commented Aug 25, 2016 at 11:37
4

It's not secure:

returnto=//google.com

This is a protocol relative URL, it will redirect to https://google.com if your site is https, otherwise it will redirect to http://google.com

Improve this answer
2
  • Thank you. I updated my code (see above) to take this into consideration, and would love to find out what you think of my solution. Commented Aug 24, 2016 at 17:29
  • see my solution for a long-term safe technique.. Your code is error-prone and is not solid against standards evolution Commented Aug 25, 2016 at 11:41
3

So, because I have better things to do, I've just spent a while trying to find a white space character which isn't trimmed by trim, and therefore messes up parse_url, but is a valid white space in the header function.

I found that if I encode the unicode character formfeed, U+000C, and place it before the URL as so:

%0Chttp%3A%2F%2Fwww.google.com

Your code will still redirect to Google. Sorry! :D

Improve this answer
1
  • nice one! Doesn't work in FF though. Commented Aug 24, 2016 at 19:12
2

The validated solution is vulnerable (see other posts).

I suggest you WHITELIST the redirection url's. Even better : use a mapper to a whitelist. 

&something=value&redirectUrl=3

3 being mapped in the backend to a nice URL. By doing this you are secure

If you Whitelist URL's, be careful about parameters as well.. That's why to be 100% sure and have a nice place where all the URL's are stored (which is easy to maintain), I would only allow specific keys (like the "3" in the example hereabove) that I would map to clean URL's.

The point of this technique is that you won't have to think about what possible hacks could break you (as you saw with the validated solution, a new idea could pop-up and break your defences). If you apply this technique, you are sure that you control the redirection.

The only (functional) drawback is that you can't redirect to a dynamic URL. Which you most probably don't plan to do, given your use-case :)

Bottom line is : having a redirect URL as a parameter value in a URL is generally a bad idea, as the validation is error-prone. If you can avoid the validation altogether, just do it ! Even if standards evolve (see : relative URL's), you are still safe.

Improve this answer
2

The best way to fix a vulnerability is to avoid it entirely.

Consider tokenizing or whitelisting what is possible. Don't use GET/POST for passing sensitive variables! Consider using $_SESSION['redirect'] or set_cookie() to pass this value. An attacker cannot set a cookie on a remote domain unless they have XSS or HTTP Response Splitting - at that point XSS is much more valuable than a simple open redirect.

Improve this answer

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.