Closed Bug 752668 Opened 8 years ago Closed 8 years ago

Wrong array access in media/libtheora/lib/decode.c causes crash

Categories

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

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox12 --- wontfix
firefox13 + verified
firefox14 --- verified
firefox-esr10 --- wontfix

People

(Reporter: octoploid, Assigned: derf)

References

Details

(Keywords: crash, Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

Firefox compiled with gcc-4.8.0 crashes on the following site
http://archive.org/details/Eisenstein-October , when one starts the movie.:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 0x7fffdd171700 (LWP 5185)]
0x00007ffff5e21f1e in oc_dec_init (_setup=0x7fffd31e2800, _info=<optimized out>, _dec=0x7fffd6e56010)
    at /var/tmp/mozilla-central/media/libtheora/lib/decode.c:403
403           qsum+=_dec->state.dequant_tables[qti][pli][qi][12]+
(gdb) bt
#0  0x00007ffff5e21f1e in oc_dec_init (_setup=0x7fffd31e2800, _info=<optimized out>, _dec=0x7fffd6e56010)
    at /var/tmp/mozilla-central/media/libtheora/lib/decode.c:403
#1  th_decode_alloc (_info=<optimized out>, _setup=0x7fffd31e2800) at /var/tmp/mozilla-central/media/libtheora/lib/decode.c:1963
#2  0x00007ffff5727dbc in Init (this=0x7fffdb902c00) at /var/tmp/mozilla-central/content/media/ogg/nsOggCodecState.cpp:282
#3  nsTheoraState::Init (this=0x7fffdb902c00) at /var/tmp/mozilla-central/content/media/ogg/nsOggCodecState.cpp:264
#4  0x00007ffff572dfa2 in nsOggReader::ReadMetadata (this=0x7fffd9b28000, aInfo=0x7fffdd170ce8)
    at /var/tmp/mozilla-central/content/media/ogg/nsOggReader.cpp:268
#5  0x00007ffff571d81c in nsBuiltinDecoderStateMachine::DecodeMetadata (this=this@entry=0x7fffd6e48460)
    at /var/tmp/mozilla-central/content/media/nsBuiltinDecoderStateMachine.cpp:1792
#6  0x00007ffff571e1aa in nsBuiltinDecoderStateMachine::DecodeThreadRun (this=0x7fffd6e48460)
    at /var/tmp/mozilla-central/content/media/nsBuiltinDecoderStateMachine.cpp:507
#7  0x00007ffff4f58bd7 in nsRunnableMethodImpl<void (nsPACMan::*)(), true>::Run (this=<optimized out>) at ../../../dist/include/nsThreadUtils.h:345
#8  0x00007ffff5ca1a8e in nsThread::ProcessNextEvent (this=0x7fffd7855710, mayWait=<optimized out>, result=0x7fffdd170e0f)
    at /var/tmp/mozilla-central/xpcom/threads/nsThread.cpp:656
#9  0x00007ffff5c62d72 in NS_ProcessNextEvent_P (thread=<optimized out>, mayWait=<optimized out>)
    at /var/tmp/mozilla-central/moz-build-dir/xpcom/build/nsThreadUtils.cpp:245
#10 0x00007ffff5ca1349 in nsThread::ThreadFunc (arg=0x7fffd7855710) at /var/tmp/mozilla-central/xpcom/threads/nsThread.cpp:289
#11 0x00007ffff4604bc3 in ?? () from /usr/lib64/libnspr4.so
#12 0x00007ffff7bc8dff in start_thread (arg=0x7fffdd171700) at pthread_create.c:304
#13 0x00007ffff72a495d in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:114

From media/libtheora/lib/decode.c:

 368 static int oc_dec_init(oc_dec_ctx *_dec,const th_info *_info,
 369  const th_setup_info *_setup){
 370   int qti;
 371   int pli;
 372   int qi;
 373   int ret;
 374   ret=oc_state_init(&_dec->state,_info,3);
 375   if(ret<0)return ret;
 376   ret=oc_huff_trees_copy(_dec->huff_tables,
 377    (const ogg_int16_t *const *)_setup->huff_tables);
 378   if(ret<0){
 379     oc_state_clear(&_dec->state);
 380     return ret;
 381   }
 382   /*For each fragment, allocate one byte for every DCT coefficient token,
plus
 383      one byte for extra-bits for each token, plus one more byte for the
long
 384      EOB run, just in case it's the very last token and has a run length
of
 385      one.*/
 386   _dec->dct_tokens=(unsigned char *)_ogg_malloc((64+64+1)*
 387    _dec->state.nfrags*sizeof(_dec->dct_tokens[0]));
 388   if(_dec->dct_tokens==NULL){
 389     oc_huff_trees_clear(_dec->huff_tables);
 390     oc_state_clear(&_dec->state);
 391     return TH_EFAULT;
 392   }
 393   for(qi=0;qi<64;qi++)for(pli=0;pli<3;pli++)for(qti=0;qti<2;qti++){
 394     _dec->state.dequant_tables[qi][pli][qti]=
 395      _dec->state.dequant_table_data[qi][pli][qti];
 396   }
 397   oc_dequant_tables_init(_dec->state.dequant_tables,_dec->pp_dc_scale,
 398    &_setup->qinfo);
 399   for(qi=0;qi<64;qi++){
 400     int qsum;
 401     qsum=0;
 402     for(qti=0;qti<2;qti++)for(pli=0;pli<3;pli++){
 403       qsum+=_dec->state.dequant_tables[qti][pli][qi][12]+
 404        _dec->state.dequant_tables[qti][pli][qi][17]+
 405        _dec->state.dequant_tables[qti][pli][qi][18]+
 406        _dec->state.dequant_tables[qti][pli][qi][24]<<(pli==0);
 407     }
 408     _dec->pp_sharp_mod[qi]=-(qsum>>11);
 409   }

In the for loop (starting on line 402) "state.dequant_tables[qti][pli][qi]" is wrong and should read: "state.dequant_tables[qi][pli][qti]":

 402     for(qti=0;qti<2;qti++)for(pli=0;pli<3;pli++){
 403       qsum+=_dec->state.dequant_tables[qi][pli][qti][12]+
 404        _dec->state.dequant_tables[qi][pli][qti][17]+
 405        _dec->state.dequant_tables[qi][pli][qti][18]+
 406        _dec->state.dequant_tables[qi][pli][qti][24]<<(pli==0);
 407     }
Marking Security-Sensitive until it has been analyzed.
Group: core-security
Severity: normal → critical
Keywords: crash
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(In reply to Mats Palmgren [:mats] from comment #1)
> Marking Security-Sensitive until it has been analyzed.

Thanks for the report. Fix committed upstream in r18268. I'll get a patch landed on inbound as soon as I get off the plane I just got on.
Attached patch Fix pp_harp_mod calculation (obsolete) — Splinter Review
Attachment #621908 - Flags: review?(kinetik)
Whoops, forgot to actually hg add the patch needed by update.sh.
Attachment #621908 - Attachment is obsolete: true
Attachment #621908 - Flags: review?(kinetik)
Attachment #621909 - Flags: review?(kinetik)
OS: Linux → All
Hardware: x86_64 → All
Attachment #621909 - Flags: review?(kinetik) → review+
Attachment #621909 - Attachment description: Fix pp_harp_mod calculation v2 → Fix pp_sharp_mod calculation v2
https://hg.mozilla.org/integration/mozilla-inbound/rev/81eed0782699
Flags: in-testsuite-
Target Milestone: --- → mozilla15
Comment on attachment 621909 [details] [diff] [review]
Fix pp_sharp_mod calculation v2

[Approval Request Comment]
Regression caused by (bug #): 507554

User impact if declined: Crash on any page with a Theora file if the browser is compiled with gcc 4.8.0 or later.

Testing completed (on m-c, etc.): Landed on try and m-i, test results greenish:
https://tbpl.mozilla.org/?tree=Try&rev=f9bedd41b1e4
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=81eed0782699

Risk to taking this patch (and alternatives if risky):
Low. The calculations affected here are never used, because we don't enable libtheora's post-processing filters.

String changes made by this patch: None.
Attachment #621909 - Flags: approval-mozilla-beta?
Attachment #621909 - Flags: approval-mozilla-aurora?
mats: I'm not sure this really needs to be security-sensitive. The invalid accesses here are only reads, not writes, and the result of the calculation is never used, so I don't think this can trigger anything more than a DoS. Even if I'm wrong, we aren't using a gcc this recent on any official builds, and even the most minimal release testing should uncover this problem (just running the media tests we already have should be sufficient to trigger it).
Blocks: 507554
Opening up based on comment 7.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/81eed0782699
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Assignee: nobody → tterribe
Comment on attachment 621909 [details] [diff] [review]
Fix pp_sharp_mod calculation v2

[Triage Comment]
Very low risk of there being any user visible regressions from this patch, so we'll approve for Aurora and Beta.
Attachment #621909 - Flags: approval-mozilla-beta?
Attachment #621909 - Flags: approval-mozilla-beta+
Attachment #621909 - Flags: approval-mozilla-aurora?
Attachment #621909 - Flags: approval-mozilla-aurora+
That being said, adding qawanted to make sure we test the patch once it's landed.
Keywords: qawanted
Keywords: checkin-needed
Removing qawanted as we'll test this as part of our normal verifications. Is there a testcase we can use for this?
Keywords: qawanted
Whiteboard: [qa+]
We don't have anything in the test suite. The file from the original crash report is http://ia700707.us.archive.org/20/items/Eisenstein-October/Oktyabr1927.ogv
That should work, thanks Ralph.
Mozilla/5.0 (X11; Linux i686; rv:13.0) Gecko/20100101 Firefox/13.0
No crash with a normal Firefox build, but I understand this only occurred with a 4.8.0 compiled Firefox.

Is there a Firefox build anyone could point me to without requiring to compile Firefox with gcc 4.8.0?
Spoke with Ralph and he said you need to build Firefox with GCC 4.8 to reproduce the crash. He's going to get a build going now but it will take some time to build.
I wasn't able to get a copy of gcc 4.8 built today to confirm the issue is resolved for ff13, but the risk is low: we're confident in the patch, no problems have been seen since commit, and 4.8 is an unreleased compiler.
Thanks for the effort, Ralph. Marking this [qa-] indicating that we won't be exerting anymore effort trying to verify this bug. We're still open to verifying this if it becomes more necessary and easier to verify.
Whiteboard: [qa+] → [qa-]
Confirmed for firefox 13.

I built this morning's mozilla-release tree hg rev 92659:55ba50a35fd9 with yesterday's gcc 4.8 git rev e8802ff73608.

Playing the indicated video works with the the build, and segfaults if I reverse-apply the patch. So the fix is both valid and needed in ff 13.
Thank you very much Ralph!
Same result for firefox 14 beta, built with gcc 4.8 from mozilla-beta hg rev 95980:206e57393758.
You need to log in before you can comment on or make changes to this bug.