730425 - Crash [@ nsMediaCacheStream::Read(char*, unsigned int, unsigned int*) ]

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://www.youtube.com/watch?v=SmgT-rwn5W4&feature=player_embedded#at=15
2. Crash Beta/11, Aurora/12, Nightly/13 Mac, Linux, Windows

After you crash once, it may be difficult to reproduce on the same system. Not sure why...

Ted's exploitable tool rates this as hightly exploitable.

I couldn't reproduce with a locally saved version.

bp-ec8057be-9224-4b25-85fa-8f9b02120224
bp-39b7c534-980f-4d60-bb49-e98a62120224

Comment 1

[Matthew Gregan [:kinetik]]

    if (mStreamLength >= 0) {
      // Don't try to read beyond the end of the stream
      PRInt64 bytesRemaining = mStreamLength - mStreamOffset;
      if (bytesRemaining <= 0) {
        // Get out of here and return NS_OK
        break;
      }
      size = NS_MIN(size, PRInt32(bytesRemaining));
    }

mStreamLength is larger than 2**31, so if mStreamOffset is small enough, it'll overflow a PRInt32 once it hits the NS_MIN.

Comment 2

[Matthew Gregan [:kinetik]]

Attachment: patch v0

This crash has been known for a while (see bug 500784) , but I guess it wasn't considered from a security perspective due to the way it was discovered then.

I'm going to attach a simple fix for the crash to this bug, and leave the larger 64-bit cleanliness fixes for bug 500784 (which I had been planning to look at soonish anyway).

I might have a way to test this... I'll add a second patch to the bug if it pans out.

Comment 3

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9b7d19dbb281

Comment 4

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9b7d19dbb281

Comment 5

[Daniel Veditz [:dveditz]]

(In reply to Bob Clary [:bc:] from comment #0)
> 1. http://www.youtube.com/watch?v=SmgT-rwn5W4&feature=player_embedded#at=15
> 2. Crash Beta/11, Aurora/12, Nightly/13 Mac, Linux, Windows

Does that imply Release/10 does not crash, or did you simply not try it?

Comment 6

[Daniel Veditz [:dveditz]]

Release/10 crashes, too
bp-bbfdb912-3f01-4684-955c-465292120229

Testing note: by default you probably get the Flash YouTube player. Switch to HTML5 before testing this by going to http://www.youtube.com/html5

The patch makes the problem kind of obvious (create something with size > PR_INT32_MAX), and the patch is small, contained, and safe. Is there something non-obvious that makes this unsafe to get into Firefox 11/Beta?

Comment 7

[Daniel Veditz [:dveditz]]

Comment on attachment 601170 [details] [diff] [review]
patch v0

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

::: content/media/nsMediaCache.cpp
@@ +2231,1 @@
>      if (NS_FAILED(rv)) {

Would it make sense for this check to be a real bail-out rather than a debug-mode assert?

Comment 8

[Bob Clary [:bc] (inactive)]

(In reply to Daniel Veditz [:dveditz] from comment #5)

> Does that imply Release/10 does not crash, or did you simply not try it?

I didn't try it. For the most part I consider released versions done deals but yes it does crash Firefox 10.

bp-59933c84-e4cc-4d4e-8e8a-fbe752120229

Comment 9

[Matthew Gregan [:kinetik]]

Attachment: make assert fatal, fix same bug in similar code

(In reply to Daniel Veditz [:dveditz] from comment #7)
> Would it make sense for this check to be a real bail-out rather than a
> debug-mode assert?

Yes, thanks for pointing that out.  I assumed PR_Read would fail, but it looks like the result would be undefined.

I also noticed the same PRInt32 downcast bug in ReadFromCache, so I've applied a similar fix there.  This patch applies on top of the current tree, and I'll produce a rollup patch for the other branches.

> Is there something non-obvious that makes this unsafe to get into Firefox 11/Beta?

This fix is safe for all of the branches.

Comment 10

[Matthew Gregan [:kinetik]]

Attachment: rollup v0

This is the first two patches combined and tested to apply cleanly to aurora and beta.

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Matthew Gregan [:kinetik] from comment #10)
> Created attachment 601810 [details] [diff] [review]
> rollup v0
> 
> This is the first two patches combined and tested to apply cleanly to aurora
> and beta.

Please provide a risk analysis of this patch so that we can deliberate on whether to take this for FF11 and our next ESR tomorrow.

Comment 12

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/eb9b3853f6c1

Comment 13

[Matthew Gregan [:kinetik]]

> (In reply to Matthew Gregan [:kinetik] from comment #10)
> Please provide a risk analysis of this patch so that we can deliberate on
> whether to take this for FF11 and our next ESR tomorrow.

Sorry, here it is:

[Approval Request Comment]
Regression caused by (bug #): Not really a regression, bug present since inception (bug 475441).
User impact if declined: Crash (possibly exploitable?) when loading files larger than 2**31 bytes.
Testing completed (on m-c, etc.): First patch has been in mozilla-central overnight, second patch has just landed on inbound.
Risk to taking this patch (and alternatives if risky): Very low, the fix is simple.  Worst case is that the possibly exploitable crash becomes an unexploitable fatal assertion
String changes made by this patch: None(In reply to Alex Keybl [:akeybl] from comment #11)

Comment 14

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/mozilla-central/rev/eb9b3853f6c1

Comment 15

[Alex Keybl [:akeybl]]

Comment on attachment 601810 [details] [diff] [review]
rollup v0

[Triage Comment]
Approving for Aurora 12 and Beta 11 given this is sg:crit and we think the exploit will be obvious because of the landing on m-c.

Comment 16

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 601810 [details] [diff] [review]
rollup v0

Also approving this to land on ESR branch and mozilla-1.9.2 today please as we are going to build tomorrow.  See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for details.

Approving for 1.9.2 even though this patch may need some tweaking if things have moved around. Please email [e-mail] if there are any concerns about this landing cleanly.

Comment 17

[Matthew Gregan [:kinetik]]

Attachment: patch adjusted for esr10

Comment 18

[Matthew Gregan [:kinetik]]

Attachment: patch adjusted for 1.9.2

Comment 19

[Matthew Gregan [:kinetik]]

Patch applies to ESR10 with only indentation changes, and that patch then applies to 1.9.2 with a change to account for the move from PR_{MIN,MAX} to NS_{MIN,MAX}.  I've attached them both and emailed release-mgmt@ to be thorough, but I'm pretty sure these changes are trivial enough not to matter.

Comment 20

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e09de6c68982
https://hg.mozilla.org/releases/mozilla-beta/rev/8760c0f2ef9c

Comment 21

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/releases/mozilla-esr10/rev/083c775306da

Comment 22

[Matthew Gregan [:kinetik]]

Comment on attachment 602207 [details] [diff] [review]
patch adjusted for 1.9.2

This builds fine, but it has a mismash of NS_MIN and PR_MIN.  I'll make them consistent and attach a new patch later tonight.

Comment 23

[Matthew Gregan [:kinetik]]

Attachment: patch adjusted for 1.9.2 (v2)

Fixed one place where I used NS_MIN instead of PR_MIN.

Comment 24

[Matthew Gregan [:kinetik]]

https://hg.mozilla.org/releases/mozilla-1.9.2/rev/c23ed8a949b2

Comment 25

[Tracy Walker [:tracy]]

no crash on latest 10.0.3esr, 11, 12 or 13 on Mac and Win 7

Comment 26

[u279076]

(In reply to Tracy Walker [:tracy] from comment #25)
> no crash on latest 10.0.3esr, 11, 12 or 13 on Mac and Win 7

No crash on Firefox 3.6.28 either.