Bug #69511 [Com]: Off-by-one bufferoverflow in php_sys_readlink

Edit report at https://bugs.php.net/bug.php?id=69511&edit=1 ID: 69511 Comment by: jan dot starke at t-systems dot com Reported by: jan dot starke at t-systems dot com Summary: Off-by-one bufferoverflow in php_sys_readlink Status: Feedback 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: 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 Previous Comments: ------------------------------------------------------------------------ [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