Jump to content

Problem allowing users to download files from my website


Recommended Posts

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.

Link to comment
Share on other sites

Posted (edited)

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 by mac_gyver
Link to comment
Share on other sites

Posted (edited)

@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 by foxclone
Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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
Link to comment
Share on other sites

@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.

Link to comment
Share on other sites

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;

 

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

@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.

Link to comment
Share on other sites

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.

Link to comment
Share on other sites

Posted (edited)

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 by foxclone
Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Restore formatting

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

 Share

×
×
  • Create New...

Important Information

We have placed cookies on your device to help make this website better. You can adjust your cookie settings, otherwise we'll assume you're okay to continue.