280522 - Possible Buffer overflow due to missing terminating null [windows/nsToolkit.cpp:ConvertWtoA()]

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Christian Franke]

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.

Comment 1

[Christian Franke]

Attachment: Demo of unexpected overflow handling of ConvertWtoA()

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.

Comment 2

[Christian Franke]

Attachment: Patch for ConvertWtoA() and nsSendMessage()

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).

Comment 3

[Daniel Veditz [:dveditz]]

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 4

[Daniel Veditz [:dveditz]]

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

r=dveditz

Comment 5

[Daniel Veditz [: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.

Comment 6

[Christian Franke]

(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.

Comment 7

[Daniel Veditz [:dveditz]]

Attachment: fix GetACPString()

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 8

[Boris Zbarsky [:bzbarsky]]

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 9

[Mike Kaply [:mkaply]]

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

a=mkaply for both

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173302 [details] [diff] [review]
fix GetACPString()

Never mind me.	I misunderstood the patch.  r+sr=bzbarsky

Comment 11

[chris hofmann]

Comment on attachment 173302 [details] [diff] [review]
fix GetACPString()

a=chofmann for the branches

Comment 12

[Christian Franke]

(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?

Comment 13

[Daniel Veditz [:dveditz]]

> 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.

Comment 14

[Daniel Veditz [:dveditz]]

Landed on trunk

Comment 15

[David :Bienvenu]

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