Closed Bug 957452 Opened 10 years ago Closed 10 years ago

MediaRecorder: use-after-free crash [@mozilla::dom::MediaRecorder::Session::GetEncodedData]

Categories

(Core :: Audio/Video: Recording, defect)

Product:
Core
Component:
Audio/Video: Recording
Platform:
x86_64
macOS
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla30
Project Flags:
blocking-b2g 1.4+
Tracking Flags:
Tracking Status
firefox27 --- unaffected
firefox28 --- unaffected
firefox29 + fixed
firefox30 + fixed
firefox-esr24 --- unaffected
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- unaffected
b2g-v1.3T --- unaffected
b2g-v1.4 --- fixed

People

(Reporter: posidron, Assigned: rlin)

Details

(4 keywords, Whiteboard: [asan])

Attachments

(7 files, 3 obsolete files)

testcase
103.15 KB, application/x-zip-compressed
Details
callstack
6.09 KB, text/plain
Details
testcase 2
437 bytes, text/html
Details
patch v1
3.63 KB, patch
roc
: review+
abillings
: approval-mozilla-aurora+
abillings
: sec-approval+
Details | Diff | Splinter Review
test case v4
3.81 KB, patch
jsmith
: review+
Details | Diff | Splinter Review
check-in patch p1
3.64 KB, patch
Details | Diff | Splinter Review
check-in test case p2
3.82 KB, patch
Details | Diff | Splinter Review
Attached file callstack — 
Assignee: nobody → rlin

    
    

    
  
Note - make sure we get a mochitest or crashtest with this patch. We need to make sure we protect against this security bug in the future.

    
    

    
  
Is this asan only by any chance? I can't seem to reproduce this on a nightly build off of nightly.mozilla.org.

    
    

    
  
(In reply to Jason Smith [:jsmith] from comment #3)
> Is this asan only by any chance?

Correct.
Whiteboard: [asan]
Blocks: MediaRecording
blocking-b2g: --- → 1.4?
Keywords: regression

    
    

    
  

      
      
      
      

        Attached file
          
        testcase 2
        — 
      

    


  
This testcase requires:
  user_pref("media.ogg.enabled", false);

Under ASan, it crashes in the same way as the other testcase in this bug.

In non-ASan debug builds, it crashes with a not-very-helpful stack full of freed memory [@ mozilla::BlockingResourceBase::CheckAcquire].

    
    

    
  
Hit the timing issue...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch v1
        — 
          — Splinter Review
      

    


  
Directly pass blob object to CreateAndDispatchBlobEvent function.

        Attachment #8359081 -
        Flags: review?(roc)

    
    

    
  
Comment on attachment 8359081 [details] [diff] [review]
patch v1

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

Don't forget to add the test.

        Attachment #8359081 -
        Flags: review?(roc) → review+

    
    

    
  

      
      
      
      

        Attached patch
          
          
        testcase v1 (obsolete)
        — 
          — Splinter Review
      

    


  
Use testcase 2 to create mochitest to capture this crash.

        Attachment #8359702 -
        Flags: review?(jsmith)

    
    

    
  
Comment on attachment 8359702 [details] [diff] [review]
testcase v1

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

review- - the reduced test case focuses on having ogg enabled, not disabled. We need to update the test case to focus on the case when ogg is enabled.

::: content/media/test/test_mediarecorder_crash_getencodeddata.html
@@ +3,5 @@
> +<head>
> +  <title>Bug 957452 Test MediaRecorder crash at GetEncodedData on asan build</title>
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  <script type="text/javascript" src="manifest.js"></script>

Nit - this test doesn't depend on media framework, so we can remove this line.

@@ +10,5 @@
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +
> +function startTest() {
> +  SpecialPowers.setBoolPref("media.ogg.enabled", false);

This alters how the reduced test case executes to generate an error. We should remove this.

@@ +20,5 @@
> +  // Hit the timing issue in media recorder.
> +  // Only crash when use setTimeOut on asan build.
> +  setTimeout(function() {
> +    var recorder = new MediaRecorder(stream);
> +    recorder.onstop = function(e) {

Nit - indentation on each onXXX handler

@@ +34,5 @@
> +    onErrorFired = true;
> +    }
> +    recorder.start(0);
> +    recorder.requestData();
> +    recorder.stop();

With the special powers pref removed, what should happen here is:

1. ondataavailable fires
2. ondataavailable fires
3. onstop fires

We should add checks for each of these.

@@ +35,5 @@
> +    }
> +    recorder.start(0);
> +    recorder.requestData();
> +    recorder.stop();
> +  }, 1000);

Is this ASAN bug possible to trigger without the setTimeout call? If setTimeout is required, shouldn't this be 100, not 1000?

        Attachment #8359702 -
        Flags: review?(jsmith) → review-

    
    

    
  
How far back does this go? What versions are affected?

    
    

    
  
(In reply to Al Billings [:abillings] from comment #11)
> How far back does this go? What versions are affected?

This is a regression from the video encoder landing, which implies this only affects Gecko 29+.

          status-firefox26:
          ? → unaffected

          status-firefox27:
          ? → unaffected

          status-firefox28:
          ? → unaffected

    
    

    
  
Hi Jason, 
I try to put the test case 1 on mochitest but can't hit the timing issue.
That may case by mochitest runner run slower than normal one.
So I move to use test case 2 to capture this problem.
blocking-b2g: 1.4? → 1.4+

    
    

    
  
Randy can you request sec approval on the patch? (Thanks)

          status-firefox30:
          --- → affected

          tracking-firefox30:
          --- → +
Flags: needinfo?(rlin)

    
    

    
  
Comment on attachment 8359081 [details] [diff] [review]
patch v1

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
=>this patch remove the possible timing issue cause crash.
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
==>there is no comment talk about this security problem. (should I?)
Which older supported branches are affected by this flaw?
Firefox 30+
If not all supported branches, which bug introduced the flaw?
bug 879669
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
==>This issue start from firefox 30+. Not hard to merge into that branch.
How likely is this patch to cause regressions; how much testing does it needed
==>This change is safe and can fix QA provided test case.

        Attachment #8359081 -
        Flags: sec-approval?
Flags: needinfo?(rlin)

    
    

    
  
Hi Jason, 
Could I land the gecko fix first and open follow bug for test case landing?
Flags: needinfo?(jsmith)

    
    

    
  
(In reply to Randy Lin [:rlin] from comment #16)
> Hi Jason, 
> Could I land the gecko fix first and open follow bug for test case landing?

Given the severity of the bug, I don't think we should track this in a followup. We really need to land this patch with a test on landing.
Flags: needinfo?(jsmith)

    
    

    
  
OK, I try to write some js code to hit this problem on mochitest but I'm stuck for how to hit this bug, Even I put all of those testing code in the test page.
Do you have any suggestion for this one? https://bugzilla.mozilla.org/attachment.cgi?id=8356943

    
    

    
  
(In reply to Randy Lin [:rlin] from comment #18)
> OK, I try to write some js code to hit this problem on mochitest but I'm
> stuck for how to hit this bug, Even I put all of those testing code in the
> test page.
> Do you have any suggestion for this one?
> https://bugzilla.mozilla.org/attachment.cgi?id=8356943

Is the problem that the mochitest isn't triggering the crash without the bug fix here? Can you clarify?

Note - you'll need to run the mochitest under an ASAN configuration to confirm that the mochitest triggers the crash.

    
    

    
  
Comment 15 says this is Trunk only. Comment 12 says it is Trunk and Aurora (29). Can someone clarify?

In any case, I'll give it sec-approval+ to go into trunk. I assume we need an aurora patch here as well. Please check in tests separate and AFTER it is fixed in both Trunk and Aurora (aka "Everywhere") so we don't zero day ourselves.

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

    
    

    
  
(In reply to Al Billings [:abillings] from comment #20)
> Comment 15 says this is Trunk only. Comment 12 says it is Trunk and Aurora
> (29). Can someone clarify?

It should be Gecko 29. This feature landed on trunk in the Firefox 29 timeframe.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test case v2 (obsolete)
        — 
          — Splinter Review
      

    


  
The report report two ways to hit this issue, one can hit by crashtest, another can catch by mochitest.

        Attachment #8359702 -
        Attachment is obsolete: true

        Attachment #8372927 -
        Flags: review?(jsmith)

    
    

    
  
Comment on attachment 8372927 [details] [diff] [review]
test case v2

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

review- - the crashtest here isn't going to work & I still don't think we should be disabling ogg support in the mochitest.

Note - Given Al's comment above, we should land the fix here anyways before the test. Just mark [leave open] in the whiteboard when you land it, so that the bug remains open for landing the test.

::: content/media/test/crashtests/957452.html
@@ +1,1 @@
> +<script>

I don't think this is going to work - you need to make sure the crashtest doesn't finish here until after you finish the code you think needs to be executed.

See http://hg.mozilla.org/mozilla-central/file/7133bb431eba/dom/media/tests/crashtests/791330.html for an example of how to control a crashtest to end at your command in the code - specifically:

* Use of the class reftest-wait
* Use of document.documentElement.removeAttribute("class") to end the test

::: content/media/test/test_mediarecorder_getencodeddata.html
@@ +8,5 @@
> +<body>
> +<pre id="test">
> +<script class="testbody" type="text/javascript">
> +SimpleTest.waitForExplicitFinish();
> +SpecialPowers.pushPrefEnv({"set": [["media.ogg.enabled", false]]},

Why are we disabling ogg support here?

@@ +25,5 @@
> +      }
> +      recorder.ondataavailable = function(e) {
> +        onondataavailableFired = true;
> +      }
> +      recorder.onerror = function(e) {

The onerror workflow seems unnecessary here - the test case in question doesn't use this. We really should use an enabled ogg workflow with ondataavailable & onstop.

        Attachment #8372927 -
        Flags: review?(jsmith) → review-

    
    

    
  
1. The qa provide the test that disabling ogg support, without this step, we can't hit this crash. 
So do you mean we should have a normal mochitest and change those crash step into another crashtest?
2. I also make sure that crashtest can end successfully, if there has good method to do, I will change.

    
    

    
  
(In reply to Randy Lin [:rlin] from comment #24)
> 1. The qa provide the test that disabling ogg support, without this step, we
> can't hit this crash. 
> So do you mean we should have a normal mochitest and change those crash step
> into another crashtest?

Okay - didn't know that. In that case, we're doing the right thing in the mochitest then. We then just need to add the normal data attribute checks in that mochitest.

> 2. I also make sure that crashtest can end successfully, if there has good
> method to do, I will change.

See my review comments - you'll want to use reftest-wait as a class attribute & remove that attribute at the end of the test. The test case example showcases how to use it.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test case v3 (obsolete)
        — 
          — Splinter Review
      

    


  
Thanks, This patch add wait for crashtest and more check for mochitest.

        Attachment #8372927 -
        Attachment is obsolete: true

        Attachment #8373149 -
        Flags: review?(jsmith)

    
    

    
  
Comment on attachment 8373149 [details] [diff] [review]
test case v3

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

review- the crashtest isn't modeling the reload operation seen in the test case. Some cleanup needed in the mochitest as well.

::: content/media/test/crashtests/957452.html
@@ +7,5 @@
> +  <title>test for requestData problem</title>
> +  <script type="application/javascript">
> +    var intervals = [];
> +    function finish() {
> +      window.setTimeout(function anonymous() {

Why is this additional setTimeout needed?

@@ +16,5 @@
> +      }, 2000);
> +    }
> +
> +    function runTest() {
> +      for ( i = 0 ; i < 10; i++) {

This isn't exactly the same as the test case - the test case involves doing reload operations, where as this crashtest uses a for loop. As such, I'm not sure this is going to trigger the crash here. You need to use reload operations here instead of a for loop.

@@ +22,5 @@
> +        o0.src = "cors.webm";
> +        o1 = o0.mozCaptureStreamUntilEnded();
> +        o2 = new MediaRecorder(o1);
> +        intervals.push(setInterval(function anonymous() {
> +	        try { o2.start(0) } catch(e) { }

Nit - indentation

::: content/media/test/test_mediarecorder_getencodeddata.html
@@ +19,5 @@
> +    setTimeout(function() {
> +      var mediaRecorder = new MediaRecorder(stream);
> +      mediaRecorder.onstop = function(e) {
> +        ok(onErrorFired, 'onStop after onError');
> +        ok(onondataavailableFired, 'onondataavailableFired');

Variable name isn't right here - should be ondataavailableFired

@@ +22,5 @@
> +        ok(onErrorFired, 'onStop after onError');
> +        ok(onondataavailableFired, 'onondataavailableFired');
> +        SimpleTest.finish();
> +      }
> +      mediaRecorder.ondataavailable = function(e) {

Shouldn't we be checking the attributes of e here?

@@ +24,5 @@
> +        SimpleTest.finish();
> +      }
> +      mediaRecorder.ondataavailable = function(e) {
> +        if (onErrorFired) {
> +          onondataavailableFired = true;

Variable name isn't right here - should be ondataavailableFired

@@ +27,5 @@
> +        if (onErrorFired) {
> +          onondataavailableFired = true;
> +        }
> +      }
> +      mediaRecorder.onerror = function(e) {

Shouldn't we be checking the attributes e & mediaRecorder here?

@@ +39,5 @@
> +      mediaRecorder.stop();
> +      is(mediaRecorder.state, 'inactive',
> +         'Media recorder is inactive after being stopped');
> +      is(mediaRecorder.stream, stream,
> +         'Media recorder stream = element stream post recording');

Nit - probably don't need to check this twice

        Attachment #8373149 -
        Flags: review?(jsmith) → review-

    
    

    
  
Oh btw - to use a reload operation in a crashtest, you'll want to take advantage of local storage to track an index of how many reloads have occurred. See http://hg.mozilla.org/mozilla-central/file/ecf20a2484b6/dom/media/tests/crashtests/812785.html for an example of how to do this.

    
    

    
  
This test case patch generate base on 1/7.
I try to rebase to today's build without my patch.
The crashtests/957452.html (refer to test case 1) 
The symption become memory leak. both on aurora branch and mc. =.=
The mochitest one still works.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        test case v4
        — 
          — Splinter Review
      

    


  
Separate the mochitest into one patch.
But how about the crashtest one? create another bug for fixing the memory leak problem?

        Attachment #8373149 -
        Attachment is obsolete: true

        Attachment #8373193 -
        Flags: review?(jsmith)

    
    

    
  
(In reply to Randy Lin [:rlin] from comment #30)
> Created attachment 8373193 [details] [diff] [review]
> test case v4
> 
> Separate the mochitest into one patch.
> But how about the crashtest one? create another bug for fixing the memory
> leak problem?

Yeah - let's file a separate bug for the crashtest & memory leak issue.

    
    

    
  
Comment on attachment 8373193 [details] [diff] [review]
test case v4

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

::: content/media/test/test_mediarecorder_getencodeddata.html
@@ +33,5 @@
> +          ok(evt instanceof BlobEvent,
> +             'Events fired from ondataavailable should be BlobEvent');
> +          is(evt.type, 'dataavailable',
> +             'Event type should dataavailable');
> +          ok(evt.data.size == 0,

Nit - I would use is(evt.data.size, 0, 'Blob data size received is equal to zero'); here

        Attachment #8373193 -
        Flags: review?(jsmith) → review+

    
    

    
  
Thanks, I create Bug 970236 for fixing the memory issue.

    
    

    
  


  
check-in patch, carry reviewer.

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/825f12a34b11
https://hg.mozilla.org/mozilla-central/rev/a21db9070b60
Status: NEW → RESOLVED
Closed: 10 years ago

          status-firefox30:
          affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

    
    

    
  
Verified via clean landing on trunk.
Status: RESOLVED → VERIFIED

    
    

    
  
Can we get an Aurora patch too so we never ship this problem?

          status-firefox-esr24:
          --- → unaffected

    
    

    
  
Comment on attachment 8359081 [details] [diff] [review]
patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 879669
User impact if declined: crash Firefox by some step
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky):no 
String or IDL/UUID changes made by this patch:
no IDL change in this patch.

        Attachment #8359081 -
        Flags: approval-mozilla-aurora?

        Attachment #8359081 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
Does somebody will help to check-in that patch? 
We don't need to rebase it.
Keywords: checkin-needed
Component: Video/Audio → Video/Audio: Recording
No longer blocks: MediaRecording
Group: core-security

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: