How not to create a PHP downloader script.



  • Just found this snippet on a webserver i inherited. 

             $filename = urldecode($_SERVER[argv][0]);
            if(file_exists($filename))
            {
                    header("Pragma: public");
                    header("Cache-Control: private",false); //required for certain browsers
                    header("Expires: 0");
                    header('Content-Disposition: attachment; filename='.basename($filename).';');
                    header("Content-Type: application/force-download");
                    header("Content-Transfer-Encoding: binary");
                    header('Content-Length: '.filesize($filename));
                    @readfile($filename);
            }

    And, yes passing %2Fetc%2Fpasswd does indeed give you what you want.

    Needless to say, the server isn't sandboxed. At least it's not running as root, though.



  • This is the kind of "programmer" that's given PHP a bad name.

    Oh, and plz send me teh urls 

     



  • I've seen that script passed around (or similar), and it is awesome.

    Here are the problems I can see at first glance:

    1. Security hole allowing downloading of any file on the server (that the webserver process has read access to)
    2. Technically invalid headers "Expires: 0" and "Pragma: public"
    3. Header "Cache-Control: private" contradicts "Pragma: public"
    4. Use of invalid (and wrong for the file) content-type of "application/force-download". "application/octet-stream" should be used if the file content is unknown.
    5. Download script causes excessive load on the server by not sending "Last-Modified: " and not handling "If-Modified-Since: " headers, forcing the file to be retransmitted every time.
    For point 5, allowing caching (but forcing the client / caching proxy servers to verify their cached copy every time by using "Cache-control: must-revalidate" header) is a great way to gather download statistics without wasting bandwidth by having to retransmit the file to a cache that already has it.


  •  At least the file wasn't include()d! Couple that with URL fopen and you could run arbitrary code on the server. (I've seen it...)



  • New revision below. I checked the access.log it doesn't seem to be used. And, yes, I did get a bit paranoid with both and exit and commenting it out. If it is needed, I'll fix it up, to restrict what files are downloaded, where they can be downloaded, etc....:

    This script has been disabled by Nicolas, being that it is an incredibly stupid security risk, and whoever created it should be shot. Let me know if it is needed.
    <?php
            exit(0);
            /*$filename = urldecode($_SERVER[argv][0]);
            if(file_exists($filename))
            {
                    header("Pragma: public");
                    header("Cache-Control: private",false); //required for certain browsers
                    header("Expires: 0");
                    header('Content-Disposition: attachment; filename='.basename($filename).';');
                    header("Content-Type: application/force-download");
                    header("Content-Transfer-Encoding: binary");
                    header('Content-Length: '.filesize($filename));
                    @readfile($filename);
            }*/
    ?>
     



  • It's a tracking site, so the vast majority of the bandwidth is sending the maps to the client. And, oh, an extra parameter is added to each tile to ensure that each client has to download each tile every session directly from the server.

    http://<server>/map+3003003130+draw.php+b6a4603fc86b53171152b8624503ea92

    http://<server>/map+3003003130+draw.php+493a3d4eb4614bf90fff6f383da476cb



  • I've seen this type of code way too much... You never get used to it though.

     

    @Zemm said:

     At least the file wasn't include()d! Couple that with URL fopen and you could run arbitrary code on the server. (I've seen it...)

     

     Actually, if it'd include it, you usually don't even need URL opening to run arbitrary code on the server...



  • Heh; of seven headers he got one right:

    • There is no "public" Pragma, not in HTTP/1.1, at least.
    • Cache-Control: private is valid syntax, but since the file served is apparently [b]not[/b] specific to the user, it doesn't make sense to mark it with this.
    • The value of the Expires header must be a date, although clients still must handle a value of 0 as being in the past.
    • The Content-Disposition header doesn't exist, however, it is widely implemented and I wouldn't object if he actually got the syntax right (there is no colon before the value and a superfluous semicolon in the end).
    • There is no application/force-download media type.
    • There is no Content-Transfer-Encoding header, either. Assuming he meant Transfer-Encoding, binary is not a valid value.


  •  No, PHP sucks. That doesn't mean you can't make good stuff in it though, just that it's harder.


Log in to reply