842687 - Stack corruption in plug-in code caused by PRBool to bool switch

Status: RESOLVED FIXED

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

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Filed as security bug because "stack corruption" == security in my head :)

This is the cause of Fennec crash, bug 827171. Basically PRBool is 4-bytes and bool is 1-byte, but as part of the PRBool-to-bool conversion, we are passing 1-byte bools to plugins that still expect 4-byte PRBools. Depending on ordering and packing of variables, this could end up corrupting another variable which is the case in Bug 827171.

I see at least two cases of this in nsPluginStreamListenerPeer,
[1] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginStreamListenerPeer.cpp#129
[2] http://mxr.mozilla.org/mozilla-central/source/dom/plugins/base/nsPluginStreamListenerPeer.cpp#479
But there could be more instances.

Comment 1

[John Schoenick [:johns]]

We'll that's... bad

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

There are some other names for NPP_GetValue that return bool, but looks like we don't use these. I agree that changing bool back to PRBool or some other 32 bit type should fix things.

Comment 3

[Jeff Walden [:Waldo]]

Strange.  I would have thought this stuff in NPAPI would be represented by a NPBool, which in npapi.h is:

    typedef unsigned char NPBool;

which I understand is binary-equivalent to bool with most/all compilers we care about these days.  Is it at all possible our underlying plugin stuff is using PRBool where it should have used NPBool?

Comment 4

[Benjamin Smedberg]

Dammit, again! We have explicit code to avoid this in the OOPP codepath, see e.g. http://mxr.mozilla.org/mozilla-central/source/dom/plugins/ipc/PluginInstanceChild.cpp#604

I'm going to mark this critical (for Android only) even though in practice it would be extremely difficult to construct an exploit. It doesn't affect desktop because we use OOPP by default there and get the behavior correct.

Comment 5

[Benjamin Smedberg]

Oh, and note that even though the NPAPI officially should use 1-byte type, plugins do all sorts of weird shit and we should expect a 4-byte write at the address.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #3)
> Strange.  I would have thought this stuff in NPAPI would be represented by a
> NPBool, which in npapi.h is:
> 
>     typedef unsigned char NPBool;
> 
> which I understand is binary-equivalent to bool with most/all compilers we
> care about these days.  Is it at all possible our underlying plugin stuff is
> using PRBool where it should have used NPBool?

This call goes straight into the plugin, so it depends entirely on the plugin behavior. Apparently Flash on Android expects a 32 bit value.

Comment 7

[Georg Fritzsche [:gfritzsche]]

I only found two more places that need fixing:
http://dxr.mozilla.org/mozilla-central/dom/plugins/ipc/PluginInstanceChild.cpp#l1265
http://dxr.mozilla.org/mozilla-central/dom/plugins/ipc/PluginModuleParent.cpp#l1414

... using this query:
http://dxr.mozilla.org/search?tree=mozilla-central&q=path%3Adom%2Fplugins%2F+getvalue&redirect=true

I can't see another affected NPP function, but maybe i'm missing something.

AFAIR we do have regular test runs on ASAN-enabled builds? If so, we could just make the test-plugin do a 4-byte write where-ever this can happen and have it caught by the ASAN builds if regressed.

Comment 8

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> I only found two more places that need fixing:
> http://dxr.mozilla.org/mozilla-central/dom/plugins/ipc/PluginInstanceChild.
> cpp#l1265
> http://dxr.mozilla.org/mozilla-central/dom/plugins/ipc/PluginModuleParent.
> cpp#l1414

Nevermind, those are covered or irrelevant.

Comment 9

[Alex Keybl [:akeybl]]

Tracking for all upcoming releases, but this is unlikely to be taken in FF19 or a FF19 chemspill (unless there are extreme circumstances here such as visibility or being a recent regression).

Comment 10

[Benjamin Smedberg]

The only reason to put this up the trains is because it is a topcrash. Actually constructing an exploit for this is probably impossible.

Comment 11

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> AFAIR we do have regular test runs on ASAN-enabled builds? If so, we could
> just make the test-plugin do a 4-byte write where-ever this can happen and
> have it caught by the ASAN builds if regressed.

Confirmed this, decoder is currently running them daily on try until releng has this integrated in an automated fashion.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Use uint32 instead of bool for certain plug-in values (v1)

This patch changes bool to uint32_t, which matches the OOPP codepath

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #11)
> (In reply to Georg Fritzsche [:gfritzsche] from comment #7)
> > AFAIR we do have regular test runs on ASAN-enabled builds? If so, we could
> > just make the test-plugin do a 4-byte write where-ever this can happen and
> > have it caught by the ASAN builds if regressed.
> 
> Confirmed this, decoder is currently running them daily on try until releng
> has this integrated in an automated fashion.

I'm not familiar with ASAN or the test plugin. Can you file a bug for adding the test?

Comment 14

[Georg Fritzsche [:gfritzsche]]

Attachment: Use 4-byte writes for outgoing bool values in NPP_GetValues

Just putting the minor test modification up here if that's alright.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Commit: https://hg.mozilla.org/integration/mozilla-inbound/rev/b12d2ab1a3e2
Test: https://hg.mozilla.org/integration/mozilla-inbound/rev/1641778f1619

Thanks for writing the test, Georg!

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

With this crash affecting all the way to Fx19, what's the likelihood of uplifting?

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Benjamin told me on IRC that we might be OK with uplifting these patches to Aurora and Beta. 

Jim and Georg - What's the risk? If you think it's OK could you request approval?

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Mark Finkle (:mfinkle) from comment #17)
> Benjamin told me on IRC that we might be OK with uplifting these patches to
> Aurora and Beta. 
> 
> Jim and Georg - What's the risk? If you think it's OK could you request
> approval?

I think Aurora and Beta sound good. This was the #1 topcrash for Aurora, now Beta.

There is no risk for this, but since we're early in the cycle, I was thinking about waiting a few days and first confirming we stop getting more crashes on Nightly.

Comment 19

[Georg Fritzsche [:gfritzsche]]

(In reply to Jim Chen [:jchen :nchen] from comment #18)
> There is no risk for this

+1, i don't think this really needs to wait long.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b12d2ab1a3e2
https://hg.mozilla.org/mozilla-central/rev/1641778f1619

Comment 21

[Daniel Veditz [:dveditz]]

We're not shipping an Android ESR so Firefox 17 ESR Desktop versions should not need this fix per comment 4.

b2g18 doesn't support plugins and also should not need this fix.

Comment 22

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 716111 [details] [diff] [review]
Use uint32 instead of bool for certain plug-in values (v1)

Only crashes over last 7 days are before this landed; so requesting uplift. Should I request to uplift the test too?


[Approval Request Comment]

Bug caused by (feature/regressing bug #): N/A

User impact if declined: Random stack corruption / crash when using Flash on Android

Testing completed (on m-c, etc.): m-c

Risk to taking this patch (and alternatives if risky): No risk; patch only fixes crash and does not impact functionality in any way.

String or UUID changes made by this patch: None

Comment 23

[Benjamin Smedberg]

Uplift the test assuming it applies, yes.

Comment 24

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 716140 [details] [diff] [review]
Use 4-byte writes for outgoing bool values in NPP_GetValues

[Approval Request Comment]
Bug caused by (feature/regressing bug #): See comment 22.
User impact if declined: n/a, this patch only provides test-coverage for the issue.
Testing completed (on m-c, etc.): See comment 22.
Risk to taking this patch (and alternatives if risky): None.
String or UUID changes made by this patch: None.

Comment 25

[bhavana bajaj [:bajaj]]

Comment on attachment 716111 [details] [diff] [review]
Use uint32 instead of bool for certain plug-in values (v1)

low risk patch helps fixing a top-crasher . Approving for uplift on aurora/beta

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/22d26a3e75e3
https://hg.mozilla.org/releases/mozilla-aurora/rev/3dbdde25f5f5

https://hg.mozilla.org/releases/mozilla-beta/rev/ed89526b7d3f
https://hg.mozilla.org/releases/mozilla-beta/rev/9d5f767079ec

Comment 27

[Kevin Brosnan [Ex-Mozilla]]

Worth relnoting this? I think this made Flash a bit more stable on Android.

Comment 28

[Alex Keybl [:akeybl]]

(In reply to Kevin Brosnan [:kbrosnan] from comment #27)
> Worth relnoting this? I think this made Flash a bit more stable on Android.

Likely won't, since it would mean implying that improving our current stability situation is worth noting (we're already pretty good).