Invalid write [@ mozilla::MediaStream::Destroy] with mozCaptureStream, onloadedmetadata

RESOLVED FIXED in Firefox 17

Status

()

Core
Audio/Video
--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Jesse Ruderman, Assigned: roc)

Tracking

(Blocks: 1 bug, 5 keywords)

Trunk
mozilla19
x86_64
Mac OS X
crash, csectype-uaf, regression, sec-critical, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox16- unaffected, firefox17+ fixed, firefox18+ fixed, firefox19 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [asan][adv-track-main17-])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
Created attachment 660677 [details]
testcase

1. Build Firefox with Address Sanitizer.
https://developer.mozilla.org/en-US/docs/Building_Firefox_with_Address_Sanitizer

2. Load the testcase.
3. Quit Firefox.

Result: During a shutdown CC, ASan reports an invalid write to a freed heap object.
(Reporter)

Comment 1

6 years ago
Created attachment 660681 [details]
stack trace
Is this in old media code, or something that might have been tweaked by recent WebRTC changes?
status-firefox-esr10: --- → ?
status-firefox18: --- → affected
tracking-firefox18: --- → +
Keywords: csec-uaf
Very likely related to the MediaStream changes (Bug 664918 and the follow-on)
Assigning to Randell, please reassign as necessary.
Assignee: nobody → rjesup
status-firefox-esr10: ? → unaffected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox16: --- → -
tracking-firefox17: --- → +

Comment 5

6 years ago
FYI, the only crash in the wild during the last 4 weeks that contains mozilla::MediaStream::Destroy that I could find is bp-8378d53a-7474-4b6b-9562-7c08a2120928
Yeah, that MediaStream::Destroy is something else (Beta channel, 2 weeks ago, not during CC)
This was back on Alder with a much older overall codeset and older MediaStream impl.  The crash line appears to be: "  GraphImpl()->AppendMessage(new Message(this));"  Probably GraphImpl had gone away already.  This would have been rev 104778 of MediaStreamGraph.cpp (Aug 29th).

Any thoughts roc?

Updated

6 years ago
Flags: needinfo?(roc)
Created attachment 674444 [details] [diff] [review]
fix

During a forced shutdown, after the MediaStreamGraph thread has been stopped, AppendMessage will synchronously process messages (via RunDuringShutdown()). In MediaStream::Destroy, the AppendMessage actually destroys the MediaStream. So we need to set mMainThreadDestroyed before AppendMessage.
Assignee: rjesup → roc
Attachment #674444 - Flags: review?(rjesup)
Flags: needinfo?(roc)
Comment on attachment 674444 [details] [diff] [review]
fix

Review of attachment 674444 [details] [diff] [review]:
-----------------------------------------------------------------

That works.  Can't the function have a nsRefPtr<> to itself to keep 'this' from going away until the function exits?
Attachment #674444 - Flags: review?(rjesup) → review+
(In reply to Randell Jesup [:jesup] from comment #9)
> That works.  Can't the function have a nsRefPtr<> to itself to keep 'this'
> from going away until the function exits?

I suppose so, but I hope the comment suffices.
Comment on attachment 674444 [details] [diff] [review]
fix

[Security approval request comment]
How easily can the security issue be deduced from the patch?
Very easily.

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

Which older supported branches are affected by this flaw?
Just trunk, Aurora and Beta.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
This patch should apply easily to all affected branches.

How likely is this patch to cause regressions; how much testing does it need?
Very unlikely to cause regressions; it's a very small change.
Attachment #674444 - Flags: sec-approval?
Comment on attachment 674444 [details] [diff] [review]
fix

Let's get this in now. Please prepare patches for Aurora and Beta and nominate for those releases. We should fix this across the board.
Attachment #674444 - Flags: sec-approval? → sec-approval+
Comment on attachment 674444 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 779721
User impact if declined: Potential security issue
Testing completed (on m-c, etc.): none yet
Risk to taking this patch (and alternatives if risky): Simple patch, very low risk, no alternative
String or UUID changes made by this patch: none
Attachment #674444 - Flags: approval-mozilla-beta?
Attachment #674444 - Flags: approval-mozilla-aurora?
The problem here is just that by setting mMainThreadDestroyed before calling AppendMessage, we're triggering the assertion in AppendMessage that mMainThreadDestroyed is not set.
Created attachment 675365 [details] [diff] [review]
fix v2

You were right all along, Randell :-)
Attachment #674444 - Attachment is obsolete: true
Attachment #674444 - Flags: approval-mozilla-beta?
Attachment #674444 - Flags: approval-mozilla-aurora?
Attachment #675365 - Flags: review?(rjesup)

Updated

6 years ago
Attachment #675365 - Flags: review?(rjesup) → review+
https://hg.mozilla.org/mozilla-central/rev/d21b8f2c4cb6
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox19: --- → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment on attachment 675365 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 779721
User impact if declined: Potential security issue
Testing completed (on m-c, etc.): a couple of days on m-c
Risk to taking this patch (and alternatives if risky): Simple patch, very low risk, no alternative
String or UUID changes made by this patch: none
Comment on attachment 675365 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 779721
User impact if declined: Potential security issue
Testing completed (on m-c, etc.): a couple of days on m-c
Risk to taking this patch (and alternatives if risky): Simple patch, very low risk, no alternative
String or UUID changes made by this patch: none
Attachment #675365 - Flags: approval-mozilla-beta?
Attachment #675365 - Flags: approval-mozilla-aurora?
Comment on attachment 675365 [details] [diff] [review]
fix v2

sec-critical, been on trunk a few days, approving for uplift to branches.
Attachment #675365 - Flags: approval-mozilla-beta?
Attachment #675365 - Flags: approval-mozilla-beta+
Attachment #675365 - Flags: approval-mozilla-aurora?
Attachment #675365 - Flags: approval-mozilla-aurora+
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)

> Bug caused by (feature/regressing bug #): 779721

If this is caused by 779721, which came in during August, why is Firefox 16 affected? I only see a Mozilla Central checkin (series) on August 9, 2012.
I don't know why dveditz marked this as affecting FF16. I don't think it is.
I marked it based on your guess in comment 3 that it was related to bug 664918 (landed in Fx 15) and it wasn't updated based on the information in comment 11.
Blocks: 779721
No longer blocks: 664918
Keywords: regression
status-firefox16: affected → unaffected
Whiteboard: [asan] → [asan][adv-track-main17-]
Keywords: verifyme
Group: core-security
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.