3

I've got this code on my page:

header("Location: $page");

$page is passed to the script as a GET variable, do I need any security? (if so what)

I was going to just use addslashes() but that would stuff up the URL...

hakre
  • 193,403
  • 52
  • 435
  • 836
Hintswen
  • 3,987
  • 12
  • 40
  • 45
  • I don't actually care if they can use it to get to a different page. I just want to be sure they can't do anything to the server, see my code etc. – Hintswen May 22 '09 at 12:50

4 Answers4

8

I could forward your users anywhere I like if I get them to click a link, which is definitely a big security flaw (Please login on www.yoursite.com?page=badsite.com). Now think of a scenario where badsite.com looks exactly like your site, except that it catches your user's credentials.

You're better off defining a $urls array in your code and passing only the index to an entry in that array, for example:

$urls = array(
    'pageName1' => '/link/to/page/number/1',
    'pageNumber2' => '/link/to/page/number/2',
    'fancyPageName3' => '/link/to/page/number/3',
);
# Now your URL can look like this:
# www.yoursite.com?page=pageName1
soulmerge
  • 73,842
  • 19
  • 118
  • 155
  • Do you mean define an array on URL's on that page and make sure the url that was passed to the script is in that array? I was originally going to do that but I realized that means I need to edit that page every time I add a URL. – Hintswen May 22 '09 at 12:26
  • Hintswen, if that's a problem, add a database table, give it a RESTful API for adding data (wouldn't worry about making a fancy method of editing or deleting stuff until you can be bothered) and make a bookmarklet which adds a URL to the list. – Samir Talwar May 22 '09 at 12:45
  • Why not just check if the url is on the same domain or not? I didn't end up using a header redirect for my purpose but I just thought of that. – Hintswen May 26 '10 at 05:06
3

This is a code injection vulnerability by the book. The user can enter any value he wants and your script will obey without any complaints.

But one of the most important rules – if even not the most important rule – is:

Never trust the user data!

So you should check what value has been passed and validate it. Even though a header injection vulnerability was fixed with PHP 4.4.2 and 5.1.2 respectivly, you can still enter any valid URI and the user who calls it would be redirected to it. Even such cryptic like ?page=%68%74%74%70%3a%2f%2f%65%76%69%6c%2e%65%78%61%6d%70%6c%65%2e%63%6f%6d%2f what’s URL encoded for ?page=http://evil.example.com/.

Gumbo
  • 643,351
  • 109
  • 780
  • 844
  • lol just so you know, I don't really care what URL they get directed to, as long as they can't stuff up any code, view my code, edit files on my server. – Hintswen May 22 '09 at 12:35
  • No, `header` can only modify the HTTP header that’s being sent back to the client. – Gumbo May 22 '09 at 12:44
  • yeah but couldn't someone do something like this... ?page=");othercode – Hintswen May 22 '09 at 12:52
  • @Hintswen: No, that’s impossible. The string declaration cannot be left. – Gumbo May 22 '09 at 13:19
2

Yes, you do. Just because you or I can't immediately think of a way to take advantage of that little bit of code doesn't mean a more clever person can't. What you want to do is make sure that the redirect is going to a page that you deem accessible. Even this simple validation could work:

$safe_pages = array('index.php', 'login.php', 'signup.php');
if (in_array($page, $safe_pages)) {
  header("Location: $page");
}
else {
  echo 'That page is not accessible.';
}
Justin Poliey
  • 16,289
  • 7
  • 37
  • 48
  • got it. that's actually what I originally had. but as I said to soulmerge I realized I would need to edit it every time I add a page/url – Hintswen May 22 '09 at 12:28
0

Or, at the very least, define a whitelist of allowed URLs, and only forward the user if the URL they supplied is in the GET variable is in the list.

Brian
  • 1,097
  • 11
  • 11