php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #77630 rename() across the device may allow unwanted access during processing
Submitted: 2019-02-18 00:39 UTC Modified: 2019-03-12 19:57 UTC
From: manabu dot matsui at gmail dot com Assigned: stas (profile)
Status: Closed Package: Filesystem function related
PHP Version: 7.1.26 OS:
Private report: No CVE-ID: 2019-9637
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: manabu dot matsui at gmail dot com
New email:
PHP Version: OS:

 

 [2019-02-18 00:39 UTC] manabu dot matsui at gmail dot com
Description:
------------
I tried to strace rename across devices, and the process was done as follows.
(Only system calls that should be noticed are pulled out so as not to become too long)

openat(AT_FDCWD, "/boot/hoge/oldfile", O_RDONLY) = 3
openat(AT_FDCWD, "/tmp/dstdir/newfile", O_WRONLY|O_CREAT|O_TRUNC, 0666) = 4
(mmap and write contents)
chmod("/tmp/dstdir/newfile", 0100664)       = 0
chown("/tmp/dstdir/newfile", 1000, 1000)    = 0
unlink("oldfile")                           = 0

Until chown is done, users who were not allowed to access to oldfile may be able to access newfile.

It is not impossible to avoid this problem by properly controlling environments such as umask, effective-gid (in case of linux semantics), group of destination directory (in case of bsd semantics).

However,  it is handled like mv command of GNU-oreutils, for example, there is no need for special preparation, so I think it will be safer.

* unlink newfile before open and open with O_EXCL to ensure that we do not open inappropriate file created by someone.
* open newfile with mode=600 to ensure that access from unwanted users is prevented during copying process.
* do chmod after chown to ensure that group access is not enabled until the group to which file belongs is set properly.



Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2019-02-18 21:27 UTC] stas@php.net
-Status: Open +Status: Feedback
 [2019-02-18 21:27 UTC] stas@php.net
On UNIX, PHP uses libc rename function. Thus, if you think renaming is done wrong, I think it should be submitted to libc maintainers for your system. If you use non-UNIX system, please specify which one.
 [2019-02-18 21:29 UTC] stas@php.net
-Status: Feedback +Status: Open
 [2019-02-18 21:29 UTC] stas@php.net
Correction - there's a separate code for handling EXDEV, so it's possible the issue is in the code. I'll check it.
 [2019-02-18 21:52 UTC] manabu dot matsui at gmail dot com
This is a problem when VCWD_RENAME() fails with EXDEV.

In this case, the current implementation will execute php_copy_file(), VCWD_CHMOD(), VCWD_CHOWN() in this order.

This is around the php_plain_files_rename() function in main/streams/plain_wrapper.c.


	ret = VCWD_RENAME(url_from, url_to);

	if (ret == -1) {
#ifndef PHP_WIN32
# ifdef EXDEV
		if (errno == EXDEV) {
			zend_stat_t sb;
			if (php_copy_file(url_from, url_to) == SUCCESS) {
				if (VCWD_STAT(url_from, &sb) == 0) {
#  ifndef TSRM_WIN32
					if (VCWD_CHMOD(url_to, sb.st_mode)) {
						if (errno == EPERM) {
							php_error_docref2(NULL, url_from, url_to, E_WARNING, "%s", strerror(errno));
							VCWD_UNLINK(url_from);
							return 1;
						}
						php_error_docref2(NULL, url_from, url_to, E_WARNING, "%s", strerror(errno));
						return 0;
					}
					if (VCWD_CHOWN(url_to, sb.st_uid, sb.st_gid)) {
						if (errno == EPERM) {
							php_error_docref2(NULL, url_from, url_to, E_WARNING, "%s", strerror(errno));
							VCWD_UNLINK(url_from);
							return 1;
						}
						php_error_docref2(NULL, url_from, url_to, E_WARNING, "%s", strerror(errno));
						return 0;
					}
#  endif
					VCWD_UNLINK(url_from);
					return 1;
				}
			}
			php_error_docref2(NULL, url_from, url_to, E_WARNING, "%s", strerror(errno));
			return 0;
		}
# endif
#endif
 [2019-02-18 21:57 UTC] stas@php.net
I have a feeling that the best practice here would be just to have proper umask, if this is indeed a problem. Switching CHOWN/CHMOD may make sense (error handling probably will have to be adjusted... not sure).
 [2019-03-03 07:47 UTC] stas@php.net
Proposed fix in https://gist.github.com/smalyshev/4198b4573b002884ff552cffccbf050a, please verify.
 [2019-03-03 07:48 UTC] stas@php.net
-PHP Version: 7.3.2 +PHP Version: 7.1.26 -Assigned To: +Assigned To: stas -CVE-ID: +CVE-ID: needed
 [2019-03-04 07:38 UTC] stas@php.net
-Status: Assigned +Status: Closed
 [2019-03-04 07:38 UTC] stas@php.net
The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.


 [2019-03-04 17:21 UTC] stas@php.net
Automatic comment on behalf of stas
Revision: http://git.php.net/?p=php-src.git;a=commit;h=e3133e4db70476fb7adfdedb738483e2255ce0e1
Log: Fix bug #77630 - safer rename() procedure
 [2019-03-12 19:57 UTC] stas@php.net
-CVE-ID: needed +CVE-ID: 2019-9637
 [2019-03-30 12:24 UTC]
The following pull request has been associated:

Patch Name: list interfaces/adding just "binary" state status.
On GitHub:  https://github.com/php/php-src/pull/3766
Patch:      https://github.com/php/php-src/pull/3766.patch
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Thu Mar 28 18:01:29 2024 UTC