Closed Bug 1238558 Opened 4 years ago Closed 4 years ago

BMP: [@mozilla::image::imgFrame::ImageUpdated]

Categories

(Core :: ImageLib, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 + fixed
firefox46 + fixed
firefox47 + fixed
firefox-esr38 --- unaffected
firefox-esr45 --- fixed

People

(Reporter: posidron, Assigned: njn)

Details

(4 keywords, Whiteboard: [gfx-noted][adv-main45+][post-critsmash-triage])

Crash Data

Attachments

(3 files, 5 obsolete files)

Testcase might not work, looks related to Threads only.

The following testcase crashes on en-us.linux-x86_64-asan.tar.bz2 revision e55a86dcfdc632d4975a5cebddc8d6f83ab3d28d

See attachment.

Backtrace:

==24096==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000008 (pc 0x7f9a7b572866 sp 0x7f9a5bb8bf10 bp 0x7f9a5bb8bf30 T23)
    #0 0x7f9a7b572865 in Lock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:69
    #1 0x7f9a7b572865 in Lock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Monitor.h:35
    #2 0x7f9a7b572865 in MonitorAutoLock /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Monitor.h:78
    #3 0x7f9a7b572865 in mozilla::image::imgFrame::ImageUpdated(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/imgFrame.cpp:613
    #4 0x7f9a7b53677e in mozilla::image::Decoder::PostInvalidation(mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:474
    #5 0x7f9a7b586935 in mozilla::image::nsBMPDecoder::FinishRow() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:420
    #6 0x7f9a7b586262 in mozilla::image::nsBMPDecoder::FinishInternal() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:243
    #7 0x7f9a7b533ec0 in mozilla::image::Decoder::CompleteDecode() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:196
    #8 0x7f9a7b532aa8 in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:122
    #9 0x7f9a7b532392 in mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:455
    #10 0x7f9a7b55164c in mozilla::image::DecodePoolWorker::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:281
    #11 0x7f9a793f57a4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:989
    #12 0x7f9a7946f39a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #13 0x7f9a79da047f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #14 0x7f9a79d0c9fc in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #15 0x7f9a79d0c9fc in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #16 0x7f9a79d0c9fc in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #17 0x7f9a793f12f0 in nsThread::ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:401
    #18 0x7f9a8702b4b5 in _pt_root /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #19 0x7f9a8766a181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/mozilla/Mutex.h:69 Lock
Thread T23 (ImgDecoder #4) created by T0 (Web Content) here:
    #0 0x461945 in pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0x7f9a87027e3d in _PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:453
    #2 0x7f9a870279ba in PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:544
    #3 0x7f9a793f2a2d in nsThread::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:521
    #4 0x7f9a793f921e in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThreadManager.cpp:249
    #5 0x7f9a7946e428 in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:71
    #6 0x7f9a7b530ef8 in mozilla::image::DecodePool::DecodePool() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:357
    #7 0x7f9a7b53073b in mozilla::image::DecodePool::Singleton() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:314
    #8 0x7f9a7b582e08 in mozilla::image::InitModule() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/build/nsImageModule.cpp:95
    #9 0x7f9a793c6c2d in Load /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:898
    #10 0x7f9a793c6c2d in nsFactoryEntry::GetFactory() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1934
    #11 0x7f9a793c81e7 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1232
    #12 0x7f9a793bf0d4 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1591
    #13 0x7f9a7945f351 in CallGetService /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:67
    #14 0x7f9a7945f351 in nsGetServiceByContractID::operator()(nsID const&, void**) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:280
    #15 0x7f9a79454426 in nsCOMPtr_base::assign_from_gs_contractid(nsGetServiceByContractID, nsID const&) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsCOMPtr.cpp:103
    #16 0x7f9a7b3b8771 in nsCOMPtr /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsCOMPtr.h:540
    #17 0x7f9a7b3b8771 in gfxPlatform::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:627
    #18 0x7f9a7b3b6b04 in gfxPlatform::GetPlatform() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:460
    #19 0x7f9a7e984183 in mozilla::dom::ContentProcess::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/ipc/ContentProcess.cpp:83
    #20 0x7f9a80f04da0 in XRE_InitChildProcess /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:603
    #21 0x48d760 in content_process_main(int, char**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
    #22 0x7f9a76cd6ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
Attached file Testcase (obsolete) —
Attached file test_case.bmp.zip (obsolete) —
I can consistently reproduce the issue with this test case.
Is this a dup of bug 1133133?
After bug 1238551 we just mark this bmp as invalid and don't try to decode it. I could reproduce the crash before bug 1238551, but not after. So let's call this a dupe.
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1238551
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
This also triggers this assertion on debug builds:
Assertion failure: mInFrame (Can't invalidate when not mid-frame!), at /builds/slave/m-cen-l64-asan-d-0000000000000/build/src/image/Decoder.cpp:467
OS: Mac OS X → All
Hardware: x86_64 → All
This one is similar to bug 1238551. The BMP is truncated. Not all the required header information is present, so we don't fully initialize everything about the image. We eventually end up in nsBMPDecoder::FinishInternal() (for the reason described in bug 1238551 comment 24) where the lack of full initialization bites us.

tnikkel, I don't have a good sense of what the preconditions for calling FinishInternal() are. I could move the rest of it -- the calls to PostInvalidation(), PostFrameStop(), PostDecodeDone() -- into the |if (mImageData)| block, but should we even call FinishInternal() for these kinds of truncated images? It looks like we can't do anything useful in that case. I can't remember if you filed a bug about handling this case better in StreamingLexer.

The crash is caused by |this| being null in imgFrame::ImageUpdated() and then accessing |this->mMonitor|. So it's a near-nullptr crash, which means it might not qualify as being critical.
As per comment 7, I don't recall whether you were working on a general fix for
this issue in StreamingLexer. If not, this does the trick.
Attachment #8711534 - Flags: review?(tnikkel)
Assignee: nobody → n.nethercote
Status: REOPENED → ASSIGNED
Group: core-security
I didn't put any more time into the StreamingLexer issue, but perhaps we should fix that, and mark these types of files as corrupt, instead of trying to proceed as if they are okay.

So, perhaps we should add a virtual "BeforeFinishInternal" to Decoder, and decoders that use StreamingLexer (or any that would find it useful) can flag themselves as having an error in BeforeFinishInternal if the lexer state is not an acceptable state to finish on (any non-terminal state?). Seem reasonable?
Sure.
Keywords: sec-high
Group: core-security → gfx-core-security
Here's my attempt at the BeforeFinishInternal() idea. Currently I'm just
checking for mImageData, similar to before. The StreamingLexer state is SUCCESS
at this point, for this test case, so checking that doesn't seem to be useful.
Attachment #8712432 - Flags: feedback?(tnikkel)
(In reply to Nicholas Nethercote [:njn] from comment #11)
> Created attachment 8712432 [details] [diff] [review]
> Add Decoder::BeforeFinishInternal()
> 
> Here's my attempt at the BeforeFinishInternal() idea. Currently I'm just
> checking for mImageData, similar to before. The StreamingLexer state is
> SUCCESS
> at this point, for this test case, so checking that doesn't seem to be
> useful.

In nsBMPDecoder.cpp:

> nsBMPDecoder::BeforeFinishInternal()
> {
>   bool ok = true;
>   if (!IsMetadataDecode() && HasSize() && !mImageData) {
>     ok = false;
>   }
> 
>   if (!ok) {
>     PostDataError();
>   }
> }

How about this?
nsBMPDecoder::BeforeFinishInternal()
{
  if (!IsMetadataDecode() && HasSize() && !mImageData) {
    PostDataError();
  }
}
Sure. I originally was planning to add more conditions, so that structure would have made more sense, but it's not necessary in the end.
Attachment #8712468 - Flags: review?(tnikkel)
Attachment #8712432 - Attachment is obsolete: true
Attachment #8712432 - Flags: feedback?(tnikkel)
Attachment #8712469 - Flags: review?(tnikkel)
Attachment #8706376 - Attachment is obsolete: true
Attachment #8711534 - Attachment is obsolete: true
Attachment #8711534 - Flags: review?(tnikkel)
Attachment #8712469 - Flags: review?(tnikkel) → review+
Comment on attachment 8712468 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

> void
>+nsBMPDecoder::BeforeFinishInternal()
>+{
>+  if (!IsMetadataDecode() && HasSize() && !mImageData) {
>+    PostDataError();
>+  }

We only need (!IsMetadataDecode() && !mImageData) for this to be a data error I would think. (I realize this is the condition from FinishInternal.)
Attachment #8712468 - Flags: review?(tnikkel) → review+
Attachment #8712468 - Attachment is obsolete: true
Comment on attachment 8712951 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

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

I made tnikkel's suggested change. Carrying over r+.
Comment on attachment 8712951 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

> [Security approval request comment]
> How easily could an exploit be constructed based on the patch?
 
Not sure. It's a deterministic near-null crash.

> Do comments in the patch, the check-in comment, or tests included in the
> patch paint a bulls-eye on the security problem?

One comment mentions that the new function is used to handle truncation. More
of a hint than a bull's eye.

> Which older supported branches are affected by this flaw?

Aurora, Beta and Release are all affected. I haven't tried ESR38.

> If not all supported branches, which bug introduced the flaw?
> 
> Do you have backports for the affected branches? If not, how different, hard
> to create, and risky will they be?

The patch is simple, and backporting will be simple.

> How likely is this patch to cause regressions; how much testing does it need?

Unlikely. The patch is simple and we have a test.
Attachment #8712951 - Flags: sec-approval?
sec-approval+ for checkin on February 9.

Please nominate for Aurora and Beta as well so we can get it in on those branches immediately after trunk.

We should find out if it affects ESR38.
Whiteboard: [gfx-noted] → [gfx-noted][checkin on 2/9]
Attachment #8712951 - Flags: sec-approval? → sec-approval+
> We should find out if it affects ESR38.

I checked. No crash.
Comment on attachment 8712951 [details] [diff] [review]
(part 1) - Add Decoder::BeforeFinishInternal()

Approval Request Comment
[Feature/regressing bug #]: Not sure. Possibly bug 1062066.

[User impact if declined]: crashes on some BMP images.

[Describe test coverage new/current, TreeHerder]: there is a test, but it hasn't landed yet.

[Risks and why]: fairly low. Patch is simple, just adds some checks to the end of BMP image decoding.

[String/UUID change made/needed]: none.
Attachment #8712951 - Flags: approval-mozilla-beta?
Attachment #8712951 - Flags: approval-mozilla-aurora?
Attachment #8712951 - Flags: approval-mozilla-beta?
Attachment #8712951 - Flags: approval-mozilla-beta+
Attachment #8712951 - Flags: approval-mozilla-aurora?
Attachment #8712951 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c58a56444fc9
https://hg.mozilla.org/mozilla-central/rev/2bbc5b79d857
Status: ASSIGNED → RESOLVED
Closed: 4 years ago4 years ago
Resolution: --- → FIXED
Whiteboard: [gfx-noted][checkin on 2/9] → [gfx-noted]
Target Milestone: --- → mozilla47
has problems uplifting to aurora 

warning: conflicts while merging image/decoders/nsBMPDecoder.cpp! (edit, then use 'hg resolve --mark')                                                                                                    
warning: conflicts while merging image/decoders/nsBMPDecoder.h! (edit, then use 'hg resolve --mark') 

can you take a look, thanks!
Flags: needinfo?(n.nethercote)
> has problems uplifting to aurora 

I can apply the patch above to aurora and beta without problem using |hg qimport|.
Flags: needinfo?(n.nethercote)
Thank you for the uplifts, Tomcat.
Group: gfx-core-security → core-security-release
Keywords: regression
Whiteboard: [gfx-noted] → [gfx-noted][adv-main45+]
Whiteboard: [gfx-noted][adv-main45+] → [gfx-noted][adv-main45+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.