php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #69511 Off-by-one bufferoverflow in php_sys_readlink
Submitted: 2015-04-23 09:57 UTC Modified: 2015-04-23 20:18 UTC
From: jan dot starke at t-systems dot com Assigned: ab
Status: Closed Package: Filesystem function related
PHP Version: master-Git-2015-04-23 (Git) OS: Windows
Private report: No CVE-ID:
 [2015-04-23 09:57 UTC] jan dot starke at t-systems dot com
Description:
------------
php_sys_readlink ignores the target_len parameter (which equals MAXPATHLEN-1) and instead passes MAXPATHLEN to GetFinalPathNameByHandle. Because GetFinalPathNameByHandle actually writes a terminating null character, this could lead to a off-by-one buffer overflow.

However, php_sys_readlink should not assume the length of the target buffer, but should  use target_len instead.


Patches

0001-use-of-target_len-instead-of-MAXPATHLEN-as-buffer-si.patch (last revision 2015-04-23 09:59 UTC) by jan dot starke at t-systems dot com)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2015-04-23 14:26 UTC] ab@php.net
-Status: Open +Status: Feedback -Assigned To: +Assigned To: ab
 [2015-04-23 14:26 UTC] ab@php.net
In the core this is not an issue, looking through the codes - any target in a buf[MAXPATHLEN] . Then it's also checked with >=, so there's the room for \0. However yep, the php_sys_readlink is an exported symbol, so when ignoring target_len and a user passed target_len < MAXLENPATH, it'll overflow the target. So I'd rather not touch the places where it's used, but make it respect the target_len and check also dwRet >= target_len || dwRet >= MAXLENPATH ...

Thanks.
 [2015-04-23 19:23 UTC] jan dot starke at t-systems dot com
You're fully correct. I don't like making assumptions about buffer lengths, wether it is in the core or not. But it's your policy, and I can live with that.

According to its documentation, GetFinalPathNameByHandleA writes up to cchFilePath+1 bytes, which in our case equals to MAXPATHLEN+1: "The size of lpszFilePath, in TCHARs. This value does not include a NULL termination character." But currently there seems to be a bug/misbehaviour/feature in GetFinalPathNameByHandleA, so that cchFilePath includes the terminating null character (have a look at the comments below: https://msdn.microsoft.com/en-us/library/windows/desktop/aa364962%28v=vs.85%29.aspx). The correctness of your code relies on a bug in foreign code. One more assumption :-(

In my eyes, you should fix this and remove the assumptions, as this is cheap and improves your code quality. But you you're right in that this is not really a bug :-(

Kind regards, Jan
 [2015-04-23 20:18 UTC] cmb@php.net
-Status: Feedback +Status: Open
 [2015-05-19 22:07 UTC] ab@php.net
Automatic comment on behalf of ab
Revision: http://git.php.net/?p=php-src.git;a=commit;h=890a28d4b97b5785f155618fc34134acb77f7a64
Log: Fixed bug #69511 Off-by-one bufferoverflow in php_sys_readlink
 [2015-05-19 22:07 UTC] ab@php.net
-Status: Assigned +Status: Closed
 [2016-07-20 11:38 UTC] davey@php.net
Automatic comment on behalf of ab
Revision: http://git.php.net/?p=php-src.git;a=commit;h=890a28d4b97b5785f155618fc34134acb77f7a64
Log: Fixed bug #69511 Off-by-one bufferoverflow in php_sys_readlink
 
PHP Copyright © 2001-2017 The PHP Group
All rights reserved.
Last updated: Tue Feb 28 14:01:40 2017 UTC