Closed Bug 1210745 Opened 9 years ago Closed 8 years ago

Specially crafted gif triggers an assertion in Firefox 44.0a1

Categories

(Core :: Graphics: ImageLib, defect)

43 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: gustavo.grieco, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

Attached image DoS.gif
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150918100310

Steps to reproduce:

Opening a specially crafted gif file results in a crashed tab in Firefox 44.0a1 (2015-10-01). Find attached a small test case to reproduce it.


Actual results:

Before the crash, this message is printed in the console:

Assertion failure: aProgress & FLAG_SIZE_AVAILABLE, at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/ProgressTracker.cpp:50

Here you can see the stack trace of the crashed:
#0  0x00007f59d1178f3d in nanosleep () at ../sysdeps/unix/syscall-template.S:81
#1  0x00007f59d1178dd4 in __sleep (seconds=0) at ../sysdeps/unix/sysv/linux/sleep.c:137
#2  0x00007f59d9ff482f in ah_crap_handler (signum=11) at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/toolkit/xre/nsSigHandlers.cpp:103
#3  0x00007f59d9ff4a5f in child_ah_crap_handler (signum=11)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/toolkit/xre/nsSigHandlers.cpp:115
#4  <signal handler called>
#5  mozilla::image::CheckProgressConsistency (aProgress=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/ProgressTracker.cpp:50
#6  0x00007f59d5cba650 in mozilla::image::ProgressTracker::SyncNotifyProgress (this=0x6080000eb520, aProgress=<optimized out>, aInvalidRect=...)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/ProgressTracker.cpp:386
#7  0x00007f59d5cc48cb in mozilla::image::RasterImage::NotifyProgress (this=<optimized out>, aProgress=<optimized out>, aInvalidRect=..., 
    aSurfaceFlags=<optimized out>) at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/RasterImage.cpp:1717
#8  0x00007f59d5ccdb50 in mozilla::image::RasterImage::FinalizeDecoder (this=<optimized out>, aDecoder=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/RasterImage.cpp:1761
#9  0x00007f59d5d22fad in mozilla::image::NotifyDecodeCompleteWorker::Run (this=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/DecodePool.cpp:101
#10 0x00007f59d4233cef in nsThread::ProcessNextEvent (this=<optimized out>, aMayWait=<optimized out>, aResult=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/xpcom/threads/nsThread.cpp:960
#11 0x00007f59d42b4d5f in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=false)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:277
#12 0x00007f59d4a3ee4e in mozilla::ipc::MessagePump::Run (this=<optimized out>, aDelegate=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/glue/MessagePump.cpp:95
#13 0x00007f59d4a3fbb1 in mozilla::ipc::MessagePumpForChildProcess::Run (this=<optimized out>, aDelegate=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/glue/MessagePump.cpp:289
#14 0x00007f59d4998692 in MessageLoop::RunInternal (this=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
#15 0x00007f59d4998539 in MessageLoop::Run (this=0x7fffe837c7b0)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
#16 0x00007f59d898d2a7 in nsBaseAppShell::Run (this=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/widget/nsBaseAppShell.cpp:156
#17 0x00007f59d9fec077 in XRE_RunAppShell () at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:785
#18 0x00007f59d4a3fa49 in mozilla::ipc::MessagePumpForChildProcess::Run (this=<optimized out>, aDelegate=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/glue/MessagePump.cpp:259
#19 0x00007f59d4998692 in MessageLoop::RunInternal (this=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
#20 0x00007f59d4998539 in MessageLoop::Run (this=0x7fffe837c7b0)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
#21 0x00007f59d9feb461 in XRE_InitChildProcess (aArgc=<optimized out>, aArgv=<optimized out>, aGMPLoader=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:621
#22 0x000000000049add7 in content_process_main (argc=<optimized out>, argv=<optimized out>)
    at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
#23 0x00007f59d10d9ec5 in __libc_start_main (main=0x49b120 <main(int, char**)>, argc=9, argv=0x7fffe837cc68, init=<optimized out>, 
    fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffe837cc58) at libc-start.c:287
#24 0x000000000049a07d in _start ()


Expected results:

It shouldn't crash.
Group: firefox-core-security → core-security
Component: Untriaged → ImageLib
Flags: needinfo?(seth)
Product: Firefox → Core
I'm taking a look at this now.
Flags: needinfo?(seth)
There's no crash here, just an assertion failure.
Group: core-security
Summary: DoS with a specially crafted gif in Firefox 44.0a1 → Specially crafted gif triggers an assertion in Firefox 44.0a1
Here's the assertion in question:

https://dxr.mozilla.org/mozilla-central/source/image/ProgressTracker.cpp?case=true&from=ProgressTracker.cpp#50

This checks that we fire FLAG_IS_ANIMATED either at the same time or after FLAG_SIZE_AVAILABLE. Since bug 1194059, that's not a correction assertion; we detect FLAG_IS_ANIMATED during the metadata decode, and it may happen that we fire it before we fire FLAG_SIZE_AVAILABLE. (Indeed, we may then never fire FLAG_SIZE_AVAILABLE if the image is corrupt, as happens in this case.)

This would be caught by a version of the multichunk tests in image/test/gtest/TestDecoders.cpp that went through RasterImage instead of just dealing with the decoders directly. We should add tests like that. (But not in this bug.)

Instead what I'll do here is just update these ProgressTracker assertions to match the current behavior.
Blocks: 1194059
Here's the patch. I went ahead and just updated all the preconditions to match
current behavior. All tests pass locally.

In order to write an accurate assertion for FLAG_HAS_TRANSPARENCY, I had to pass
the old progress value into CheckProgressConsistency() as well.
Attachment #8669227 - Flags: review?(tnikkel)
Assignee: nobody → seth
Now with crashtest.
Attachment #8669237 - Flags: review?(tnikkel)
Attachment #8669227 - Attachment is obsolete: true
Attachment #8669227 - Flags: review?(tnikkel)
Blocks: 1210116
Depends on: 1214054
Depends on: 1214055
The failures in the try job revealed two bugs (which is why we have assertions, after all). Bug 1214054 and bug 1214055 take care of those problems. There may be more, since the entire test suite didn't get to run because of the assertion failures. The try job in comment 7 will tell us if everything has been fixed or not.
Attachment #8669237 - Flags: review?(tnikkel) → review+
I had found some different failed assertions fuzzing gifs in this firefox 44.0a1, in particular:

* Assertion failure: mMapCount == 0, at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/2d/SourceSurfaceRawData.h:88

* Assertion failure: nsIntRect(0, 0, aOriginalSize.width, aOriginalSize.height) .Contains(mFrameRect) (Frame rect must fit inside image), at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Downscaler.cpp:81

Should i report these in different bugs or wait a little to re-test?
ni for comment 9.

Is this patch ready to land Seth?
Flags: needinfo?(seth)
ni?(In reply to Timothy Nikkel (:tn) from comment #10)
> ni for comment 9.
> 

ni?
ni is short for needinfo. It's a way to flag someone that a question was asked of them in a bug so they get notified. So I was asking Seth if his patch was ready to land.
Unfortunately one of the dependent patches is not green on try. We'll get this in soon.
Flags: needinfo?(seth)
confirming and noting that I do see this in bughunter crash automation in the wild on beta/43, aurora/44, nightly/45 on all platforms.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Version: 44 Branch → 43 Branch
(In reply to Gustavo Grieco from comment #9)
> I had found some different failed assertions fuzzing gifs in this firefox
> 44.0a1, in particular:
> 
> * Assertion failure: mMapCount == 0, at
> /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/2d/
> SourceSurfaceRawData.h:88
> 
> * Assertion failure: nsIntRect(0, 0, aOriginalSize.width,
> aOriginalSize.height) .Contains(mFrameRect) (Frame rect must fit inside
> image), at
> /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Downscaler.cpp:
> 81
> 
> Should i report these in different bugs or wait a little to re-test?

These are both likely different bugs. Can you please file new bugs (with testcases if you have) and cc me? Thanks!
Flags: needinfo?(gustavo.grieco)
There is one issue in that push. We hit this assert in some pngs:

+  if (aNewProgress & FLAG_HAS_TRANSPARENCY) {
+    // We should never discover this after FLAG_SIZE_AVAILABLE except in the
+    // case of animated images, which may have an opaque first frame but
+    // transparent frames later on in the animation.
+    MOZ_ASSERT((aNewProgress & FLAG_IS_ANIMATED) ||
+               (aOldProgress & FLAG_HAS_TRANSPARENCY) ||
+               !(aOldProgress & FLAG_SIZE_AVAILABLE));
   }

Which says that we should have size available before transparency is posted (unless the image is animated). However the png decoder trivially violates this. It posts the size almost immediately. Then later it checks the transparency.

I think the intent of this assert is that transparency is posted during metadata decodes (unless the image is animated). Metadata decodes used to be called size decodes, so I think the assert is trying to use "size has been posted" as "metadata decode is complete".

Not sure there is anything we can assert in it's place.
If you have a better idea of what we can assert I'm happy to change it.

We could also change the png decoder to hold the size once it has it, and only post it after posting transparency (or not).
Attachment #8727006 - Flags: review?(seth)
(In reply to Timothy Nikkel (:tnikkel) from comment #15)
> (In reply to Gustavo Grieco from comment #9)
> > I had found some different failed assertions fuzzing gifs in this firefox
> > 44.0a1, in particular:
> > 
> > * Assertion failure: mMapCount == 0, at
> > /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/gfx/2d/
> > SourceSurfaceRawData.h:88
> > 
> > * Assertion failure: nsIntRect(0, 0, aOriginalSize.width,
> > aOriginalSize.height) .Contains(mFrameRect) (Frame rect must fit inside
> > image), at
> > /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Downscaler.cpp:
> > 81
> > 
> > Should i report these in different bugs or wait a little to re-test?
> 
> These are both likely different bugs. Can you please file new bugs (with
> testcases if you have) and cc me? Thanks!


I will be able to re-test (and probably create new bug reports) next week.
Flags: needinfo?(gustavo.grieco)
(In reply to Timothy Nikkel (:tnikkel) from comment #19)
> Created attachment 8727006 [details] [diff] [review]
> get rid of transparency assert
> 
> If you have a better idea of what we can assert I'm happy to change it.
> 
> We could also change the png decoder to hold the size once it has it, and
> only post it after posting transparency (or not).

Hmm. I don't love this, because it leaves us with no way to assert that we find transparency during the metadata decode. (Which is what we want to assert, as you mention.)

Let me file a followup about adding a way to do that.
Blocks: 1254892
Comment on attachment 8727006 [details] [diff] [review]
get rid of transparency assert

Review of attachment 8727006 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but please add an XXX comment that we'll add an assertion that transparency is only set during the metadata decode once bug 1254892 is fixed.
Attachment #8727006 - Flags: review?(seth) → review+
https://hg.mozilla.org/mozilla-central/rev/d1baa3d6f78b
https://hg.mozilla.org/mozilla-central/rev/699c1d51d6f3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.