Bug #69511 [Asn->Csd]: Off-by-one bufferoverflow in php_sys_readlink

Edit report at https://bugs.php.net/bug.php?id=69511&edit=1 ID: 69511 Updated by: ab@php.net Reported by: jan dot starke at t-systems dot com Summary: Off-by-one bufferoverflow in php_sys_readlink -Status: Assigned +Status: Closed Type: Bug Package: Filesystem function related Operating System: Windows PHP Version: master-Git-2015-04-23 (Git) Assigned To: ab Block user comment: N Private report: N New Comment: 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 Previous Comments: ------------------------------------------------------------------------ [2015-04-23 19:23:46] 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 14:26:03] 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 09:57:53] 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. ------------------------------------------------------------------------ -- Edit this bug report at https://bugs.php.net/bug.php?id=69511&edit=1