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)
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)
1.44 KB,
text/html
|
Details | |
4.32 KB,
patch
|
aosmond
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•6 years ago
|
Flags: sec-bounty?
Reporter | ||
Comment 1•6 years ago
|
||
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()
Reporter | ||
Comment 2•6 years ago
|
||
(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.
Comment 3•6 years ago
|
||
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
Updated•6 years ago
|
Group: gfx-core-security
Comment 4•6 years ago
|
||
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)
Reporter | ||
Comment 5•6 years ago
|
||
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?
Comment 6•6 years ago
|
||
(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.
status-firefox58:
--- → unaffected
status-firefox59:
--- → unaffected
status-firefox60:
--- → unaffected
status-firefox-esr52:
--- → unaffected
Flags: sec-bounty? → sec-bounty+
Updated•6 years ago
|
Priority: -- → P5
Whiteboard: gfx-noted
Updated•6 years ago
|
Component: Graphics → ImageLib
Assignee | ||
Comment 7•6 years ago
|
||
Assignee: nobody → tnikkel
Attachment #9008907 -
Flags: review?(aosmond)
Updated•6 years ago
|
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
Comment 9•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0236f549f73f
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•5 years ago
|
Whiteboard: gfx-noted → [adv-main64+] gfx-noted
Updated•5 years ago
|
status-firefox-esr60:
--- → unaffected
You need to log in
before you can comment on or make changes to this bug.
Description
•