Closed
Bug 1224979
Opened 9 years ago
Closed 8 years ago
BMP: heap-overflow crash [@mozilla::image::Downscaler::CommitRow]
Categories
(Core :: Graphics: ImageLib, defect)
Tracking
()
People
(Reporter: posidron, Assigned: tnikkel)
References
Details
(4 keywords, Whiteboard: [adv-main45+])
Crash Data
Attachments
(3 files, 4 obsolete files)
19.52 KB,
text/plain
|
Details | |
617 bytes,
application/x-zip-compressed
|
Details | |
1.49 KB,
patch
|
eflores
:
review+
abillings
:
approval-mozilla-aurora+
abillings
:
approval-mozilla-beta+
abillings
:
approval-mozilla-esr38+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•9 years ago
|
||
Comment 2•9 years ago
|
||
Could this be a dup of bug 1216178?
Updated•9 years ago
|
Group: core-security → gfx-core-security
Comment 4•9 years ago
|
||
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)
Reporter | ||
Comment 5•9 years ago
|
||
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)
Reporter | ||
Comment 6•9 years ago
|
||
(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.
Reporter | ||
Comment 7•9 years ago
|
||
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.
Comment 8•9 years ago
|
||
> 2) Valgrind sucks.
Valgrind detects a superset of the problems that ASAN detects.
Comment 9•9 years ago
|
||
(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.
Reporter | ||
Comment 10•9 years ago
|
||
(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
Comment 11•9 years ago
|
||
> > 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.
Reporter | ||
Comment 12•9 years ago
|
||
(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.
Comment 13•9 years ago
|
||
(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/
Comment 14•9 years ago
|
||
(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?
I cannot reproduce this, but I suspect the patch on bug 1224200 will take care of it.
Assignee | ||
Comment 16•8 years ago
|
||
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?
Comment 17•8 years ago
|
||
(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.
Assignee | ||
Comment 18•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #17) > As I said in comment 4: Yep, I was just confirming your findings.
Comment 19•8 years ago
|
||
> Yep, I was just confirming your findings.
Great, thanks!
Assignee | ||
Comment 20•8 years ago
|
||
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.
Updated•8 years ago
|
Assignee: nobody → tnikkel
Comment 21•8 years ago
|
||
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.
Comment 22•8 years ago
|
||
Comment 23•8 years ago
|
||
Finally got a working test case
Attachment #8687753 -
Attachment is obsolete: true
Attachment #8703508 -
Attachment is obsolete: true
Comment 24•8 years ago
|
||
Including both the BMP and the HTML file. Open the HTML file to trigger the bug.
Attachment #8710275 -
Attachment is obsolete: true
Assignee | ||
Comment 25•8 years ago
|
||
(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.
Assignee | ||
Comment 26•8 years ago
|
||
I can reproduce on Linux (even without ASAN).
Assignee | ||
Comment 27•8 years ago
|
||
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.
Assignee | ||
Comment 28•8 years ago
|
||
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.
Assignee | ||
Comment 29•8 years ago
|
||
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)
Assignee | ||
Comment 30•8 years ago
|
||
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+
Assignee | ||
Comment 32•8 years ago
|
||
(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.
Assignee | ||
Comment 33•8 years ago
|
||
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 34•8 years ago
|
||
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+
Updated•8 years ago
|
status-firefox44:
--- → wontfix
status-firefox45:
--- → affected
status-firefox46:
--- → affected
status-firefox47:
--- → affected
status-firefox-esr38:
--- → ?
status-firefox-esr45:
--- → affected
tracking-firefox45:
--- → +
tracking-firefox46:
--- → +
tracking-firefox47:
--- → +
Assignee | ||
Comment 35•8 years ago
|
||
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?
Assignee | ||
Comment 36•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae73aa4cc482
Updated•8 years ago
|
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+
Assignee | ||
Comment 37•8 years ago
|
||
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: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Updated•8 years ago
|
Group: gfx-core-security → core-security-release
Comment 38•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ca1f6d742f7f https://hg.mozilla.org/releases/mozilla-beta/rev/7f69f3e51a3c
Updated•8 years ago
|
Flags: qe-verify+
Comment 40•8 years ago
|
||
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)
Assignee | ||
Comment 41•8 years ago
|
||
(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)
Comment 42•8 years ago
|
||
Thank you! Marking as verified based on previous comments.
Status: RESOLVED → VERIFIED
Updated•8 years ago
|
Whiteboard: [adv-main45+]
Updated•8 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•