969226 - (CVE-2014-1523) Heap-buffer-overflow in read_u32

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Abhishek Arya]

To reproduce, visit http://www.forumsfamilie.de/index.php

==7222==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200036d473 at pc 0x7fd89ea02b83 bp 0x7fd84df71950 sp 0x7fd84df71948
READ of size 4 at 0x60200036d473 thread T63 (ImageDecoder #2)
    #0 0x7fd89ea02b82 in read_u32 gfx/qcms/iccread.c:100
    #1 0x7fd89e9fe76a in qcms_profile_from_memory gfx/qcms/iccread.c:1023
    #2 0x7fd889bf8d4f in mozilla::image::GetICCProfile(jpeg_decompress_struct&) image/decoders/nsJPEGDecoder.cpp:68
    #3 0x7fd889bf4d44 in mozilla::image::nsJPEGDecoder::WriteInternal(char const*, unsigned int, mozilla::image::DecodeStrategy) image/decoders/nsJPEGDecoder.cpp:258
    #4 0x7fd889b0ab64 in mozilla::image::Decoder::Write(char const*, unsigned int, mozilla::image::DecodeStrategy) image/src/Decoder.cpp:107
    #5 0x7fd889ab904b in mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int, mozilla::image::DecodeStrategy) image/src/RasterImage.cpp:2148
    #6 0x7fd889acb74a in mozilla::image::RasterImage::DecodeSomeData(unsigned int, mozilla::image::DecodeStrategy) image/src/RasterImage.cpp:2779
    #7 0x7fd889adadb9 in mozilla::image::RasterImage::DecodePool::DecodeSomeOfImage(mozilla::image::RasterImage*, mozilla::image::DecodeStrategy, mozilla::image::RasterImage::DecodePool::DecodeType, unsigned int) image/src/RasterImage.cpp:3419
    #8 0x7fd889adc477 in mozilla::image::RasterImage::DecodePool::DecodeJob::Run() image/src/RasterImage.cpp:3285
    #9 0x7fd883c91475 in nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:208
    #10 0x7fd883c91fd4 in non-virtual thunk to nsThreadPool::Run() objdir-ff-asan-sym/xpcom/threads/Unified_cpp_xpcom_threads0.cpp:222
    #11 0x7fd883c7d718 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:643
    #12 0x7fd88368f442 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
    #13 0x7fd885ebeb0d in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:303
    #14 0x7fd885b823b7 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:226
    #15 0x7fd885b8200a in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:219
    #16 0x7fd885b81ee5 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:193
    #17 0x7fd883c731ec in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:258
    #18 0x7fd8aadb3242 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:205
    #19 0x43c240 in __asan::AsanThread::ThreadStart(unsigned long) _asan_rtl_
    #20 0x7fd8af67fe99 in start_thread /build/buildd/eglibc-2.15/nptl/pthread_create.c:308
    #21 0x7fd8ae78e3fc in
0x60200036d473 is located 2 bytes to the right of 1-byte region [0x60200036d470,0x60200036d471)
allocated by thread T63 (ImageDecoder #2) here:
    #0 0x434065 in malloc _asan_rtl_
    #1 0x7fd889bd819c in read_icc_profile image/decoders/iccjpeg.c:163
    #2 0x7fd889bf8cbf in mozilla::image::GetICCProfile(jpeg_decompress_struct&) image/decoders/nsJPEGDecoder.cpp:67
    #3 0x7fd889bf4d44 in mozilla::image::nsJPEGDecoder::WriteInternal(char const*, unsigned int, mozilla::image::DecodeStrategy) image/decoders/nsJPEGDecoder.cpp:258
    #4 0x7fd889b0ab64 in mozilla::image::Decoder::Write(char const*, unsigned int, mozilla::image::DecodeStrategy) image/src/Decoder.cpp:107
    #5 0x7fd889ab904b in mozilla::image::RasterImage::WriteToDecoder(char const*, unsigned int, mozilla::image::DecodeStrategy) image/src/RasterImage.cpp:2148
    #6 0x7fd889acb74a in mozilla::image::RasterImage::DecodeSomeData(unsigned int, mozilla::image::DecodeStrategy) image/src/RasterImage.cpp:2779
    #7 0x7fd889adadb9 in mozilla::image::RasterImage::DecodePool::DecodeSomeOfImage(mozilla::image::RasterImage*, mozilla::image::DecodeStrategy, mozilla::image::RasterImage::DecodePool::DecodeType, unsigned int) image/src/RasterImage.cpp:3419
    #8 0x7fd889adc477 in mozilla::image::RasterImage::DecodePool::DecodeJob::Run() image/src/RasterImage.cpp:3285
    #9 0x7fd883c91475 in nsThreadPool::Run() xpcom/threads/nsThreadPool.cpp:208
    #10 0x7fd883c91fd4 in non-virtual thunk to nsThreadPool::Run() objdir-ff-asan-sym/xpcom/threads/Unified_cpp_xpcom_threads0.cpp:222
    #11 0x7fd883c7d718 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:643
    #12 0x7fd88368f442 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
    #13 0x7fd885ebeb0d in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:303
    #14 0x7fd885b823b7 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:226
    #15 0x7fd885b8200a in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:219
    #16 0x7fd885b81ee5 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:193
    #17 0x7fd883c731ec in nsThread::ThreadFunc(void*) xpcom/threads/nsThread.cpp:258
    #18 0x7fd8aadb3242 in _pt_root nsprpub/pr/src/pthreads/ptthread.c:205
    #19 0x43c240 in __asan::AsanThread::ThreadStart(unsigned long) _asan_rtl_
Thread T63 (ImageDecoder #2) created by T0 here:
    #0 0x414f7e in __interceptor_pthread_create _asan_rtl_
    #1 0x7fd8aada4894 in _PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:445
    #2 0x7fd8aada2c6a in PR_CreateThread nsprpub/pr/src/pthreads/ptthread.c:528
    #3 0x7fd883c76951 in nsThread::Init() xpcom/threads/nsThread.cpp:329
    #4 0x7fd883c8995e in nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**) xpcom/threads/nsThreadManager.cpp:232
    #5 0x7fd883c8ea5f in nsThreadPool::PutEvent(nsIRunnable*) xpcom/threads/nsThreadPool.cpp:92
    #6 0x7fd883c92858 in nsThreadPool::Dispatch(nsIRunnable*, unsigned int) xpcom/threads/nsThreadPool.cpp:246
    #7 0x7fd883b86456 in nsInputStreamReadyEvent::OnInputStreamReady(nsIAsyncInputStream*) xpcom/io/nsStreamUtils.cpp:72
    #8 0x7fd883b8679e in non-virtual thunk to nsInputStreamReadyEvent::OnInputStreamReady(nsIAsyncInputStream*) objdir-ff-asan-sym/xpcom/io/Unified_cpp_xpcom_io1.cpp:79
    #9 0x7fd883b07456 in nsPipeEvents::~nsPipeEvents() xpcom/io/nsPipe3.cpp:592
    #10 0x7fd883b0fdf1 in nsPipeInputStream::AsyncWait(nsIInputStreamCallback*, unsigned int, unsigned int, nsIEventTarget*) xpcom/io/nsPipe3.cpp:854
    #11 0x7fd8842a78ae in nsInputStreamPump::EnsureWaiting() netwerk/base/src/nsInputStreamPump.cpp:142
    #12 0x7fd8842afde1 in nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) netwerk/base/src/nsInputStreamPump.cpp:475
    #13 0x7fd8842b37ae in non-virtual thunk to nsInputStreamPump::OnInputStreamReady(nsIAsyncInputStream*) objdir-ff-asan-sym/netwerk/base/src/Unified_cpp_netwerk_base_src1.cpp:490
    #14 0x7fd883b861a8 in nsInputStreamReadyEvent::Run() xpcom/io/nsStreamUtils.cpp:85
    #15 0x7fd883c7d718 in nsThread::ProcessNextEvent(bool, bool*) xpcom/threads/nsThread.cpp:643
    #16 0x7fd88368f442 in NS_ProcessNextEvent(nsIThread*, bool) xpcom/glue/nsThreadUtils.cpp:263
    #17 0x7fd885eba818 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) ipc/glue/MessagePump.cpp:95
    #18 0x7fd885b823b7 in MessageLoop::RunInternal() ipc/chromium/src/base/message_loop.cc:226
    #19 0x7fd885b8200a in MessageLoop::RunHandler() ipc/chromium/src/base/message_loop.cc:219
    #20 0x7fd885b81ee5 in MessageLoop::Run() ipc/chromium/src/base/message_loop.cc:193
    #21 0x7fd88ea2463f in nsBaseAppShell::Run() widget/xpwidgets/nsBaseAppShell.cpp:161
    #22 0x7fd8992b6319 in nsAppStartup::Run() toolkit/components/startup/nsAppStartup.cpp:276
    #23 0x7fd898af4c65 in XREMain::XRE_mainRun() toolkit/xre/nsAppRunner.cpp:4090
    #24 0x7fd898af950a in XREMain::XRE_main(int, char**, nsXREAppData const*) toolkit/xre/nsAppRunner.cpp:4158
    #25 0x7fd898afbed4 in XRE_main toolkit/xre/nsAppRunner.cpp:4368
    #26 0x44dc07 in do_main(int, char**, nsIFile*) browser/app/nsBrowserApp.cpp:280
    #27 0x44abe5 in main browser/app/nsBrowserApp.cpp:648
    #28 0x7fd8ae6bb76c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226
Shadow bytes around the buggy address:
  0x0c0480065a30: fa fa fd fd fa fa fd fa fa fa 00 00 fa fa 00 fa
  0x0c0480065a40: fa fa 00 00 fa fa fd fa fa fa 00 00 fa fa 00 fa
  0x0c0480065a50: fa fa 00 00 fa fa fd fa fa fa 00 00 fa fa 00 fa
  0x0c0480065a60: fa fa 00 00 fa fa fd fd fa fa fd fa fa fa 00 00
  0x0c0480065a70: fa fa 00 00 fa fa 00 fa fa fa 00 00 fa fa 00 fa
=>0x0c0480065a80: fa fa 00 00 fa fa 00 fa fa fa 00 00 fa fa[01]fa
  0x0c0480065a90: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480065aa0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480065ab0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c0480065ac0: fa fa fd fa fa fa 00 02 fa fa 00 00 fa fa 00 fa
  0x0c0480065ad0: fa fa fd fa fa fa 00 00 fa fa 00 04 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
  ASan internal:         fe
==7222==ABORTING

Comment 1

[Christoph Diehl [:posidron]]

Attachment: testcase

Comment 2

[Christoph Diehl [:posidron]]

The picture does not seem to contain a ICC profile at all.

convert saraamarie-buggy-solidpp-pink.jpg icc:profile.icm && hexdump -C profile.icm

  Profiles:
    Profile-exif: 389 bytes
    Profile-icc: 1 bytes

That 1 byte is a 0x00 byte

Comment 3

[Christoph Diehl [:posidron]]

  qcms_profile* qcms_profile_from_memory(const void *mem, size_t size)
  {
      [...]
*     length = read_u32(src, 0);
      [...]
  }

  static uint32_t read_u32(struct mem_source *mem, size_t offset)
  {
      /* Subtract from mem->size instead of the more intuitive adding to offset.
       * This avoids overflowing offset. The subtraction is safe because
       * mem->size is guaranteed to be > 4 */
      if (offset > mem->size - 4) {
          invalid_source(mem, "Invalid offset");
          return 0;
      } else {
          be32 k;
*         memcpy(&k, mem->buf + offset, sizeof(k));
          return be32_to_cpu(k);
      }
  }

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Attachment: A fix

Comment 5

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8373698 [details] [diff] [review]
A fix

Ignore the qcmstypes.h stuff

Comment 6

[Benoit Girard (:BenWa)]

Comment on attachment 8373698 [details] [diff] [review]
A fix

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

r+ for iccread.c changes. Please remove the other changes

Comment 7

[Andrew McCreight [:mccr8]]

Jeff, this has a reviewed patch, is it ready for sec-approval and landing?  Thanks.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

sec-approval? was set but the template for sec-approval was not filled out so I can't approve it. Can you set it again and fill out the template, please?

Comment 9

[Abhishek Arya]

Jeff, ping! this bug is getting old and hits in my fuzzing every now and then. Will be great to get this out of the queue.

Comment 10

[Al Billings [:abillings - ex-MoCo]]

Milan, can we get someone to provide the requested information? This is going to miss the current release cycle and I've been waiting for a response to my request for two weeks.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Check if there is enough data to read uint32 to avoid buffer overflow. Jeff's patch rebased, only iccread.c, carry r=bgirard.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily, I think.

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?
All.  This has been there since 2009.

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

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

How likely is this patch to cause regressions; how much testing does it need?
Not likely.  It is a trivial check and all we're doing is avoiding the buffer overflow by avoiding reading data that isn't there. Invalid profile is the correct action in this case.

Comment 12

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8373698 [details] [diff] [review]
A fix

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

Not very easily, I don't think this is exploitable. It's just a fixed offset out of bounds read

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The problem is pretty obvious from the patch, but there's but it feels like it's pretty harmless

Which older supported branches are affected by this flaw? All of them

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? Easy

How likely is this patch to cause regressions; how much testing does it need? Very unlikely to cause regressions. It shouldn't need any testing as the patch is pretty obvious.

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8404156 [details] [diff] [review]
Check if there is enough data to read uint32 to avoid buffer overflow.  Jeff's patch rebased, only iccread.c, carry r=bgirard.

sec-approval+ for trunk.

Once this lands and is green, we'd like to see Aurora, Beta, and ESR24 patches created and nominated for branch landing.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7f295bcceea0

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f295bcceea0

Please request Aurora/Beta/ESR24 approval on this when you get a chance :)

Comment 16

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Aurora patch

[Approval Request Comment]

See the details in the security approval request, comment 11.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Beta patch.

[Approval Request Comment]

See the details in the security approval request, comment 11.

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: ESR24 patch

[Approval Request Comment]

See the details in the security approval request, comment 11.

Comment 19

[Tony Payne]

Thanks for cc:ing me to let us know about the problem. I haven't worked on Chromium in a while. Noel Gordon ([e-mail]) is the best person to notify. I'm not sure what his bugzilla login is, so I can't cc: him on the bug.

Comment 20

[Abhishek Arya]

(In reply to Tony Payne from comment #19)
> Thanks for cc:ing me to let us know about the problem. I haven't worked on
> Chromium in a while. Noel Gordon ([e-mail]) is the best person to
> notify. I'm not sure what his bugzilla login is, so I can't cc: him on the
> bug.

It does not crash Chrome, i had checked before filing it here. Still i just filed https://code.google.com/p/chromium/issues/detail?id=362762 so that Noel can double check.

Comment 21

[Abhishek Arya]

(In reply to Abhishek Arya from comment #20)
> (In reply to Tony Payne from comment #19)
> > Thanks for cc:ing me to let us know about the problem. I haven't worked on
> > Chromium in a while. Noel Gordon ([e-mail]) is the best person to
> > notify. I'm not sure what his bugzilla login is, so I can't cc: him on the
> > bug.
> 
> It does not crash Chrome, i had checked before filing it here. Still i just
> filed https://code.google.com/p/chromium/issues/detail?id=362762 so that
> Noel can double check.

From Noel@ on why it does not reproduce in Chrome.
----------
[e-mail]
ImageDecoder.h defines the color profile header length

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/ImageDecoder.h&l=157

  enum { iccColorProfileHeaderLength = 128 };

The decoders check the header size when reading profile data from an encoded image

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/png/PNGImageDecoder.cpp&l=284
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/jpeg/JPEGImageDecoder.cpp&&l=230
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/image-decoders/webp/WEBPImageDecoder.cpp&l=360

and ignore color profiles with short headers.

So qcms never gets to see such profiles, hence no reproduction.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f6879fbf3c94
https://hg.mozilla.org/releases/mozilla-beta/rev/05c933823ad8
https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/29e51c1f5990
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/459bad565e6d
https://hg.mozilla.org/releases/mozilla-esr24/rev/6beca96c3bd3

Comment 23

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial crash on Nightly ASAN from 2014-02-06. Verified as fixed on Firefox 29 beta 8, latest Aurora and latest Nightly all ASAN builds.

Comment 24

[Daniel Veditz [:dveditz]]

This is not enough of an info-leak to warrant a sec-high on privacy grounds, but Abhishek has seen lots of clever Chrome exploits so maybe he can rebut my lowering of the severity to sec-moderate.

Comment 25

[Abhishek Arya]

(In reply to Daniel Veditz [:dveditz] from comment #24)
> This is not enough of an info-leak to warrant a sec-high on privacy grounds,
> but Abhishek has seen lots of clever Chrome exploits so maybe he can rebut
> my lowering of the severity to sec-moderate.

Looking more, this does not seem exploitable. Length is arbitrary based on value read from memory. Size is < 4. This causes it to run into INVALID_PROFILE due to the next two checks.

source.size = size;
....
....
length = read_u32(src, 0);
1027 	if (length <= size) {
1028 		// shrink the area that we can read if appropriate
1029 		source.size = length;
1030 	} else {
1031 		return INVALID_PROFILE;
1032 	}
1033 
1034 	/* ensure that the profile size is sane so it's easier to reason about */
1035 	if (source.size <= 64 || source.size >= MAX_PROFILE_SIZE)
1036 		return INVALID_PROFILE;

Comment 26

[Bogdan Maris, Desktop Test Engineering]

I'm trying to verify the fix on esr ASAN build as well but while building I am prompted with 'clang-3.4: error: linker command failed with exit code 1' error and I can't bypass it. Can someone help me on this or if someone has successfully build latest ESR give me access to a build?