Closed Bug 1154876 Opened 5 years ago Closed 5 years ago

Flash calls PPluginModuleChild::SendSetCursor() off the main thread

Categories

(Core :: Plug-ins, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
mozilla41
Tracking Status
firefox37 --- wontfix
firefox38 --- wontfix
firefox39 + verified
firefox38.0.5 --- wontfix
firefox40 + verified
firefox41 --- verified
firefox-esr31 39+ verified
firefox-esr38 39+ verified
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-master --- unaffected

People

(Reporter: mccr8, Assigned: smichaud)

References

Details

(Keywords: csectype-race, sec-high, Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+])

Crash Data

Attachments

(2 files)

I came across this signature on crash-stats:
  https://crash-stats.mozilla.com/report/index/67b8450e-48d8-4b67-9fcc-dabd02150414#allthreads

It looks like the Flash player is trying to send a message, but it hits an IPC thread assertion that Bill added recently.
Flags: needinfo?(aklotz)
Another similar signature, also inside PPluginModuleChild::SendSetCursor():
https://crash-stats.mozilla.com/report/index/c2c850e1-96e4-4b78-b02a-265ce2150414
Another (sorry for the bug spam):
https://crash-stats.mozilla.com/report/index/c6bfe48f-67bd-4a9e-afd3-1c88b2150414
Summary: Flash tries to send a message off the main thread → Flash calls PPluginModuleChild::SendSetCursor() off the main thread
I'm not all that familiar with the OSX interpose stuff. Clearly sending the message off the main thread is bad, but it's not clear to me if the interpose stuff should be proxying that to the main thread, or what. I'd be curious what smichaud has to say.
Flags: needinfo?(aklotz) → needinfo?(smichaud)
> but it's not clear to me if the interpose stuff should be proxying that to the main thread

I don't know, either.  We *could*, if it's really necessary.  But the added complexity might cause trouble down the line.  We could also drop any hooked/interposed call that comes in on anything but the main thread, without crashing -- which would be a lot better than our current behavior.

As far as I know, none of the interposed calls is supported on anything but the main thread.  So it's technically a bug to call them (as Flash does with -[NSCursor set]) on secondary threads.  So we'd be well within our rights to drop them.  But I don't know how much Flash depends on these calls working from secondary threads, and whether or not this is an issue for other plugins.

One way to find out would be to start dropping them and see what happens :-)

Jeromie, what do you think?
Flags: needinfo?(smichaud) → needinfo?(jeclark)
We have an ongoing engineering engagement happening between the Mozilla and Adobe engineering teams.  Working together to devise an optimal solution for this seems like an ideal agenda item for the next time we get together.
Flags: needinfo?(jeclark)
As an aside, if you need contact information for the people on your side directly involved in that effort, I'd be more than happy to put you in touch.
(In reply to comment #5)

Sounds good to me.  Thanks!

I really hope Adobe can adopt something like the DOM Cursors spec (https://wiki.mozilla.org/NPAPI:DOMCursors).  It would work around this problem, and also allow us to stop proxying cursor management methods from a background process to the main process.

It was suggested a long time ago (for example it's mentioned at bug 621117 comment #57).  Apparently it's gone nowhere, though -- which I think is really too bad.
(In reply to comment #6)

I don't know who's involved in these discussions on the Mozilla side.  In another context you mentioned Johnny Stenback's name (jst@mozilla.com) as one of the people who might be involved.  If he is, I'm sure he'd appreciate the information.

I'm not involved in these discussions ... at least not yet.  I'm not particularly eager to do so.  But I'm happy to give advice, if it's needed.
Yep, let's try and get this on the list for the next time Johnny's guys come out.  It far more effective to just have engineers look at the code together and hash out the most optimal solution.
Aklotz:  In light of the future discussion with Adobe, it'd be very good if you (or someone) could explain here exactly why we don't want plugin-related IPC calls (like the one involved here) happening off the main thread.
Flags: needinfo?(aklotz)
I think that applies to all IPC, not just plugin-related.  Billm would probably be a better person to ask.
Flags: needinfo?(wmccloskey)
I looked into this particular instance of NSCursor usage, and it appears that we set the arrow cursor before we show the file browse dialog (which we display from a background thread) to fix an issue with the cursor becoming invisible when the dialog was shown. 

Our bug notes mention that this was an issue in our standalone player (not the plugin), so a potential fix could be simply disabling that set cursor call in the plugin. Would need QE to verify that removing the call doesn't introduce the disappearing cursor issue (As I said, our bug notes only mention our standalone player having the issue, but it could have been an issue in the plugin as well)

Of course, we could pursue the DOM Cursors work in parallel, but just wanted to mention that there is a potentially quicker fix if this is a troublesome crash that needs to be addressed sooner.
Well, our IPC code isn't threadsafe and we aren't going to change that. I think the alternatives are pretty simple, as Steven laid out: send the message on the main thread or drop it. Sending on the main thread seems like it would be fine, but dropping the message would be cleaner.
Flags: needinfo?(wmccloskey)
Flags: needinfo?(aklotz)
(In reply to comment #12)

> it appears that we set the arrow cursor before we show the file
> browse dialog

Could you post a Flash "movie" that demonstrates this behavior,
along with instructions on how to use it?  In other words, could
you post an example of a Flash movie that, when you click on a
button somewhere in it, it causes a filepicker to be displayed?

I'd like to test with it.
Flags: needinfo?(mtalistu)
.zip file with simple .html/.swf test case and .fla project.
Clicking "Browse..." button opens a FileBrowse dialog for local file upload, which triggers a call to two NSCursor methods from a background thread.
Flags: needinfo?(mtalistu)
Blocks: 1155375
Here's a possible fix for this bug.  It's very simple -- it just drops all calls to the hooked/interposed methods that come in on secondary threads.

Mtalistu, I tested this patch with your testcase, and didn't see any problems.  So it's possible we can drop these calls without having any ill effect on Flash.

I've started a set of tryserver builds:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b73a8618ec14

A test build should eventually be available.  When it is, I'll post a link to it.
Here's a link to my test build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-b73a8618ec14/try-macosx64/firefox-40.0a1.en-US.mac.dmg

Please try it, mtalistu, and let us know your results.
Flags: needinfo?(mtalistu)
Thanks, I will ask QE to do some tests with this build. Will let you know the results.
For what it's worth: the imgur.com uploader triggered this and it seems to behave fine with the patch here.
This seems potentially bad, and is fairly easy to trigger, so I'm going to be conservative and mark it sec-high.
Ok, so the simple test case I provided earlier was related to an issue in the Standalone player, but we identified another issue that was fixed by the calls to NSCursor API that is broken in the test build of Firefox.

Here are the repro steps. The mouse is visible when you switch tabs in Release Firefox, but it stays hidden in the test build of Firefox. I had to click on another app, like a Finder window, to get the mouse to become visible again.

Method:
1. Open a Firefox window with multiple tabs opened.
2. Go to http://cbtcafe.com/flash/mousecursor/mousecursor.html
3. Mouse over the flash movie(sample swf for 'Flash: Mouse.Hide meets With'),  The mouse pointer should disappear.
4. Switch tabs using command-option-arrow
5. Move the mouse

Result:
The mouse cursor disappears. Have to move the mouse out of this tab or browser to get the cursor back

Expected: 
mouse arrow cursor should be visible.
Flags: needinfo?(mtalistu)
(In reply to comment #21)

I see this bug even without my patch (for example in the 2015-04-22 and 2015-04-17 mozilla-central nightlies).  So I suspect it's unrelated.

I'll look for a regression range.  If/when I find one, I'll open a new bug, paste in your STR, and CC you on it.
Actually it's my patch for bug 1130435 that triggered the behavior you describe in comment #21 ... which means that it *is* related to this bug, indirectly.  I'm very puzzled how that patch could have caused/triggered *any* bug.  So I need to get to the bottom of this, over the next few days (and probably into next week).  Once I do, I'll (probably) open another bug, which I may or may not mark security-related, depending on what I find.
(Following up comment #23)

I was right the first time:  The bug you (mtalistu) report in comment #21 is completely unrelated.  It's still our bug, though -- which I think is caused by us not sending an NPCocoaEventFocusChanged event to Flash in step 4 when running in e10s mode.  Notice that the bug doesn't happen in non-e10s mode.  When the bug doesn't happen in step 4 (and Flash does receive an NPCocoaEventFocusChanged event), Flash calls +[NSCursor unhide] *on the main thread* (so my patch from comment #16 wouldn't drop the call).

I'm working on a related bug at bug 1153145.  I'll make sure that whatever fix I come up with there also fixes this bug.

The true trigger for this bug (and probably also for bug 1153145) is bug 1092630.

My patch for bug 1130435 isn't the trigger.  Before it landed, *no* native cursor calls from Flash (or any plugin) worked in e10s mode -- including the call to +[NSCursor hide] that happens when (in step 3) you move the mouse over the newCursor block.  So before that patch landed you'd see *both* the custom cursor and the default native cursor (the northwest-pointing arrow) just above it.  And it didn't matter when the call to +[NSCursor unhide] didn't happen in step 4, and the custom cursor disappeared -- because the default native cursor was already visible.
So as best I can now tell, my patch from comment #21 will have no ill effects on Flash.

Any objections to my seeking review for that patch and landing it on the trunk?
Flags: needinfo?(mtalistu)
Duplicate of this bug: 1159800
We may have STR for this bug at bug 1159800.
I should have said we may have STR for this bug's crashes at bug 1159800.
Any chance we can move forward with this?
I'm ready to land my patch from comment #16 ... though it may have bitrotted.
Attachment #8593605 - Flags: review?(spohl.mozilla.bugs)
Attachment #8593605 - Flags: review?(spohl.mozilla.bugs) → review+
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

I will land this when mozilla-inbound reopens.
Thanks!  It would also be good to get this at least on Aurora at some point, as the thread safety assertions have made it that far along.
> It would also be good to get this at least on Aurora at some point

I'll let the patch bake a while on trunk, then request permission to uplift.

Note that we could also request uplift to beta, since the problem also exists there.
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3950e0f52532

It's frankly hard to land a patch like this without giving the game away.  I tried to disguise it a bit by giving it an obscure commit message, instead of just a bug number (which would have drawn peoples' attention).
https://hg.mozilla.org/mozilla-central/rev/3950e0f52532
Flags: in-testsuite?
Target Milestone: --- → mozilla41
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
This should have had sec-approval before landing. Please request it retroactively. Also, does this affect esr31? Please nominate this for Aurora/Beta/esr38 (and esr31 if applicable) ASAP.
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

Sorry for overlooking this :-(

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Fairly easily, I think, but only from a malicious plugin (which could, of course, do all other kinds of nasty things).

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?

This is really a Flash bug.  So all branches are effected that support OOP plugins.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

The same patch should apply to all current branches -- the code they patch is quite old and hasn't changed much recently.

How likely is this patch to cause regressions; how much testing does it need?

It's very unlikely to cause regressions in Flash -- Adobe has already had a chance to test with a test build that contained the patch.  But if other plugins (for example Silverlight) have the same bug, this patch may also cause (apparent) regressions in them.
Flags: needinfo?(smichaud)
Attachment #8593605 - Flags: sec-approval?
> Also, does this affect esr31? Please nominate this for
> Aurora/Beta/esr38 (and esr31 if applicable) ASAP.

It effects all branches -- since it's really a Flash bug.

Since other plugins may also have the same bug, I'd like to let this
bake on the trunk for a while (say a week) before requesting uplift.

The most important other plugin is Silverlight.  So I'll do some
simple tests with Silverlight to see if it also has this bug.
OK, I've verified locally that this rebases all the way to esr31 w/o issue. Will leave it to you and Al to determine proper bake time.
> How easily could an exploit be constructed based on the patch?
>
> Fairly easily, I think, but only from a malicious plugin (which
> could, of course, do all other kinds of nasty things).

Let me revise this.

I think it'd be fairly easy to crash the main process.  But I really
don't know how easy it'd be do an exploit.  Others who know more about
this than I do may be able to comment.
I'll give sec-approval+ on this after the fact (thanks for filling out the form). I agree with letting it bake a week on trunk.

We'll want this on Aurora, Beta, ESR38, and (hopefully) ESR31 after that.
Attachment #8593605 - Flags: sec-approval? → sec-approval+
I tested briefly with both the Silverlight and Unity plugins, with a debug logging version of my patch (which logs to console and stdout whenever it drops a hooked event that comes in on a secondary thread), in both e10s and non-e10s mode.  I didn't see any drops at all.

For Silverlight I played around with http://demos.telerik.com/silverlight/.  QA has a Netflix test account, to which I have access -- but it apparently no longer works, so I was unable to test with it.

For Unity I played around with some of the demos at https://unity3d.com/showcase/live-demos.
Steven can you request uplift for this if you think it's ready to go? Thanks!
Flags: needinfo?(smichaud)
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Potential trouble from malicious plugins
Fix Landed on Version: 41
Risk to taking this patch (and alternatives if risky): Low to moderate risk. It's impossible to know how many other plugins than Flash have this bug (the bug this patch works around) without actually getting this patch into a release. But the risk for the most important plugins (Flash, Silverlight, Unity) is low.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: Plugin bug
[User impact if declined]: Potential trouble from malicious plugins
[Describe test coverage new/current, TreeHerder]: baked for a week on m-c
[Risks and why]: Low to moderate. See above.
[String/UUID change made/needed]: none
Flags: needinfo?(smichaud)
Attachment #8593605 - Flags: approval-mozilla-esr38?
Attachment #8593605 - Flags: approval-mozilla-beta?
Attachment #8593605 - Flags: approval-mozilla-aurora?
Flags: needinfo?(mtalistu)
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

Approving to uplift to Aurora, Beta and ESR38+ as this is a sec-high bug and the fix has been in m-c for a week without showing any negative impact.
Attachment #8593605 - Flags: approval-mozilla-esr38?
Attachment #8593605 - Flags: approval-mozilla-esr38+
Attachment #8593605 - Flags: approval-mozilla-beta?
Attachment #8593605 - Flags: approval-mozilla-beta+
Attachment #8593605 - Flags: approval-mozilla-aurora?
Attachment #8593605 - Flags: approval-mozilla-aurora+
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

Landed on mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/583c3552c9e4
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

Landed on mozilla-beta:
https://hg.mozilla.org/releases/mozilla-beta/rev/c9cc5b1e1b0f
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

Landed on mozilla-esr38:
https://hg.mozilla.org/releases/mozilla-esr38/rev/2fa7846306f2
> We'll want this on Aurora, Beta, ESR38, and (hopefully) ESR31 after that.

How about ESR31?  Should I request uplift for that now, or should I wait?
Flags: needinfo?(abillings)
Yes, please uplift to ESR31.
Flags: needinfo?(abillings)
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Potential trouble from malicious plugins
Fix Landed on Version: 41
Risk to taking this patch (and alternatives if risky): Low to moderate risk. It's impossible to know how many other plugins than Flash have this bug (the bug this patch works around) without actually getting this patch into a release. But the risk for the most important plugins (Flash, Silverlight, Unity) is low.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8593605 - Flags: approval-mozilla-esr31?
Flags: qe-verify+
Attachment #8593605 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Comment on attachment 8593605 [details] [diff] [review]
Possible fix -- drop calls from secondary threads

Landed on mozilla-esr31:
https://hg.mozilla.org/releases/mozilla-esr31/rev/772f5b18522c
I`m unable to reproduce the initial crash using old Nightly (2015-04-15) on Windows 7 64-bit, Windows XP 32-bit, Ubuntu 14.04 32-bit.
Socorro shows many crashes with this signature (73 crashes in the last 7 days), most of them on Windows XP and latest builds:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Aipc%3A%3AMessageChannel%3A%3ACxxStackFrame%3A%3ACxxStackFrame%28mozilla%3A%3Aipc%3A%3AMessageChannel%26%2C+mozilla%3A%3Aipc%3A%3ADirection%2C+IPC%3A%3AMessage+const%2A%29#tab-sigsummary

Any thoughts on this?
Flags: needinfo?(smichaud)
> Any thoughts on this?

Those crash stacks are completely unrelated to this bug.  It's yet another case of the so-called "signature" being misleading.

Even if they *weren't* unrelated, though, they'd need to go into another bug:  The specific problem that this bug reports (and which my patch fixes) is Mac-specific.
Flags: needinfo?(smichaud)
(In reply to Steven Michaud [:smichaud] from comment #54)
> The specific problem that this bug reports (and which my patch fixes)
> is Mac-specific.

I retested using old Nightly (2015-04-15) on Mac OS X 10.9.5 and I was able to get a crash:
https://crash-stats.mozilla.com/report/index/f1868dc0-6a52-4027-b8b1-ebd0d2150617

Verified that using the attached testcase and latest builds the crash did not occur:
- Firefox 31.7 esr:
Built from https://hg.mozilla.org/releases/mozilla-esr31/rev/4028bc28f4c6
20150615064244
- Firefox 38.0.1 esr: 
Built from https://hg.mozilla.org/releases/mozilla-esr38/rev/bbac190ad1e7
20150616140147
- latest Nightly 41.0a1
- latest Aurora 40.0a2
- Firefox 39 beta 6
(Following up comment #24)

Mtalistu, the bug you report in comment #21 is fixed by my patch for bug 1153145.  That patch is now in current mozilla-central and mozilla-aurora (i.e. Developer Edition) nightlies, in case you care to test with them.
Whiteboard: [adv-main39+][adv-esr38.1+][adv-esr31.8+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.