Security Vulnerability in my 'Download File' script?

Hi,

I have the following script which if this URL is visited: get-file.php?id=9 will ask the user to save or open the file.

I just want to know how ‘secure’ it is, or what changes I should be looking to make to the script to improve it’s security.


<?php
// get-file.php
define('PDF_DIR','/home/the_actual_physical_path/documents/pdfs/');
require_once('../mysql_connect.php');
$id = intval($_GET['id']);
$sql = "SELECT id, pdf FROM pdf_table WHERE id = '$id'";
$res = mysql_query($sql);
$row = mysql_fetch_array($res);
if (!empty($row['1'])) {
 header("Content-type: application/pdf");
 header("Content-Disposition: attachment;filename=" . $row['pdf']);
 readfile( PDF_DIR . $row['pdf']);
}
else {
 $referer = isset($_SERVER['HTTP_REFERER']) ? $_SERVER['HTTP_REFERER'] : '';
 header("Location: $referer");
}
?>

Many thanks for any advice, really appreciate it.

I think you’re fine. Security aside, why do you have single quotes around a numeric value in the query?

Good call. Think it’s just a bad habit I’ve got myself into.
It’s definitely an integer I’m taking in, so I think I’ll remove the single quotes.

Many thanks again for looking Dan. Really appreciate it.
I may post up some more code later, suddenly got a worrying feeling on things :slight_smile:

You could use a bit of error checking, not for security but because if you’re passed a bad query string you’ll get no row back and call mysql_fetch_array on an empty result set. Maybe that’s what your conditional is trying to do, but you could use mysql_num_rows instead of forcing an error and ignoring it.

Hey you.

Not so much security, but IMO, it’s a little bit more robust.

  • Not sure why you define the file path, you only use it once.
  • You’re quoting an integer in your SQL.
  • Use sprintf to cast AND format your SQL, works great for ints.
  • Didn’t like the look of ! empty($row[‘1’]), shouldn’t that be $row[1] anyway?
  • Used ob_* to control output, 1 stray char would bugger your PDF.
  • Add a few conditionals, id is set, query worked, file exists et al.
  • Referrer may not be set, you need to address that, provide a default which must be a FQDN for the location header.

<?php
ob_start();
if(false === array_key_exists('id', $_GET)){
    $result = mysql_query(sprintf('SELECT pdf FROM pdf_table WHERE id = &#37;d LIMIT 1', $_GET['id']));
    if(true === is_resource($result)){
        $record = mysql_fetch_assoc($result);
        $pdf_file_path = sprintf('/path/to/files/%s', $record['pdf']);
        if(true === file_exists($pdf_file_path) && true === is_readable($pdf_file_path)){
            ob_end_clean();
            header("Content-type: application/pdf");
            header("Content-Disposition: attachment;filename=" . $record['pdf']);
            readfile($pdf_file_path);
            exit;
        }
    }
}
?>

Edit:
Blimey, there’s been a full on convo during composition! I must be slowing…

Whoops. Typo. :blush:


<?php
ob_start();
if(true === array_key_exists('id', $_GET)){
    $result = mysql_query(sprintf('SELECT pdf FROM pdf_table WHERE id = %d LIMIT 1', $_GET['id']));
    if(true === is_resource($result)){
        $record = mysql_fetch_assoc($result);
        $pdf_file_path = sprintf('/path/to/files/%s', $record['pdf']);
        if(true === file_exists($pdf_file_path) && true === is_readable($pdf_file_path)){
            ob_end_clean();
            header("Content-type: application/pdf");
            header("Content-Disposition: attachment;filename=" . $record['pdf']);
            readfile($pdf_file_path);
            exit;
        }
    }
}
?>

Wow, been sitting on this reply since 10am. Thought I’d clicked ‘Post’.

Flamin’ 'eck Anthony, that’s the business!

Now if I could just get you to go through the rest of these scripts I’ve got lined up. Just name the figure :wink:

Just wondering the ‘Referrer’ you mention. How would I best use that? Is it a case of just changing my:


$referer = isset($_SERVER['HTTP_REFERER']) ? $_SERVER['HTTP_REFERER'] : '';
header("Location: $referer");

snippet ?

Off Topic:

Ha! A Scotchman offering money, well I never… :stuck_out_tongue:


<?php
ob_start();
if(true === array_key_exists('id', $_GET)){
    $result = mysql_query(sprintf('SELECT pdf FROM pdf_table WHERE id = &#37;d LIMIT 1', $_GET['id']));
    if(true === is_resource($result)){
        $record = mysql_fetch_assoc($result);
        $pdf_file_path = sprintf('/path/to/files/%s', $record['pdf']);
        if(true === file_exists($pdf_file_path) && true === is_readable($pdf_file_path)){
            ob_end_clean();
            header("Content-type: application/pdf");
            header("Content-Disposition: attachment;filename=" . $record['pdf']);
            readfile($pdf_file_path);
            exit;
        }
    }
}
header(
    sprintf(
        'Location: %s',
        (true === isset($_SERVER['HTTP_REFERER']) && false === empty($_SERVER['HTTP_REFERER'])) ? $_SERVER['HTTP_REFERER'] : 'http://www.server.com/default.location/'
    )
);
exit;
?>

It’ll work, but it looks messy to me. :slight_smile:

Personally, I’d display an error message with a href back to a sensible location. If you have another script/page redirecting to this download script you’ll end up in a constant loop which will bork the client’s browser.

Ha, I’ve got a little Irish(not the drink) in me, so perhaps that has something to do with it :wink:

Thanks again Anthony, too kind gent.

Still not seeing a response to one of Dan’s points, if I send this:

get-file.php?id=a_loada_crap

I don’t see where you fail early and stop needless selects.

Maybe this:

if( 
      ( true === array_key_exists('id', $_GET) )
&& ( (int) $_GET['id'] > 0 )
){

as (int) evaluates a string to 0

Or maybe use filter_var()

Indeed, nice catch Cups.

Great stuff Cups, that makes sense.

Just a quick follow-up, if it’s ok.

I’m using the following code to take an ID from the query string and use it to look up the DB.


<?php

$id = intval($_GET['id']);

require_once('../mysql_connect.php');

if (( true === array_key_exists('id', $_GET)) && ( (int) $_GET['id'] > 0 )) {
  $sql = "SELECT ft.* FROM football_teams as fft WHERE ft.id = $id AND ft.approved = 'Y'";
  $res = mysql_query($sql);
  $row = mysql_fetch_array($res, MYSQL_NUM);
} else {
# no valid ID
}
?>

Should I be looking to sanitize my data more, or use sprintf(parameterised queries) for the query?

Thanks again for any thoughts.

While we are at it, one of my pet peeves is spotting occasions where you have to read a load of code before finding out what the “or else” situation is.

In the above example, I have heard argued that the same code laid out in such a way might be easier to follow;


if( 
      ( false === array_key_exists('id', $_GET) )
|| ( (int) $_GET['id'] === 0 )
){
   // you failed the test so, fail early
   send_away_function() ;

}else{

// OK, NOW write many lines
// the main guts of the logic in the 'happy path'



}

I find it does make code easier to follow - although I fancy not everyone will agree, a trick I picked up from PHP In Action I think …

Should I be looking to sanitize my data more, or use sprintf for the query?

No, you’ve done a good job of bounds-checking by type-casting the potentially dangerous incoming variable.

If there is a problem it is that you have no fall-back position if you personally make a typong error, forget to type-cast etc.

You are not (judging by this post) habitually using prepared statements as found in PDO, mysqli and other DBALs and ORMs which automatically escape the data ready for insertion into the database.

I used PDO from when I first heard about it and now sleep soundly at night.

Checking for unset variables, and typecasting in order to bounds-check becomes a matter of good usability, gaining control of the program flow, testing - but for me is not a matter of security because I use/rely upon PDO.

Thanks for the reply Paul.

Can you show me how to use PDO with my query.

I think I’ll also sleep better if I can move over to this technique and rejig old scripts :slight_smile:

Hmmm, so Cups, are you proposing something like this? Or have I taken you too literally?


<?php

if(false === array_key_exists('id', $_GET) || 0 === (int)$_GET['id']){
    invalid_request(1);
}

if(false === ($result = mysql_query(sprintf('SELECT pdf FROM files WHERE id = &#37;d LIMIT 1', $_GET['id'])))){
    invalid_request(2);
}

if(1 !== mysql_numrows($result)){
    invalid_request(3);
}

$record = mysql_fetch_assoc($result);

$pdf_path = '/path/to/file/' . $record['pdf'];

if(false === file_exists($pdf_path) || false === is_readable($pdf_path)){
    invalid_request(4);
}

header("Content-type: application/pdf");
header("Content-Disposition: attachment;filename=" . $record['pdf']);
readfile($pdf_path);
exit;
            

function invalid_request($status_code = null){
    #handle error, exit.
}
?>

Nah, you’re taking me too literally now (and possibly teasing me) but you make a good point that I did not.

Like most (I think) I believe my code is going to follow a “happy-path” once we have got past the BOFH data entry stage.

It’s that which I expect to be broken/abused/hacked, and that is generally where I expect to fail early, though its not a hard and fast rule.

Once past that big fail point, the rest of the logical “happy-path” operations : connecting to database, finding an entry matching an integer (unless the user is actually guessing numbers) and so on I do reasonably expect to work - and failure handling is then handed off to a combination of UnitTests, Exceptions, and error checking in the functions or objects invoked.

Not easy to explain using the example given maybe, but I noticed I am occasionally refactoring blocks in the same fashion, it has to do with proximity of the failure code to the start of the block.

instead of:


if( everything === good ){
// write 30 lines



}else{
ooer_handle_failure();

}

I’m doing this:


if( everything === not_good ){
ooer_handle_failure();

}else{
// write 30 lines



}

The question of proximity is a personal thing, if the above code instead contained:


// write 5 lines

Then I’d just as likely leave it as it was, because I dont need to scroll down and down to find out what the hell happens when the incoming user var test fails.

To take things to the extreme, you could have a 200 line block of code running on success, but if you were debugging (“where the hell did that message come from?” or “what happens of this fails again?”) you’d have to scroll through 200 lines to find out the “or else”.

When I think about it, TBH, its becoming more and more a moot point in my own code as more and more of the complexity is moved into classes and functions.

re: PDO - the best place to learn is of course the manual page, but another place is the site of the guy who first wrote PDO (as a PECL package I think) Dr Evil (Wez Furlong).

The second link on that page might be the slides he prepared and showed at many conferences a few years ago, and I referred to them a lot.

PDO took me quite some time to adjust to, y’know, to have all the syntax at your fingertips all the time, assembling those trusted snippets of code you know just work for the mysql_* functions.

It is class which you have to instantiate, but once you get used to using it and you learn even a few things about OOP, you realise you can simply extend it, or pass a PDO object to any class which need to access your database.

There are lots of benefits to using PDO, security is probably my main one, for the reasons I outlined previously.

I think it was when I started using PDO that I naturally tended to start separating data access from the rest of my code, so you start moving html and css in one direction ( towards templates and views) and data access in another direction ( models and business rules).

So its important for that reason too, although there is a bit of a hill to climb.

Hi Cups,

Thanks again for the detailed response.

Is there a way I can use PDO for my SELECT query example above or is it just for INSERT queries.

And generally, is there anything more I can do to improve security on that snippet of code? I can’t think of any :slight_smile: I’m hoping to take that and use it as a template to securify all my sites a little more.

My knowledge of security isn’t perfect though, so looking through the sites, I could be missing bits :frowning:

Thanks again

At the most basic level you can simply do this


$pdo = new PDO('mysql:dbname=testdb;host=localhost', 'user', 'pass');
$st = $pdo->prepare("
    SELECT ft.* 
    FROM football_teams as fft 
    WHERE ft.id = ? 
    AND ft.approved = 'Y'");
$st->execute(array($id));

$res = $st->fetchAll(PDO::FETCH_ASSOC);

$id will be bound to the placeholder ?
There are other methods that you should look at as well though.

Be careful here, the docs state you should the instantiation of the PDO object in a try/catch block as the exception backtrace may contain database credentials.

I’m no PDO expert, but I’m sure you can pass some configuration parameters upon instantiation to prevent this, failing that, you could pass the construction to a factory of sorts.

Rough example…


class DatabaseFactory
{
    public static function Build(){
        try{
            return new PDO('dsn', 'username', 'password');
        }catch(Exception $exception){
            trigger_error('Cannot connect to database: ' . $exception->getMessage(), E_USER_ERROR);
            return false;
        }
    }
}

$db = DatabaseFactory::Build();

Thanks for the clarification Cups, that makes things a lot clearer.