php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Doc Bug #72609 Insecure example in php.net documentation
Submitted: 2016-07-17 15:41 UTC Modified: 2016-07-18 12:37 UTC
From: adrian dot testaavila at gmail dot com Assigned: cmb (profile)
Status: Closed Package: Documentation problem
PHP Version: Irrelevant OS: n/a
Private report: No CVE-ID: None
 [2016-07-17 15:41 UTC] adrian dot testaavila at gmail dot com
Description:
------------
---
From manual page: http://www.php.net/function.move-uploaded-file
---

Example #1 (which is intended to be a _good_ example) demonstrates a filesystem traversal attack by allowing the $destination filename to be determined by user input:

<?php
        $name = $_FILES["pictures"]["name"][$key];
        move_uploaded_file($tmp_name, "$uploads_dir/$name");
?>

Examples in the manual should demonstrate best practices, especially where security is involved.  

Suggest changing the example to include validation for the $destination path, e.g., 
<?php
$uploads_dir = '/uploads';
foreach ($_FILES["pictures"]["error"] as $key => $error) {
    if ($error == UPLOAD_ERR_OK) {
        $tmp_name = $_FILES["pictures"]["tmp_name"][$key];
        $name = $_FILES["pictures"]["name"][$key];

        // $name comes from user input, so we MUST NOT trust that it is safe or correct.
        // realpath() will fail if the destination directory does not exist,
        // and then we check that the destination directory *is* the uploads directory, as intended.
        $destination = "{$uploads_dir}/{$name}";
        $dest_dir = realpath(dirname($destination));
        if ($dest_dir !== $uploads_dir) {
            trigger_error('Bad file name!', E_USER_WARNING);
        } else {
            move_uploaded_file($tmp_name, $destination);
        }
    }
}
?>

Alternatively, the example could not use the user-provided filename at all; generating a hash or random string as the $destination filename.

Also suggest adding a "Warning" box explaining the risks involved in allowing a user to choose where to save files on your filesystem.

Test script:
---------------
n/a

Expected result:
----------------
n/a

Actual result:
--------------
n/a

Patches

not_applicable (last revision 2016-07-17 15:43 UTC by adrian dot testaavila at gmail dot com)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2016-07-18 12:36 UTC] cmb@php.net
Automatic comment from SVN on behalf of cmb
Revision: http://svn.php.net/viewvc/?view=revision&amp;revision=339679
Log: Fix #72609: Insecure example in php.net documentation
 [2016-07-18 12:37 UTC] cmb@php.net
-Status: Open +Status: Closed -Package: Website problem +Package: Documentation problem -Assigned To: +Assigned To: cmb
 [2016-07-18 12:37 UTC] cmb@php.net
This bug has been fixed in the documentation's XML sources. Since the
online and downloadable versions of the documentation need some time
to get updated, we would like to ask you to be a bit patient.

Thank you for the report, and for helping us make our documentation better.
 [2020-02-07 06:07 UTC] phpdocbot@php.net
Automatic comment on behalf of cmb
Revision: http://git.php.net/?p=doc/en.git;a=commit;h=5308bbedf6e6a83980a13d3dfa45ca0f2469ddd6
Log: Fix #72609: Insecure example in php.net documentation
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Mon Apr 29 21:01:30 2024 UTC