688885 - Investigate stack buffer overflow in nsWindowsShellService::OpenApplication

Status: RESOLVED INVALID

(Firefox :: Shell Integration, defect)

Description

[Brandon Sterne (:bsterne)]

Method: nsWindowsShellService::OpenApplication

Component:
The bug is contained in the ShellService implementation for Windows in method nsWindowsShellService::OpenApplication.

Summary:
The method nsWindowsShellService::OpenApplication contains a call to the Win32 API function GetEnvironmentVariableW, which potentially overflows a stack­based buffer.

Overview:
The method nsWindowsShellService::OpenApplication contains a loop, which is used to read environment variables into a local stack­buffer by using the Win32 API function GetEnvironmentVariableW. This function expects the number of wide­characters that the destination buffer can hold as its last argument. In OpenApplication, 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 by supplying a overly long environment variable.

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

Expected Results:
The buffer cannot be overflowed.

Additional Information:
NS_IMETHODIMP nsWindowsShellService::OpenApplication(PRInt32 aApplication)
{
  [..]
  PRUnichar buf[MAX_BUF];
  [..]

  // Look for any embedded environment variables and substitute their
  // values, as |::CreateProcessW| is unable to do this.
  nsAutoString path(buf);
  PRInt32 end = path.Length();
  PRInt32 cursor = 0, temp = 0;
  ::ZeroMemory(buf, sizeof(buf));
  do {
    cursor = path.FindChar('%', cursor);
    if (cursor < 0)
      break;

    temp = path.FindChar('%', cursor + 1);
    ++cursor;

    ::ZeroMemory(&buf, sizeof(buf));
    ::GetEnvironmentVariableW(nsAutoString(Substring(path, cursor, temp – cursor)).get(), buf, sizeof(buf));

    // "+ 2" is to subtract the extra characters used to delimit the environment
    // variable ('%').
    path.Replace((cursor - 1), temp - cursor + 2, nsDependentString(buf));
    ++cursor;
  }
  [..]
  return NS_OK;
}

Comment 1

[Brandon Sterne (:bsterne)]

Whoops, forgot the preamble to say:

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

(insert comment 0)

Comment 2

[Daniel Veditz [:dveditz]]

I'm not sure who's hacking who here with an environment variable.

Comment 3

[Mark Banner (:standard8)]

Bug 324361 has just removed nsWindowsShellService::OpenApplication as it is unused code and hasn't been used in ages.