Specially crafted gif triggers an assertion in Firefox 44.0a1

RESOLVED FIXED in Firefox 48

Status

()

Core
ImageLib
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: Gustavo Grieco, Assigned: seth)

Tracking

(Blocks: 1 bug)

43 Branch
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
Created attachment 8668885 [details]
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.

Updated

2 years ago
Group: firefox-core-security → core-security
Component: Untriaged → ImageLib
Flags: needinfo?(seth)
Product: Firefox → Core
(Assignee)

Comment 1

2 years ago
I'm taking a look at this now.
Flags: needinfo?(seth)
(Assignee)

Comment 2

2 years ago
There's no crash here, just an assertion failure.
Group: core-security
(Assignee)

Updated

2 years ago
Summary: DoS with a specially crafted gif in Firefox 44.0a1 → Specially crafted gif triggers an assertion in Firefox 44.0a1
(Assignee)

Comment 3

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1194059
(Assignee)

Comment 4

2 years ago
Created attachment 8669227 [details] [diff] [review]
Update CheckProgressConsistency() to match current ImageLib behavior.

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)

Updated

2 years ago
Assignee: nobody → seth
(Assignee)

Comment 5

2 years ago
Created attachment 8669237 [details] [diff] [review]
Update CheckProgressConsistency() to match current ImageLib behavior.

Now with crashtest.
Attachment #8669237 - Flags: review?(tnikkel)
(Assignee)

Updated

2 years ago
Attachment #8669227 - Attachment is obsolete: true
Attachment #8669227 - Flags: review?(tnikkel)
(Assignee)

Comment 6

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1fbb6bc6bbf1
(Assignee)

Updated

2 years ago
Blocks: 1210116
(Assignee)

Updated

2 years ago
Depends on: 1214054
(Assignee)

Updated

2 years ago
Depends on: 1214055
(Assignee)

Comment 7

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=db904139ad25
(Assignee)

Comment 8

2 years ago
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+
(Reporter)

Comment 9

2 years ago
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)
(Reporter)

Comment 11

2 years ago
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.
(Assignee)

Comment 13

2 years ago
Unfortunately one of the dependent patches is not green on try. We'll get this in soon.
Flags: needinfo?(seth)

Comment 14

2 years ago
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)
New try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97578abcfab7&group_state=expanded

Still some issues.
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.
With changing that assert we are green on try

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a96df769aad9
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).
Attachment #8727006 - Flags: review?(seth)
(Reporter)

Comment 20

a year ago
(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)
(Assignee)

Comment 21

a year ago
(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.
(Assignee)

Updated

a year ago
Blocks: 1254892
(Assignee)

Comment 22

a year ago
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+

Comment 23

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1baa3d6f78b
https://hg.mozilla.org/integration/mozilla-inbound/rev/699c1d51d6f3
Duplicate of this bug: 1210116

Comment 25

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d1baa3d6f78b
https://hg.mozilla.org/mozilla-central/rev/699c1d51d6f3
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.