1410140 - PluginModuleChromeParent::AnswerGetFileName - Heap Buffer Overflow

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Stephen Fewer]

As part of a code review I am conducting for Paul Theriault against the sandbox, the following vulnerability has been discovered.

A heap based buffer overflow exists in the chrome processes PluginModuleChromeParent::AnswerGetFileName method. This method is intended to be called via IPC by an NPAPI process when an intercepted API call to comdlg32!GetSaveFileNameW or comdlg32!GetOpenFileNameW is encountered (to proxy the call over the the chrome process for servicing). A compromised NPAPI process may provide malicious arguments to the AnswerGetFileName method and trigger a heap based buffer overflow in the chrome process.

When the chrome process receives an IPC call to AnswerGetFileName, aOfnIn is a structure provided by the attacker, which amongst other members, contains a uint32_t member called mNMaxFile and a std::wstring called mFile.

> PluginModuleChromeParent::AnswerGetFileName(const GetFileNameFunc& aFunc,
>                                             const OpenFileNameIPC& aOfnIn,
>                                             OpenFileNameRetIPC* aOfnOut,
>                                             bool* aResult)
> {
> #if defined(XP_WIN) && defined(MOZ_SANDBOX)
>     OPENFILENAMEW ofn;
>     memset(&ofn, 0, sizeof(ofn));
>     aOfnIn.AllocateOfnStrings(&ofn);
>     aOfnIn.AddToOfn(&ofn);
    
First various buffers for ofn are allocated via AllocateOfnStrings() based on the aOfnIn structures values. In particular lpstrFile is allocated with the attacker controlled mNMaxFile value.

> OpenFileNameIPC::AllocateOfnStrings(LPOPENFILENAMEW aLpofn) const
> {
>   //...snip...
>   
>   aLpofn->lpstrFile =
>     static_cast<LPTSTR>(moz_xmalloc(sizeof(wchar_t) * mNMaxFile));
>     
>   //...snip...
  
However, the contents of mFile are string copied over to lpstrFile which will overflow the allocated heap buffer it the attacker supplies a value for mNMaxFile which is less that the length of the attacker controlled string mFile.

> OpenFileNameIPC::AddToOfn(LPOPENFILENAMEW aLpofn) const
> {
>   //...snip...
>   
>   wcscpy(aLpofn->lpstrFile, mFile.c_str());
> 
>   //...snip...

Comment 1

[Jim Mathies [:jimm]]

David, please take a look.

Comment 2

[David Parks [:handyman]]

Attachment: Safely copy file dialog fields when brokering GetOpenFileName for NPAPI process

The patch truncates the copy to the maximum valid length to avoid the overrun spelled out in the bug.  I added a null termination guarantee since the Win32 docs aren't clear on what would happen otherwise (and it would only matter if we got bad data from the client anyway).

While I was there, I noticed another buffer issue -- this one was a char vs wide-char byte count deal -- mNMaxCustFilterOut is a wide-char count.

FWIW, normal (non-malicious) use of the APIs involved can be tested here:
http://www.tinywebgallery.com/en/tfu/web_demo2.php

Comment 3

[David Parks [:handyman]]

Comment on attachment 8920905 [details] [diff] [review]
Safely copy file dialog fields when brokering GetOpenFileName for NPAPI process

On further thought, I also need to take care of the case where the client sends mNMaxFile == 0.

Comment 4

[David Parks [:handyman]]

Attachment: Safely copy file dialog fields when brokering GetOpenFileName for NPAPI process

Fixed size == 0 case.

Comment 5

[David Parks [:handyman]]

Comment on attachment 8921145 [details] [diff] [review]
Safely copy file dialog fields when brokering GetOpenFileName for NPAPI process

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The patch fixes an exploitable IPC message.  In order to take advantage of it, an attacker would likely need to already control the NPAPI process in order to issue an invalid IPDL message.  They would also need to be able to use a buffer overrun in the main process.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
Introduced by bug 1284897 -- firefox 54.  It has existed in every version since.

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should be trivially backportable.

How likely is this patch to cause regressions; how much testing does it need?
Regressions are very unlikely.  I do not feel that any further testing beyond the use case in comment 2 is needed.

Comment 6

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk now. I don't think we should take this on 57 at this point.

Comment 7

[Julien Cristau [:jcristau] (back April 22)]

57 wontfix per comment 6.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d2a3e89c197e85b393b581d50a2ff9070c46ff4

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d2a3e89c197