BMP: heap-overflow crash [@mozilla::image::Downscaler::CommitRow]

VERIFIED FIXED in Firefox 45

Status

()

defect
--
critical
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: posidron, Assigned: tnikkel)

Tracking

(4 keywords)

Trunk
mozilla47
x86_64
macOS
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox44 wontfix, firefox45+ verified, firefox46+ verified, firefox47+ verified, firefox-esr38 fixed, firefox-esr45 fixed)

Details

(Whiteboard: [adv-main45+], crash signature)

Attachments

(3 attachments, 4 obsolete attachments)

The following testcase crashes on mozilla-inbound-linux64-asan revision df12438d4091e89409a5449b067867e57c24858b

See attachment.

Backtrace:

==1781==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60400042f980 at pc 0x7fa29190bfdc bp 0x7fa27539b660 sp 0x7fa27539b658
READ of size 8 at 0x60400042f980 thread T23 (ImgDecoder #4)
    #0 0x7fa29190bfdb in mozilla::image::Downscaler::CommitRow() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Downscaler.cpp:204
    #1 0x7fa291958582 in mozilla::image::nsBMPDecoder::FinishRow() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:325
    #2 0x7fa29195dd8c in mozilla::image::nsBMPDecoder::ReadPixelRow(char const*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:733
    #3 0x7fa291982198 in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0::operator()(mozilla::image::nsBMPDecoder::State, char const*, unsigned long) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:355
    #4 0x7fa291959a14 in Lex<<lambda at /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:346:33> > /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/StreamingLexer.h:288
    #5 0x7fa291959a14 in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:346
    #6 0x7fa291907ff2 in mozilla::image::Decoder::Write(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:183
    #7 0x7fa29196c984 in mozilla::image::nsICODecoder::WriteToContainedDecoder(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:738
    #8 0x7fa291980b8c in ReadBMP /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:506
    #9 0x7fa291980b8c in mozilla::image::nsICODecoder::WriteInternal(char const*, unsigned int)::$_1::operator()(mozilla::image::ICOState, char const*, unsigned long) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:706
    #10 0x7fa2919709e9 in Lex<<lambda at /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:689:16> > /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/StreamingLexer.h:297
    #11 0x7fa2919709e9 in mozilla::image::nsICODecoder::WriteInternal(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:688
    #12 0x7fa291907ff2 in mozilla::image::Decoder::Write(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:183
    #13 0x7fa2919062ac in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:128
    #14 0x7fa291905cd2 in mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:453
    #15 0x7fa2919248bc in mozilla::image::DecodePoolWorker::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:282
    #16 0x7fa28f946c34 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:964
    #17 0x7fa28f9c0f6a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #18 0x7fa29026d37f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #19 0x7fa2901da9cc in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #20 0x7fa2901da9cc in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #21 0x7fa2901da9cc in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #22 0x7fa28f942800 in nsThread::ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:376
    #23 0x7fa29cee24b5 in _pt_root /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #24 0x7fa29d521181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)

0x60400042f980 is located 0 bytes to the right of 48-byte region [0x60400042f950,0x60400042f980)
allocated by thread T23 (ImgDecoder #4) here:
    #0 0x4750b1 in __interceptor_malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:74
    #1 0x48dd5d in moz_xmalloc /builds/slave/m-in-l64-asan-0000000000000000/build/src/memory/mozalloc/mozalloc.cpp:83
    #2 0x7fa29190ca38 in operator new[] /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/image/../dist/include/mozilla/mozalloc.h:198
    #3 0x7fa29190ca38 in MakeUnique<unsigned char *[]> /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/image/../dist/include/mozilla/UniquePtr.h:642
    #4 0x7fa29190ca38 in mozilla::image::Downscaler::BeginFrame(mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> > const&, unsigned char*, bool, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Downscaler.cpp:118
    #5 0x7fa29195c1d8 in mozilla::image::nsBMPDecoder::ReadInfoHeaderRest(char const*, unsigned long) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:555
    #6 0x7fa291981c3e in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int)::$_0::operator()(mozilla::image::nsBMPDecoder::State, char const*, unsigned long) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:351
    #7 0x7fa291959a14 in Lex<<lambda at /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:346:33> > /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/StreamingLexer.h:288
    #8 0x7fa291959a14 in mozilla::image::nsBMPDecoder::WriteInternal(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:346
    #9 0x7fa291907ff2 in mozilla::image::Decoder::Write(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:183
    #10 0x7fa29196c984 in mozilla::image::nsICODecoder::WriteToContainedDecoder(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:738
    #11 0x7fa29196d7b6 in mozilla::image::nsICODecoder::ReadBIH(char const*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:472
    #12 0x7fa291980b79 in mozilla::image::nsICODecoder::WriteInternal(char const*, unsigned int)::$_1::operator()(mozilla::image::ICOState, char const*, unsigned long) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:704
    #13 0x7fa291970884 in Lex<<lambda at /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:689:16> > /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/StreamingLexer.h:288
    #14 0x7fa291970884 in mozilla::image::nsICODecoder::WriteInternal(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsICODecoder.cpp:688
    #15 0x7fa291907ff2 in mozilla::image::Decoder::Write(char const*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:183
    #16 0x7fa2919062ac in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:128
    #17 0x7fa291905cd2 in mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:453
    #18 0x7fa2919248bc in mozilla::image::DecodePoolWorker::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:282
    #19 0x7fa28f946c34 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:964
    #20 0x7fa28f9c0f6a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #21 0x7fa29026d37f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #22 0x7fa2901da9cc in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #23 0x7fa2901da9cc in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #24 0x7fa2901da9cc in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #25 0x7fa28f942800 in nsThread::ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:376
    #26 0x7fa29cee24b5 in _pt_root /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #27 0x7fa29d521181 in start_thread (/lib/x86_64-linux-gnu/libpthread.so.0+0x8181)

Thread T23 (ImgDecoder #4) created by T0 (Web Content) here:
    #0 0x461925 in pthread_create /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_interceptors.cc:175
    #1 0x7fa29cedee3d in _PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:453
    #2 0x7fa29cede9ba in PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:544
    #3 0x7fa28f943f3d in nsThread::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:496
    #4 0x7fa28f94a62e in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThreadManager.cpp:249
    #5 0x7fa28f9c01f8 in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:71
    #6 0x7fa291904831 in mozilla::image::DecodePool::DecodePool() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:355
    #7 0x7fa29190403b in mozilla::image::DecodePool::Singleton() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:315
    #8 0x7fa291955158 in mozilla::image::InitModule() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/build/nsImageModule.cpp:95
    #9 0x7fa28f9185cd in Load /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:882
    #10 0x7fa28f9185cd in nsFactoryEntry::GetFactory() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1913
    #11 0x7fa28f919aa5 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1216
    #12 0x7fa28f910c54 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1575
    #13 0x7fa28f9b0461 in CallGetService /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:67
    #14 0x7fa28f9b0461 in nsGetServiceByContractID::operator()(nsID const&, void**) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:280
    #15 0x7fa28f9a5086 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 0x7fa29179bd15 in nsCOMPtr /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/gfx/thebes/../../dist/include/nsCOMPtr.h:540
    #17 0x7fa29179bd15 in gfxPlatform::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:617
    #18 0x7fa29179a064 in gfxPlatform::GetPlatform() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:455
    #19 0x7fa294c7b343 in mozilla::dom::ContentProcess::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/ipc/ContentProcess.cpp:83
    #20 0x7fa29710d755 in XRE_InitChildProcess /builds/slave/m-in-l64-asan-0000000000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:603
    #21 0x48d740 in content_process_main(int, char**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:237
    #22 0x7fa28d2b1ec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)

SUMMARY: AddressSanitizer: heap-buffer-overflow /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Downscaler.cpp:204 mozilla::image::Downscaler::CommitRow()
Shadow bytes around the buggy address:
  0x0c088007dee0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c088007def0: fa fa fd fd fd fd fd fa fa fa fd fd fd fd fd fd
  0x0c088007df00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c088007df10: fa fa 00 00 00 00 00 fa fa fa fa fa fa fa fa fa
  0x0c088007df20: fa fa fa fa fa fa fa fa fa fa 00 00 00 00 00 00
=>0x0c088007df30:[fa]fa fd fd fd fd fd fd fa fa fa fa fa fa fa fa
  0x0c088007df40: fa fa fd fd fd fd fd fa fa fa fa fa fa fa fa fa
  0x0c088007df50: fa fa fa fa fa fa fa fa fa fa fd fd fd fd fd fa
  0x0c088007df60: fa fa fd fd fd fd fd fa fa fa fa fa fa fa fa fa
  0x0c088007df70: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c088007df80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Contiguous container OOB:fc
  ASan internal:           fe
Posted file Testcase (obsolete) —
Could this be a dup of bug 1216178?
Group: core-security → gfx-core-security
Let's let Seth figure out if it is a dupe.
Keywords: sec-high
I looked into this and I am now confused.

- The attachment is a .zip file, which I wasn't expecting. cdiehl told me on IRC to extract the file called data_1_output_Output.txt and rename it as a .ico file. Unexpected, but ok; I called it a.ico.

- I viewed it in Firefox and I get a 'The image file "a.ico" cannot be displayed because it contains errors'. Ok, so it's invalid, but maybe the buffer overflow happens before the decoder aborts.

- I did likewise in a build of Firefox running under Valgrind (which should detect the same buffer overflows) that ASAN does, but Valgrind did not complain.

- Looking at the file itself, the third byte is 0x14. In a valid ICO file the third byte must be 0x1 or 0x2, and nsICODecoder will abort decoding if that's not the case. I confirmed that this abort path is hit on this file.

So I have no idea how the stack trace in comment 0 could occur. Maybe I'm looking at the wrong file. Hints are welcome! :)
Flags: needinfo?(cdiehl)
1) Usually a .zip exists for getting extracted, inside is only one text file which contains binary content and this is a bitmap bug. =)

2) Valgrind sucks. Use a ASan build from: https://ftp.mozilla.org/pub/firefox/tinderbox-builds/mozilla-inbound-linux64-asan/latest/

The crash is repeating on our cluster, it's not a one-time crasher.
Flags: needinfo?(cdiehl)
(In reply to Al Billings [:abillings] from comment #3)
> Let's let Seth figure out if it is a dupe.

This can not be a dup, cause the other bug is fixed.
Okay, as it turns out we define "mozilla-inbound-linux64-asan/latest/" as in "builds from October 24th".  This might be indeed a duplicate of bug 1216178.
> 2) Valgrind sucks. 

Valgrind detects a superset of the problems that ASAN detects.
(In reply to Christoph Diehl [:cdiehl] from comment #7)
> Okay, as it turns out we define "mozilla-inbound-linux64-asan/latest/" as in
> "builds from October 24th".  This might be indeed a duplicate of bug 1216178.

ftp.mozilla.org isn't being updated any more. You should use http://archive.mozilla.org instead.
(In reply to Nicholas Nethercote [:njn] from comment #8)
> > 2) Valgrind sucks. 
> 
> Valgrind detects a superset of the problems that ASAN detects.

Not anymore, in the meantime other sanitizers evolved like T/Ub/L/M/ASan. Valgrind is simply too slow.

(In reply to Andrew McCreight [:mccr8] from comment #9)
> ftp.mozilla.org isn't being updated any more. You should use
> http://archive.mozilla.org instead.

It does not contain updated builds for mozilla-inbound-linux64-asan.

I have tested it on the cluster with:

http://hg.mozilla.org/integration/mozilla-inbound/rev/e7fa9a0b61f69e6f61d0e22298ddf3848620e997

and it still crashes reliably.


Use the following script to download a latest build (en-US.linux-x86_64-asan) from TaskCluster.

https://gist.github.com/posidron/48d7de90a91609d0aabb

Alternatively download the build here: https://tools.taskcluster.net/index/artifacts/#gecko.v2.mozilla-inbound.latest.firefox/gecko.v2.mozilla-inbound.latest.firefox.linux64-asan
> > Valgrind detects a superset of the problems that ASAN detects.
> 
> Not anymore, in the meantime other sanitizers evolved like T/Ub/L/M/ASan.
> Valgrind is simply too slow.

I'd be happy to hear of any examples of problems that ASAN detects that Valgrind doesn't.
(In reply to Nicholas Nethercote [:njn] from comment #4)

> - I did likewise in a build of Firefox running under Valgrind (which should
> detect the same buffer overflows) that ASAN does, but Valgrind did not
> complain.

:-)

I did not imply that ASan detects a superset of Valgrind but it is also not the other way around.
(In reply to Christoph Diehl [:cdiehl] from comment #10)
> It does not contain updated builds for mozilla-inbound-linux64-asan.

That's odd. I did find ASan builds at https://archive.mozilla.org/pub/firefox/nightly/2015/11/2015-11-20-03-02-27-mozilla-central/

I filed bug 1226696 for there being no ASan builds in https://archive.mozilla.org/pub/firefox/nightly/latest-mozilla-central/
(In reply to Nicholas Nethercote [:njn] from comment #11)
> > > Valgrind detects a superset of the problems that ASAN detects.
> > 
> > Not anymore, in the meantime other sanitizers evolved like T/Ub/L/M/ASan.
> > Valgrind is simply too slow.
> 
> I'd be happy to hear of any examples of problems that ASAN detects that
> Valgrind doesn't.

Could we do that in email instead of the bug?
See Also: → 1232989
I cannot reproduce this, but I suspect the patch on bug 1224200 will take care of it.
I too tried and failed to reproduce any problem here. I made three copies of the binary file in the zip, one with ico, icon, and bmp extensions. I tried loading each one directly in firefox. I created an html page that included each one in an img element, both at intrinsic size, and at 2x2 pixels. I no case did I ever get a Downscaler object created (based on the stacks this is necessary to see the problem). Is there some step we are all missing to try to reproduce this?
(In reply to Timothy Nikkel (:tn) from comment #16)
> I too tried and failed to reproduce any problem here. I made three copies of
> the binary file in the zip, one with ico, icon, and bmp extensions. I tried
> loading each one directly in firefox. I created an html page that included
> each one in an img element, both at intrinsic size, and at 2x2 pixels. I no
> case did I ever get a Downscaler object created (based on the stacks this is
> necessary to see the problem). Is there some step we are all missing to try
> to reproduce this?

As I said in comment 4:

> Looking at the file itself, the third byte is 0x14. In a valid ICO file the third byte must be 0x1 or
> 0x2, and nsICODecoder will abort decoding if that's not the case. I confirmed that this abort path is
> hit on this file.

So I can't see how this file could trigger this ASAN complaint.
(In reply to Nicholas Nethercote [:njn] from comment #17)
> As I said in comment 4:

Yep, I was just confirming your findings.
> Yep, I was just confirming your findings.

Great, thanks!
Posted file testcase (obsolete) —
I head edited the file from the testcase while stepping through the ico decoder to produce a file that imagelib does not reject as invalid. I had to edit two different bytes. The one byte that Nick mentioned, and one further byte.

The image then triggered this assertion

http://mxr.mozilla.org/mozilla-central/source/image/ProgressTracker.cpp?rev=48011a9ee633#53

but I suspect it the same root problem as bug 1210116 and/or bug 1210745, and not related to this bug. So I commented out that assertion in my local build and continued.

I then created an html file that included the ico file in two img elements. One at intrinsic size, one at 2px x 2px size (to trigger a downscaler). I then hit this assert

http://mxr.mozilla.org/mozilla-central/source/image/Downscaler.cpp?rev=e6be119a536c#68

which seems like it could lead to the reported problems of this bug.
Assignee: nobody → tnikkel
I saw this bug again last night with a nightly build from Jan 19. I have made the modification mentioned in bug 1232989 to the fuzzer but the test case doesn't reproduce the issue unfortunately.

I also ran the test case on a debug build and did not see any assertions.
Posted file test_case.zip (obsolete) —
Finally got a working test case
Attachment #8687753 - Attachment is obsolete: true
Attachment #8703508 - Attachment is obsolete: true
Posted file test_case.zip
Including both the BMP and the HTML file.
Open the HTML file to trigger the bug.
Attachment #8710275 - Attachment is obsolete: true
(In reply to Tyson Smith [:tsmith] from comment #24)
> Created attachment 8710279 [details]
> test_case.zip
> 
> Including both the BMP and the HTML file.
> Open the HTML file to trigger the bug.

I couldn't get a crash or assertion with this. It used a lot of memory though.
I can reproduce on Linux (even without ASAN).
Ah, the reason is didn't reproduce on mac is that I have a retina screen, so 2x2 becomes 4x4 pixels, and hence the downscale isn't quite as extreme, so doesn't trip the same problem.
The problem is this line

http://mxr.mozilla.org/mozilla-central/source/gfx/2d/image_operations.cpp?rev=a1bf9a99ec4b#106

src_support is about 3 billion, so when we convert the result of that float expressed to a signed 32 bit int it overflows and becomes negative, and thus we don't produce any filter values, and the Downscaler's mWindow (which is the size of largest filter) is zero sized.
Posted patch patch (obsolete) — Splinter Review
We're going to limit the maximum sized frame that the downscale can operate on in bug 1241728, so that should make this overflow impossible. But we should probably also protect ourselves against failure in ComputeFilters and not just rely on limiting the frame size to be safe. This patch will check the result of ComputeFilters to see if it fails, so the downscaler doesn't try to use the erroneous filter values.
Attachment #8712424 - Flags: review?(edwin)
Posted patch patch v2Splinter Review
Add more checks. Also check that we the expected number of filters.
Attachment #8712424 - Attachment is obsolete: true
Attachment #8712424 - Flags: review?(edwin)
Attachment #8712479 - Flags: review?(edwin)
Comment on attachment 8712479 [details] [diff] [review]
patch v2

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

::: image/Downscaler.cpp
@@ +100,5 @@
>                                 mXFilter.get());
>  
> +  if (mXFilter->max_filter() <= 0 || mXFilter->num_values() != mTargetSize.width) {
> +    return NS_ERROR_OUT_OF_MEMORY;
> +  }

Hm, I'm not sure this is the correct nsresult value for this, but I can't think of anything better. We should add an NS_WARNING, though.
Attachment #8712479 - Flags: review?(edwin) → review+
(In reply to Edwin Flores [:eflores] [:edwin] from comment #31)
> Hm, I'm not sure this is the correct nsresult value for this, but I can't
> think of anything better. We should add an NS_WARNING, though.

Yes, I looked and couldn't find anything better. With the patch for bug 1241728 we will avoid the int overflow failure mode of ComputeFilters, so that means that OOM is the only likely way for it to fail. So it is reasonable. I will add an NS_WARNING.
Comment on attachment 8712479 [details] [diff] [review]
patch v2

Note that the patch for bug 1241728 (non-security bug) likely make it impossible to trigger this bug (outside of an OOM). I'll be landing bug 1241728 shortly.



[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not that easily. It would point out that ComputeFilters can fail, and thus an attacker would study that function to find out how to make it fail. The int overflow is possible to deduce, but not obvious.

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

No, it takes some work to find out where the problem lies. I'm not planning on landing the testcase until the bug is opened.

Which older supported branches are affected by this flaw?

I didn't check specifically, but the code in question has been in the tree for quite a while, so I'm guessing all supported branches are affected.

If not all supported branches, which bug introduced the flaw?

I didn't spend any time to determine which specific bug, likely all supported branches as I said above.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backports would be trivial.

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

The patch makes us fail to display an image for conditions are true that will almost certainly lead us to a crash almost immediately (long before an image could be displayed). So regressions are very unlikely.
Attachment #8712479 - Flags: sec-approval?
Comment on attachment 8712479 [details] [diff] [review]
patch v2

sec-approval+ to go in following your other bug being checked in.

We should take this on Aurora and Beta as well (needs nominations from you for patches) and ESR38 if it is affected.
Attachment #8712479 - Flags: sec-approval? → sec-approval+
Comment on attachment 8712479 [details] [diff] [review]
patch v2

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: sec-issue
User impact if declined: crash, sec-issue
Fix Landed on Version: 47
Risk to taking this patch (and alternatives if risky): the patch adds checks for conditions that will almost certainly lead to a crash, and safely puts us into an unusable error state if so
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Approval Request Comment
[Feature/regressing bug #]: the code that makes this possible has existed for a while, I didn't put any time into determining what caused this specifically
[User impact if declined]: crash, sec-issue
[Describe test coverage new/current, TreeHerder]: none, land testcase when bug is open
[Risks and why]: the patch adds checks for conditions that will almost certainly lead to a crash, and safely puts us into an unusable error state if so
[String/UUID change made/needed]: none
Attachment #8712479 - Flags: approval-mozilla-esr38?
Attachment #8712479 - Flags: approval-mozilla-beta?
Attachment #8712479 - Flags: approval-mozilla-aurora?
Attachment #8712479 - Flags: approval-mozilla-esr38?
Attachment #8712479 - Flags: approval-mozilla-esr38+
Attachment #8712479 - Flags: approval-mozilla-beta?
Attachment #8712479 - Flags: approval-mozilla-beta+
Attachment #8712479 - Flags: approval-mozilla-aurora?
Attachment #8712479 - Flags: approval-mozilla-aurora+
This made it to m-c, not sure why it wasn't marked here.

https://hg.mozilla.org/mozilla-central/rev/ae73aa4cc482
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Group: gfx-core-security → core-security-release
Flags: qe-verify+
I could only reproduce this on Linux x64 asan builds.

I tried to verify the fix using the following builds: 
- http://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1456460484/ 26-Feb-2016 for Firefox 45 beta (looks like Nightly when opened)
- http://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-26-00-40-32-mozilla-aurora/	firefox-46.0a2.en-US.linux-x86_64-asan.tar.bz2 build for Aurora 46.0a2
- http://archive.mozilla.org/pub/firefox/nightly/2016/02/2016-02-25-03-02-09-mozilla-central/ firefox-47.0a1.en-US.linux-x86_64-asan.tar.bz2 build for Nightly 47.0a1 

In these builds, the crash doesn't occur anymore, but if the testcase is opened when closing the browser, the Beta and Aurora builds from above are freezing the computer for several minutes:

[Child 4266] WARNING: NS_ENSURE_TRUE(context) failed: file /builds/slave/m-aurora-l64-asan-d-ntly-00000/build/src/xpcom/threads/nsThread.cpp, line 800
]: [Parent 4188] WARNING: NS_ENSURE_TRUE(mReady) failed: file /builds/slave/m-aurora-l64-asan-d-ntly-00000/build/src/xpfe/appshell/nsWindowMediator.cpp, line 196
JavaScript error: resource:///modules/sessionstore/SessionStore.jsm, line 3579: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIWindowMediator.getEnumerator]
Hit MOZ_CRASH(Shutdown too long, probably frozen, causing a crash.) at /builds/slave/m-aurora-l64-asan-d-ntly-00000/build/src/toolkit/components/terminator/nsTerminator.cpp:149
#01: ???[/home/petrutarasa/Downloads/46Aasan/firefox/libnspr4.so +0x75c52]
#02: ???[/lib/x86_64-linux-gnu/libpthread.so.0 +0x76aa]
#03: clone[/lib/x86_64-linux-gnu/libc.so.6 +0x106eed]
#04: ??? (???:???)

Please let me know how I could help in further investigating this.
Flags: needinfo?(tnikkel)
(In reply to Petruta Rasa [QA] [:petruta] from comment #40)
> In these builds, the crash doesn't occur anymore, but if the testcase is
> opened when closing the browser, the Beta and Aurora builds from above are
> freezing the computer for several minutes:

That sounds right. Bug 1241728 should have fixed the freezing, but it only landed on nightly.
Flags: needinfo?(tnikkel)
Thank you! Marking as verified based on previous comments.
Whiteboard: [adv-main45+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.