Last Comment Bug 688877 - Investigate stack buffer overflow in EndCrashRepoterDialog
: Investigate stack buffer overflow in EndCrashRepoterDialog
Status: RESOLVED FIXED
[sg:moderate]
:
Product: Toolkit
Classification: Components
Component: Breakpad Integration (show other bugs)
: unspecified
: x86 Windows XP
: -- normal (vote)
: mozilla9
Assigned To: Ted Mielczarek [:ted.mielczarek]
:
Mentors:
: 688888 688889 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-23 14:21 PDT by Brandon Sterne (:bsterne)
Modified: 2013-02-03 13:26 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
fix some usage of GetDlgItemText (1.72 KB, patch)
2011-09-26 08:50 PDT, Ted Mielczarek [:ted.mielczarek]
ehsan: review+
Details | Diff | Splinter Review

Description Brandon Sterne (:bsterne) 2011-09-23 14:21:39 PDT
Fabian Yamaguchi of Recurity Labs on behalf of BSI (German Federal Office for Information Security) reported the following to security@mozilla.org:

Function: EndCrashRepoterDialog

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

Summary:
The function EndCrashReporterDialog 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 EndCrashReporterDialog, 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 EndCrashReporterDialog(HWND hwndDlg, int code)
{
  // Save the current values to the registry
  wchar_t email[MAX_EMAIL_LENGTH];
  GetDlgItemText(hwndDlg, IDC_EMAILTEXT, email, sizeof(email));
  SetStringKey(gCrashReporterKey.c_str(), EMAIL_VALUE, email);

  SetBoolKey(gCrashReporterKey.c_str(), INCLUDE_URL_VALUE,
             IsDlgButtonChecked(hwndDlg, IDC_INCLUDEURLCHECK) != 0);
  SetBoolKey(gCrashReporterKey.c_str(), EMAIL_ME_VALUE,
             IsDlgButtonChecked(hwndDlg, IDC_EMAILMECHECK) != 0);
  SetBoolKey(gCrashReporterKey.c_str(), SUBMIT_REPORT_VALUE,
             IsDlgButtonChecked(hwndDlg, IDC_SUBMITREPORTCHECK) != 0);
  EndDialog(hwndDlg, code);
}
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-23 14:45:24 PDT
If I understand the code correctly, exploitation of this would require crashing the application (easy), then getting the user to type an email address longer than 1024 characters into the crash reporter dialog text field for the user's email address (not so easy).  That address would probably have to have something squirrelly in the characters past 1024 in that address.  Stranger things are possible, but I have my doubts that very many people at all would be fooled into exploiting themselves here, which is the only way I think it could happen.

That's not to say this shouldn't be fixed -- just that this seems like the rare buffer overflow that's not knee-jerk [sg:critical].
Comment 2 Ted Mielczarek [:ted.mielczarek] 2011-09-26 06:28:14 PDT
I would agree with that assessment. This should be trivial to patch, in any case.
Comment 3 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-09-26 08:01:08 PDT
sg:moderate, is there any reason to keep this bug private?
Comment 4 Ted Mielczarek [:ted.mielczarek] 2011-09-26 08:50:35 PDT
Created attachment 562443 [details] [diff] [review]
fix some usage of GetDlgItemText

There were a few other places making the same mistake, so I fixed them as well. Ehsan: could you review this? It's just some simple Win32 API usage.
Comment 5 Ted Mielczarek [:ted.mielczarek] 2011-09-26 09:23:39 PDT
*** Bug 688888 has been marked as a duplicate of this bug. ***
Comment 6 :Ehsan Akhgari 2011-09-26 11:12:37 PDT
Comment on attachment 562443 [details] [diff] [review]
fix some usage of GetDlgItemText

I generally prefer sizeof(array)/sizeof(array[0]) to make sure that the type name in the second sizeof will be correct, but I don't care much.  r=me either way.
Comment 7 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2011-09-26 11:33:58 PDT
Comment on attachment 562443 [details] [diff] [review]
fix some usage of GetDlgItemText

We have NS_ARRAY_LENGTH for this purpose...
Comment 8 Ted Mielczarek [:ted.mielczarek] 2011-09-26 11:36:31 PDT
The crash reporter client doesn't pull in any headers from core Mozilla code, since it's a standalone native platform app. I'd like to stay at the top of that slippery slope.
Comment 9 Ted Mielczarek [:ted.mielczarek] 2011-09-26 11:44:16 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/f0630e6aac18
Comment 10 :Ehsan Akhgari 2011-09-26 11:46:51 PDT
I think we should also take this on Aurora and Beta.  Well, today's trunk is going to be tomorrow's Aurora... ;-)
Comment 11 Ted Mielczarek [:ted.mielczarek] 2011-09-26 12:09:03 PDT
I probably should have landed that on m-c instead of inbound then. I don't think this is really a serious vulnerability given how roundabout the exploit would have to be. There's no way to pre-fill any of these fields except by writing data to the Windows registry, and any app that can do that doesn't really need to exploit you.
Comment 12 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-26 13:41:14 PDT
Rather than repeat the same trick several times, I'd just add this and use it the various places:

template<typename T, size_t N> size_t
ArrayLength(T (&arr)[N])
{
  return N;
}

It probably doesn't matter here, but this also avoids the problem that the sizeof-division trick has that it'll happily "work" on a plain old pointer that's not an array-of-type.
Comment 13 :Ehsan Akhgari 2011-09-26 14:11:58 PDT
(In reply to Jeff Walden (remove +bmo to email) from comment #12)
> Rather than repeat the same trick several times, I'd just add this and use
> it the various places:
> 
> template<typename T, size_t N> size_t
> ArrayLength(T (&arr)[N])
> {
>   return N;
> }
> 
> It probably doesn't matter here, but this also avoids the problem that the
> sizeof-division trick has that it'll happily "work" on a plain old pointer
> that's not an array-of-type.

Can you please file a bug about that in Core::XPCOM?  Maybe we can rewrite NS_ARRAY_LENGTH to use this better logic?
Comment 14 Justin Wood (:Callek) 2011-09-27 04:29:13 PDT
Merged from inbound:
https://hg.mozilla.org/mozilla-central/rev/f0630e6aac18
Comment 15 Jeff Walden [:Waldo] (remove +bmo to email) 2011-10-20 20:09:34 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #13)
> Can you please file a bug about that in Core::XPCOM?  Maybe we can rewrite
> NS_ARRAY_LENGTH to use this better logic?

I filed bug 693469 on this and did the work to convert most of the codebase to mozilla::ArrayLength, which fixes this.  (There are niggling C++ limitations preventing complete replacement and removal; see the bug.)  That couldn't be used here, alas, because mfbt is not sufficiently minimal to be used in Breakpad, depending as it does on the JS engine.  Maybe someone will fix that someday...
Comment 16 Justin Dolske [:Dolske] 2013-02-03 13:26:38 PST
*** Bug 688889 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.