559344 - (CVE-2010-3185) vorbis_staticbook_unpack doesn't handle failure from oggpack_read

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[timeless]

149 int vorbis_staticbook_unpack(oggpack_buffer *opb,static_codebook *s){
158   s->dim=oggpack_read(opb,16);
159   s->entries=oggpack_read(opb,24);
160   if(s->entries==-1)goto _eofout;

Comment 1

[timeless]

Attachment: proposal

Comment 2

[Matthew Gregan [:kinetik]]

Needs to be made into a separate patch with updated README_MOZILLA and update.sh before checking in.  Would also like Monty to cast an eye over this, I'll try to get in touch with him now.

Comment 3

[Monty Montgomery (:xiphmont)]

I'm not sure what the reported bug is.  The first read tries to read 16 bits; if there are not 16 bits to read it fails.  The second read tries to read 24 bits, which is longer than 16, which will then also fail.  Adding another check after the first read is redundant unless something is wrong in libogg.

Comment 4

[Daniel Veditz [:dveditz]]

Coverity assumes there's something wrong everywhere unless proven otherwise.

In between the two oggpack_read() calls the oggpack_buffer structure is changed. Is there a way to get b->endbyte to overflow to a negative value? If so the first check could fail and the second succeed. That'd take a buffer of just shy of 2G, is there anything that limits the size to less than that? 2G is not really an unreasonable size for high-quality long video. 

If your buffers are near that border I bet there are problems everywhere. In the same oggpack_read() there's the line

   if(b->endbyte*8+bits>b->storage*8)goto overflow;

which will screw up when the buffer gets above ~250Mb (0xFFFFFFF*8+8 is negative).

Comment 5

[Daniel Veditz [:dveditz]]

Given lack of data safest to assume the worst?

Comment 6

[Monty Montgomery (:xiphmont)]

Ah, so the possibility is that coverity is complaining about the line:

   if(b->endbyte*8+bits>b->storage*8)goto overflow;

not the lack of a check between the read calls?

The stacked read calls are not themselves a concern; the first consumes bits to the point of EOF, and the buffer is still at EOF in the second call.

The concern about overflow at ~250MB is valid and fixable.

Comment 7

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

I think we need a new patch for that, then. But that should be in a new bug, and this bug should be fixed and closed.

Comment 8

[Damon Sicore (:damons)]

So, do we need to land this one?

Comment 9

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

Actually, rereading this bug I think Monty is saying that this patch is not right. There is an underlying issue we need to fix, the line in comment #6. Matthew, since this is already assigned to you, would you mind fixing that?

Comment 10

[Matthew Gregan [:kinetik]]

No problem, I'll get to it later this week.

Comment 11

[Damon Sicore (:damons)]

Matthew, any update on ETA here?

Comment 12

[Matthew Gregan [:kinetik]]

I'll look at this first thing tomorrow.

Comment 13

[Matthew Gregan [:kinetik]]

Per the Ogg spec, packets have no theoretical size limit.  Use of 'long' in libogg's public API limits practical packet size to LONG_MAX.  The oggpack interfaces should therefore also work with data buffers up to LONG_MAX in size.  I talked to Monty about this and it turned out he's already working on a fix.

While reviewing this area (including how Theora and Vorbis use the oggpack API), I found a couple of related problems.  Theora doesn't use oggpack, so it's fine.  Vorbis does use it, mostly with hardcoded values.  I reviewed all of the places where a value read from the bitstream is later passed as the bits parameter to oggpack.

1. Packet size accumulation in ogg_framing.c:_packetout uses 'int', limiting practical packet size to INT_MAX.  This is a fixable bug.

2. oggpack is intended to deal with units no larger than 32-bits.  It has no checks for larger values, allowing out-of-bounds reads via the mask lookup table (which has 33 entries, 0..32) if > 32 bits is ever requested.

Vorbis uses fixed (32-bit or less) parameters to most oggpack_read calls, but does include a small number of oggpack calls using bit sizes read from the bitstream.  All of these appear to be safe (guaranteed to pass 0..32), except for the following:

floor0 unpack:
  info->ampbits=oggpack_read(opb,6); // ampbits may be 0..63

floor0 inverse1:
  int ampraw=oggpack_read(&vb->opb,info->ampbits); // bits param only valid 0..32

The Vorbis spec confirms that ampbits is a 6 bit field (http://xiph.org/vorbis/doc/Vorbis_I_spec.html#x1-890006).  As far as I can tell, there is no sanitization of ampbits after it's read from the bitstream until it's passed into oggpack_read.  ampbits values of 33..63 will read outside of the mask lookup table.

Comment 14

[Matthew Gregan [:kinetik]]

libogg r17268 in SVN fixes the issue in this bug and will avoid OOB reads for issue #2 mentioned above by returning an error from oggpack_read when bits > 32.

Comment 15

[Matthew Gregan [:kinetik]]

Attachment: patch v0

r17269 fixes issue #1 mentioned above.  r17270 fixes additional issues to do with the main bug report.

Import libogg SVN r17290.  This includes all of the fixes necessary for this issue.  There aren't a lot of other changes upstream between our current in-tree version and this.  The other major change is to do with writer flushing behaviour, which won't affect us.

Requesting r? from doublec to double check/rubber stamp.

Comment 16

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

Great, thanks.

Comment 17

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

http://hg.mozilla.org/mozilla-central/rev/9b49ef1d36dd

Comment 18

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

Backed out...
http://hg.mozilla.org/mozilla-central/rev/8b2f4c1f0d67

Due to build failures on Windows:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1276143391.1276144427.12217.gz

Building deps for /e/builds/moz2_slave/mozilla-central-win32/build/media/libogg/src/ogg_bitwise.c
cl -Foogg_bitwise.obj -c  -DOSTYPE=\"WINNT5.2\" -DOSARCH=WINNT  -I/e/builds/moz2_slave/mozilla-central-win32/build/media/libogg/src -I. -I../../../dist/include -I../../../dist/include/nsprpub  -Ie:/builds/moz2_slave/mozilla-central-win32/build/obj-firefox/dist/include/nspr -Ie:/builds/moz2_slave/mozilla-central-win32/build/obj-firefox/dist/include/nss         -TC -nologo -W3 -Gy -Fdgenerated.pdb  -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -O1 -MD            -FI ../../../dist/include/mozilla-config.h -DMOZILLA_CLIENT /e/builds/moz2_slave/mozilla-central-win32/build/media/libogg/src/ogg_bitwise.c
ogg_bitwise.c
e:/builds/moz2_slave/mozilla-central-win32/build/media/libogg/src/ogg_bitwise.c(87) : warning C4013: 'typeof' undefined; assuming extern returning int
e:/builds/moz2_slave/mozilla-central-win32/build/media/libogg/src/ogg_bitwise.c(87) : error C2143: syntax error : missing ')' before '~'
e:/builds/moz2_slave/mozilla-central-win32/build/media/libogg/src/ogg_bitwise.c(130) : error C2143: syntax error : missing ')' before '~'
make[6]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32/build/obj-firefox/media/libogg/src'
make[5]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32/build/obj-firefox/media/libogg'
make[4]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32/build/obj-firefox'
make[3]: Leaving directory `/e/builds/moz2_slave/mozilla-central-win32/build/obj-firefox'
make[6]: *** [ogg_bitwise.obj] Error 2

Comment 19

[Matthew Gregan [:kinetik]]

typeof is a gcc extension.  We'll need to find a way to implement the type min/max macros for non-gcc compilers, or find a different solution.

Comment 20

[Monty Montgomery (:xiphmont)]

Apologies.  A fix for this is in Xiph.Org SVN as r17287.  I'd simply tried to get too fancy on the typing.

Comment 21

[Matthew Gregan [:kinetik]]

Cool, that's the same patch I was going to email you today.  Thanks Monty!  I'll reroll the patch today.

Comment 22

[Matthew Gregan [:kinetik]]

Attachment: patch v1

Updates to SVN r17287.  This one should be fine on Win32, but I've pushed it to the try servers to be safe.

Comment 23

[Matthew Gregan [:kinetik]]

This is fine on tryserver, so ready for landing.

Comment 24

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/mozilla-central/rev/4b7350039ab1

Comment 25

[Matthew Gregan [:kinetik]]

This has baked on trunk for almost two weeks without any problems.  We should take this fix for the 1.9.2 branch to reduce our risk in released builds.  1.9.2 is still on libogg 1.1.3, so a different patch is required.  I'll post it shortly and request approval.

Updating from 1.1.3 to the trunk version of libogg on 1.9.2 will also fix bug 468296 on the branch.

Comment 26

[Matthew Gregan [:kinetik]]

Attachment: patch v1 for 1.9.2

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

Before approving, this patch is not a direct fix for the bug but rather fixes the bug by upgrading the branch to a newer version (1.2.0?) of libogg? That really seems outside the scope of fixes on the stable branches.

Comment 28

[Matthew Gregan [:kinetik]]

I think this approach is safer than trying to cherry pick the patches we're interested in.  There aren't a large number of changes between the library versions as libogg is pretty stable and not under heavy development.  It's likely that we want almost all of the changes in the new version (e.g. we already want the changes for this bug and bug 468296) as they're mostly stability fixes (except for the pagination changes that only affect apps that write Ogg files).

The attached patch is artificially large because it contains two file deletions (bitwise.c and framing.c which were unused dupes of ogg_bitwise.c and ogg_framing.c), and the changes between libogg 1.1.3 and 1.2.0 also contain some whitespace only changes.  The whitespace changes may make cherry picking the patches we're interested more difficult and higher risk.

We've also done this before with the libraries in the media/ hierarchy, with good results, for example:

https://bugzilla.mozilla.org/show_bug.cgi?id=507167#c16
https://bugzilla.mozilla.org/show_bug.cgi?id=501279#c19
https://bugzilla.mozilla.org/show_bug.cgi?id=512328#c32

Comment 29

[Daniel Veditz [:dveditz]]

Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

OK, upgrading libraries is something we'll have to take at the beginning of a cycle rather than at the end.

Comment 30

[Daniel Veditz [:dveditz]]

Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

Approved for 1.9.2.10 if you can land in the next week or so to get the maximum amount of nightly testing, a=dveditz for release-drivers.

What about the 1.9.1 branch?

Comment 31

[Matthew Gregan [:kinetik]]

Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

1.9.1 has the same version of libogg as 1.9.2, so this patch will apply directly.

Comment 32

[Matthew Gregan [:kinetik]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/a776a2590177

Comment 33

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

Is there a way to verify this fix on branch in a meaningful way?

Comment 34

[Matthew Gregan [:kinetik]]

I don't think so.

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

Approved for 1.9.1.14, a=dveditz for release-drivers

Comment 36

[LegNeato]

We want this for 1.9.1 too, right?

Comment 37

[Matthew Gregan [:kinetik]]

We do:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/66b4b4001a5b

Comment 38

[LegNeato]

Looks like this caused build bustage on 1.9.1...

Comment 39

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

(In reply to comment #38)
> Looks like this caused build bustage on 1.9.1...

Looks more likely to be caused by landing of Bug 595300.

Comment 40

[Josh Bressers]

The current integer overflow checks added in the upstream commit (https://trac.xiph.org/changeset/17268) will be optimized out on newer gcc's. I'm not sure about other compilers (overflowing signed integers is considered undefined in the C standard, so this can't be trusted).

I'm talking with Monty about this, there will be a proper fix, but in the meantime, this issue should not be considered fixed in Firefox until upstream produces a new patch.

Comment 41

[Matthew Gregan [:kinetik]]

That would be why that code was changed in r17270, which this landing also included (as well as r17287 to fix non-portable typeof use).  Can you point to specific code in the tip of libogg (or Firefox trunk) that still contains unsafe overflow checking?

Comment 42

[Matthew Gregan [:kinetik]]

I discussed with Monty on IRC and confirmed that the patch Bressers is talking about is r17270, which we have shipped already as part of the fix for this bug.  This bug is fixed.  Restoring flags and reclosing.