Doc #72609 [Opn->Csd]: Insecure example in php.net documentation
Edit report at https://bugs.php.net/bug.php?id=72609&edit=1
ID: 72609
Updated by: cmb@php.net
Reported by: adrian dot testaavila at gmail dot com
Summary: Insecure example in php.net documentation
-Status: Open
+Status: Closed
Type: Documentation Problem
-Package: Website problem
+Package: Documentation problem
Operating System: n/a
PHP Version: Irrelevant
-Assigned To:
+Assigned To: cmb
Block user comment: N
Private report: N
New Comment:
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.
Previous Comments:
------------------------------------------------------------------------
[2016-07-18 12:36:55] cmb@php.net
Automatic comment from SVN on behalf of cmb
Revision: http://svn.php.net/viewvc/?view=revision&revision=339679
Log: Fix #72609: Insecure example in php.net documentation
------------------------------------------------------------------------
[2016-07-17 15:41:57] 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
------------------------------------------------------------------------
--
Edit this bug report at https://bugs.php.net/bug.php?id=72609&edit=1
Thread (2 messages)