How to improve security

[larry29936]

I'd like to improve security of my website. Currently, the download filename is passed to the download script. The filenames are stored in a database table with an index number (primary key). I'd like to pass that index number to the download script instead of the filename and do a lookup in the download script but any sql code before the headers introduces problems with changes in md5sums of the downloaded files. Does anyone have any ideas n how to accomplish this?
Here's my download script:

<?php

function mydloader($l_filename=NULL)  {

    if( isset( $l_filename ) ) {
        $filename = preg_replace("/\s+/u", " ", $l_filename);
        $ext = pathinfo($filename, PATHINFO_EXTENSION);
            {
            if ($ext == '.iso')
                header('Content-Type: application/x-cd-image');
            elseif ($ext =='.gz')
                header('Content-Type: application/zip');
            else
                header('Content-Type: octet-stream'); 
            }
        header('Content-Length: ' .filesize($filename));
        header("Content-Disposition: attachment; filename={$filename}");
        header('Pragma: no-cache');
        header('Expires: 0');        
        readfile($filename);


        $php_scripts = '../../php/';
        require $php_scripts . 'PDO_Connection_Select.php';
        require $php_scripts . 'GetUserIpAddr.php';

        $ip = GetUserIpAddr();
        if (!$pdo = PDOConnect("foxclone_data"))   {    
               exit;
        }

        $test = $pdo->query("SELECT lookup.id FROM lookup WHERE inet_aton('$ip') >= lookup.ipstart AND inet_aton('$ip') <= lookup.ipend");
        $ref = $test->fetchColumn();
        $ref = intval($ref);

        $ext = pathinfo($l_filename, PATHINFO_EXTENSION);
        $stmt = $pdo->prepare("INSERT INTO download (`address`, `filename`,`ip_address`, `lookup_id`) VALUES (?, ?, inet_aton('$ip'),?)");
        $stmt->execute([$ip, $ext,$ref]) ; 


      }

    else {
        echo "isset failed";
        }  
}
mydloader($_GET["f"]);  // passed from download page
exit;

Thanks in advance

[Dani]

Please post the code you have so far that works with passing a file_id into mydloader() instead of a filename. Do you already have the MySQL query written to do the lookup and convert the file_id to filename?

The reason SQL code before the headers introduces problems has to do with a bug or issue in your database code ... perhaps introducing whitespace or such. The problem most likely exists in PDO_Connection_Select.php or in GetUserIpAddr.php but we can't diagnose the problem unless you post those two files here to investigate.

[larry29936]

@Dani, the code that I've got so far follows:

<?php
function mydloader($filenum=NULL)  {
    if( isset($filenum)) {
        $test = $pdo->query("SELECT filename FROM files WHERE index = $filenum); //conversion query 
        $filename = $test->fetchColumn();

        $ext = pathinfo($filename, PATHINFO_EXTENSION);
            {
            if ($ext == '.iso')
                header('Content-Type: application/x-cd-image');
            elseif ($ext =='.gz')
                header('Content-Type: application/zip');
            else
                header('Content-Type: octet-stream');
            }
        header('Content-Length: ' .filesize($filename));
        header("Content-Disposition: attachment; filename={$filename}");
        header('Pragma: no-cache');
        header('Expires: 0');
        readfile($filename);

    }

    else {
        echo "isset failed";
        }
}
mydloader($_GET["f"]);

[Dani]

Is that not working? What error message is it giving you?

Also, always sanitize before using any variable in a MySQL query.

In this case, right before line 4 of your code above, do $filenum = absval(intval($filenum)); to make sure that $filenum is a positive integer.

[larry29936]

@Dani, It works but the downloaded files are having the filename injected into the beginning of the download, causing an md5sum difference from the original file.

[Dani]

The only reason this would happen is because you have another PHP script that you're including that is spitting out the filename. There is nothing in the code you provided here that would do that.

The only other thing I could possibly think of is what happens if you comment out this line:

header("Content-Disposition: attachment; filename={$filename}");

[larry29936]

@Dani - Adding $filenum = absval(intval($filenum)); created an error.

"Parse error: syntax error, unexpected identifier "Content", expecting ")" in /home/foxclo98/public_html/download/mydloader_number.php on line 16"

Here's the code as it exists.

<?php
function mydloader_number($filenum=NULL) {
    if (isset($filenum)) {
        $filenum = absval(intval($filenum));
        $test = $pdo->query("SELECT filename FROM files WHERE index = $filenum);
        $filename = $test->fetchColumn();
        $filename = preg_replace("/\s+/u", " ", $filename);
        $ext = pathinfo($filename, PATHINFO_EXTENSION);
            if ($ext == '.iso')
                header('Content-Type: application/x-cd-image');
            elseif ($ext =='.gz')
                header('Content-Type: application/zip');
            else
                header('Content-Type: octet-stream'); 

        header('Content-Length: ' .filesize($filename));
        header("Content-Disposition: attachment; filename={$filename}");
        header('Pragma: no-cache');
        header('Expires: 0');        
        readfile($file_url);
    }

    else {
        echo "isset failed";
        }
}
mydloader($_GET["f"]);

In VS code, all the ; were red except the one after $filenum = absval(intval($filenum)); and the one on line 16.

[Dani]

So sorry! I meant abs() not absval(). I got my programming languages screwed up.

[larry29936]

@Dani - I'm still getting an error - Parse error: syntax error, unexpected token "isset", expecting ")" in /home/foxclo98/public_html/download/mydloader_number.php on line 24

Here's the current code:

<?php
$php_scripts = '../../php/';
require $php_scripts . 'PDO_Connection_Select.php';
require $php_scripts . 'GetUserIpAddr.php';

function mydloader_number($filenum=NULL)  {
    if( isset( $filenum ) ) {   
        $filenum = abs(intval($filenum));
        $test = $pdo->query("SELECT filename FROM `files` WHERE `index` = $filenum");
        $filename = $test->fetchColumn();

        $ext = pathinfo($filename, PATHINFO_EXTENSION);
            {
            if ($ext == '.iso')
                header('Content-Type: application/x-cd-image');
            elseif ($ext =='.gz')
                header('Content-Type: application/zip');
            else
                header('Content-Type: octet-stream'); 
            }
        header('Content-Length: ' .filesize($filename));
        header("Content-Disposition: attachment; filename={$filename}");
        header('Pragma: no-cache');
        header('Expires: 0');        
        readfile($filename);

        $ip = GetUserIpAddr();
        if (!$pdo = PDOConnect("foxclone_data"))   {    
               exit;
        }

        $test = $pdo->query("SELECT lookup.id FROM lookup WHERE inet_aton('$ip') >= lookup.ipstart AND inet_aton('$ip') <= lookup.ipend");
        $ref = $test->fetchColumn();
        $ref = intval($ref);

        $ext = pathinfo($l_filename, PATHINFO_EXTENSION);
        $stmt = $pdo->prepare("INSERT INTO download (`address`, `filename`,`ip_address`, `lookup_id`) VALUES (?, ?, inet_aton('$ip'),?)");
        $stmt->execute([$ip, $ext,$ref]) ; 


      }

    else {
        echo "isset failed";
        }  
}
mydloader_number($_GET["f"]);

[Dani]

You have a parse error somewhere, which means you're missing a ) or }.

Is this the entire file?

It says that the isset() error is on line 24, but in the code you provided, isset() is on line 7.

[larry29936]

OK, I got the downloads working and the md5sums are correct. One question though, how would I make sure a string is valid? Would preg_replace do it?

Thanks for your help.

[Dani]

Yes, you can use preg_replace() to strip out invalid characters from a string.

What do you mean by making sure the string is valid, though? Does it need to follow a specific pattern? Just not include invalid characters? etc.

[larry29936]

I was having trouble on another part of the website and was told that I should "sanitize" the string variable being passed to a function.

[Dani]

That’s what I keep telling you. Always sanitize user input to make sure that it’s in the format you are expecting.

[larry29936]

I guess I didn't make myself clear. I'd like to know how to sanitize a string variable.

[Dani]

Suppose you have a php script where a user is prompted to enter a number. You then do something with that number ... you increment it, perform some other math calculation on it, search the database for records with the ID # the user passed in as a query string.

But what if your script is expecting a number, but they passed in something like apple.

What if you were expecting www.example.com?id=5 and they went to the URL www.example.com?id=apple

You can't increment apple, it would throw an error. You can't look up the ID # apple in your database ... or worse yet, a malicious person can use an SQL injection attack string that could actually destroy your database!

Therefore, you always want to sanitize user input into the format you are expecting.

If you are expecting $variable to be an integer, then do $variable = intval($variable); and that will convert whatever $variable happens to be to its integer equivalent. If you are expecting $variable to be a positive integer (e.g. an ID # in a database) then do $variable = abs(intval($variable));.

Suppose you want to pass a string into MySQL. I've seen MySQL queries from you (in previous posts here at DaniWeb) look something like this: SELECT * FROM user_table WHERE string = '$string';

The problem with this, is what if $string contains a quote?! What if the value of string is My name is 'Dani'.

Then, you'd actually be running the SQL query: SELECT * FROM user_table WHERE string = 'My name is 'Dani''; Notice the extra single quote at the end there. That would throw a MySQL error. But it gets worse! What if the value of the string that the end-user passed into the form or URL is: My name is 'Dani'; DROP TABLE user_table; You would then execute two SQL queries:

SELECT * FROM user_table WHERE string = 'My name is 'Dani';
DROP TABLE user_table;

The end-user could literally delete your entire table!

If you want to pass a string into MySQL, then MySQL has a sanitization function that automatically escapes potentially dangerous characters from the string, so that you aren't susceptible to these types of hacks, and the string stays within the quotes the way you intended it to. This is the function you want to use anytime you need to pass a string into a MySQL query: https://www.php.net/manual/en/mysqli.real-escape-string.php

Hope this helps!

[larry29936]

Thank you for taking the time to give a detailed response.

[Dani]

You've motivated me to create a tutorial about sanitizing PHP strings :)

[Dani]

Here's a link to the tutorial I just wrote, based on my post here: https://www.daniweb.com/programming/web-development/tutorials/537376/sanitize-php-user-input-strings