672789 - (CVE-2011-2992) this script reliably crashes Firefox 5 on Linux, Mac and Windows by streaming an ogg file through a proxy

Status: VERIFIED FIXED

(Core :: Audio/Video, defect)

Description

[ahu]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/534.30 (KHTML, like Gecko) Chrome/12.0.742.124 Safari/534.30

Steps to reproduce:

Connect to a proxyserver & stream a specific ogg file to an HTML5 player. Then I seek to a part that has not yet been loaded. 


Actual results:

On firefox 4 this works, on 5 and 6 I get a crossplatform crash (Windows, Linux, Mac), a segfault


Expected results:

No segfault.

Comment 1

[Mats Palmgren (inactive)]

Please let us know the Crash ID for the crashes you submitted.
https://developer.mozilla.org/En/How_to_get_a_stacktrace_for_a_bug_report

Comment 2

[Ali Juma [:ajuma]]

Also, please provide a link to the ogg video and/or script that causes the crash.

Comment 3

[Theo Snelleman]

Attachment: a little proxy-server that feeds the video-file slowly

This is a tarball containing a little proxy-server that helped me to reproduce the error.

After starting the proxy-server, let firefox connect to port 8000.
Then browse to www.video.com

A webcam-video will be shown. The proxy-server will feed the data very slowly, which allows you to seek to the end of the video before it is fully loaded.
When you seek to a part that isn't fetched yet, a segfault will occur.

the proxy-server is replying to the request with HTTP/1.1 200 OK and not with 206 Partial content. No filesize will be transmitted. To report the duration of the video to firefox, the "X-Content-Duration:"-header is used. Maybe this is relevant for this issue.

Comment 4

[Thomas Ahlblom]

Moving Report IDs from the Crash Signature field, and adding the signatures there instead.
bp-8a8b28c4-3cb3-4e1c-9a28-cddc02110720
bp-e409ab9b-bcd4-4eba-a0ec-d161f2110720

Comment 5

[Mats Palmgren (inactive)]

I can reproduce this reliably in a local trunk debug build on Linux64.
Before the crash I see this on the console:

###!!! ASSERTION: Guess must be before range end: 'guess < endOffset', file content/media/ogg/nsOggReader.cpp, line 1252
WARNING: NS_ENSURE_TRUE(ret == 0) failed: file content/media/ogg/nsOggReader.cpp, line 1127
WARNING: NS_ENSURE_TRUE(res != PAGE_SYNC_ERROR) failed: file content/media/ogg/nsOggReader.cpp, line 1268
WARNING: NS_ENSURE_SUCCESS(res, res) failed with result 0x80004005: file content/media/ogg/nsOggReader.cpp, line 1066

then I crash with a SEGV in random parts of the code.
Our video code is overwriting random pieces of memory?

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Bug 462960 should prevent this error by ensuring we never try to seek into ranges which are unbuffered in a resource/connection which doesn't support HTTP byte range requests.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

Paul, it would be really great if we could get bug 462960 and its dependencies landed. The changes to the seek algorithm you make in bug 462960 should prevent this crash happening.

Comment 8

[Mats Palmgren (inactive)]

Bug 462960 changes an existing scriptable API (nsIDOMHTMLMediaElement).
Does our process allow that on mozilla-aurora and -beta ?

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Not sure.

If we can't get bug 462960 on m-a or m-b, we could probably do a simple fix in each of the Ogg/Wave/WebM backend's to not seek into unbuffered ranges if the media isn't reporting that it supports byte range requests.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch: minimal fix

I suspect that since the patch from bug 462960 changes guids on interfaces wouldn't be wanted on m-{a,b} because it may affect binary add-ons?

Here's a patch to implement the minimal fix, pretty much what I described in comment 9. With this if you try to seek into an unbuffered range, the seek just aborts and playback continues from the previous playback position.

Comment 11

[Matthew Gregan [:kinetik]]

Comment on attachment 547320 [details] [diff] [review]
Patch: minimal fix

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

::: content/media/nsBuiltinDecoderReader.cpp
@@ +44,5 @@
>  #include "nsBuiltinDecoderReader.h"
>  #include "nsBuiltinDecoderStateMachine.h"
>  #include "mozilla/mozalloc.h"
>  #include "VideoUtils.h"
> +#include "nsTimeRanges.h"

This can be included in the .cpp file.

::: content/media/nsBuiltinDecoderStateMachine.h
@@ +417,5 @@
>    void DecodeSeek();
>  
> +  // Returns PR_TRUE if mSeekTarget is not in a buffered range and we
> +  // cannot seek the media into unbuffered ranges.
> +  PRBool IsSeekTargetUnreachable();

Would you mind renaming this to IsSeekTargetReachable and inverting the result and the test where we use it?

Comment 12

[Daniel Veditz [:dveditz]]

How is this potentially exploitable? It appears we'd read "ogg data" from uninitialized or unowned memory. If it doesn't crash on an access violation (safe) how is that different than someone feeding us a garbage ogg stream (which shouldn't be exploitable or else we've got worse problems!).

The two crash stacks attached to this bug are both null derefs.

Comment 13

[Johnny Stenback (:jst)]

I think we should take this minimal fix for branches.

Comment 14

[Mats Palmgren (inactive)]

Attachment: more stacks

(In reply to comment #12)
>It appears we'd read "ogg data" from uninitialized or unowned memory.

That's not what I'm seeing using a local debug build on Linux64.
I'm not crashing in the ogg decoder, but some time later in arbitrary
parts of the code.  I looked at more than a dozen crashes and all of
them appeared to have been caused by arbitrary memory being overwritten.
I'm assuming that the bug in the ogg code caused this to happen and I'm
worried that what was written can be influenced by the stream.

Here's a few of the crash stacks.
(JaegerTrampoline() sounds like a bad place to crash...)

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to comment #12)
> How is this potentially exploitable?

In this case we're reading data from file and writing it to memory. Due to unsigned integer overflow, in this case we'll end up writing however much we've downloaded from file into our 8KB memory buffer. Chances are we'll have downloaded more than 8KB by this stage, and we'll end up overflowing the bounds of that buffer and stomping memory with whatever was read from file.

This is caused by unsigned values overflowing at nsOggReader.cpp:1100 (aEndOffset==-1 as the stream's length is unknown). Matthew Gregan picked this up in bug 652721. With his patch applied, when we seek in this case, rather than crashing the media is just rendered unplayable (due the playback engine not recovering from a failed seek correctly, it's not an artefact of his patch).

Ideally we should take Matthew's patch in bug 652721 on central + branches to fix the crash, and then take Paul's patches in bug 462960 on central to prevent us trying to seek into unbuffered ranges when the stream isn't seekable. Paul's patches make my patch in this bug unnecessary, so we may as well take his patches instead.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

Fixed on central, aurora, and beta by landing of bug 652721.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Sorry, the tracking flags got reset (maybe a bug in bugzilla helper?). I can't reset the tracking-firefoxN flags, but have reset the other ones.

Comment 18

[Johnny Stenback (:jst)]

No worries, thanks for landing the fix!

Comment 19

[ahu]

Hi everybody,

On behalf of Theo and me, thank you very much for resolving this issue so quickly, we are much impressed!

Keep up the great work.

   Bert

Comment 21

[Daniel Veditz [:dveditz]]

Is this crash a regression? I can't find the affected code on the 1.9.2 branch.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Yes, this is a regression in 5.0. I was unable to reproduce the crash in [Mozilla/4.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.16) Gecko/20110319 Firefox/3.6.16].

Comment 23

[Chris Pearce [:cpearce (Not reading bugmail)]]

(In reply to Chris Pearce [:cpearce] from comment #22)
> Yes, this is a regression in 5.0.

Sorry, I meant 4.0.

Comment 24

[Daniel Veditz [:dveditz]]

"unaffected" is a better status

Comment 25

[u279076]

qa+ for verification in Firefox 7 and 8

Comment 26

[juan becerra [:juanb]]

Verified this on Linux on 6.0.2, 7.0RC(build2), and 8.0a2 using the steps in comment #0 and comment #3. In Fx5 moving the seek handle beyond what it has loaded crashed the application. In the fixed versions it does not crash.

Comment 27

[u279076]

Verified fixed based on comment 26.