Problem allowing users to download files from my website

[foxclone]

I want to allow users on my hosted website to download files. Most of the following code was copied from php.net but when I run it, I'm only getting 495 bytes of a 5.7M file when testing downloads from my webhost. This code exists in a window that is never seen by the user. $l_filename is passed from another window. I'm not seeing any errors. My code follows:

<?php
error_reporting(E_ALL);
ini_set('display_errors', '1');
function usdloader($l_filename=NULL){
    if (file_exists($l_filename)) {
        $file = preg_replace("/\s+/u", " ", $l_filename);
        header('Content-Description: File Transfer');
        header('Content-Type: ' . mime_content_type($file));
        header('Content-Disposition: inline; filename="'.basename($file).'"');
        header('Expires: 0');
        header('Cache-Control: must-revalidate');
        header('Pragma: public');
        header('Content-Length: ' . filesize($file));
        readfile($file);
    else
        die;
    }
}
usdloader($_GET["f"]);      

I thank you for your help.

[ginerjm]

Have you debugged the code to ensure that the values you are using are exactly what you expect?  How about the filesize value?

[mac_gyver]

if you open the downloaded file in your programming editor, you will probably see a clue as to the problem.

[foxclone]

@mac_gyver The downloaded file is only 495 bytes of a 5 meg .deb file. I'll check for an error file on my web host.

[ginerjm]

Have you opened the original file BEFORE the download to see what is at position 496 perhaps?

[mac_gyver]

i told you to open the downloaded file in your programming editor to see what is in it. whatever you are using to translate our replies into your native language is not working.

Edited May 9, 2022 by mac_gyver

[foxclone]

@mac_gyver Contents of the partial file follow

<br />
<b>Warning</b>:  Undefined variable $file_url in <b>/home/foxclo98/public_html/mydloader.php</b> on line <b>21</b><br />
<br />
<b>Fatal error</b>:  Uncaught ValueError: Path cannot be empty in /home/foxclo98/public_html/mydloader.php:21
Stack trace:
#0 /home/foxclo98/public_html/mydloader.php(21): readfile('')
#1 /home/foxclo98/public_html/mydloader.php(44): mydloader('foxclone50_amd6...')
#2 {main}
  thrown in <b>/home/foxclo98/public_html/mydloader.php</b> on line <b>21</b><br />

 

Edited May 9, 2022 by foxclone

[foxclone]

What is the correct setting in php.ini to prevent server caching?

[foxclone]

Never mind about web caching, my web host told me how to do it thru cpanel.

[foxclone]

2 hours ago, ginerjm said:

Have you opened the original file BEFORE the download to see what is at position 496 perhaps?

It's a .deb file. VScode won't open it because it's binary.

[gizmola]

The errors are telling you what the problem is.  You aren't showing us the full code either.  You're cherry picking the wrong code.

With that said, the code you posted has an obvious bug in it:

 

function usdloader($l_filename=NULL){
    if (file_exists($l_filename)) {
        $file = preg_replace("/\s+/u", " ", $l_filename);
        header('Content-Description: File Transfer');
        header('Content-Type: ' . mime_content_type($file));
        header('Content-Disposition: inline; filename="'.basename($file).'"');
        header('Expires: 0');
        header('Cache-Control: must-revalidate');
        header('Pragma: public');
        header('Content-Length: ' . filesize($file));
        readfile($file);
    else
        die;
    }
}

Look at your blocks.  Blocks need to be separated by { }.   Your code has a superfluous else in it.  Why?

This is what you meant to do I guess:

function usdloader($l_filename=NULL){
    if (file_exists($l_filename)) {
        $file = preg_replace("/\s+/u", " ", $l_filename);
        header('Content-Description: File Transfer');
        header('Content-Type: ' . mime_content_type($file));
        header('Content-Disposition: inline; filename="'.basename($file).'"');
        header('Expires: 0');
        header('Cache-Control: must-revalidate');
        header('Pragma: public');
        header('Content-Length: ' . filesize($file));
        readfile($file);
        die;
    }
}

 

Even with fixing that, we are missing the code that explains why "$file_url in <b>/home/foxclo98/public_html/mydloader.php</b> on line <b>21<".

After your preg_replace, you have an empty string.  Why is that?  What are you trying to do with this preg_replace?  $file = preg_replace("/\s+/u", " ", $l_filename);

All you appear to be doing with this is take whitespace and replace it with space. What is the value of that?  Why do you allow for any spaces in a file name?

It's also important to note that this code will let a person download just about any file on your server that the apache server can read.  For example, if someone crafts a url and passes this to your script what happens?

mydloader.php?f=%2Fetc%2Fpasswd

[foxclone]

@gizmola  The filename is passed from a button click on the on the download page. Here's a sample:

<a href="download/mydloader.php?f=<?php echo $debname;?>"><center><img src="images/us_download.png" style="height:35px; width:144px; margin-bottom:.5rem;"  alt="download deb file"> </center></a>              
               

The mydloader.php is located in the folder with the files that are available to download. 

My intent with the die statement was intended as a failsafe in case the file_exists was false.

[kicken]

18 minutes ago, foxclone said:

My intent with the die statement was intended as a failsafe in case the file_exists was false.

Flip your if statement so that it checks if the file does not exist and if that is true, exit.

if (!file_exists($l_filename)){
    exit;
} 

//...

While testing, it's easier if you use text files, then you can easily see the results and any potential errors directly in the browser.  Once you have it working with the text files you can try binary files.

As mentioned before, you also need to validate the filename selected or someone could use your code to download any of your files, not just the ones you intend.  If you want to only allow the files within the folder to be downloaded, then you could use basename so the user only inputs a file name and not a directory, then combine that with __DIR__ to make a full path.

$file=basename($_GET['f']);
$file=__DIR__.'/'.$file;

 

[foxclone]

@kicken Thanks for the suggestion. I'll implement it.

[gizmola]

2 hours ago, foxclone said:

@gizmola  The filename is passed from a button click on the on the download page. Here's a sample:

<a href="download/mydloader.php?f=<?php echo $debname;?>"><center><img src="images/us_download.png" style="height:35px; width:144px; margin-bottom:.5rem;"  alt="download deb file"> </center></a>              
               

The mydloader.php is located in the folder with the files that are available to download. 

My intent with the die statement was intended as a failsafe in case the file_exists was false.

In this case, it's incorrect.  Your code is essentially lifted from the manual.  You need the die/exit after the readline().  die() is the same as exit()

See https://www.php.net/manual/en/function.readfile.php

There is no need to die() if the file doesn't exist really, as the script will do nothing.  If you really want to handle it in the right way you should deliver a 404 error.

You also seem to miss the point I made in that your downloader can be used to download any file that is passed to the get parameter.  The fact that you are using a button is irrelevant.  There is some problem with your code, because no path to the file was being picked up by the get parameter.

[mac_gyver]

2 hours ago, kicken said:

As mentioned before, you also need to validate the filename selected or someone could use your code to download any of your files, not just the ones you intend.

the OP has been told this multiple times. the OP has a database table with the downloadable file information in it and has also been told to use the id (autoincrement primary index) from this table as the value in the link, which can then be validated via a database query, which he/she has acknowledged seeing and has stated will be implemented.

[foxclone]

@gizmola - I've modified the code using many of the suggestions made in this thread. Here's the code that I have now:

<?php
error_reporting(E_ALL);
ini_set('display_errors', '1');

$file=basename($_GET['f']);
$file=__DIR__.'/download/'.$file;

function mydloader($file) {
    if (!file_exists($file)) {
        die;
    }
    else {
        header('Content-Description: File Transfer');
        header('Content-Type: ' . mime_content_type($file));
        header('Content-Disposition: inline; filename="'.basename($file).'"');
        header('Expires: 0');
        header('Cache-Control: must-revalidate');
        header('Pragma: public');
        header('Content-Length: ' . filesize($file));
        readfile($file); 
        exit;
    }

}
mydloader($_GET["f"]);      

I thank everyone for their input.

[kicken]

2 hours ago, foxclone said:

I have now

This is pointless the way you have it:

$file=basename($_GET['f']);
$file=__DIR__.'/download/'.$file;

You're still using $_GET['f'] when you call the function below.  You need to change it to use $file.

if (!file_exists($file)) {
    die;
}
else {
    //...
}

While it's fine to have it, you don't need the else block here since the if branch will cause the script to end. The code is cleaner looking without it IMO.

[ginerjm]

If what you have posted here is all there is, why are you using a function at all?

[foxclone]

I have everything working now, including updating the database, which is new code. Here's the code:

<?php
error_reporting(E_ALL);
ini_set('display_errors', '1');

function mydloader($l_filename=NULL){
$file=basename($l_filename);
$file=__DIR__.'/'.$file;

if (file_exists($file)) {  // Make database entry
  require '../../php/PDO_Connection_Select.php';  
  require '../../php/GetUserIpAddr.php';
  $ip = GetUserIpAddr();
  if (!$pdo = PDOConnect("foxclone_data"))
  {
    exit("unable to connect to database");
  }
  $ext = pathinfo($l_filename, PATHINFO_EXTENSION);
  $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);

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




    if (!file_exists($file)) {
      exit("Requested file ($file) does not exist");
    }
    else {
        header('Content-Description: File Transfer');
        header('Content-Type: ' . mime_content_type($file));
        header('Content-Disposition: inline; filename="'.basename($file).'"');
        header('Expires: 0');
        header('Cache-Control: must-revalidate');
        header('Pragma: public');
        header('Content-Length: ' . filesize($file));
        readfile($file); 
        exit;
    }
}
mydloader($_GET["f"]);      

I had to put the database update before the file send otherwise the md5sum of the downloaded file was incorrect.

Thanks to everyone for their input. I'm currently testing a change for the calling page to pass an integer instead of the filename and doing a lookup for the filename in this script.

 

Edited May 10, 2022 by foxclone