1411631 - PluginModuleChromeParent::AnswerGetFileName - Grant Arbitrary File Read Access.

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.

The chrome processes PluginModuleChromeParent::AnswerGetFileName method can be abused by an attacker in a compromised NPAPI process to force the chrome process to grant read access to the NPAPI process for arbitrary files, allowing an attacker to bypass existing sandbox file access restrictions.

The chrome process receives a call to AnswerGetFileName from an NPAPI process when servicing the GetFileName IPC request (See PPluginModule.ipdl). The attacker can supply an arbitrary file path in the aOfnIn structure which will be copied over to a local ofn structure. The aFunc parameter is an enum of type GetFileNameFunc which should contain only two valid values OPEN_FUNC or SAVE_FUNC. However the IPC de-serialization for this enum type is incorrect and allows an attacker to supply any value, including values out of the valid enum range. This can cause the "switch (aFunc)" statement to fall through and attempt to process the result. This allows the attacker to bypass the user interaction portion of AnswerGetFileName, which under normal circumstances would display a dialog to the user to confirm opening or saving a file. The aResult bool variable is tested for true before processing the result, this value is uninitialized and its value is undefined (it will be latent stack/register data) and there is a reasonably chance this test will pass (or attacker can keep trying until it does). The auto genned IPDL code which calls AnswerGetFileName creates this bool value before calling AnswerGetFileName and it is uninitialized in this autogenned code.

> PluginModuleChromeParent::AnswerGetFileName(const GetFileNameFunc& aFunc,  // <-- attacker controlled.
>                                             const OpenFileNameIPC& aOfnIn, // <-- attacker controlled.
>                                             OpenFileNameRetIPC* aOfnOut,
>                                             bool* aResult)
> {
> #if defined(XP_WIN) && defined(MOZ_SANDBOX)
>     OPENFILENAMEW ofn;
>     memset(&ofn, 0, sizeof(ofn));
>     aOfnIn.AllocateOfnStrings(&ofn);
>     aOfnIn.AddToOfn(&ofn);
>     switch (aFunc) { // <--- attacker can supply an invalid value. switch statement has no default case so we can just fall through.
>     case OPEN_FUNC:
>         *aResult = GetOpenFileName(&ofn);
>         break;
>     case SAVE_FUNC:
>         *aResult = GetSaveFileName(&ofn);
>         break;
>     }
>     if (*aResult) { // <-- aResult is uninitialized so attacker may pass this check if *aResult is not 0 (which is probable).
>         if (ofn.Flags & OFN_ALLOWMULTISELECT) {
>             
>             // ...snip...
>             
>         }
>         else {
>             mSandboxPermissions.GrantFileAccess(OtherPid(), ofn.lpstrFile, // <-- attacker can control lpstrFile and therefore grant read access to this file.
>                                                  aFunc == SAVE_FUNC);
>         }
>         aOfnOut->CopyFromOfn(&ofn);
>     }

Below we can see how the GetFileNameFunc enum is incorrectly de serialized, casting any value back into the enum type.

> struct ParamTraits<mozilla::plugins::GetFileNameFunc>
> {
>    
>     // ...snip...
>     
>     static bool Read(const Message* aMsg, PickleIterator* aIter, paramType* aResult)
>     {
>         uint32_t result;
>         if (ReadParam(aMsg, aIter, &result)) {
>             *aResult = static_cast<paramType>(result); // <-- any uint32_t value is simply cast into the enum type.
>             return true;
>         }
>         return false;
>     }

Here we can see the auto genned IPDL code (.\mozilla-unified\obj-x86_64-pc-mingw32\ipc\ipdl\PPluginModuleParent.cpp) to construct an IPC call to AnswerGetFileName. Of note is how "bool aResult;" is uninitialized, allowing the attacker to pass the "if (*aResult) {" check in AnswerGetFileName if the latent stack data in this uninitialized variable is not 0. If the data is 0, the attacker could just keep trying until the uninitialized variable is not 0.

> PPluginModule::Transition(PPluginModule::Msg_GetFileName__ID, (&(mState)));
> int32_t id__ = MSG_ROUTING_CONTROL;
> OpenFileNameRetIPC aOfnOut;
> bool aResult; // <-- this is uninitialized so may be true depending on latent stack data at this location.
> if ((!(AnswerGetFileName(mozilla::Move(aFunc), mozilla::Move(aOfnIn), (&(aOfnOut)), (&(aResult)))))) {
>     mozilla::ipc::ProtocolErrorBreakpoint("Handler returned error code!");
>     // Error handled in mozilla::ipc::IPCResult
>     return MsgProcessingError;
> }

The fix for this bug should contain the following:

    1. Use ContiguousEnumSerializer for ParamTraits<mozilla::plugins::GetFileNameFunc> so an attacker cannot specify an unexpected value.
    
    2. Add a default case to the switch (aFunc) which sets "*aResult = false;", while this may not be necessary if 1. is in place, it is best practise.

Comment 1

[Wennie]

Hi Jim:

Please reassign this bug to appropriate developer.

Thanks!

Wennie

Comment 2

[Jim Mathies [:jimm]]

David, please take a look. Might be a dupe.

Comment 3

[David Parks [:handyman]]

Attachment: Plug holes in OpenFileName enum serialization

Comment 0 already says everything anyone could ever want to know about this bug.

Comment 4

[David Parks [:handyman]]

Comment on attachment 8924252 [details] [diff] [review]
Plug holes in OpenFileName enum serialization

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
The attacker would need to synthesize a phony IPDL message so presumably would already need to have corrupted the NPAPI process.  So, not easy.

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?
This is a simple switch to the IPDL enum helpers so regressions are highly unlikely.  Additional testing is not needed.

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to David Parks (dparks) [:handyman] from comment #4)
> How easily could an exploit be constructed based on the patch?
> The attacker would need to synthesize a phony IPDL message so presumably
> would already need to have corrupted the NPAPI process.  So, not easy.

Assume the attacker has control of the NPAPI process -- that's the point of the sandbox. Given that, does the patch point to how to exploit it? Seems to: clear how to make aResult uninitialized, and you can follow from there what it's used for.

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

Don't forget the ESR-52 branch is still supported so this question definitely applies! But you gave the regressing bug in the previous question, and thankfully ESR-52 doesn't need to be patched.

I agree with your risk assessment that this would be safe to take on Beta 57, but we're pretty close to merging/shipping so approving that is up to release-mgmt. Please request beta approval: safe patch, fairly obvious sandbox escape once the patch lands.

Comment 6

[Daniel Veditz [:dveditz]]

Comment on attachment 8924252 [details] [diff] [review]
Plug holes in OpenFileName enum serialization

sec-approval=dveditz

Comment 7

[David Parks [:handyman]]

Will consider this for beta after landing.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e195ac70ab165bd6806d0bffc3db25fcdd708c51

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e195ac70ab16

Comment 10

[Julien Cristau [:jcristau]]

we've built the 2nd release candidate for 57 now, it's too late to take this.