Is this code vulnerable to hacker attack?

Question

Hi all, I am really new to online web application. I am using php, I got this code:

if(isset($_GET['return']) && !empty($_GET['return'])){
return = $_GET['return'];
header("Location: ./index.php?" . $return);    
} else {
header("Location: ./index.php");

}

the $return variable is URL variable which can be easily changed by hacker.

E.g i get the $return variable from this : www.web.com/verify.php?return=profile.php

Is there anything I should take care? Should I use htmlentities in this line:

header("Location: ./index.php?" . htmlentities($return));

Is it vulnerable to attack by hacker?

What should i do to prevent hacking?

Answer 1

What would happen if you put in a page that didn't exist. For example:

return=blowup.php

or

return=http://www.google.co.uk

or

return=http%3A%2F%2Fwww.google.co.uk%2F

You could obfuscate the reference by not including the .php in the variable. You could then append it in the code-behind and check for the existence of the file in your directory / use a switch statement of allowable values before redirecting to it.

Answer 2

Apart from that typo on line 2 (should be $return = $_GET['return'];) you should do $return = urlencode($return) to make sure that $return is a valid QueryString as it's passed as parameter to index.php.

index.php should then verify that return is a valid URL that the user has access to. I do not know how your index.php works, but if it simply displays a page then you could end up with someting like index.php?/etc/passwd or similar, which could indeed be a security problem.

Edit: What security hole do you get? There are two possible problems that I could see, depending how index.php uses the return value:

• If index.php redirects the user to the target page, then I could use your site as a relay to redirect the user to a site I control. This could be either used for phishing (I make a site that looks exactly like yours and asks the user for username/password) or simply for advertising.
  • For example, http://yoursite/index.php?return?http%3A%2F%2Fwww.example.com looks like the user accesses YourSite, but then gets redirected to www.example.com. As I can encode any character using the %xx notation, this may not even be obvious to the user.
• If index.php displays the file from the return-parameter, I could try to pass in the name of some system file like /etc/passwd and get a list of all users. Or I could pass something like ../config.php and get your database connection
  • I don't think that's the case here, but this is such a common security hole I'd still like to point it out.

As said, you want to make sure that the URL passed in through the querystring is valid. Some ways to do that could be:

• $newurl = "http://yoursite/" . $return;
  • this could ensure that you are always only on your domain and never redirect to any other domain
• $valid = file_exists($return)
  • This works if $return is always a page that exists on the hard drive. By checking that return indeed points to a valid file you can filter out bogus entries
  • If return would accept querystrings (i.e. return=profile.php?step=2) then you would need to parse out the "profile.php" path
• Have a list of valid values for $return and compare against it
  • this is usually impractical, unless you really designed your application so that index.php can only return t a given set of pages

There are many ways to skin this cat, but generally you want to somehow validate that $return points to a valid target. What those valid targets are depends on your specification.

Answer 3

In this case, it more depends on what's done with that part of the query string on index.php. If it's being sent to a database query, output, eval(), or exec() yes, its a very common security hole. Most other situations will be safe unfiltered, but its best to write your own general purpose sanitizing function which converts quotes of all varieties to their HTML entity, as well as equals symbols.

Answer 4

If you're running an older version of both PHP 4 or 5, then I think you will be vulnerable to header injection - someone can set return to a URL, followed by a line return, followed by any other headers they want to make your server send.

You could avoid this by sanitising the string first. It might be enough to strip line returns but it would be better to have an allowed list of characters - this might be impractical.

4.4.2 and 5.1.2: This function now prevents more than one header to be sent at once as a protection against header injection attacks.

http://php.net/manual/en/function.header.php

Answer 5

The things I would do are:

1. Define, what type of return values are allowed?
2. Write down all types of possible return values.
3. Then, make conclusions: what characters are not allowed, what is the maximum url length, what domains are allowed, etc.
4. Finally: make a filter function according to above conclusions.