Bug 559344 (CVE-2010-3185)

vorbis_staticbook_unpack doesn't handle failure from oggpack_read




7 years ago
6 years ago


(Reporter: timeless, Assigned: kinetik)




Firefox Tracking Flags

(status1.9.2 .11-fixed, status1.9.1 .14-fixed)


(Whiteboard: [sg:critical?][critsmash:patch] [qa-examined-192] [qa-needs-STR] , URL)


(2 attachments, 2 obsolete attachments)



7 years ago
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;


7 years ago
Group: core-security

Comment 1

7 years ago
Created attachment 438998 [details] [diff] [review]
Attachment #438998 - Flags: review?(kinetik)
Attachment #438998 - Flags: review?(kinetik) → review+
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.
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.
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).
Given lack of data safest to assume the worst?
Ever confirmed: true
Whiteboard: [sg:critical?]


7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:patch]
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.
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.
Keywords: checkin-needed
So, do we need to land this one?
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?
No problem, I'll get to it later this week.
Keywords: checkin-needed
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?]


7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:investigating]
Matthew, any update on ETA here?
I'll look at this first thing tomorrow.
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.
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.
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.
Attachment #438998 - Attachment is obsolete: true
Attachment #449206 - Flags: review?(chris.double)


7 years ago
Attachment #449206 - Flags: review?(chris.double) → review+
Great, thanks.
Whiteboard: [sg:critical?][critsmash:investigating] → [sg:critical?][critsmash:investigating][needs landing]


7 years ago
Whiteboard: [sg:critical?][critsmash:investigating][needs landing] → [sg:critical?][critsmash:patch][needs landing]
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical?][critsmash:patch][needs landing] → [sg:critical?][critsmash:patch]
Attachment #449206 - Flags: approval1.9.2.5?
Attachment #449206 - Flags: approval1.9.1.11?
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
Resolution: FIXED → ---
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.
Apologies.  A fix for this is in Xiph.Org SVN as r17287.  I'd simply tried to get too fancy on the typing.
Attachment #449206 - Flags: approval1.9.2.5?
Attachment #449206 - Flags: approval1.9.1.11?
Cool, that's the same patch I was going to email you today.  Thanks Monty!  I'll reroll the patch today.
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.
Attachment #449206 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:patch][needs landing]
This is fine on tryserver, so ready for landing.
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
Whiteboard: [sg:critical?][critsmash:patch][needs landing] → [sg:critical?][critsmash:patch]
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.
Created attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2
Attachment #454417 - Flags: approval1.9.2.8?
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.
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 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.
Attachment #454417 - Flags: approval1.9.2.9? → approval1.9.2.10?
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?
Attachment #454417 - Flags: approval1.9.2.10? → approval1.9.2.10+
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.
Attachment #454417 - Flags: approval1.9.1.13?
Attachment #454417 - Flags: approval1.9.1.12?
Attachment #454417 - Flags: approval1.9.1.12?
status1.9.2: --- → .10-fixed
Is there a way to verify this fix on branch in a meaningful way?
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:patch] [qa-examined-192] [qa-needs-STR]
I don't think so.
blocking1.9.1: --- → .14+
blocking1.9.2: --- → .11+
status1.9.1: --- → wanted
Comment on attachment 454417 [details] [diff] [review]
patch v1 for 1.9.2

Approved for, a=dveditz for release-drivers
Attachment #454417 - Flags: approval1.9.1.14? → approval1.9.1.14+

Comment 36

7 years ago
We want this for 1.9.1 too, right?
We do:
status1.9.1: wanted → .14-fixed

Comment 38

7 years ago
Looks like this caused build bustage on 1.9.1...
(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.
Alias: CVE-2010-3185

Comment 40

7 years ago
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.
Resolution: FIXED → ---
status1.9.1: .14-fixed → ?
status1.9.2: .11-fixed → ?


7 years ago
blocking1.9.1: .14+ → .15+
blocking1.9.2: .11+ → .12+
status1.9.1: ? → wanted
status1.9.2: ? → wanted
Whiteboard: [sg:critical?][critsmash:patch] [qa-examined-192] [qa-needs-STR] → [sg:critical?]vorbis upgrade landed, but new patch needed
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?
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.
blocking1.9.1: .15+ → ---
blocking1.9.2: .12+ → ---
Last Resolved: 7 years ago7 years ago
status1.9.1: wanted → .14-fixed
status1.9.2: wanted → .11-fixed
Resolution: --- → FIXED
Whiteboard: [sg:critical?]vorbis upgrade landed, but new patch needed → [sg:critical?][critsmash:patch] [qa-examined-192] [qa-needs-STR]
Group: core-security
You need to log in before you can comment on or make changes to this bug.