688888 - Investigate stack buffer overflow in UpdateComment

Status: RESOLVED DUPLICATE of bug 688877

(Toolkit :: Crash Reporting, defect)

Description

[Brandon Sterne (:bsterne)]

Fabian Yamaguchi of Recurity Labs on behalf of BSI (German Federal Office for Information Security) reported the following to [e-mail]:

Function: UpdateComment

Component:
The bug is contained in the Crash Reporter Implementation for Windows, in function UpdateComment in particular.

Summary:
The function UpdateComment reads textual data from a dialog item into a local buffer using the Win32 API function GetDlgItemText. This function expects the number of wide­characters that the destination buffer can hold as its last argument. In UpdateComment, this third parameter has mistakenly been chosen to contain the number of bytes the buffer consists of, which is twice as much as the number of wide­characters it can hold. Therefore, it may be possible to trigger a stack­ based buffer­overflow.

Actual Results:
A local stack­buffer can potentially be overflowed.

Expected Results:
The buffer cannot be overflowed.

Additional Information:
static void UpdateComment(HWND hwndDlg)
{
  wchar_t comment[MAX_COMMENT_LENGTH + 1];
  GetDlgItemText(hwndDlg, IDC_COMMENTTEXT, comment, sizeof(comment));
  if (wcslen(comment) > 0)
    gQueryParameters[L"Comments"] = comment;
  else
    gQueryParameters.erase(L"Comments");
}

Comment 1

[Benjamin Smedberg]

Ted, does your other patch fix this also?

Comment 2

[(not currently active) Ted Mielczarek]

Yes, that patch fixes all instances of this.