Closed Bug 842687 Opened 7 years ago Closed 7 years ago

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

Categories

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

ARM
Android
defect

Tracking

()

RESOLVED FIXED
mozilla22
Tracking Status
firefox19 + affected
firefox20 + fixed
firefox21 + fixed
firefox22 + fixed
firefox-esr17 - unaffected
b2g18 --- unaffected
b2g18-v1.0.0 --- affected
b2g18-v1.0.1 --- affected
relnote-firefox --- -

People

(Reporter: jchen, Assigned: jchen)

References

Details

(Keywords: sec-critical, Whiteboard: [Android only][adv-main20+])

Attachments

(2 files)

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.
We'll that's... bad
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.
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?
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.
Keywords: sec-critical
Priority: -- → P1
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.
(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.
Whiteboard: [Android only]
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.
(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.
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).
The only reason to put this up the trains is because it is a topcrash. Actually constructing an exploit for this is probably impossible.
(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.
This patch changes bool to uint32_t, which matches the OOPP codepath
Attachment #716111 - Flags: review?(benjamin)
(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?
Assignee: nobody → nchen
Status: NEW → ASSIGNED
Just putting the minor test modification up here if that's alright.
Attachment #716140 - Flags: review?(benjamin)
Attachment #716111 - Flags: review?(benjamin) → review+
Attachment #716140 - Flags: review?(benjamin) → review+
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!
Flags: in-testsuite+
Target Milestone: --- → mozilla22
With this crash affecting all the way to Fx19, what's the likelihood of uplifting?
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?
(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.
(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.
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 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
Attachment #716111 - Flags: approval-mozilla-beta?
Attachment #716111 - Flags: approval-mozilla-aurora?
Uplift the test assuming it applies, yes.
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.
Attachment #716140 - Flags: approval-mozilla-beta?
Attachment #716140 - Flags: approval-mozilla-aurora?
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
Attachment #716111 - Flags: approval-mozilla-beta?
Attachment #716111 - Flags: approval-mozilla-beta+
Attachment #716111 - Flags: approval-mozilla-aurora?
Attachment #716111 - Flags: approval-mozilla-aurora+
Attachment #716140 - Flags: approval-mozilla-beta?
Attachment #716140 - Flags: approval-mozilla-beta+
Attachment #716140 - Flags: approval-mozilla-aurora?
Attachment #716140 - Flags: approval-mozilla-aurora+
Whiteboard: [Android only] → [Android only][adv-main20+]
Worth relnoting this? I think this made Flash a bit more stable on Android.
relnote-firefox: --- → ?
(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).
Group: core-security
You need to log in before you can comment on or make changes to this bug.