Closed Bug 1434490 Opened 6 years ago Closed 6 years ago

Write and read beyond bounds in nsPNGEncoder::WriteCallback()

Categories

(Core :: Graphics: ImageLib, defect, P5)

57 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox58 --- unaffected
firefox59 --- unaffected
firefox60 --- unaffected
firefox64 --- fixed

People

(Reporter: mozillabugs, Assigned: tnikkel)

References

(Blocks 1 open bug)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main64+] gfx-noted)

Attachments

(2 files)

Attached file bug_1036_poc_4.htm
nsPNGEncoder::WriteCallback() (image\encoders\png\nsPNGEncoder.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 beyond the end of its output buffer. WriteCallback() also unsafely assumes that doubling the existing buffer size always provides sufficient room to copy the input data. WriteCallback() also contains an addition that is overflow-safe only by coincidence.

The first bug is on line 712, which doesn't check for overflow. The second bug is that line 712 isn't followed by code to determine whether |that->mImageBufferSize >= that->mImageBufferUsed + size| [1]. The third bug is that the addition on line 706 is only coincidentally safe. [2]

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

697: void // static
698: nsPNGEncoder::WriteCallback(png_structp png, png_bytep data,
699:                             png_size_t size)
700: {
701:   nsPNGEncoder* that = static_cast<nsPNGEncoder*>(png_get_io_ptr(png));
702:   if (!that->mImageBuffer) {
703:     return;
704:   }
705: 
706:   if (that->mImageBufferUsed + size > that->mImageBufferSize) {
707:     // When we're reallocing the buffer we need to take the lock to ensure
708:     // that nobody is trying to read from the buffer we are destroying
709:     ReentrantMonitorAutoEnter autoEnter(that->mReentrantMonitor);
710: 
711:     // expand buffer, just double each time
712:     that->mImageBufferSize *= 2;
713:     uint8_t* newBuf = (uint8_t*)realloc(that->mImageBuffer,
714:                                         that->mImageBufferSize);
715:     if (!newBuf) {
716:       // can't resize, just zero (this will keep us from writing more)
717:       free(that->mImageBuffer);
718:       that->mImageBuffer = nullptr;
719:       that->mImageBufferSize = 0;
720:       that->mImageBufferUsed = 0;
721:       return;
722:     }
723:     that->mImageBuffer = newBuf;
724:   }
725:   memcpy(&that->mImageBuffer[that->mImageBufferUsed], data, size);
726:   that->mImageBufferUsed += size;
727:   that->NotifyListener();
}

Use the POC by starting FF x64 on a machine with >= 16 GB of memory. Disable D2D (if on Windows) by setting gfx.direct2d.disable to |true|. Enable large-surface support (see 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 712, above. Load the POC and wait for the BP at which the value of |that->mImageBufferSize| is 0x80000000. Now step line 712, observe the overflow, and step through line 725, which copies compressed image data [3] at a huge offset from the beginning of the 0-length "buffer" that lines 713-14 just allocated.



[1] At present, this doubling is coincidentally safe because the maximum |size| passed into WriteCallback() is determined by |PNG_ZBUF_SIZE| in media/libpng/pnglibconf.h , which happens to be 8192, which is the same size as the initial size of |mImageBuffer| that is set by nsPNGEncoder::StartImageEncode(). If libpng were changed to use a larger buffer size, say 10k, the first doubling (from 8k to 16k) would leave only 8k of allocated space to contain potentially as much as 10k of data, causing a write beyond bounds. Since libpng is published by a non-Mozilla provider, and nothing in nsPNGEncoder appears to check |PNG_ZBUF_SIZE|, this bug could occur. This bug would be worse than the overflow, since it could be invoked with a much-smaller image.

[2] Because |size| isa |size_t|, C++ promotes |that->mImageBufferUsed| to a size_t before adding it to |size|.

[3] The POC generates random data that is generally uncompressible, and thus causes libpng to generate a compressed data stream that is sufficiently-long to cause the overflow.
Flags: sec-bounty?
In addition, if the browser session survives long enough after the write beyond bounds for the JS line

            var img = e.toBlob(function (b) { }, "image/png");

to completely execute, |img| will contain data read from beyond bounds. since it comes from |that->mImageBuffer|.
Summary: Write beyond bounds in nsPNGEncoder::WriteCallback() → Write and read beyond bounds in nsPNGEncoder::WriteCallback()
(In reply to mozillabugs from comment #1)
> In addition, if the browser session survives long enough after the write
> beyond bounds for the JS line
> 
>             var img = e.toBlob(function (b) { }, "image/png");
> 
> to completely execute, |img| will contain data read from beyond bounds.
> since it comes from |that->mImageBuffer|.

The amount of data that could be copied is slightly > 2GB.
If we remove the limits as proposed in bug 1282074 we will have to fix integer overflows in lots of places. That was one of the reasons we imposed the 32K pixel limit, because we kept overflowing on stride calculations.

Going to make this public, though, because most of the people who will have to do that work need to know about the problem.
Blocks: 1282074
Group: core-security → gfx-core-security
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(nical.bugzilla)
Keywords: sec-moderate
Group: gfx-core-security
Thanks for the heads up. I doubt we'll have resources to put on removing this limit in the foreseeable future so we should be safe for a while.
Flags: needinfo?(nical.bugzilla)
Since this bug is public, would it be OK to put a warning in https://bugzilla.mozilla.org/show_bug.cgi?id=1282074 for people who currently are using that workaround to get large-canvas support?
(In reply to mozillabugs from comment #5)
> Since this bug is public, would it be OK to put a warning in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1282074 for people who
> currently are using that workaround to get large-canvas support?

yes, please.
Flags: sec-bounty? → sec-bounty+
Priority: -- → P5
Whiteboard: gfx-noted
Component: Graphics → ImageLib
Assignee: nobody → tnikkel
Attachment #9008907 - Flags: review?(aosmond)
Attachment #9008907 - Flags: review?(aosmond) → review+
Pushed by tnikkel@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0236f549f73f
Avoid overflow in nsPNGEncoder::WriteCallback. r=aosmond
https://hg.mozilla.org/mozilla-central/rev/0236f549f73f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Whiteboard: gfx-noted → [adv-main64+] gfx-noted
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: