Closed Bug 1221852 Opened 4 years ago Closed 4 years ago

Crash [@ mozilla::dom::SharedWorkerBinding::get_port ] after closing a SharedWorker

Categories

(Core :: DOM: Workers, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- affected
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- fixed
firefox-esr38 --- affected

People

(Reporter: jruderman, Assigned: baku)

References

(Blocks 1 open bug)

Details

(4 keywords)

Crash Data

Attachments

(3 files, 3 obsolete files)

Debug build: Assertion failure: value, at ../../dist/include/mozilla/dom/BindingUtils.h:966

Nightly: bp-2b9136ce-8bd1-4599-805e-4b07b2151105
Attached file stack
Assignee: nobody → amarchesini
Group: dom-core-security
Attached patch foo.patch (obsolete) — Splinter Review
Attachment #8683597 - Flags: review?(khuey)
Comment on attachment 8683597 [details] [diff] [review]
foo.patch

serviceworker.port.foo = serviceworker; shouldn't leak with this change, but please test.
Attachment #8683597 - Flags: review?(khuey) → review+
Attached patch foo.patch (obsolete) — Splinter Review
[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It's relatively easy to reproduce but I don't think it can exploited.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

Yes: we return a nullptr in a getter method where we were supposed to return a MessagePort.

Which older supported branches are affected by this flaw?

all.

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

Easy to be backported.

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

I cannot imagine any regression with this patch.
Attachment #8683698 - Flags: sec-approval?
Andrea: If we were returning a null and crashing sometimes, with this patch we'll be handing a pointer to a closed MessagePort back to whoever was crashing. Is that safe?
Flags: needinfo?(amarchesini)
(In reply to Daniel Veditz [:dveditz] from comment #5)
> Andrea: If we were returning a null and crashing sometimes, with this patch
> we'll be handing a pointer to a closed MessagePort back to whoever was
> crashing. Is that safe?

It is. You already can do: var a = new MessageChannel(); a.port1.close();
Or, a port is in a 'closed' state when you transfer it to a different 'context' via postMessage.
Flags: needinfo?(amarchesini)
Group: dom-core-security
Keywords: csectype-dos
Comment on attachment 8683698 [details] [diff] [review]
foo.patch

This doesn't need sec-approval since it isn't sec-high or sec-critical. Just check it in.
Attachment #8683698 - Flags: sec-approval?
Depends on: 1224825
Hmm, don't we want to fix this in some simple way so that this could land on branches and
Bug 1224825 would then land on trunk?
I prefer to land both patch (bug 1224825) in other branches as well. Otherwise we can have intermittent failures.
Had to back this out together with bug 1224825 for M(3) failures:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4922c78419fa

http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-win32/1447588173/mozilla-inbound_win7-ix_test-mochitest-3-bm126-tests1-windows-build74.txt.gz

05:02:40  WARNING -  TEST-UNEXPECTED-FAIL | dom/security/test/csp/test_upgrade_insecure_reporting.html | application terminated with exit code 1
05:02:40     INFO -  runtests.py | Application ran for: 0:01:26.973000
05:02:40     INFO -  zombiecheck | Reading PID log: c:\users\cltbld\appdata\local\temp\tmpczcznlpidlog
05:02:40     INFO -  mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/9IEpt2YhSKiHy2Y4angz2w/artifacts/public/build/firefox-45.0a1.en-US.win32.crashreporter-symbols.zip
05:02:51     INFO -  mozcrash Saved minidump as C:\slave\test\build\blobber_upload_dir\4a5d6160-41b7-4840-9ee8-2492525fefb6.dmp
05:02:51     INFO -  mozcrash Saved app info as C:\slave\test\build\blobber_upload_dir\4a5d6160-41b7-4840-9ee8-2492525fefb6.extra
05:02:51  WARNING -  PROCESS-CRASH | dom/security/test/csp/test_upgrade_insecure_reporting.html | application crashed [@ mozilla::`anonymous namespace'::RunWatchdog(void *)]
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
And out again in https://hg.mozilla.org/integration/mozilla-inbound/rev/a63e2f7359f3, RunWatchdog crashes in desktop mochitest-3 running dom/security/test/csp/test_upgrade_insecure_reporting.html, and if my bet is right also Android mochitest hangs.
Attached patch bar.patch (obsolete) — Splinter Review
Approval Request Comment
[Feature/regressing bug #]: SharedWorker
[User impact if declined]: a crash
[Describe test coverage new/current, TreeHerder]: treeherder
[Risks and why]: none
[String/UUID change made/needed]: none

For beta we don't want to land the test because it introduces a intermittent failure already fixed by bug 1224825. But we don't want to land the patches for bug 1224825 in beta.
Attachment #8683597 - Attachment is obsolete: true
Attachment #8683698 - Attachment is obsolete: true
Attachment #8688699 - Flags: approval-mozilla-beta?
Attachment #8688699 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d9251b9fb03e
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
I'll wait a day or two, to see if this crash signature is gone in Nightly, before taking it in Aurora.
I don't see the crash signature at all in crash-stats so I'm not sure waiting is going to help us determine anything. But we could verify from the test case. I'll go do that now.
Can the test be added as a crashtest? Perhaps with the timeout replaced with something less timery?
Looks good now on a current build from m-c!
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #21)
> I don't see the crash signature at all in crash-stats so I'm not sure
> waiting is going to help us determine anything. But we could verify from the
> test case. I'll go do that now.

Liz, if you look at the first signature and look for data from 28 days, you will see one instance from 11-05 45.0a2 build.
Comment on attachment 8688699 [details] [diff] [review]
bar.patch

Fix looks simple and safe enough (been on Nightly for 2 days) to uplift to Aurora44.
Attachment #8688699 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Andrea, if you want to leave the tests out do you need to make a new patch for uplift to beta?
Flags: needinfo?(amarchesini)
Flags: needinfo?(amarchesini)
Comment on attachment 8689775 [details] [diff] [review]
patch without tests

[Triage Comment]

Crash fix. Patch rebased for beta. OK to uplift.
Attachment #8689775 - Flags: approval-mozilla-beta+
Comment on attachment 8688699 [details] [diff] [review]
bar.patch

Making the old beta patch obsolete for clarity.
Attachment #8688699 - Attachment is obsolete: true
Attachment #8688699 - Flags: approval-mozilla-beta?
You need to log in before you can comment on or make changes to this bug.