515882 - Unchecked dependencies on frame_height * frame_width not overflowing a 32 bit int

Status: RESOLVED FIXED

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

Description

[Reed Loden [:reed]]

Dan Kaminsky reported some more problems with Theora to [e-mail] on Fri, 11 Sep 2009 06:53:21 +0200.

-------------------------------
Almost missed this.  There are dependencies *all over the place* that
frame_height * frame_width not overflow a 32 bit int -- both in theora
code and yours.  Almost missed because height and width are 16 bit
fields in Theora...ahh, but that's 16 bit before a 4 bit left shift.
Close, but no cigar :)

decinfo.c:

static int oc_info_unpack(oggpack_buffer *_opb,th_info *_info){
  long val;
  /*Check the codec bitstream version.*/
  theorapackB_read(_opb,8,&val);
  _info->version_major=(unsigned char)val;
  theorapackB_read(_opb,8,&val);
  _info->version_minor=(unsigned char)val;
  theorapackB_read(_opb,8,&val);
  _info->version_subminor=(unsigned char)val;
  /*verify we can parse this bitstream version.
     We accept earlier minors and all subminors, by spec*/
  if(_info->version_major>TH_VERSION_MAJOR||
   _info->version_major==TH_VERSION_MAJOR&&
   _info->version_minor>TH_VERSION_MINOR){
    return TH_EVERSION;
  }
  /*Read the encoded frame description.*/
  theorapackB_read(_opb,16,&val);
  _info->frame_width=(ogg_uint32_t)val<<4; // note this isn't actually
2**16-1, it's shifted by four bits, so it's 2**20-1
  theorapackB_read(_opb,16,&val);
  _info->frame_height=(ogg_uint32_t)val<<4;// note this isn't actually
2**16-1, it's shifted by four bits, so it's 2**20-1
  theorapackB_read(_opb,24,&val);
  _info->pic_width=(ogg_uint32_t)val;
  theorapackB_read(_opb,24,&val);
  _info->pic_height=(ogg_uint32_t)val;
  theorapackB_read(_opb,8,&val);
  _info->pic_x=(ogg_uint32_t)val;
  theorapackB_read(_opb,8,&val);
  _info->pic_y=(ogg_uint32_t)val;
  theorapackB_read(_opb,32,&val);
  _info->fps_numerator=(ogg_uint32_t)val;
  theorapackB_read(_opb,32,&val);
  _info->fps_denominator=(ogg_uint32_t)val;
  if(_info->frame_width==0||_info->frame_height==0||
   _info->pic_width+_info->pic_x>_info->frame_width|| // this check
won't bust anything below 2**20-1
   _info->pic_height+_info->pic_y>_info->frame_height|| // this check
won't bust anything below 2**20-1
   _info->fps_numerator==0||_info->fps_denominator==0){
    return TH_EBADHEADER;
  }
  /*Note: The sense of pic_y is inverted in what we pass back to the
     application compared to how it is stored in the bitstream.
    This is because the bitstream uses a right-handed coordinate system, while
     applications expect a left-handed one.*/
  _info->pic_y=_info->frame_height-_info->pic_height-_info->pic_y;
  theorapackB_read(_opb,24,&val);
  _info->aspect_numerator=(ogg_uint32_t)val;
  theorapackB_read(_opb,24,&val);
  _info->aspect_denominator=(ogg_uint32_t)val;
  theorapackB_read(_opb,8,&val);
  _info->colorspace=(th_colorspace)val;
  theorapackB_read(_opb,24,&val);
  _info->target_bitrate=(int)val;
  theorapackB_read(_opb,6,&val);
  _info->quality=(int)val;
  theorapackB_read(_opb,5,&val);
  _info->keyframe_granule_shift=(int)val;
  theorapackB_read(_opb,2,&val);
  _info->pixel_fmt=(th_pixel_fmt)val;
  if(_info->pixel_fmt==TH_PF_RSVD)return TH_EBADHEADER;
  if(theorapackB_read(_opb,3,&val)<0||val!=0)return TH_EBADHEADER;
  return 0;
}

We see the overflow itself all over the place...

decode.c:

static int oc_dec_postprocess_init(oc_dec_ctx *_dec){
...
  if(_dec->variances==NULL||
   _dec->pp_frame_has_chroma!=(_dec->pp_level>=OC_PP_LEVEL_DEBLOCKC)){
    size_t frame_sz;
    frame_sz=_dec->state.info.frame_width*_dec->state.info.frame_height;
// overflow
    if(_dec->pp_level<OC_PP_LEVEL_DEBLOCKC){
      _dec->variances=(int *)_ogg_realloc(_dec->variances,
       _dec->state.fplanes[0].nfrags*sizeof(_dec->variances[0]));
      _dec->pp_frame_data=(unsigned char *)_ogg_realloc(
       _dec->pp_frame_data,frame_sz*sizeof(_dec->pp_frame_data[0]));
// victim of overflow
      _dec->pp_frame_buf[0].width=_dec->state.info.frame_width;
      _dec->pp_frame_buf[0].height=_dec->state.info.frame_height;
      _dec->pp_frame_buf[0].stride=-_dec->pp_frame_buf[0].width;
      _dec->pp_frame_buf[0].data=_dec->pp_frame_data+
       (1-_dec->pp_frame_buf[0].height)*_dec->pp_frame_buf[0].stride;
// actual buffer to be written into, little messy but derived from
overflow
    }
...
}

nsOggDecoder.cpp (possible there's a filter before this is hit though):

void nsOggDecodeStateMachine::PlayVideo(FrameData* aFrame)
{
  //  NS_ASSERTION(PR_InMonitor(mDecoder->GetMonitor()), "PlayVideo()
called without acquiring decoder monitor");
  if (aFrame && aFrame->mVideoHeader) {
    OggPlayVideoData* videoData =
oggplay_callback_info_get_video_data(aFrame->mVideoHeader);

    OggPlayYUVChannels yuv;
    yuv.ptry = videoData->y;
    yuv.ptru = videoData->u;
    yuv.ptrv = videoData->v;
    yuv.uv_width = aFrame->mUVWidth;
    yuv.uv_height = aFrame->mUVHeight;
    yuv.y_width = aFrame->mVideoWidth;
    yuv.y_height = aFrame->mVideoHeight;

    size_t size = aFrame->mVideoWidth * aFrame->mVideoHeight * 4; //
note this'll actually overflow on 32767*32767
    nsAutoArrayPtr<unsigned char> buffer(new unsigned char[size]);
    if (!buffer)
      return;
...
}
-------------------------------

Comment 1

[Dan Kaminsky]

Also, Theora should probably be range checking keyframe_granule_shift.  The field is bound from 0 to 31, but it's used to shift bits, and various other things inside of Theora.  See:

Decode.c#th_decode_packetin:

    /*Update granule position.
      This must be done before the striped decode callbacks so that the
       application knows what to do with the frame data.*/
    _dec->state.granpos=
     (_dec->state.keyframe_num<<_dec->state.info.keyframe_granule_shift)+
     (_dec->state.curframe_num-_dec->state.keyframe_num);
    _dec->state.curframe_num++;

I haven't done the full trace-through to see just what you can do by overflowing granpos, but "what to do with the frame data" does not exactly fill me with joy.

Comment 2

[cajbir (:cajbir)]

> size_t size = aFrame->mVideoWidth * aFrame->mVideoHeight * 4; 
> // note this'll actually overflow on 32767*32767

Correct me if I'm wrong but this can't happen due to the check for MAX_VIDEO_WIDTH, etc:

http://hg.mozilla.org/mozilla-central/file/0e157c793c5c/content/media/ogg/nsOggDecoder.cpp#l865

Comment 3

[cajbir (:cajbir)]

I've added Tim, the Theora maintainer. He might like to comment on the comments on the Theora code.

Comment 4

[Dan Kaminsky]

Chris--

   Yes, that should work against that particular overflow -- though you're lucky Theora stops you from having anything greater than 2**31 in either dimension; you're comparing with ints instead of unsigned ints.

   Question is, if *Firefox* detects the overflow, will this suppress any overflowed allocs or copies inside of the Theora code.  It actually *might*, since oc_info_unpack is so very distant from oc_dec_postprocess_init.  Could be a theora bug w/o affecting Firefox in that scenario.

   Still worried about the extraordinary flexibility on granule shift.

Comment 5

[Timothy B. Terriberry (:derf)]

The relevant check is in oc_state_ref_bufs_init(), which is called fairly early on in oc_state_init(), and actually ensures that there is enough room to index, not just the luma plane (frame_width*frame_height), but a padded luma plane (frame_width+32)*(frame_height+32), plus two chroma planes (possibly as large as the luma plane, each), in a variable of size size_t. You cannot get a working decoder if it does not pass this check. Post-processing is not initialized until much later, and only if it's enabled, which AFAIK, for Firefox, it is not.

There _was_ a bug in the above mentioned code, because the multiply frame_width*frame_height was done without a cast to (size_t), so it could overflow if that was different from ogg_uint32_t (e.g., only on 64-bit platforms). This was fixed in Thusnelda svn r15989, over 4 months ago (note that Thusnelda has since moved to trunk).

Chris is also quite right. If you're now checking against a maximum width and height before creating a decoder, this should solve the problem. libtheora goes through the above to ensure it can still process files with ridiculously large frame sizes in files that are valid according to the spec on machines that have the address space to do so. Firefox does not need to ensure it can do this.

The granule position has type ogg_int64_t. It is only used for timing information (e.g., _when_ to display a frame). I don't even think liboggplay uses the one generated by libtheora. Theora is a fixed-framerate codec, so many media frameworks (including liboggplay/liboggz) choose to calculate timing information on their own. Either code path could probably be made to overflow even a 64-bit integer, since they're compute relative to the timestamps on the user-supplied Ogg pages. The worst this could do is hand you back frames scheduled to display in the far distant future or the far distant past. It may be worthwhile checking that the latter does not violate any of your assumptions.

Comment 6

[Dan Kaminsky]

Actually, the check you describe (unless you're looking at different code than I am -- possible) is in oc_state_init(), and I totally forgot about it.  It's this:

  if((_info->frame_width&0xF)||(_info->frame_height&0xF)||
   _info->frame_width>=0x100000||_info->frame_height>=0x100000||
   _info->pic_x+_info->pic_width>_info->frame_width||
   _info->pic_y+_info->pic_height>_info->frame_height||
   _info->pic_x>255||
   _info->frame_height-_info->pic_height-_info->pic_y>255||
   _info->colorspace<0||_info->colorspace>=TH_CS_NSPACES||
   _info->pixel_fmt<0||_info->pixel_fmt>=TH_PF_NFORMATS){
    return TH_EINVAL;
  }

0x100000 * 0x100000 will overflow on 32 bit, and both are <2**20-1 so these are expressible values.  This will also bash your luma planes.

Post-proc was an example...you multiply frame_width*frame_height all over (as would be expected).

If we're *extremely* confident that Moz's external check prevents you from going down any frame allocation paths, then we should close this bug here.  Do you have your own bug-tracking system?

Comment 7

[cajbir (:cajbir)]

(In reply to comment #6)
> If we're *extremely* confident that Moz's external check prevents you from
> going down any frame allocation paths, then we should close this bug here.  Do
> you have your own bug-tracking system?

Due to a liboggplay issue (see bug 504843 comment 8) it's possible for Theora to decode frames before our check is done. This will be fixed in liboggplay according to bug 504843 comment 9.

Comment 8

[Timothy B. Terriberry (:derf)]

(In reply to comment #6)
> Actually, the check you describe (unless you're looking at different code than
> I am -- possible) is in oc_state_init(), and I totally forgot about it.  It's

No, I think we _are_ looking at very different code. The check you describe is merely checking that the parameters are spec-compliant (mostly useful for initializing an encoder, which shares that code path). The check I describe was actually added in r15977, committed on May 2nd (more recent than I remembered, but still over 4 months ago).

> Post-proc was an example...you multiply frame_width*frame_height all over (as
> would be expected).

Which is why the check is done early in the initialization.
 
> If we're *extremely* confident that Moz's external check prevents you from
> going down any frame allocation paths, then we should close this bug here.  Do
> you have your own bug-tracking system?

If you do not create a decoder, libtheora will not allocate any memory for a full frame. As Chris notes, that will require the liboggplay fix in bug 504843.

We don't have a real bug tracking system, but we do have Trac: https://trac.xiph.org/report
I vaguely recall someone disabling the creation of new logins due to spam, though. Maybe that got fixed later.

Comment 9

[Dan Kaminsky]

Yes, we are looking at different code.  I'm looking at:

http://mxr.mozilla.org/mozilla1.9.2/source/media/libtheora/lib/dec/state.c#466

(OK, technically I'm auditing the 1.9.1 branch, but the code is the same.)

You are looking at:

http://svn.xiph.org/trunk/theora/lib/state.c

The rather important diff:

 /*Check for overflow.
    The same caveats apply as for oc_state_frarray_init().*/
  if(yplane_sz/yhstride!=yheight||2*cplane_sz<cplane_sz||
   ref_frame_sz<yplane_sz||ref_frame_data_sz/_nrefs!=ref_frame_sz){
    return TH_EIMPL;
  }

Firefox 3.5.3 just doesn't have that code.  It's just not there.  Looks like that wouldn't matter, but it also doesn't have the liboggplay code eiher.

Sounds like the real bug is integration delay...

Comment 10

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

(In reply to comment #9)
> Firefox 3.5.3 just doesn't have that code.  It's just not there.  Looks like
> that wouldn't matter, but it also doesn't have the liboggplay code eiher.

I'm attempting to update liboggplay in bug 512328. Unfortunately it causes random shutdown failure on our test machines, so it may take a while.

Comment 11

[Daniel Veditz [:dveditz]]

And we're trying to update libvorbis in bug 501279 (which is fixed for 1.9.2, but needs testing before dropping a 600K patch on a stable/security-release branch).

Comment 12

[Daniel Veditz [:dveditz]]

oops, wrong bug. meant to comment in bug 515889

Comment 13

[Daniel Veditz [:dveditz]]

We've updated lots of media libraries in 1.9.1.4 -- do those fix this bug or do we need the Theora update as well? If it requires the Theora update I think we're going to wait for 1.9.1.5 on it.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The libtheora update in bug 515882 fixed this on trunk. Are we going to take that on the 1.9.2 branch?

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Er, that update was in bug 507554.

Comment 16

[Matthew Gregan [:kinetik]]

I'm not sure.  My only concern is that Theora 1.1.0 has had no exposure to fuzzers yet, so we could be opening ourselves up to a whole lot of new attacks since 1.1.0 is a near complete rewrite as I understand it.

Comment 17

[Matthew Gregan [:kinetik]]

Discussing this in #theora, Gregory Maxwell did about a month of fuzzing (of just libtheora using dump_video) on eight cores a little while ago, and found no issues.

Given that, and earlier fuzzing of the media library integration during 1.9.1, we've got some coverage.  It's possible that there are new problems in the integration that would be discovered by fuzzing, but the chance of that is probably no higher for the Theora update versus any of the other library updates we've taken.  It's also possible there are new problems in areas that weren't fuzzed, such as the MSVC assembly optimizations, but I don't think we fuzzed on Windows the first time around either.

Comment 18

[Matthew Gregan [:kinetik]]

Fixed on trunk and 1.9.2 by landing the Theora 1.1.0 update (bug 507554).

Comment 19

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

Fixed on 1.9.1 by landing of bug 504843.

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Is there a way to specifically verify this fix for 1.9.1.6?

Comment 21

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

The test file from bug 504843 will not crash if this bug is fixed.

Comment 22

[Al Billings [:abillings - ex-MoCo]]

Verified for 1.9.1.6 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9.1.6pre) Gecko/20091119 Shiretoko/3.5.6pre (.NET CLR 3.5.30729) using
existing testcase in bug 504843. Crashes cleanly with 1.9.1.5.