Investigate stack buffer overflow in EndCrashRepoterDialog

RESOLVED FIXED in mozilla9

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: bsterne, Assigned: ted)

Tracking

unspecified
mozilla9
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:moderate])

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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);
}
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].
OS: Mac OS X → Windows XP
(Assignee)

Comment 2

6 years ago
I would agree with that assessment. This should be trivial to patch, in any case.
Assignee: nobody → ted.mielczarek
Status: NEW → ASSIGNED
sg:moderate, is there any reason to keep this bug private?
Whiteboard: [sg:moderate]
(Assignee)

Comment 4

6 years ago
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.
Attachment #562443 - Flags: review?(ehsan)
(Assignee)

Updated

6 years ago
Duplicate of this bug: 688888
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.
Attachment #562443 - Flags: review?(ehsan) → review+
Comment on attachment 562443 [details] [diff] [review]
fix some usage of GetDlgItemText

We have NS_ARRAY_LENGTH for this purpose...
(Assignee)

Comment 8

6 years ago
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.
(Assignee)

Comment 9

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/f0630e6aac18
I think we should also take this on Aurora and Beta.  Well, today's trunk is going to be tomorrow's Aurora... ;-)
(Assignee)

Comment 11

6 years ago
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.
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.
(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?
Merged from inbound:
https://hg.mozilla.org/mozilla-central/rev/f0630e6aac18
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
Target Milestone: --- → mozilla9
(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...
Group: core-security
Duplicate of this bug: 688889
You need to log in before you can comment on or make changes to this bug.