688839 - Investigate stack buffer overflow in nsWindowsShellService::GetMailAccountKey

Status: RESOLVED WORKSFORME

(Firefox :: Security, 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: GetMailAccountKey

Component: The bug is contained in the nsWindowsShellService Implementation for Windows and in particular in method GetMailAccountKey.

Summary:
The method nsWindowsShellService::GetMailAccountKey reads entries from the Windows registry using the Win32 API function RegEnumKeyExW. This function expects the number of wide­ characters that the destination buffer can hold as its last argument. In GetMailAccountKey, 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:
PRBool nsWindowsShellService::GetMailAccountKey(HKEY* aResult)
{
  NS_NAMED_LITERAL_STRING(unread,
    "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\UnreadMail\\");

  HKEY mailKey;
  DWORD res = ::RegOpenKeyExW(HKEY_CURRENT_USER, unread.get(), 0,
                              KEY_ENUMERATE_SUB_KEYS, &mailKey);

  PRInt32 i = 0;
  do {
    PRUnichar subkeyName[MAX_BUF];
    DWORD len = sizeof subkeyName;
    res = ::RegEnumKeyExW(mailKey, i++, subkeyName, &len, NULL, NULL,
                          NULL, NULL);
    if (REG_SUCCEEDED(res)) {
      HKEY accountKey;
      res = ::RegOpenKeyExW(mailKey, PromiseFlatString(subkeyName).get(),
                            0, KEY_READ, &accountKey);
      [..]
    }
    else
      break;
  }
  while (1);
  // Close the key we opened.
  ::RegCloseKey(mailKey);
  return PR_FALSE;
}

Comment 1

[Mark Banner (:standard8)]

> Function: GetMailAccountKey
> 
> Component: The bug is contained in the nsWindowsShellService Implementation
> for Windows and in particular in method GetMailAccountKey.

nsWindowsShellService::GetMailAccountKEy is not actually in Thunderbird its in Firefox:

http://hg.mozilla.org/mozilla-central/diff/9b2a99adc05e/browser/components/shell/src/nsWindowsShellService.cpp

It appears to be an unused (but available to extensions) function available from this interface:

http://hg.mozilla.org/mozilla-central/diff/9b2a99adc05e/browser/components/shell/public/nsIWindowsShellService.idl

> Summary:
> The method nsWindowsShellService::GetMailAccountKey reads entries from the
> Windows registry using the Win32 API function RegEnumKeyExW. This function
> expects the number of wide­ characters that the destination buffer can hold
> as its last argument. In GetMailAccountKey, 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.

What documentation are they looking at? I'm looking at:

http://msdn.microsoft.com/en-us/library/windows/desktop/ms724862%28v=vs.85%29.aspx

=================== BEGIN EXTRACT =======================

LONG WINAPI RegEnumKeyEx(
  __in         HKEY hKey,
  __in         DWORD dwIndex,
  __out        LPTSTR lpName,
  __inout      LPDWORD lpcName,
  __reserved   LPDWORD lpReserved,
  __inout      LPTSTR lpClass,
  __inout_opt  LPDWORD lpcClass,
  __out_opt    PFILETIME lpftLastWriteTime
);

lpName [out]

    A pointer to a buffer that receives the name of the subkey, including the terminating null character. The function copies only the name of the subkey, not the full key hierarchy, to the buffer. If the function fails, no information is copied to this buffer.

lpcName [in, out]

    A pointer to a variable that specifies the size of the buffer specified by the lpName parameter, in characters. This size should include the terminating null character. If the function succeeds, the variable pointed to by lpcName contains the number of characters stored in the buffer, not including the terminating null character.

    To determine the required buffer size, use the RegQueryInfoKey function to determine the size of the largest subkey for the key identified by the hKey parameter.

=================== END EXTRACT =======================

The code is:

res = ::RegEnumKeyExW(mailKey, i++, subkeyName, &len, NULL, NULL,
                      NULL, NULL);

This says to me that the bit about which parameter is being used is incorrect.

Whether or not the length itself is wrong I'm not quite sure.

Comment 2

[:Cykesiopka]

Bug 693227 removed GetMailAccountKey():
  https://hg.mozilla.org/mozilla-central/rev/cb9611719bc7#l2.32