Last Comment Bug 559344 - (CVE-2010-3185) vorbis_staticbook_unpack doesn't handle failure from oggpack_read
: vorbis_staticbook_unpack doesn't handle failure from oggpack_read
[sg:critical?][critsmash:patch] [qa-e...
: coverity
Product: Core
Classification: Components
Component: Audio/Video (show other bugs)
: Trunk
: All All
-- major (vote)
: ---
Assigned To: Matthew Gregan [:kinetik]
: Maire Reavy [:mreavy] Please needinfo me
Depends on:
  Show dependency treegraph
Reported: 2010-04-14 08:50 PDT by timeless
Modified: 2011-03-25 14:11 PDT (History)
12 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

proposal (600 bytes, patch)
2010-04-14 08:54 PDT, timeless
kinetik: review+
Details | Diff | Splinter Review
patch v0 (49.70 KB, patch)
2010-06-03 23:17 PDT, Matthew Gregan [:kinetik]
cajbir.bugzilla: review+
Details | Diff | Splinter Review
patch v1 (49.40 KB, patch)
2010-06-10 15:53 PDT, Matthew Gregan [:kinetik]
no flags Details | Diff | Splinter Review
patch v1 for 1.9.2 (188.54 KB, patch)
2010-06-27 19:18 PDT, Matthew Gregan [:kinetik]
dveditz: approval1.9.2.11+
dveditz: approval1.9.1.14+
Details | Diff | Splinter Review

Description User image timeless 2010-04-14 08:50:45 PDT
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 User image timeless 2010-04-14 08:54:21 PDT
Created attachment 438998 [details] [diff] [review]
Comment 2 User image Matthew Gregan [:kinetik] 2010-04-19 16:39:06 PDT
Needs to be made into a separate patch with updated README_MOZILLA and 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 User image Monty Montgomery (:xiphmont) 2010-04-19 17:18:15 PDT
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 User image Daniel Veditz [:dveditz] 2010-04-20 08:23:08 PDT
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 User image Daniel Veditz [:dveditz] 2010-04-27 12:58:37 PDT
Given lack of data safest to assume the worst?
Comment 6 User image Monty Montgomery (:xiphmont) 2010-04-28 21:30:02 PDT
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-04 13:43:41 PDT
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 User image Damon Sicore (:damons) 2010-05-11 13:47:19 PDT
So, do we need to land this one?
Comment 9 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-05-11 19:25:28 PDT
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 User image Matthew Gregan [:kinetik] 2010-05-16 16:58:48 PDT
No problem, I'll get to it later this week.
Comment 11 User image Damon Sicore (:damons) 2010-06-01 13:42:03 PDT
Matthew, any update on ETA here?
Comment 12 User image Matthew Gregan [:kinetik] 2010-06-01 20:49:48 PDT
I'll look at this first thing tomorrow.
Comment 13 User image Matthew Gregan [:kinetik] 2010-06-03 22:07:59 PDT
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 (  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 User image Matthew Gregan [:kinetik] 2010-06-03 22:31:12 PDT
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 User image Matthew Gregan [:kinetik] 2010-06-03 23:17:42 PDT
Created attachment 449206 [details] [diff] [review]
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 User image Robert O'Callahan (:roc) (email my personal email if necessary) 2010-06-05 15:00:43 PDT
Great, thanks.
Comment 17 User image Chris Pearce (:cpearce) 2010-06-09 21:14:41 PDT
Comment 18 User image Chris Pearce (:cpearce) 2010-06-09 21:43:51 PDT
Backed out...

Due to build failures on Windows:

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
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 User image Matthew Gregan [:kinetik] 2010-06-09 21:47:01 PDT
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 User image Monty Montgomery (:xiphmont) 2010-06-10 06:43:14 PDT
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 User image Matthew Gregan [:kinetik] 2010-06-10 14:22:02 PDT
Cool, that's the same patch I was going to email you today.  Thanks Monty!  I'll reroll the patch today.
Comment 22 User image Matthew Gregan [:kinetik] 2010-06-10 15:53:19 PDT
Created attachment 450482 [details] [diff] [review]
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 User image Matthew Gregan [:kinetik] 2010-06-13 20:21:33 PDT
This is fine on tryserver, so ready for landing.
Comment 24 User image Matthew Gregan [:kinetik] 2010-06-15 15:57:22 PDT
Comment 25 User image Matthew Gregan [:kinetik] 2010-06-27 19:14:59 PDT
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 User image Matthew Gregan [:kinetik] 2010-06-27 19:18:27 PDT
Created attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2
Comment 27 User image Daniel Veditz [:dveditz] 2010-08-06 11:16:45 PDT
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 User image Matthew Gregan [:kinetik] 2010-08-06 17:30:43 PDT
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:
Comment 29 User image Daniel Veditz [:dveditz] 2010-08-09 10:31:42 PDT
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 User image Daniel Veditz [:dveditz] 2010-08-27 10:45:46 PDT
Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

Approved for 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 User image Matthew Gregan [:kinetik] 2010-08-30 20:43:25 PDT
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 User image Matthew Gregan [:kinetik] 2010-08-30 23:28:41 PDT
Comment 33 User image Al Billings [:abillings] 2010-09-20 17:45:32 PDT
Is there a way to verify this fix on branch in a meaningful way?
Comment 34 User image Matthew Gregan [:kinetik] 2010-09-20 17:49:30 PDT
I don't think so.
Comment 35 User image Daniel Veditz [:dveditz] 2010-09-27 16:38:59 PDT
Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

Approved for, a=dveditz for release-drivers
Comment 36 User image christian 2010-09-28 14:49:46 PDT
We want this for 1.9.1 too, right?
Comment 37 User image Matthew Gregan [:kinetik] 2010-09-28 15:40:20 PDT
We do:
Comment 38 User image christian 2010-09-28 16:55:36 PDT
Looks like this caused build bustage on 1.9.1...
Comment 39 User image Chris Pearce (:cpearce) 2010-09-28 17:05:39 PDT
(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 User image Josh Bressers 2010-10-11 11:40:59 PDT
The current integer overflow checks added in the upstream commit ( 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 User image Matthew Gregan [:kinetik] 2010-10-14 17:07:14 PDT
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 User image Matthew Gregan [:kinetik] 2010-10-21 18:42:00 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.