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.

  • 2
    urlencode('//google.com') returns true – Joseph Young Aug 24 '16 at 16:27
  • @JosephYoung Thank you. What about the updated code? – Inkbug Aug 24 '16 at 17:29
  • Found another exploit, posted it as an answer as it's a little longer – Joseph Young Aug 24 '16 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. – rook Aug 25 '16 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... – niilzon Aug 25 '16 at 11:35
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.

  • 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. – tim Aug 24 '16 at 19:24
  • 1
    @tim See the new standard which permits relative URLs. – Inkbug Aug 24 '16 at 19:43
  • @GeorgeBailey would header("Location:http://" . $_SERVER["HTTP_HOST"] . '/' . ltrim(urldecode( $_GET["returnto"] ), "/") ); be safe? – Inkbug Aug 24 '16 at 19:59
  • @Inkbug, Using the HTTP Host header is fine. ltrim might no be sufficient to remove \r and \n. You should ask for the correct method to do this on StackOverflow. – Bryan Field Aug 24 '16 at 20:17
  • 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. – Anders Aug 25 '16 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

  • 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. – Inkbug Aug 24 '16 at 17:29
  • see my solution for a long-term safe technique.. Your code is error-prone and is not solid against standards evolution – niilzon Aug 25 '16 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

  • nice one! Doesn't work in FF though. – tim Aug 24 '16 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.

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.