Closed Bug 1450353 (CVE-2020-12422) Opened 3 years ago Closed 1 year ago

Write and read beyond bounds caused by nsJPEGEncoder::emptyOutputBuffer()

Categories

(Core :: ImageLib, enhancement, P1)

58 Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla79
Tracking Status
firefox-esr68 - wontfix
firefox-esr78 78+ fixed
firefox77 --- wontfix
firefox78 + fixed
firefox79 + fixed

People

(Reporter: mozillabugs, Assigned: aosmond)

Details

(Keywords: csectype-bounds, sec-moderate, Whiteboard: [adv-main78+])

Attachments

(4 files)

Attached file bug_1091_poc_3.htm
nsJPEGEncoder::emptyOutputBuffer (image\encoders\jpeg\nsJPEGEncoder.cpp) can experience an integer overflow. When it does, it causes compressed data (which can be derived from a script-provided image) to be written far beyond the end of its output buffer.

The bug is on line 433, which doesn't check for overflow.

This bug is similar to https://bugzilla.mozilla.org/show_bug.cgi?id=1434490 .

Attached is a POC that demonstrates the overflow and write beyond bounds.

If FF survives long enough to execute the lambda function in the |e.toBlob()| call in the POC, the script will then be able to read data from beyond bounds by reading the Blob, since that data comes from memory pointed to by |nsJPEGEncoder::mImageBuffer|. Often, however, FF dies reading beyond bounds while attempting to copy the buffer into the Blob, usually in NS_CopySegmentToBuffer().


420: boolean // static
421: nsJPEGEncoder::emptyOutputBuffer(jpeg_compress_struct* cinfo)
422: {
423:   nsJPEGEncoder* that = static_cast<nsJPEGEncoder*>(cinfo->client_data);
424:   NS_ASSERTION(that->mImageBuffer, "No buffer to empty!");
425: 
426:   // When we're reallocing the buffer we need to take the lock to ensure
427:   // that nobody is trying to read from the buffer we are destroying
428:   ReentrantMonitorAutoEnter autoEnter(that->mReentrantMonitor);
429: 
430:   that->mImageBufferUsed = that->mImageBufferSize;
431: 
432:   // expand buffer, just double size each time
433:   that->mImageBufferSize *= 2;
434: 
435:   uint8_t* newBuf = (uint8_t*)realloc(that->mImageBuffer,
436:                                       that->mImageBufferSize);
437:   if (!newBuf) {
438:     // can't resize, just zero (this will keep us from writing more)
439:     free(that->mImageBuffer);
440:     that->mImageBuffer = nullptr;
441:     that->mImageBufferSize = 0;
442:     that->mImageBufferUsed = 0;
443: 
444:     // This seems to be the only way to do errors through the JPEG library.  We
445:     // pass an nsresult masquerading as an int, which works because the
446:     // setjmp() caller casts it back.
447:     longjmp(((encoder_error_mgr*)(cinfo->err))->setjmp_buffer,
448:             static_cast<int>(NS_ERROR_OUT_OF_MEMORY));
449:   }
450:   that->mImageBuffer = newBuf;
451: 
452:   cinfo->dest->next_output_byte = &that->mImageBuffer[that->mImageBufferUsed];
453:   cinfo->dest->free_in_buffer = that->mImageBufferSize - that->mImageBufferUsed;
454:   return 1;
455: }

Use the POC by starting FF, disabling D2D (if on Windows) (by setting gfx.direct2d.disable to |true|), and enabling large-surface support (c.f. https://bugzilla.mozilla.org/show_bug.cgi?id=1282074 ) by setting gfx.max-alloc-size == 2147483647 (0x7fffffff). Restart FF.

Now attach a debugger to FF and set a BP on line 433, above. Load the POC and wait for the BP at which the value of |that->mImageBufferSize| is 0x80000000. Now step line 433 and observe the overflow and subsequent allocation of a 0-length buffer for |newBuf| (and thus |mImageBuffer|, line 450) and the setting of the next writable buffer location (|next_output_byte|, line 452) to 0x80000000 beyond the end of the buffer.

Now set a BP on encode_one_block_simd() (in media\libjpeg\jchuff.c) and proceed. When you hit the BP, observe the function writing compressed image data into hyperspace at |state->next_output_byte|.


The POC generates random data that is generally uncompressible, and thus causes the JPEG compressor to generate a compressed data stream that is sufficiently-long to cause the overflow.
Summary: Write and read beyond bounds caused by nsJPEGEncoder::emptyOutputBuffer → Write and read beyond bounds caused by nsJPEGEncoder::emptyOutputBuffer()
Group: core-security → gfx-core-security
Flags: sec-bounty?
Tyson: please try this in an ASAN build. I'm not seeing a problem in a Mac nightly.
Flags: needinfo?(twsmith)
Attached file ASan_log.txt
Log created with m-c:
BuildID=20180331213254
SourceStamp=3f9a70b125f6fb9be2b209159657fd7ae5515c01

Note: this only work when gfx.max-alloc-size is set properly and it takes a few minutes to trigger (and uses a lot of memory)
Flags: needinfo?(twsmith)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: sec-bounty? → sec-bounty+
Component: Graphics → ImageLib
Assignee: nobody → aosmond
Flags: needinfo?(aosmond)

This is unlikely to manifest due to the pref settings and our limitations on the maximum surface size. But I have put together a patch to check allocation size.

Flags: needinfo?(aosmond)
Attached file Bug 1450353.

Comment on attachment 9155948 [details]
Bug 1450353.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Somewhat hard. To overflow the encoder buffer, you would need a very large source surface, which we restrict the maximum size of, or combine it with a flaw in the JPEG encoder, to produce >2GB output.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: All
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Not hard, I think the patch will likely apply cleanly as this code doesn't change much.
  • How likely is this patch to cause regressions; how much testing does it need?: Not likely, we are just adding a bounds check.
Attachment #9155948 - Flags: sec-approval?

Hi. I cannot get my Phabricator account to work to review this patch. Can you find someone to delete the account so I can recreate it? Thanks.

Flags: needinfo?(aosmond)

Comment on attachment 9155948 [details]
Bug 1450353.

sec-approval+

Attachment #9155948 - Flags: sec-approval? → sec-approval+

https://hg.mozilla.org/integration/autoland/rev/955ac3fe48e84ef3814bd5053bb758608944cbb6

Please nominate this for Beta approval when you get a chance. It grafts cleanly as-landed. It does graft cleanly to ESR68 too, but I'm not sure this is really needed there given how close to EOL it is?

Changing the priority to p1 as the bug is tracked by a release manager for the current beta.
See What Do You Triage for more information

Priority: P3 → P1

Comment on attachment 9155948 [details]
Bug 1450353.

Beta/Release Uplift Approval Request

  • User impact if declined: Theoretical vulnerability to buffer overflow in the JPEG encoder
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): We are just adding a bounds check, low risk.
  • String changes made/needed:
Flags: needinfo?(aosmond)
Attachment #9155948 - Flags: approval-mozilla-beta?

Comment on attachment 9155948 [details]
Bug 1450353.

approved for 78 rc1

Attachment #9155948 - Flags: approval-mozilla-beta? → approval-mozilla-release+
Group: gfx-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.