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

RESOLVED FIXED in Firefox 13

Status

()

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

People

(Reporter: Octoploid, Assigned: derf)

Tracking

({crash})

Trunk
mozilla15
crash
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox12 affected, firefox13+ verified, firefox14 verified, firefox-esr10 affected)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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
(Assignee)

Updated

5 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 2

5 years ago
(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.
(Assignee)

Comment 3

5 years ago
Created attachment 621908 [details] [diff] [review]
Fix pp_harp_mod calculation
Attachment #621908 - Flags: review?(kinetik)
(Assignee)

Comment 4

5 years ago
Created attachment 621909 [details] [diff] [review]
Fix pp_sharp_mod calculation v2

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)
(Assignee)

Updated

5 years ago
OS: Linux → All
Hardware: x86_64 → All
Attachment #621909 - Flags: review?(kinetik) → review+
(Assignee)

Updated

5 years ago
Attachment #621909 - Attachment description: Fix pp_harp_mod calculation v2 → Fix pp_sharp_mod calculation v2
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/81eed0782699
Flags: in-testsuite-
Target Milestone: --- → mozilla15
(Assignee)

Comment 6

5 years ago
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?
(Assignee)

Comment 7

5 years ago
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).
status-firefox-esr10: --- → affected
status-firefox12: --- → affected
status-firefox13: --- → affected
status-firefox14: --- → affected
status-firefox15: --- → affected
(Assignee)

Updated

5 years ago
Blocks: 507554
Opening up based on comment 7.
Group: core-security
https://hg.mozilla.org/mozilla-central/rev/81eed0782699
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.
tracking-firefox13: --- → +
Keywords: qawanted

Updated

5 years ago
status-firefox15: affected → ---

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/12fa346baa18
https://hg.mozilla.org/releases/mozilla-beta/rev/77191fa1fc78
status-firefox13: affected → fixed
status-firefox14: affected → fixed
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!
status-firefox13: fixed → verified
Same result for firefox 14 beta, built with gcc 4.8 from mozilla-beta hg rev 95980:206e57393758.
Thanks Ralph.
status-firefox14: fixed → verified
You need to log in before you can comment on or make changes to this bug.