php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #55576 Race condition in move_uploaded_file()
Submitted: 2011-09-03 11:34 UTC Modified: 2011-09-04 23:01 UTC
From: cjk at wwwtech dot de Assigned: cataphract (profile)
Status: Closed Package: Filesystem function related
PHP Version: 5.3.8 OS: All
Private report: No CVE-ID: None
 [2011-09-03 11:34 UTC] cjk at wwwtech dot de
Description:
------------
There is a race condition in the move_uploaded_file() function: if you don't want 
to overwrite a file, the standard mechanism is:

$fd = fopen($file,"x");
fclose($fd);
move_uploaded_file($uploaded_file,$file);

But since move_uploaded_file() unlink()s a file first, there may be a race 
condition: file gets created exclusively via fopen(…,"x"), move_uploaded_file() 
removes the same file and the process gets suspended. Another process creates the 
file via fopen(…,"x"), voila, race condition.

Expected result:
----------------
We need a concurrency save implementation of move_uploaded_file(). This can be 
achieved by implementing a third parameter, boolean $dont_overwrite. When set to 
true, move_uploaded_file() will ensure that the file does not exist by using 
open(…,O_RDWR|O_CREAT|O_EXCL) and returning false in error case. The patch I 
attached does exactly this.


Actual result:
--------------
When two concurrent processes, they may overwrite the same file twice w/o the 
possibility to prevent it. 

Patches

php-move-upladed-files-race-condition.patch (last revision 2011-09-03 11:35 UTC by cjk at wwwtech dot de)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2011-09-03 18:16 UTC] cataphract@php.net
The patch makes sense for paths in the filesystem, but this function also supports an arbitrary stream wrapper in the destination.

In any case, I'm puzzled by the first unlink() call (on new_path), it seems redundant. It was introduced in r32313.
 [2011-09-04 10:57 UTC] cjk at wwwtech dot de
Removing the unlink() would at least give us the possibility to make a file 
upload concurrency safe when using move_uploaded_file()
 [2011-09-04 23:00 UTC] cataphract@php.net
Automatic comment from SVN on behalf of cataphract
Revision: http://svn.php.net/viewvc/?view=revision&revision=316122
Log: - Fixed bug #55576: Cannot conditionally move uploaded file without race
  condition.
 [2011-09-04 23:01 UTC] cataphract@php.net
-Status: Open +Status: Closed -Assigned To: +Assigned To: cataphract
 [2011-09-04 23:01 UTC] cataphract@php.net
Fixed by removing unlink of destination file.

Thank you for your report.
 [2012-04-18 09:49 UTC] laruence@php.net
Automatic comment on behalf of cataphract
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ac20ab11f77b9ad409bb5cc4c7ac09aa10392175
Log: - Fixed bug #55576: Cannot conditionally move uploaded file without race   condition.
 [2012-07-24 23:40 UTC] rasmus@php.net
Automatic comment on behalf of cataphract
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ac20ab11f77b9ad409bb5cc4c7ac09aa10392175
Log: - Fixed bug #55576: Cannot conditionally move uploaded file without race   condition.
 [2013-11-17 09:36 UTC] laruence@php.net
Automatic comment on behalf of cataphract
Revision: http://git.php.net/?p=php-src.git;a=commit;h=ac20ab11f77b9ad409bb5cc4c7ac09aa10392175
Log: - Fixed bug #55576: Cannot conditionally move uploaded file without race   condition.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Mar 19 04:01:31 2024 UTC