On 09/06/2011 06:23 AM, Octavian Voicu wrote:
First one is an off-by-one error: RtlGetFullPathName_U, on success, returns the number of bytes written, without counting the terminating NULL. The allocated size for ntpath->Buffer didn't account for that NULL byte, so for UNC paths the NULL byte would be overflown.
Second one is caused by an USHORT overflow in ntpath->MaximumLength. Calling the function with a path longer than 65535-8 bytes would allocate a much shorter buffer and lead to a buffer overflow.
-- Steps to reproduce second issue are described here: http://packetstormsecurity.org/files/view/76170/wine-overflow.txt
Funny fact: Windows had the same USHORT overflow bug: http://www.securityfocus.com/archive/1/323508
dlls/ntdll/path.c | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/dlls/ntdll/path.c b/dlls/ntdll/path.c index 3207720..6138fa8 100644 --- a/dlls/ntdll/path.c +++ b/dlls/ntdll/path.c @@ -383,8 +383,14 @@ BOOLEAN WINAPI RtlDosPathNameToNtPathName_U(PCWSTR dos_path, if (!(ptr = RtlAllocateHeap(GetProcessHeap(), 0, sz))) return FALSE; sz = RtlGetFullPathName_U(dos_path, sz, ptr, file_part); }
- sz += (1 /* NUL */ + 4 /* unc\ */ + 4 /* ??\ */) * sizeof(WCHAR);
- if (sz> MAXWORD)
- {
if (ptr != local) RtlFreeHeap(GetProcessHeap(), 0, ptr);return FALSE;- }
- ntpath->MaximumLength = sz + (4 /* unc\ */ + 4 /* ??\ */) * sizeof(WCHAR);
- ntpath->MaximumLength = sz; ntpath->Buffer = RtlAllocateHeap(GetProcessHeap(), 0, ntpath->MaximumLength); if (!ntpath->Buffer) {
The fix is not entirely correct. UNICODE_STRING does not have to have a terminating \0 character. The code should not use str* functions on not zero-terminated strings.
Vitaliy
On Tue, Sep 6, 2011 at 4:40 PM, Vitaliy Margolen wine-devel@kievinfo.com wrote:
The fix is not entirely correct. UNICODE_STRING does not have to have a terminating \0 character. The code should not use str* functions on not zero-terminated strings.
I was also unsure about this when coding the patch, but I read here [1] that:
"Specifies the length, in bytes, of the string pointed to by the Buffer member, not including the terminating NULL character, if any."
So it's not a mistake to include a NULL character. I figured it's not worth the trouble to change the last strcpy into memcpy, especially because it could also be the case that it would introduce regressions (in case some other code wrongly depends on that NULL).
Should I change offending strcpyW to memcpy?
Octavian
[1] http://msdn.microsoft.com/en-us/library/aa380518(v=vs.85).aspx
On 09/06/2011 08:46 AM, Octavian Voicu wrote:
On Tue, Sep 6, 2011 at 4:40 PM, Vitaliy Margolen wine-devel@kievinfo.com wrote:
The fix is not entirely correct. UNICODE_STRING does not have to have a terminating \0 character. The code should not use str* functions on not zero-terminated strings.
I was also unsure about this when coding the patch, but I read here [1] that:
"Specifies the length, in bytes, of the string pointed to by the Buffer member, not including the terminating NULL character, if any."
So it's not a mistake to include a NULL character. I figured it's not worth the trouble to change the last strcpy into memcpy, especially because it could also be the case that it would introduce regressions (in case some other code wrongly depends on that NULL).
Should I change offending strcpyW to memcpy?
Some ntdll functions do put terminating \0 character into UNICODE_STRINGs. You can write a test to see if RtlDosPathNameToNtPathName_U is one of them. But by definition U_S does not require terminating \0. And many places don't put it there.
Regardless, last strcpy & strlen should go and be replaced with memcpy & pointer arithmetic.
Vitaliy.
-
Octavian Voicu -
Vitaliy Margolen