8

A webpage redirects to an URL given in parameter redirect without filtering it, like this:

$newURL=$_GET["redirect"];
header('Location: '.$newURL);

login.php?redirect=home.php

If it echoes the input I know it's possible to perform a XSS attack, but can we inject code when it is just used in the header method? Is this a safe script?

Anders
  • 64,406
  • 24
  • 178
  • 215
Fast Snail
  • 181
  • 1
  • 8

4 Answers4

10

The short answer

No. This is not safe, and should not be done. In fact, this is the last one of OWASP Top 10:

A10. Unvalidated Redirects and Forwards

Web applications frequently redirect and forward users to other pages and websites, and use untrusted data to determine the destination pages. Without proper validation, attackers can redirect victims to phishing or malware sites, or use forwards to access unauthorized pages.

Phishing and spreading malware

An attacker could craft an URL to redirect to any page they want, and then spread it everywhere:

http://trusted.com/index.php?redirect=http%3A%2F%2Fmalicious.com

The malicious URL could be obfuscated to avoid detection by the user by URL encoding all character so you get a string like %68%74%74%70%3A%2F%2F%6D%61%6C%69%63%69%6F%75%73%2E%63%6F%6D.

Now a user clicking what looks like an ordinary safe link to your site might end up anywhere:

  • On a site looking exactly like yours, asking for username and password. Boom, accounts stolen.
  • A site spreading malware by drive by downloads. You could try to claim this is not your problem, since it is technically not your site spreading it. I am not sure the victim would agree.
  • Just anything. Do you want users clicking links to your site and ending up on illegal or NSFW or NSFL content?

If this is done after the user logins, as you hint at in the question, it is even more dangerous since actually being on your page entering the credentials strengthens the users belief that he is actually on your site. It would be so easy to serve an "incorrect password, try again" type of page after the login.

Header injection

As already stated by Steffen Ullrich in his answer to this question and me in this answer to another question, on PHP versions older than 5.1.2 this could be used for header injection.

What to do instead

Whitelisting or validation

As so often, the solution is to whitelist. Here is an PHP example:

$param = $_GET["redirect"];
$allowed = array(
    "index" => "index.php",
    "blog"  => "blog/index.php",
    // ...and so on...
);
$redirect = isset($allowed[$param]) ? $allowed[$param] : "index.php";
header('Location: '.$redirect);

This will be tedious if there are many pages you want to be able to redirect to. Depending on what your URLs look like, you could validate URLs for your own site. For instance if they are on the form http://trutsted.com/?pageid=1234 you could accept only the pagied to redirect to and verify that it is in fact numeric.

Referer check

If you only want the redirect links to work from within your own site, you could check the HTTP referer heading of the request. This might run into some issues if you serve your site over both HTTP and HTTPS, since if you link from a HTTP page to an HTTPS page the referer header will not be sent.

Here is some example code:

$headers = apache_request_headers();
$referer = isset($headers['referer']) ? parse_url($headers['referer'], PHP_HOST) : "";
if($referer == "trusted.com") {
    //Do the redirect. If you want your code to be safe on old PHP versions,
    //you will need to filter out newlines or just URL encode the URL.
}
else {
    //Inform the user of the problem or just redirect to your index page.
}

More reading

  • OWASP cheat sheet on unvalidated redirects. (One of the examples on how not to do it here is alwmost identical to how you did it in your question.)
  • Troy Hunt has a great piece on this topic.
Community
  • 1
Anders
  • 64,406
  • 24
  • 178
  • 215
  • Is checking the referer considered safe in this case though? Can't the referer header be modified if it is javascript that is initiating the request, at least in older browsers? – BoomShaka Oct 12 '20 at 07:49
  • 1
    @BoomShaka I'm not aware of any browsers that let you set the referer header through JS, and I would be surprised if there are any. (Wouldn't be surprised if there is a flash exploit that lets you do it though, because lets face it, there is always a flash exploit.) – Anders Oct 12 '20 at 19:00
6

Current versions of PHP detect and prevent newline injections in the header function, see How to avoid HTTP Header Injection (new lines characters). In older versions pf PHP you could probably do something like

 login.php?redirect=%0D%0A%0D%0A<script>...

Which would break out of the header and result in 

 Location:

 <script>...

And your script then could do whatever it wants, including accessing and forwarding session cookies for session hijacking.

That does not mean that your script is safe with current PHP. Redirecting to arbitrary URL's is never a good idea so you should restrict newURL to the URL's you expect for redirection.

Community
  • 1
Steffen Ullrich
  • 184,332
  • 29
  • 363
  • 424
2

To add to what others have said :

If you have a set of known URL's to redirect to (that you could map to an identifier), it would be much better to allow only known identifiers in the "redirect" parameter value. Then you can map the identifier to your safe, known, URL.

Thanks to such a technique "all your troubles" go away.

Of course if the value of the URL is 100% arbitrary for your users, or if you don't have the set of all possible URL's, then this cannot apply. But if your URL's correspond to known pages that you have control over, go for it !

niilzon
  • 1,587
  • 2
  • 10
  • 17
1

The first vulnerability I can think of is to pass a full URL as an argument that will redirect the user to a fake copy of the site (login.php?redirect=http://malicious.com)

Aside from that, I'm sure there are several ways to prevent the redirection from happening and displaying instead malicious HTML/JavaScript.

As a general rule, any URL parameter should be sanitized.

user7094
  • 133
  • 4