Bugzilla

Comment 2

•

6 years ago

INFO: Last good revision: a1869efdd7aa8162dd68eb47288dffc528ce7980
INFO: First bad revision: ca5da952ff4c690bd34336b8a8aafec06b01e0e8
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=a1869efdd7aa8162dd68eb47288dffc528ce7980&tochange=ca5da952ff4c690bd34336b8a8aafec06b01e0e8

Not sure if this bug should be moved into DOM or not...

Blocks: 1451913

Group: core-security → media-core-security

Has Regression Range: --- → yes

status-firefox63: --- → wontfix

status-firefox64: --- → affected

status-firefox65: --- → affected

status-firefox-esr60: --- → unaffected

Flags: in-testsuite?

Keywords: crash, regression

Version: 65 Branch → 61 Branch

Comment 3

•

6 years ago

Blake: The testcase doesn't mention service workers, but the change in the regression window was about that. Also crashing on a compartment-mismatch check (that doesn't run in release builds). Can you help us understand what's going on here?

Flags: needinfo?(mrbkap)

Keywords: sec-high

Assignee

Updated

•

6 years ago

Assignee: nobody → mrbkap

Flags: needinfo?(mrbkap)

Assignee

Comment 4

•

6 years ago

This is subtle and related to patch 3 from bug 1451913. Before that patch, any objects created from an AudioContext (in this testcase, the AudioBuffer) would be created in the compartment of the global the AudioContext was constructed in (i.e. the original inner window, before document.write()). After the patch, we create those subobjects in the compartment of the *new* inner window. That's fine, as we'll happily wrap those objects in CCWs when they're returned, but there's a problem with the line `o420.buffer=o389;` Because we're still running in a function created before the document.write() (in the *old* inner window's compartment), we have a CCW around o389 that the binding layer unwraps to get at the underlying AudioBuffer. We then call into the code that deals with its JS buffers, assuming that we're in its global. Unfortunately, we're not and we assert. I suspect that we might have hit this assertion if we had chrome code manipulating content AudioContext and AudioBuffers.

After reading the code, it looks like all of the other uses of mJSChannels has the AudioBuffer as the `this` object, so we'll have to be in its own compartment, so it should be sufficient to patch `AudioBuffer::StealJSArrayDataIntoSharedChannels` to enter the correct compartment (or, realm, really).

Assignee

Comment 5

•

6 years ago

Attached file Bug 1509442 - Enter a realm here. r=qDot — Details

Karl Tomlinson (:karlt)

Updated

•

6 years ago

Component: Audio/Video → Web Audio

Julien Cristau [:jcristau] (back April 22)

Assignee

Comment 6

•

6 years ago

Comment on attachment 9028804 [details]
Bug 1509442 - Enter a realm here. r=qDot

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: This is a compartment mismatch but I think it's difficult to see how it's possible to cause it just by reading this patch (there aren't any pointers to document.open). I think it would be difficult to write an exploit because of that fact.

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?: 61

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

Do you have backports for the affected branches?: No

If not, how different, hard to create, and risky will they be?: I would expect them to be easy to create and not risky.

How likely is this patch to cause regressions; how much testing does it need?: This patch should be very safe. In the common case it should be a no-op.

Attachment #9028804 - Flags: sec-approval?

Comment 7

•

6 years ago

We're past the last beta for 64.  Is there a strong reason not to leave this for 65?

Flags: needinfo?(mrbkap)

Comment 8

•

6 years ago

(In reply to Julien Cristau [:jcristau] from comment #7)
> We're past the last beta for 64.  Is there a strong reason not to leave this
> for 65?

I had the same remark. This is too late for the current cycle without it being a potential 0day or otherwise super important to get in.

Julien Cristau [:jcristau] (back April 22)

Assignee

Comment 9

•

6 years ago

We can certainly leave this for 65.

Flags: needinfo?(mrbkap)

Updated

•

5 years ago

status-firefox64: affected → wontfix

Updated

•

5 years ago

status-firefox66: --- → affected

tracking-firefox65: --- → +

tracking-firefox66: --- → +

Comment 10

•

5 years ago

Sec-approval+ for trunk. We'll want a beta patch made and nominated as well.

Updated

•

5 years ago

Attachment #9028804 - Flags: sec-approval? → sec-approval+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Assignee

Comment 11

•

5 years ago

Comment on attachment 9028804 [details]
Bug 1509442 - Enter a realm here. r=qDot

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1451913

User impact if declined: Potential crash visiting certain sites.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Please see comment 6.

String changes made/needed:

Attachment #9028804 - Flags: approval-mozilla-beta?

Comment 12

•

5 years ago

This landed yesterday: https://hg.mozilla.org/integration/autoland/rev/134d6c86e984adc8e26c6c799867e256b8f15d7b

And got backed out for browser_webAudio_silentData.js failures: https://hg.mozilla.org/integration/autoland/rev/8d24aeda8861e73c1491f5c45f4e57c2a77422d5
Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2Cx64%2Copt%2Cmochitests%2Cwith%2Ce10s%2Ctest-linux64%2Fopt-mochitest-browser-chrome-e10s-3%2Cm-e10s%28bc3%29&fromchange=41e4eb2f9e476f279defec74ef328f2b707b0b65&tochange=8d24aeda8861e73c1491f5c45f4e57c2a77422d5&selectedJob=217801334
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217801334&repo=autoland&lineNumber=7738


[task 2018-12-19T00:33:46.554Z] 00:33:46     INFO - TEST-START | toolkit/content/tests/browser/browser_webAudio_silentData.js
[task 2018-12-19T00:33:47.043Z] 00:33:47     INFO - GECKO(10903) | ###!!! [Parent][MessageChannel] Error: (msgtype=0x1E0087,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
[task 2018-12-19T00:34:31.591Z] 00:34:31     INFO - TEST-INFO | started process screentopng
[task 2018-12-19T00:34:32.406Z] 00:34:32     INFO - TEST-INFO | screentopng: exit 0
[task 2018-12-19T00:34:32.406Z] 00:34:32     INFO - Buffered messages logged at 00:33:46
[task 2018-12-19T00:34:32.406Z] 00:34:32     INFO - Entering test bound testSilentAudioContext
[task 2018-12-19T00:34:32.406Z] 00:34:32     INFO - - create new tab -
[task 2018-12-19T00:34:32.406Z] 00:34:32     INFO - - create audio context -
[task 2018-12-19T00:34:32.408Z] 00:34:32     INFO - - check AudioContext's state -
[task 2018-12-19T00:34:32.408Z] 00:34:32     INFO - Buffered messages logged at 00:33:47
[task 2018-12-19T00:34:32.409Z] 00:34:32     INFO - Console message: [JavaScript Error: "remote browser crashed while on about:blank
[task 2018-12-19T00:34:32.409Z] 00:34:32     INFO - " {file: "chrome://mochikit/content/mochitest-e10s-utils.js" line: 8}]
[task 2018-12-19T00:34:32.409Z] 00:34:32     INFO - e10s_init/<@chrome://mochikit/content/mochitest-e10s-utils.js:8:5
[task 2018-12-19T00:34:32.409Z] 00:34:32     INFO - EventListener.handleEvent*e10s_init@chrome://mochikit/content/mochitest-e10s-utils.js:6:3
[task 2018-12-19T00:34:32.411Z] 00:34:32     INFO - testInit@chrome://mochikit/content/browser-test.js:102:5
[task 2018-12-19T00:34:32.412Z] 00:34:32     INFO - setTimeout handler*@chrome://mochikit/content/browser-test.js:22:1
[task 2018-12-19T00:34:32.413Z] 00:34:32     INFO - loadChromeScripts@jar:file:///tmp/tmpzfWLG_.mozrunner/extensions/mochikit@mozilla.org.xpi!/api.js:15:3
[task 2018-12-19T00:34:32.414Z] 00:34:32     INFO - loadMochitest@jar:file:///tmp/tmpzfWLG_.mozrunner/extensions/mochikit@mozilla.org.xpi!/api.js:111:3
[task 2018-12-19T00:34:32.415Z] 00:34:32     INFO - @tests/mochitest/runtests.py:1735:1
[task 2018-12-19T00:34:32.416Z] 00:34:32     INFO - @tests/mochitest/runtests.py:1714:2
[task 2018-12-19T00:34:32.417Z] 00:34:32     INFO - evaluate.sandbox/promise<@chrome://marionette/content/evaluate.js:128:13
[task 2018-12-19T00:34:32.418Z] 00:34:32     INFO - evaluate.sandbox@chrome://marionette/content/evaluate.js:97:17
[task 2018-12-19T00:34:32.419Z] 00:34:32     INFO - GeckoDriver.prototype.execute_@chrome://marionette/content/driver.js:1029:19
[task 2018-12-19T00:34:32.423Z] 00:34:32     INFO - async*GeckoDriver.prototype.executeScript@chrome://marionette/content/driver.js:899:24
[task 2018-12-19T00:34:32.423Z] 00:34:32     INFO - Async*despatch@chrome://marionette/content/server.js:290:20
[task 2018-12-19T00:34:32.424Z] 00:34:32     INFO - async*execute@chrome://marionette/content/server.js:263:11
[task 2018-12-19T00:34:32.428Z] 00:34:32     INFO - async*onPacket/<@chrome://marionette/content/server.js:236:15
[task 2018-12-19T00:34:32.429Z] 00:34:32     INFO - async*onPacket@chrome://marionette/content/server.js:235:8
[task 2018-12-19T00:34:32.430Z] 00:34:32     INFO - _onJSONObjectReady/<@chrome://marionette/content/transport.js:493:9
[task 2018-12-19T00:34:32.436Z] 00:34:32     INFO - 
[task 2018-12-19T00:34:32.437Z] 00:34:32     INFO - Buffered messages finished
[task 2018-12-19T00:34:32.440Z] 00:34:32     INFO - TEST-UNEXPECTED-FAIL | toolkit/content/tests/browser/browser_webAudio_silentData.js | Test timed out -

Flags: needinfo?(mrbkap)

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Assignee

Updated

•

5 years ago

Flags: needinfo?(mrbkap)

Attachment #9028804 - Flags: approval-mozilla-beta?

Comment 13

•

5 years ago

https://hg.mozilla.org/integration/autoland/rev/cdadd4fd29737444e41f5d6f3617e9d415f611b1
https://hg.mozilla.org/mozilla-central/rev/cdadd4fd2973

Group: media-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 5 years ago

status-firefox66: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla66

Comment 14

•

5 years ago

Please nominate this for Beta approval when you get a chance.

Flags: needinfo?(mrbkap)

Assignee

Comment 15

•

5 years ago

Comment on attachment 9028804 [details]
Bug 1509442 - Enter a realm here. r=qDot


[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1451913

User impact if declined: Potential crash visiting certain sites.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Please see comment 6.

String changes made/needed:

Flags: needinfo?(mrbkap)

Attachment #9028804 - Flags: approval-mozilla-beta?

Comment 16

•

5 years ago

Comment on attachment 9028804 [details]
Bug 1509442 - Enter a realm here. r=qDot

[Triage Comment]
Fixes a sec-high. Approved for 65.0b7.

Attachment #9028804 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 17

•

5 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/4a5ddf7d9e70

status-firefox65: affected → fixed

Bogdan Maris, Desktop QA

Updated

•

5 years ago

Flags: qe-verify+

Bogdan Maris, Desktop QA

Updated

•

5 years ago

Whiteboard: [post-critsmash-triage]

Stefan [:StefanG_QA]

Comment 18

•

5 years ago

Does this need manual qa? I've tried to execute the available test case on 65.0b7, however when I start the ASAN build my machine freeze and I need to hard reset it.

Assignee

Comment 19

•

5 years ago

(In reply to Stefan [:StefanG_QA] from comment #18)
> Does this need manual qa? I've tried to execute the available test case on
> 65.0b7, however when I start the ASAN build my machine freeze and I need to
> hard reset it.

I don't know why your ASAN build wouldn't work. We should definitely add a crashtest for this after we make this bug public, there's no reason we can't have an automated test for this.

Assignee

Comment 20

•

5 years ago

Attached file Bug 1509442 - Add a crashtest. r=qDot — Details

Updated

•

5 years ago

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main65+]

Updated

•

5 years ago

Alias: CVE-2018-18503

Stefan [:StefanG_QA]

Comment 21

•

5 years ago

I was able to test this on the below builds using the provided crash.html file in comment0. When loading the mentioned file, Firefox does not crash.

Mozilla/5.0 (x11; Linux x84_64; rv:66.0) Gecko/20100101 Firefox /66.0 (20181221093752)
Mozilla/5.0 (x11; Linux x84_64; rv:65.0) Gecko/20100101 Firefox /65.0 (20190103150357)

Status: RESOLVED → VERIFIED

status-firefox65: fixed → verified

status-firefox66: fixed → verified

Flags: qe-verify+

Updated

•

5 years ago

Flags: sec-bounty?

Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)

Comment 22

•

5 years ago

qDot: the original crash here appears to be triggering an intentional crash assert. Is that a runtime assert as well? If the intentional crash is going to protect us I'm unsure how this gets a sec-high rating. Is there some other way around it?

Flags: needinfo?(kyle)

Comment 23

•

5 years ago

I'm not aware of another way around it offhand.

Flags: needinfo?(kyle)

Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)

Comment 24

•

5 years ago

lowering to sec-moderate. We think the runtime assert protects us so sec-high is not appropriate, but it's still concerning enough that there was a compartment mismatch in the first place that might have bitten us in other ways that we still want to award a bug bounty.

Flags: sec-bounty? → sec-bounty+

Keywords: sec-high → sec-moderate

Comment 25

•

5 years ago

Just noticed that the crashtest for this was sitting in my phab dashboard as "needs to land". Blake left before this got landed, and it passes try:

https://treeherder.mozilla.org/#/jobs?repo=try&author=kmachulis%40mozilla.com&selectedJob=236731500

I'll go ahead and approve the Lando stack now.

Julien Cristau [:jcristau] (back April 22)

Comment 26

•

5 years ago

https://hg.mozilla.org/mozilla-central/rev/0ced14667fc5

Flags: in-testsuite? → in-testsuite+