0

I have this php code :

<?php

header("Content-Type: application/force-download");
header("Content-Disposition: attachment; filename=\"".$_GET['name']."\""); 
$file_content = file_get_contents($_GET['name']);
echo $file_content;

?>

In this case, the attacker can download files on my site with this request:

localhost/file.php?name=../../../../../../../etc/passwd

I need a way to prevent users from downloading anything except .zip file from the current directory.

Qiang
  • 656
  • 8
  • 17
user204353
  • 3
  • 1

1 Answers1

1

Before opening the file, you should check that the content of $_GET['name'] is legit.

In particular, in your case, check that:

  • it ends with .zip
  • it does not contain a NUL byte to prevent premature string termination (see Null bytes related issues)
  • it doesn't contain a path separator (neither / nor \).

Here’s an example:

$filename = $_GET['name'];
if (strpos($filename, "\0") !== false) {
    // contains NUL byte
} else if (substr(strtolower($filename), -4) !== '.zip') {
    // doesn’t end with ".zip"
} else if (basename($filename) !== $filename) {
    // contains path separator
} else if (!is_file($filename)) {
    // file does not exist
} else {
    // everything is fine
}
Gumbo
  • 643,351
  • 109
  • 780
  • 844
gturri
  • 13,807
  • 9
  • 40
  • 57
  • That’s not enough. Just think of `/etc/passwd%00.zip` (ends with `.zip` but contains string terminating NUL byte) or `./../../etc/passwd` (does not start with `..`). – Gumbo Jan 05 '14 at 13:02
  • Indeed, I missed some cases. I adapted my answer accordingly. (However, I'm not mentionning the NUL byte trick, because I'm not sure I fully understands it) – gturri Jan 05 '14 at 13:07
  • It’s still not sufficient/accurate enough. – Gumbo Jan 05 '14 at 13:16
  • Then, please, submit an answer (or edit mine): I'll be glad to learn how it should be handled – gturri Jan 05 '14 at 13:33