BMP: crash [@SetPixel]

RESOLVED FIXED in Firefox 45, Firefox OS master

Status

()

Core
ImageLib
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: posidron, Assigned: njn)

Tracking

(4 keywords)

Trunk
mozilla46
x86_64
Mac OS X
crash, regression, sec-critical, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox44 unaffected, firefox45+ fixed, firefox46+ fixed, firefox-esr38 unaffected, firefox-esr45 fixed, b2g-v2.0 unaffected, b2g-v2.0M unaffected, b2g-v2.1 unaffected, b2g-v2.1S unaffected, b2g-v2.2 unaffected, b2g-v2.5 unaffected, b2g-v2.2r unaffected, b2g-master fixed)

Details

(crash signature)

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

2 years ago
This is not a top-crasher and occurred the first time today. 

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

See attachment.

Backtrace:

==9256==ERROR: AddressSanitizer: SEGV on unknown address 0x000000069440 (pc 0x7f34ec91a1c4 sp 0x7f34ccedc180 bp 0x7f34ccedc2b0 T23)
ASAN:SIGSEGV
==9256==AddressSanitizer: while reporting a bug found another one.Ignoring.
    #0 0x7f34ec91a1c3 in SetPixel /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:136:17
    #1 0x7f34ec91a1c3 in mozilla::image::nsBMPDecoder::FinishInternal() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/decoders/nsBMPDecoder.cpp:239
    #2 0x7f34ec8c7ec0 in mozilla::image::Decoder::CompleteDecode() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:196
    #3 0x7f34ec8c6aa8 in mozilla::image::Decoder::Decode(mozilla::image::IResumable*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/Decoder.cpp:122
    #4 0x7f34ec8c6392 in mozilla::image::DecodePool::Decode(mozilla::image::Decoder*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:455
    #5 0x7f34ec8e564c in mozilla::image::DecodePoolWorker::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:281
    #6 0x7f34ea7897a4 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:989
    #7 0x7f34ea80339a in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:297
    #8 0x7f34eb13447f in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/glue/MessagePump.cpp:326
    #9 0x7f34eb0a09fc in RunInternal /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:234
    #10 0x7f34eb0a09fc in RunHandler /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:227
    #11 0x7f34eb0a09fc in MessageLoop::Run() /builds/slave/m-in-l64-asan-0000000000000000/build/src/ipc/chromium/src/base/message_loop.cc:201
    #12 0x7f34ea7852f0 in nsThread::ThreadFunc(void*) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:401
    #13 0x7f34f83bf4b5 in _pt_root /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:212
    #14 0x7f34f89fe181 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/image/decoders/nsBMPDecoder.cpp:136 SetPixel
Thread T23 (ImgDecoder #5) 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 0x7f34f83bbe3d in _PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:453
    #2 0x7f34f83bb9ba in PR_CreateThread /builds/slave/m-in-l64-asan-0000000000000000/build/src/nsprpub/pr/src/pthreads/ptthread.c:544
    #3 0x7f34ea786a2d in nsThread::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThread.cpp:521
    #4 0x7f34ea78d21e in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/threads/nsThreadManager.cpp:249
    #5 0x7f34ea802428 in NS_NewThread(nsIThread**, nsIRunnable*, unsigned int) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsThreadUtils.cpp:71
    #6 0x7f34ec8c4ef8 in mozilla::image::DecodePool::DecodePool() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:357
    #7 0x7f34ec8c473b in mozilla::image::DecodePool::Singleton() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/DecodePool.cpp:314
    #8 0x7f34ec916e08 in mozilla::image::InitModule() /builds/slave/m-in-l64-asan-0000000000000000/build/src/image/build/nsImageModule.cpp:95
    #9 0x7f34ea75ac2d in Load /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:898
    #10 0x7f34ea75ac2d in nsFactoryEntry::GetFactory() /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1934
    #11 0x7f34ea75c1e7 in nsComponentManagerImpl::CreateInstanceByContractID(char const*, nsISupports*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1232
    #12 0x7f34ea7530d4 in nsComponentManagerImpl::GetServiceByContractID(char const*, nsID const&, void**) /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/components/nsComponentManager.cpp:1591
    #13 0x7f34ea7f3351 in CallGetService /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:67
    #14 0x7f34ea7f3351 in nsGetServiceByContractID::operator()(nsID const&, void**) const /builds/slave/m-in-l64-asan-0000000000000000/build/src/xpcom/glue/nsComponentManagerUtils.cpp:280
    #15 0x7f34ea7e8426 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 0x7f34ec74c771 in nsCOMPtr /builds/slave/m-in-l64-asan-0000000000000000/build/src/obj-firefox/dist/include/nsCOMPtr.h:540
    #17 0x7f34ec74c771 in gfxPlatform::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:627
    #18 0x7f34ec74ab04 in gfxPlatform::GetPlatform() /builds/slave/m-in-l64-asan-0000000000000000/build/src/gfx/thebes/gfxPlatform.cpp:460
    #19 0x7f34efd18183 in mozilla::dom::ContentProcess::Init() /builds/slave/m-in-l64-asan-0000000000000000/build/src/dom/ipc/ContentProcess.cpp:83
    #20 0x7f34f2298da0 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 0x7f34e806aec4 (/lib/x86_64-linux-gnu/libc.so.6+0x21ec4)
(Reporter)

Comment 1

2 years ago
Created attachment 8706374 [details]
Testcase
(Reporter)

Updated

2 years ago
Severity: normal → critical
Group: core-security → gfx-core-security
I'm going to call this sec-critical because that's a not-very-near null dereference. Can it be an arbitrary value based on something in the testcase?
Keywords: sec-critical
Remind me how to use the attached testcase?  Other than unzipping, something has to be renamed, right?
Flags: needinfo?(cdiehl)
Edwin, just randomly assigned to you :)
Assignee: nobody → edwin
(Assignee)

Comment 5

2 years ago
(In reply to Milan Sreckovic [:milan] from comment #3)
> Remind me how to use the attached testcase?  Other than unzipping, something
> has to be renamed, right?

AIUI the data_1_output_Output.txt file is the BMP, and should be renamed with a .bmp extension.

I tried to reproduce this crash and failed. When I load the file it says "The image <file> cannot be displayed because it contains errors." I inspected the header and decoding aborts early because the BIH size field (bytes 14..17) has the value 1195376696, which is not a legal value. (The highest legal value for that field is 124.)

The code location given by the first stack trace in comment 0 corresponds to the end of decoding. So I don't understand how that code location could be reached with this input. I double-checked by adding a diagnostic printf at that code location. It didn't get triggered.

This is a similar outcome to the one in bug 1224979 comment 4, where I also couldn't reproduce a fuzzing fail with an obviously-malformed BMP file.
Flags: needinfo?(cdiehl)
cdiehl, do you have any other test cases on hand that reproduce this?

As njn said, the BMP in this test case is pretty corrupt and we don't get anywhere near decoding.
Flags: needinfo?(cdiehl)
status-firefox46: --- → affected
tracking-firefox46: --- → +
Created attachment 8707566 [details]
proper testcase

I hand edited the file while tracing through the bmp decoder, looking at the file, and the bmp file format to produce a valid bmp.

Continually shift-reloading it I can get my Firefox to use 6GB+ of memory, so there is some bad behaviour, I did not reproduce a crash though. This is the best I can make sense of the testcase.
(Reporter)

Comment 8

2 years ago
No, there are no other test-cases. I also have not seen another crash with the same signature since then.
Flags: needinfo?(cdiehl)
Created attachment 8708724 [details]
test_case.bmp

This test case seems to trigger the crash consistently for me.
When I try to download the testcase that Tyson just uploaded, or even the one that I uploaded I get a 0 byte file. Mine also seems to have been renamed to a png? Pretty sure I uploaded a .bmp file. I can download Christoph's attachment fine though.

Is there some bug in bugzilla?
Created attachment 8708753 [details]
proper testcase in zip
Attachment #8707566 - Attachment filename: a.png → a.bmp
Attachment #8707566 - Attachment mime type: image/png → image/bmp
Attachment #8708724 - Attachment filename: test_case.png → test_case.bmp
Attachment #8708724 - Attachment mime type: image/png → image/bmp
Tyson, maybe upload your testcase inside a zip, that seems to work.
(Assignee)

Comment 13

2 years ago
> Continually shift-reloading it I can get my Firefox to use 6GB+ of memory,
> so there is some bad behaviour, I did not reproduce a crash though. This is
> the best I can make sense of the testcase.

I can't reproduce this behaviour. (I used the version of the file from the "proper testcase in zip" attachment, which after extraction is 68 bytes and has a sha1sum of ac9deec2f2cd090a3c29e8da42d6ac77a81715da.)
(In reply to Nicholas Nethercote [:njn] from comment #13)
> > Continually shift-reloading it I can get my Firefox to use 6GB+ of memory,
> > so there is some bad behaviour, I did not reproduce a crash though. This is
> > the best I can make sense of the testcase.
> 
> I can't reproduce this behaviour. (I used the version of the file from the
> "proper testcase in zip" attachment, which after extraction is 68 bytes and
> has a sha1sum of ac9deec2f2cd090a3c29e8da42d6ac77a81715da.)

Those value check out. I only saw the 6gb memory in my own local debug build with a slightly out of date tree, it didn't seem to reproduce with official nightly builds. So I'll have to investigate that more.

Updated

2 years ago
Duplicate of this bug: 1239728
Created attachment 8708844 [details]
test_case.bmp.zip

(In reply to Timothy Nikkel (:tnikkel) from comment #12)
> Tyson, maybe upload your testcase inside a zip, that seems to work.

Strange. Give that a try.
Attachment #8708724 - Attachment is obsolete: true
Attachment #8707566 - Attachment is obsolete: true
I can reproduce with Tyson's testcase.
(Assignee)

Comment 18

2 years ago
I reproduced with Tyson's test case as well, and I've diagnosed the problem. Working on a fix now.
Assignee: edwin → n.nethercote
(Assignee)

Comment 19

2 years ago
Tyson's image is right on the cusp of valid and invalid, due to BMP being such a 
mess of a format. The BIH size is 56, which is unusual but arguably acceptable.
The Compression type is BITFIELDS which means there are 12 bytes of color mask
data after the BIH... or possibly *within* the BIH; it depends whether you
decide to accept an obscure variant of BMP that Photoshop reportedly produced
and MS documented for a brief time (see BITMAPV3INFOHEADER in
https://en.wikipedia.org/wiki/BMP_file_format).

This file looks like that obscure variant, so *within* is probably a better
interpretation, but the existing code chooses *after*. Because of that, to our
code it looks like the file is 12 bytes short. So decoding doesn't make it to
nsBMPDecoder::ReadBitfields() and we don't assign mImageData. But the image
also doesn't get marked as having an error, interestingly enough.

Then, in FinishInternal(), it looks like the file was truncated (because of
those 12 bytes) so we go to fill in the missing ones, but mImageData is
nullptr and we get a nearly-nullptr crash.

One part of the fix is to check for a null mImageData in FinishInternal(). That
protects against this and potentially similar sorts of problems. The other part
of the fix is to disallow this combination of header size and compression type,
due to the ambiguity. This outlaws the abovementioned obscure variant, but I'm
ok with that because it's so obscure. (bmpsuite has such a file in its
"questionable" section, and Chrome also doesn't show it.)
(Assignee)

Comment 20

2 years ago
Comment on attachment 8706374 [details]
Testcase

I'm going to obsolete cdiehl's test case given that (a) it doesn't seem to reproduce for anybody else, and (b) I've writing a fix for the problem exposed by Tyson's test case.
Attachment #8706374 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Attachment #8708753 - Attachment is obsolete: true
(Assignee)

Comment 21

2 years ago
Created attachment 8708880 [details] [diff] [review]
(part 1) - Reject BITMAPV3INFOHEADER BMP images
Attachment #8708880 - Flags: review?(tnikkel)
(Assignee)

Comment 22

2 years ago
Created attachment 8708882 [details] [diff] [review]
(part 2) - Add a test
Attachment #8708882 - Flags: review?(tnikkel)
(Assignee)

Comment 23

2 years ago
Bug 1236161 is open for the Bugzilla problem of 0 byte BMP attachment.
(In reply to Nicholas Nethercote [:njn] from comment #19)
> This file looks like that obscure variant, so *within* is probably a better
> interpretation, but the existing code chooses *after*. Because of that, to
> our
> code it looks like the file is 12 bytes short. So decoding doesn't make it to
> nsBMPDecoder::ReadBitfields() and we don't assign mImageData. But the image
> also doesn't get marked as having an error, interestingly enough.

Yeah, it seems StreamingLexer doesn't handle files that are shorter than expected very intelligently. It will sit in the final waiting state patiently awaiting more data that will never come until the SourceBufferIterator is marked as complete. No StreamingLexer transitions will happen at this point. Decoder::Decode will Finish the decode, and the decoder will be destroyed shortly thereafter when it's refcount reaches 0. I put a fatal assert that the state was a terminal state in the StreamingLexer destructor and pushed to try, this turned up several cases.
Attachment #8708880 - Flags: review?(tnikkel) → review+
Attachment #8708882 - Flags: review?(tnikkel) → review+
(In reply to Timothy Nikkel (:tnikkel) from comment #7)
> Created attachment 8707566 [details]
> proper testcase
> 
> I hand edited the file while tracing through the bmp decoder, looking at the
> file, and the bmp file format to produce a valid bmp.
> 
> Continually shift-reloading it I can get my Firefox to use 6GB+ of memory,
> so there is some bad behaviour, I did not reproduce a crash though. This is
> the best I can make sense of the testcase.

I made this bug 1240629.
(Assignee)

Comment 27

2 years ago
Comment on attachment 8708880 [details] [diff] [review]
(part 1) - Reject BITMAPV3INFOHEADER BMP images

Approval Request Comment

[Feature/regressing bug #]: bug 1217465.

[User impact if declined]: possible near-nullptr crashes.

[Describe test coverage new/current, TreeHerder]: there's a new reftest.

[Risks and why]: patch is extremely simple; risk is very low.

[String/UUID change made/needed]: none.
Attachment #8708880 - Flags: sec-approval?
Attachment #8708880 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 years ago
Attachment #8708880 - Flags: sec-approval?
(Assignee)

Comment 28

2 years ago
Comment on attachment 8708880 [details] [diff] [review]
(part 1) - Reject BITMAPV3INFOHEADER BMP images

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

It's a near-nullptr crash, so probably not easily.

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

No.

> Which older supported branches are affected by this flaw?

Aurora. I've requested approval to backport there.

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

Bug 1217465.

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

This patch applies cleanly to Aurora.

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

Very unlikely. The patch is extremely simple, only rejects malformed BMPs, and there is a test.
Attachment #8708880 - Flags: sec-approval?
(In reply to Nicholas Nethercote [:njn] from comment #28)
> Comment on attachment 8708880 [details] [diff] [review]
> (part 1) - Reject BITMAPV3INFOHEADER BMP images
> 
> > [Security approval request comment]
> > How easily could an exploit be constructed based on the patch?
> 
> It's a near-nullptr crash, so probably not easily.

Based on this is the rating in comment #2 accurate? The crashing address from the original log is 0x000000069440. Dan asked about control when setting the rating.
(Assignee)

Comment 30

2 years ago
> Based on this is the rating in comment #2 accurate? The crashing address
> from the original log is 0x000000069440. Dan asked about control when
> setting the rating.

As per comment 20 I've made this bug about the second testcase which demonstrated the error while the original testcase did not. Comment 19 has the details about how mImageData is null.
status-firefox45: --- → affected
https://hg.mozilla.org/mozilla-central/rev/cc10b0dfbb73
https://hg.mozilla.org/mozilla-central/rev/6f5872d14fa4

btw seems this landed without sec-approval or ?
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox46: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
status-firefox44: --- → unaffected
status-firefox-esr38: --- → unaffected
Keywords: regression
Did this even make the release candidate build for 44, which was built this weekend?

Nick, why was this checked in without sec-approval?
Flags: needinfo?(n.nethercote)
Oh, stupid question since it is 44 and 45. Going to approve now to get it on Aurora.

This still needed explicit sec-approval+ to land.
Attachment #8708880 - Flags: sec-approval?
Attachment #8708880 - Flags: sec-approval+
Attachment #8708880 - Flags: approval-mozilla-aurora?
Attachment #8708880 - Flags: approval-mozilla-aurora+
tracking-firefox45: --- → +
Keywords: checkin-needed
(Assignee)

Comment 34

2 years ago
> This still needed explicit sec-approval+ to land.

Argh, sorry.
Flags: needinfo?(n.nethercote)
status-b2g-v2.0: --- → unaffected
status-b2g-v2.0M: --- → unaffected
status-b2g-v2.1: --- → unaffected
status-b2g-v2.1S: --- → unaffected
status-b2g-v2.2: --- → unaffected
status-b2g-v2.2r: --- → unaffected
status-b2g-v2.5: --- → unaffected
status-b2g-master: --- → fixed
status-firefox-esr45: --- → fixed
Flags: in-testsuite+
Keywords: checkin-needed
Duplicate of this bug: 1238558
Group: gfx-core-security → core-security-release
Group: core-security-release
Blocks: 1217465
You need to log in before you can comment on or make changes to this bug.