Last Comment Bug 842687 - Stack corruption in plug-in code caused by PRBool to bool switch
: Stack corruption in plug-in code caused by PRBool to bool switch
Status: RESOLVED FIXED
[Android only][adv-main20+]
: sec-critical
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: mozilla22
Assigned To: Jim Chen [:jchen] [:darchons]
:
Mentors:
Depends on:
Blocks: 675553 827171
  Show dependency treegraph
 
Reported: 2013-02-19 11:41 PST by Jim Chen [:jchen] [:darchons]
Modified: 2013-11-25 13:26 PST (History)
14 users (show)
nchen: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
affected
+
fixed
+
fixed
+
fixed
-
unaffected
unaffected
affected
affected
-


Attachments
Use uint32 instead of bool for certain plug-in values (v1) (2.87 KB, patch)
2013-02-20 10:44 PST, Jim Chen [:jchen] [:darchons]
benjamin: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Review
Use 4-byte writes for outgoing bool values in NPP_GetValues (1.27 KB, patch)
2013-02-20 11:21 PST, Georg Fritzsche [:gfritzsche]
benjamin: review+
bajaj.bhavana: approval‑mozilla‑aurora+
bajaj.bhavana: approval‑mozilla‑beta+
Details | Diff | Review

Description Jim Chen [:jchen] [:darchons] 2013-02-19 11:41:06 PST
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] 2013-02-19 11:44:14 PST
We'll that's... bad
Comment 2 James Willcox (:snorp) (jwillcox@mozilla.com) 2013-02-19 11:48:42 PST
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] (remove +bmo to email) 2013-02-19 11:53:03 PST
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 [:bsmedberg] 2013-02-19 11:53:03 PST
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 [:bsmedberg] 2013-02-19 11:55:16 PST
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) (jwillcox@mozilla.com) 2013-02-19 11:57:11 PST
(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] 2013-02-20 07:41:38 PST
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] 2013-02-20 07:51:27 PST
(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] 2013-02-20 09:55:32 PST
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 [:bsmedberg] 2013-02-20 09:58:56 PST
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] 2013-02-20 10:18:14 PST
(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 Jim Chen [:jchen] [:darchons] 2013-02-20 10:44:59 PST
Created attachment 716111 [details] [diff] [review]
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 Jim Chen [:jchen] [:darchons] 2013-02-20 10:48:59 PST
(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] 2013-02-20 11:21:03 PST
Created attachment 716140 [details] [diff] [review]
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 Jim Chen [:jchen] [:darchons] 2013-02-20 12:23:11 PST
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?) 2013-02-20 13:04:20 PST
With this crash affecting all the way to Fx19, what's the likelihood of uplifting?
Comment 17 Mark Finkle (:mfinkle) (use needinfo?) 2013-02-20 14:27:47 PST
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 Jim Chen [:jchen] [:darchons] 2013-02-20 15:33:38 PST
(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] 2013-02-21 05:11:55 PST
(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 21 Daniel Veditz [:dveditz] 2013-02-21 16:24:04 PST
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 Jim Chen [:jchen] [:darchons] 2013-02-25 08:22:43 PST
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 [:bsmedberg] 2013-02-25 08:28:05 PST
Uplift the test assuming it applies, yes.
Comment 24 Georg Fritzsche [:gfritzsche] 2013-02-25 08:39:36 PST
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] 2013-02-25 10:44:20 PST
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 27 Kevin Brosnan [:kbrosnan] 2013-03-29 10:50:07 PDT
Worth relnoting this? I think this made Flash a bit more stable on Android.
Comment 28 Alex Keybl [:akeybl] 2013-03-29 13:48:15 PDT
(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).

Note You need to log in before you can comment on or make changes to this bug.