User-Agent: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041122 Build Identifier: Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8a5) Gecko/20041122 windows/nsToolkit.cpp:ConvertWtoA() does not append a null byte if the string exceeds buffer size. Win32 function WideCharToMultiByte() returns 0 on buffer overflow, but fills the buffer to the end, without trailing null byte. This behaviour is undocumented in Win32 API documentation. Reproducible: Always Steps to Reproduce: 1. Call ConvertWtoA() with a too small buffer, e.g. via a call to nsWindow::SetTitle("some string exceeding 127 chars") 2. Examine returned string, e.g. see Bug 244674 for a visible result. Actual Results: ConvertWtoA() produces a non-null terminated string, and all 16 call sites do not check the return value. Expected Results: ConvertWToA() should always produce a null terminated string, or call sites should check for return value 0. This is the reason for Bug 244674. Missing trailing null byte may lead to a buffer overflow in further processing. => Setting Security flag.

Note that ConvertWToA() returns 0 even if the output string fits exactly (size==11). The extra null Termination by ConvertWtoA() in the non-overflow case is not necessary. WideCharToMultiByte() returns #bytes *including* terminating null.

Added missing trailing null on overflow. Kept adding extra null in non-overflow case, don't know if this is necessary. Also fixes passing a too small buffersize (MAX_CLASS_NAME instead of MAX_PATH) in nsSendMessage(). The small size (<160) resulted in visibility of this bug in window title (bug 244674).

Similar problem setting menu items http://lxr.mozilla.org/mozilla/source/xpfe/bootstrap/nsNativeAppSupportWin.cpp#117 I don't see any actual overflow--something that *writes* outside the buffer--but clearly extra garbage is getting included in strings sent to windows.

Comment on attachment 172962 [details] [diff] [review] Patch for ConvertWtoA() and nsSendMessage() r=dveditz

Never mind comment about GetACPString(), missed the equals in 'outlen >= 0': if the converted string is too long we'll end up with an empty string. Since WideCharToMultiByte never returns negative the check is kind of pointless.

(In reply to comment #5) > Never mind comment about GetACPString(), missed the equals ... > ...WideCharToMultiByte never returns negative the check is kind of pointless. Even worse: Unlike ConvertWtoA(), GetACPString() definitely has a write behind buffer issue: WideCharToMultibyte() returns the chars written, not the string length. On exact fit, outlen == acplen => acp[acplen] = '\0' => *Overflow* Suggest at least the following quick fix (untested blind patch, sorry): ... if( acp ) { int outlen = ::WideCharToMultiByte( CP_ACP, 0, aStr.get(),aStr.Length(), acp, acplen, NULL, NULL ); - if ( outlen >= 0) - acp[ outlen ] = '\0'; // null terminate + if (outlen == 0) + acp[acplen-1] = '\0'; // null terminate on overflow } This does not handle other conversion errors correctly, but avoids overflow. Again note that null termination is not necessary in the non-overflow case.

The null termination is necessary because they're not converting the null in WideCharToMultiByte. We could change that and go more your way, or just do this.

Comment on attachment 173302 [details] [diff] [review] fix GetACPString() No, this is wrong. What happens for negative values of outlen? Maybe you want to null terminate at PR_MAX(0, outlen) ?

Comment on attachment 172962 [details] [diff] [review] Patch for ConvertWtoA() and nsSendMessage() a=mkaply for both

Comment on attachment 173302 [details] [diff] [review] fix GetACPString() Never mind me. I misunderstood the patch. r+sr=bzbarsky

Comment on attachment 173302 [details] [diff] [review] fix GetACPString() a=chofmann for the branches

(In reply to comment #7) > Created an attachment (id=173302) [edit] > ... We could change that and go more your way, or just do this. Looks good ;-) BTW: Could you possibly give me access (add as CC:) to the dependent Bugs?

> BTW: Could you possibly give me access (add as CC:) to the dependent Bugs? They're completely unrelated, just tracking bugs predating the creation of the blocking-aviary1.0.1 flag.

Landed on trunk

*** Bug 244674 has been marked as a duplicate of this bug. ***