Bug 1140537 (CVE-2015-2716)

Buffer overflow xml parser

RESOLVED FIXED in Firefox 38, Firefox OS v1.4

Status

()

Core
XML
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: Ucha Gobejishvili, Assigned: erahm)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {csectype-bounds, sec-critical})

36 Branch
mozilla40
csectype-bounds, sec-critical
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox36 wontfix, firefox37 wontfix, firefox38+ verified, firefox38.0.5 fixed, firefox39+ verified, firefox40+ verified, firefox-esr3138+ verified, firefox-esr38 verified, b2g-v1.4 fixed, b2g-v2.0 fixed, b2g-v2.0M fixed, b2g-v2.1 fixed, b2g-v2.1S fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [adv-main38+][adv-esr31.7+])

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.115 Safari/537.36

Steps to reproduce:

FF last version (last ASAN build)
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-release-linux64-asan/latest/firefox-36.0.1.en-US.linux-x86_64-asan.tar.bz2


Actual results:

=================================================================
==30124==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7fff018ba800 at pc 0x45f23c bp 0x7fffffff8970 sp 0x7fffffff8128
WRITE of size 4718532 at 0x7fff018ba800 thread T0
    #0 0x45f23b (/home/user/testing/firefox/firefox+0x45f23b)
    #1 0x7fffedd9dc34 (/home/user/testing/firefox/libxul.so+0x811cc34)
    #2 0x7fffe86d7e4a (/home/user/testing/firefox/libxul.so+0x2a56e4a)
    #3 0x7fffe86d9256 (/home/user/testing/firefox/libxul.so+0x2a58256)
    #4 0x7fffe86e7c59 (/home/user/testing/firefox/libxul.so+0x2a66c59)
    #5 0x7fffe86e39fd (/home/user/testing/firefox/libxul.so+0x2a629fd)
    #6 0x7fffe86e8eea (/home/user/testing/firefox/libxul.so+0x2a67eea)
    #7 0x7fffe73aef5d (/home/user/testing/firefox/libxul.so+0x172df5d)
    #8 0x7fffe72fe02e (/home/user/testing/firefox/libxul.so+0x167d02e)
    #9 0x7fffe7650ac1 (/home/user/testing/firefox/libxul.so+0x19cfac1)
    #10 0x7fffe7279ab4 (/home/user/testing/firefox/libxul.so+0x15f8ab4)
    #11 0x7fffe7278897 (/home/user/testing/firefox/libxul.so+0x15f7897)
    #12 0x7fffe70b6a69 (/home/user/testing/firefox/libxul.so+0x1435a69)
    #13 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #14 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #15 0x7fffe70e7b5e (/home/user/testing/firefox/libxul.so+0x1466b5e)
    #16 0x7fffe70f3210 (/home/user/testing/firefox/libxul.so+0x1472210)
    #17 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #18 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #19 0x7fffe70e7b5e (/home/user/testing/firefox/libxul.so+0x1466b5e)
    #20 0x7fffe70f3210 (/home/user/testing/firefox/libxul.so+0x1472210)
    #21 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #22 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #23 0x7fffe70e7b5e (/home/user/testing/firefox/libxul.so+0x1466b5e)
    #24 0x7fffe70f3210 (/home/user/testing/firefox/libxul.so+0x1472210)
    #25 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #26 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #27 0x7fffe70e7b5e (/home/user/testing/firefox/libxul.so+0x1466b5e)
    #28 0x7fffe70f3210 (/home/user/testing/firefox/libxul.so+0x1472210)
    #29 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #30 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #31 0x7fffe70e7b5e (/home/user/testing/firefox/libxul.so+0x1466b5e)
    #32 0x7fffe70f3210 (/home/user/testing/firefox/libxul.so+0x1472210)
    #33 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #34 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #35 0x7fffe70e7b5e (/home/user/testing/firefox/libxul.so+0x1466b5e)
    #36 0x7fffe70f3210 (/home/user/testing/firefox/libxul.so+0x1472210)
    #37 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #38 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #39 0x7fffe70e7b5e (/home/user/testing/firefox/libxul.so+0x1466b5e)
    #40 0x7fffe70f3210 (/home/user/testing/firefox/libxul.so+0x1472210)
    #41 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #42 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #43 0x7fffe7949d99 (/home/user/testing/firefox/libxul.so+0x1cc8d99)
    #44 0x7fffe78f8d1c (/home/user/testing/firefox/libxul.so+0x1c77d1c)
    #45 0x7fffebc8ed87 (/home/user/testing/firefox/libxul.so+0x600dd87)
    #46 0x7fffed667fa8 (/home/user/testing/firefox/libxul.so+0x79e6fa8)
    #47 0x7fffed753d6e (/home/user/testing/firefox/libxul.so+0x7ad2d6e)
    #48 0x7fffed754c93 (/home/user/testing/firefox/libxul.so+0x7ad3c93)
    #49 0x7fffed755b0d (/home/user/testing/firefox/libxul.so+0x7ad4b0d)
    #50 0x48a2fa (/home/user/testing/firefox/firefox+0x48a2fa)
    #51 0x7ffff6c0076c (/lib/x86_64-linux-gnu/libc.so.6+0x2176c)
    #52 0x48975c (/home/user/testing/firefox/firefox+0x48975c)

0x7fff018ba800 is located 0 bytes to the right of 2147237888-byte region [0x7ffe818f6800,0x7fff018ba800)
allocated by thread T0 here:
    #0 0x471d71 (/home/user/testing/firefox/firefox+0x471d71)
    #1 0x7fffedd9ed25 (/home/user/testing/firefox/libxul.so+0x811dd25)
    #2 0x7fffedd9db81 (/home/user/testing/firefox/libxul.so+0x811cb81)
    #3 0x7fffe86d7e4a (/home/user/testing/firefox/libxul.so+0x2a56e4a)
    #4 0x7fffe86d9256 (/home/user/testing/firefox/libxul.so+0x2a58256)
    #5 0x7fffe86e7c59 (/home/user/testing/firefox/libxul.so+0x2a66c59)
    #6 0x7fffe86e39fd (/home/user/testing/firefox/libxul.so+0x2a629fd)
    #7 0x7fffe86e8eea (/home/user/testing/firefox/libxul.so+0x2a67eea)
    #8 0x7fffe73aef5d (/home/user/testing/firefox/libxul.so+0x172df5d)
    #9 0x7fffe72fe02e (/home/user/testing/firefox/libxul.so+0x167d02e)
    #10 0x7fffe7650ac1 (/home/user/testing/firefox/libxul.so+0x19cfac1)
    #11 0x7fffe7279ab4 (/home/user/testing/firefox/libxul.so+0x15f8ab4)
    #12 0x7fffe7278897 (/home/user/testing/firefox/libxul.so+0x15f7897)
    #13 0x7fffe70b6a69 (/home/user/testing/firefox/libxul.so+0x1435a69)
    #14 0x7fffe70e8bbf (/home/user/testing/firefox/libxul.so+0x1467bbf)
    #15 0x7fffe714549a (/home/user/testing/firefox/libxul.so+0x14c449a)
    #16 0x7fffe7949d99 (/home/user/testing/firefox/libxul.so+0x1cc8d99)
    #17 0x7fffe78f8d1c (/home/user/testing/firefox/libxul.so+0x1c77d1c)
    #18 0x7fffebc8ed87 (/home/user/testing/firefox/libxul.so+0x600dd87)
    #19 0x7fffed667fa8 (/home/user/testing/firefox/libxul.so+0x79e6fa8)
    #20 0x7fffed753d6e (/home/user/testing/firefox/libxul.so+0x7ad2d6e)
    #21 0x7fffed754c93 (/home/user/testing/firefox/libxul.so+0x7ad3c93)
    #22 0x7fffed755b0d (/home/user/testing/firefox/libxul.so+0x7ad4b0d)
    #23 0x48a2fa (/home/user/testing/firefox/firefox+0x48a2fa)
    #24 0x7ffff6c0076c (/lib/x86_64-linux-gnu/libc.so.6+0x2176c)

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
Shadow bytes around the buggy address:
  0x10006030f4b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10006030f4c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10006030f4d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10006030f4e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x10006030f4f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x10006030f500:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10006030f510: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10006030f520: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10006030f530: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10006030f540: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x10006030f550: 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
  Sta==30124==ABORTING
(Reporter)

Updated

2 years ago
OS: Windows 7 → All
Hardware: x86 → All

Comment 1

2 years ago
Do you have steps to reproduce or a better stack? What makes you say this is an XML parser issue?
Component: Untriaged → Untriaged
Flags: needinfo?(ugobejishvili)
Product: Firefox → Core
(Reporter)

Comment 2

2 years ago
(In reply to :Gijs Kruitbosch from comment #1)
> Do you have steps to reproduce or a better stack? What makes you say this is
> an XML parser issue?

Hi, For Monday i will update the case, and upload testcase / will explain how to reproduce the case.
Flags: needinfo?(ugobejishvili)
Whiteboard: waiting on testcase from reporter
(Reporter)

Comment 3

2 years ago
Created attachment 8574558 [details]
testcase

The testcase which i used to reproduce the crash.
QA Contact: kjozwiak
Component: Untriaged → XML
Thanks for the testcase Ucha!

STR:

- download the python script from comment # 3
- sudo python testcase.py in a terminal/cmd prompt
- wait till you receive "Serving Requests."
- visit http://localhost:80 from the browser (wait till it eventually crashes, takes anywhere between 5-15 minutes on ASan builds)

Reproduced the crash using the following builds:

* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1425992124/
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1425973348/
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1425964878/
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-release-linux64-asan/1425726871/
* Reproduced with esr31 using changeset: 30cd218c32ff (asan)

Crash Reports when using non-asan builds:

* https://crash-stats.mozilla.com/report/index/d34bab48-2ed6-454a-8662-45f112150310 (m-c on OSX)
* https://crash-stats.mozilla.com/report/index/da51a7a1-c159-485b-ac43-f54df2150310 (m-c on OSX)
* https://crash-stats.mozilla.com/report/index/f002e453-9883-497d-8799-ef68f2150310 (m-c on Ubuntu)
* https://crash-stats.mozilla.com/report/index/90191bb6-addc-4e73-82f7-cf2562150310 (m-a on Ubuntu)
* https://crash-stats.mozilla.com/report/index/8584715e-61b1-4cc4-87f0-ed3ef2150310 (m-a on Ubuntu)
* https://crash-stats.mozilla.com/report/index/66420b7d-a0fd-4360-ad6c-de0a42150310 (esr31 on Ubuntu)
Status: UNCONFIRMED → NEW
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
status-firefox39: --- → affected
Ever confirmed: true
Whiteboard: waiting on testcase from reporter
Could you attach a symbolized ASan crash stack, Kamil?  Thanks.
Flags: needinfo?(kjozwiak)
Built m-c asan using changeset: bc6aeea7229

==75285==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f4970e08800 at pc 0x433588 bp 0x7fff0a747bf0 sp 0x7fff0a7473b0
WRITE of size 4718532 at 0x7f4970e08800 thread T0
    #0 0x433587 in __interceptor_memcpy _asan_rtl_
    #1 0x7f4a5c974a84 in MOZ_XML_Parse xmlparse.c:1596
    #2 0x7f4a56669faa in ParseBuffer nsExpatDriver.cpp:1017
    #3 0x7f4a5666b3b6 in ConsumeToken nsExpatDriver.cpp:1117
    #4 0x7f4a5667bab9 in Tokenize nsParser.cpp:1943
    #5 0x7f4a566772ed in ResumeParse nsParser.cpp:1464
    #6 0x7f4a5667d04a in OnDataAvailable nsParser.cpp:1841
    #7 0x7f4a54e5994d in do_OnDataAvailable nsHTTPCompressConv.cpp:357
    #8 0x7f4a54d9739e in OnDataAvailable nsStreamListenerTee.cpp:93
    #9 0x7f4a55134143 in OnDataAvailable nsHttpChannel.cpp:5737
    #10 0x7f4a54d0ea1e in OnStateTransfer nsInputStreamPump.cpp:607
    #11 0x7f4a54d0d6a9 in OnInputStreamReady nsInputStreamPump.cpp:436
    #12 0x7f4a54b2ab69 in Run nsStreamUtils.cpp:91
    #13 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #14 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #15 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #16 0x7f4a54b737b0 in apply<nsIThread, nsresult (nsIThread::*)()> nsThreadUtils.h:573
    #17 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #18 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #19 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #20 0x7f4a54b737b0 in apply<nsIThread, nsresult (nsIThread::*)()> nsThreadUtils.h:573
    #21 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #22 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #23 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #24 0x7f4a54b737b0 in apply<nsIThread, nsresult (nsIThread::*)()> nsThreadUtils.h:573
    #25 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #26 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #27 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #28 0x7f4a54b737b0 in apply<nsIThread, nsresult (nsIThread::*)()> nsThreadUtils.h:573
    #29 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #30 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #31 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #32 0x7f4a54b737b0 in apply<nsIThread, nsresult (nsIThread::*)()> nsThreadUtils.h:573
    #33 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #34 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #35 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #36 0x7f4a54b737b0 in apply<nsIThread, nsresult (nsIThread::*)()> nsThreadUtils.h:573
    #37 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #38 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #39 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #40 0x7f4a54b737b0 in apply<nsIThread, nsresult (nsIThread::*)()> nsThreadUtils.h:573
    #41 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #42 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #43 0x7f4a54b6596e in Shutdown nsThread.cpp:662
    #44 0x7f4a54b58461 in ShutdownThread LazyIdleThread.cpp:309
    #45 0x7f4a54b58e7e in Notify LazyIdleThread.cpp:500
    #46 0x7f4a54b70587 in Fire nsTimerImpl.cpp:634
    #47 0x7f4a54b711ee in Run nsTimerImpl.cpp:724
    #48 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #49 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #50 0x7f4a555191f9 in Run MessagePump.cpp:99
    #51 0x7f4a554981fc in RunInternal message_loop.cc:233
    #52 0x7f4a5a4fc7f7 in Run nsBaseAppShell.cpp:164
    #53 0x7f4a5c165648 in Run nsAppStartup.cpp:281
    #54 0x7f4a5c2636ed in XRE_mainRun nsAppRunner.cpp:4183
    #55 0x7f4a5c26461d in XRE_main nsAppRunner.cpp:4259
    #56 0x7f4a5c2654fd in XRE_main nsAppRunner.cpp:4479
    #57 0x47b1ca in do_main nsBrowserApp.cpp:294
    #58 0x7f4a6569aec4 in __libc_start_main libc-start.c:287
    #59 0x47a69a in _start ??:?

0x7f4970e08800 is located 0 bytes to the right of 2147237888-byte region [0x7f48f0e44800,0x7f4970e08800)
allocated by thread T0 here:
    #0 0x460f67 in __interceptor_malloc _asan_rtl_
    #1 0x7f4a5c975b75 in MOZ_XML_GetBuffer xmlparse.c:1698
    #2 0x7f4a5c9749d1 in MOZ_XML_Parse xmlparse.c:1592
    #3 0x7f4a56669faa in ParseBuffer nsExpatDriver.cpp:1017
    #4 0x7f4a5666b3b6 in ConsumeToken nsExpatDriver.cpp:1117
    #5 0x7f4a5667bab9 in Tokenize nsParser.cpp:1943
    #6 0x7f4a566772ed in ResumeParse nsParser.cpp:1464
    #7 0x7f4a5667d04a in OnDataAvailable nsParser.cpp:1841
    #8 0x7f4a54e5994d in do_OnDataAvailable nsHTTPCompressConv.cpp:357
    #9 0x7f4a54d9739e in OnDataAvailable nsStreamListenerTee.cpp:93
    #10 0x7f4a55134143 in OnDataAvailable nsHttpChannel.cpp:5737
    #11 0x7f4a54d0ea1e in OnStateTransfer nsInputStreamPump.cpp:607
    #12 0x7f4a54d0d6a9 in OnInputStreamReady nsInputStreamPump.cpp:436
    #13 0x7f4a54b2ab69 in Run nsStreamUtils.cpp:91
    #14 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
    #15 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #16 0x7f4a555191f9 in Run MessagePump.cpp:99
    #17 0x7f4a554981fc in RunInternal message_loop.cc:233
    #18 0x7f4a5a4fc7f7 in Run nsBaseAppShell.cpp:164
    #19 0x7f4a5c165648 in Run nsAppStartup.cpp:281
    #20 0x7f4a5c2636ed in XRE_mainRun nsAppRunner.cpp:4183
    #21 0x7f4a5c26461d in XRE_main nsAppRunner.cpp:4259
    #22 0x7f4a5c2654fd in XRE_main nsAppRunner.cpp:4479
    #23 0x47b1ca in do_main nsBrowserApp.cpp:294
    #24 0x7f4a6569aec4 in __libc_start_main libc-start.c:287

SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
Shadow bytes around the buggy address:
  0x0fe9ae1b90b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe9ae1b90c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe9ae1b90d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe9ae1b90e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0fe9ae1b90f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0fe9ae1b9100:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe9ae1b9110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe9ae1b9120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe9ae1b9130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe9ae1b9140: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0fe9ae1b9150: 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
  Sta==75285==ABORTING
Flags: needinfo?(kjozwiak)
Keywords: csectype-bounds, sec-high
(In reply to Kamil Jozwiak [:kjozwiak] from comment #6)

> 0x7f4970e08800 is located 0 bytes to the right of 2147237888-byte region
> [0x7f48f0e44800,0x7f4970e08800)
> allocated by thread T0 here:
>     #0 0x460f67 in __interceptor_malloc _asan_rtl_
>     #1 0x7f4a5c975b75 in MOZ_XML_GetBuffer xmlparse.c:1698

That's a pretty sketchy allocation size. If we take a look at |XML_GetBuffer| there's a FIXME comment [1] indicating possible integer overflow. Is it possible that's coming into play here?

[1] https://dxr.mozilla.org/mozilla-central/source/parser/expat/lib/xmlparse.c?from=xmlparse.c&case=true#1667


>     #2 0x7f4a5c9749d1 in MOZ_XML_Parse xmlparse.c:1592
>     #3 0x7f4a56669faa in ParseBuffer nsExpatDriver.cpp:1017
>     #4 0x7f4a5666b3b6 in ConsumeToken nsExpatDriver.cpp:1117
>     #5 0x7f4a5667bab9 in Tokenize nsParser.cpp:1943
>     #6 0x7f4a566772ed in ResumeParse nsParser.cpp:1464
>     #7 0x7f4a5667d04a in OnDataAvailable nsParser.cpp:1841
>     #8 0x7f4a54e5994d in do_OnDataAvailable nsHTTPCompressConv.cpp:357
>     #9 0x7f4a54d9739e in OnDataAvailable nsStreamListenerTee.cpp:93
>     #10 0x7f4a55134143 in OnDataAvailable nsHttpChannel.cpp:5737
>     #11 0x7f4a54d0ea1e in OnStateTransfer nsInputStreamPump.cpp:607
>     #12 0x7f4a54d0d6a9 in OnInputStreamReady nsInputStreamPump.cpp:436
>     #13 0x7f4a54b2ab69 in Run nsStreamUtils.cpp:91
>     #14 0x7f4a54b669f4 in ProcessNextEvent nsThread.cpp:855
>     #15 0x7f4a54bc4d1a in NS_ProcessNextEvent nsThreadUtils.cpp:265
>     #16 0x7f4a555191f9 in Run MessagePump.cpp:99
>     #17 0x7f4a554981fc in RunInternal message_loop.cc:233
>     #18 0x7f4a5a4fc7f7 in Run nsBaseAppShell.cpp:164
>     #19 0x7f4a5c165648 in Run nsAppStartup.cpp:281
>     #20 0x7f4a5c2636ed in XRE_mainRun nsAppRunner.cpp:4183
>     #21 0x7f4a5c26461d in XRE_main nsAppRunner.cpp:4259
>     #22 0x7f4a5c2654fd in XRE_main nsAppRunner.cpp:4479
>     #23 0x47b1ca in do_main nsBrowserApp.cpp:294
>     #24 0x7f4a6569aec4 in __libc_start_main libc-start.c:287
> 
> SUMMARY: AddressSanitizer: heap-buffer-overflow ??:0 ??
> Shadow bytes around the buggy address:
>   0x0fe9ae1b90b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0fe9ae1b90c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0fe9ae1b90d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0fe9ae1b90e0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>   0x0fe9ae1b90f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> =>0x0fe9ae1b9100:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0fe9ae1b9110: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0fe9ae1b9120: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0fe9ae1b9130: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0fe9ae1b9140: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
>   0x0fe9ae1b9150: 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
>   Sta==75285==ABORTING
status-firefox36: affected → wontfix
From visual inspection there's certainly an issue in libexpat, whether it's *this* issue I'll need to research further.

XML_GetBuffer will either reuse an internal buffer if there's enough space, or create a new larger one and copy any existing buffer data into it and return a ptr to after where the existing buffer data was copied. 

So there are a couple of ways for this to go bad:

Option one: 
Pass in a negative number. This can happen for instance if the string size is > max(int), it will then get converted to a uint32_t [1] and then an int [2] and will end up a negative number. That will bypass the allocation code [3] in GetBuffer and then we'll copy a gigantic (negative int cast to size_t) size into the existing buffer.
 
[1] https://hg.mozilla.org/mozilla-central/annotate/04f34e86aee1/parser/htmlparser/nsExpatDriver.cpp#l1105
[2] https://hg.mozilla.org/mozilla-central/annotate/04f34e86aee1/parser/expat/lib/xmlparse.c#l1654
[3] https://hg.mozilla.org/mozilla-central/annotate/04f34e86aee1/parser/expat/lib/xmlparse.c#l1666

But this can happen even if we don't pass in <= max(int):

#1 - If we ask for > max(int), that will get cast down to max(int).
#2 - If there's anything in the buffer, lets say 2 chars: int neededSize = max(int) + 2 = -a_bunch
#3 - Then the allocation loop will go through and allocate a larger buffer, in this case we'll just do one iteration because any_positive_number > -a_bunch
#4 - The leftover portion of the previous buffer gets copied into this slightly larger buffer
#5 - bufferEnd is returned, which is an offset into a buffer that is *not* large enough to contain len
#6 - XML_Parse then tries to copy the full buffer of size len into the buffer much smaller than len

This jibes pretty well with the stacks.

Annotated and trimmed for simplicity:

>         hg      1:1645: void * XMLCALL
>         hg      1:1646: XML_GetBuffer(XML_Parser parser, int len) [#1]
>         hg      1:1647: {
> 
>         hg      1:1658:   if (len > bufferLim - bufferEnd) {
>         hg      1:1659:     /* FIXME avoid integer overflow */
>         hg      1:1660:     int neededSize = len + (int)(bufferEnd - bufferPtr); [#2]
> 
>         hg      1:1668:     if (neededSize  <= bufferLim - buffer) { // no
> 
>         hg      1:1681:     }
>         hg      1:1682:     else { // yes
>         hg      1:1683:       char *newBuf;
>         hg      1:1684:       int bufferSize = (int)(bufferLim - bufferPtr);
>         hg      1:1685:       if (bufferSize == 0)
>         hg      1:1686:         bufferSize = INIT_BUFFER_SIZE;
>         hg      1:1687:       do {
>         hg      1:1688:         bufferSize *= 2;
>         hg      1:1689:       } while (bufferSize < neededSize); [#3]
>         hg      1:1690:       newBuf = (char *)MALLOC(bufferSize);
>         hg      1:1691:       if (newBuf == 0) {
>         hg      1:1692:         errorCode = XML_ERROR_NO_MEMORY;
>         hg      1:1693:         return NULL;
>         hg      1:1694:       }
>         hg      1:1695:       bufferLim = newBuf + bufferSize;
> 
>         hg      1:1712:       if (bufferPtr) {
>         hg      1:1713:         memcpy(newBuf, bufferPtr, bufferEnd - bufferPtr); [#4]
>         hg      1:1714:         FREE(buffer);
>         hg      1:1715:       }
>         hg      1:1716:       bufferEnd = newBuf + (bufferEnd - bufferPtr);
>         hg      1:1717:       bufferPtr = buffer = newBuf;
>         
>         hg      1:1719:     }
>         hg      1:1720:   }
>         hg      1:1721:   return bufferEnd; [#5]
>         hg      1:1722: }
Any chance you could look into a fix for this, Eric?  Thanks.
Flags: needinfo?(erahm)
I'll take a look at this today.
Flags: needinfo?(erahm)
Created attachment 8591124 [details] [diff] [review]
WIP

This patch adds various integer overflow sanity checks. Testing against the provided python script results in an out of memory error page. If someone with ASAN skills could test this against an ASAN build that would be helpful.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
:mccr8 can you check this out w/ ASAN?
Flags: needinfo?(continuation)
I applied the patch from comment # 11 and built an asan m-c. Following the test cases I added in comment # 4, I received the following error page rather than the crash that I was receiving before:

XML Parsing Error: out of memory
Location: http://localhost/
Line Number 1, Column 1:

The one thing that I did notice is that once you receive the above "XML Parsing Error: out of memory" error page, fx will enter into a "not responding" mode once you interact with the browser (opening a new tab, switching tabs and especially re-sizing the browser window). After a few minutes of "not responding", fx will be restored with the earlier interaction finally completing. I think this is happening because the earlier tab is still connected to the python script that's still serving the large amount of compressed bytes. Once you close that tab, fx performs as expected.

* used the following m-c build: e8b5ab0ce7a6 bug_1140537/qbase/qtip/tip

I'll leave the :mccr8 needinfo in case Andrew sees something that I might have missed. Let me know if this helps Eric!
Thanks, Kamil!  I don't think I need to look at it, too. :)
Flags: needinfo?(continuation)
Created attachment 8592473 [details] [diff] [review]
Sanity check size calculations

This is the same patch as before with an updated title. Peter can you take a look at this?
Attachment #8592473 - Flags: review?(peterv)
Attachment #8591124 - Attachment is obsolete: true
status-firefox37: affected → wontfix
status-firefox40: --- → affected
status-firefox-esr31: --- → affected
Comment on attachment 8592473 [details] [diff] [review]
Sanity check size calculations

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

The project looks a bit dead but you should submit this at http://sourceforge.net/p/expat/patches/ too.
Attachment #8592473 - Flags: review?(peterv) → review+
Keywords: sec-high → sec-critical
Eric said that this would probably be pretty easy to write a bunch off the end of the array with this, so I'm upgrading this to sec-critical.
Comment on attachment 8592473 [details] [diff] [review]
Sanity check size calculations

Note: as mentioned by Peter, this is a bug in libexpat which is not fixed upstream. It could effect all users of libexpat.

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

Easy.

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

Yes, the problem is quite obvious.

Which older supported branches are affected by this flaw?

All.

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?

This code hasn't changed in a long time, it should cleanly apply.

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

Unlikely, it just adds sanity checks on buffer sizes.
Attachment #8592473 - Flags: sec-approval?
sec-approval+ for trunk. We're going to want this on all supported branches though. Please prepare and nominate Aurora, Beta, and ESR31 patches.
tracking-firefox38: --- → +
tracking-firefox39: --- → +
tracking-firefox40: --- → +
tracking-firefox-esr31: --- → 38+
Attachment #8592473 - Flags: sec-approval? → sec-approval+
Comment on attachment 8592473 [details] [diff] [review]
Sanity check size calculations

I have verified this patch applies cleanly to m-c, m-b, m-a, and m-esr31.

Approval Request Comment (Aurora+Beta)
[Feature/regressing bug #]:
None, issue exists upstream.
[User impact if declined]:
Buffer overflow.
[Describe test coverage new/current, TreeHerder]:
We don't currently have libexpat tests, but anything that uses XML should exercise this code path. Verified that the attached testcase no longer crashes.
[Risks and why]: 
None.
[String/UUID change made/needed]:
N/A.


[Approval Request Comment (ESR31)]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is sec-crit.
User impact if declined: 
Buffer overflow.
Fix Landed on Version:
Targeting m-c, m-a, m-b.
Risk to taking this patch (and alternatives if risky): 
None.
String or UUID changes made by this patch: 
N/A

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8592473 - Flags: approval-mozilla-esr31?
Attachment #8592473 - Flags: approval-mozilla-beta?
Attachment #8592473 - Flags: approval-mozilla-aurora?
Attachment #8592473 - Flags: approval-mozilla-esr31?
Attachment #8592473 - Flags: approval-mozilla-esr31+
Attachment #8592473 - Flags: approval-mozilla-beta?
Attachment #8592473 - Flags: approval-mozilla-beta+
Attachment #8592473 - Flags: approval-mozilla-aurora?
Attachment #8592473 - Flags: approval-mozilla-aurora+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/438d9e2a991a
Keywords: checkin-needed
https://hg.mozilla.org/releases/mozilla-aurora/rev/f628845b9e9f
https://hg.mozilla.org/releases/mozilla-release/rev/a7d6b32a504c
https://hg.mozilla.org/releases/mozilla-esr31/rev/2f3e78643f5c
status-b2g-v1.4: --- → affected
status-b2g-v2.0: --- → affected
status-b2g-v2.0M: --- → affected
status-b2g-v2.1: --- → affected
status-b2g-v2.1S: --- → affected
status-b2g-v2.2: --- → affected
status-b2g-master: --- → affected
status-firefox38: affected → fixed
status-firefox39: affected → fixed
status-firefox-esr31: affected → fixed
status-firefox-esr38: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/438d9e2a991a
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-b2g-master: affected → fixed
status-firefox40: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
https://hg.mozilla.org/releases/mozilla-beta/rev/a7d6b32a504c
https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/c7dd12c6199f
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/800e9ea58ed6
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/800e9ea58ed6
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/58a4823c2353
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/58a4823c2353
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/c48d05987e7a
status-b2g-v1.4: affected → fixed
status-b2g-v2.0: affected → fixed
status-b2g-v2.0M: affected → fixed
status-b2g-v2.1: affected → fixed
status-b2g-v2.1S: affected → fixed
status-b2g-v2.2: affected → fixed
status-firefox38.0.5: --- → fixed
Created attachment 8599095 [details]
memoryMapError.txt

I keep getting the following asan crash on m-c changeset: 1ad65cbeb2f4 when going through the following:

STR:

- download the python script from comment # 3
- sudo python testcase.py in a terminal/cmd prompt
- wait till you receive "Serving Requests."
- visit http://localhost:80 from the browser while using e10s
- wait about 5 minutes and disable e10s via the preferences (this will take a while after selecting "OK" on the prompt)
- wait anywhere between 5-10 minutes until the browser restarts itself in normal mode (no e10s)
- wait another 5 minutes or so and the browser will eventually crash

I also get a very long memory map dump that I've attached to this ticket (not sure if it's useful). This reproducible pretty much every single time. 

==42524==AddressSanitizer CHECK failed: /home/kjozwiak/code/llvm/projects/compiler-rt/lib/sanitizer_common/sanitizer_posix.cc:68 "(("unable to mmap" && 0)) != (0)" (0x0, 0x0)
    #0 0x46738d in AsanCheckFailed _asan_rtl_
    #1 0x46bfe3 in __sanitizer::CheckFailed(char const*, int, char const*, unsigned long long, unsigned long long) sanitizer_common.cc:76
    #2 0x470220 in __sanitizer::MmapOrDie(unsigned long, char const*) sanitizer_posix.cc:68
    #3 0x41f035 in __sanitizer::LargeMmapAllocator<__asan::AsanMapUnmapCallback>::Allocate(__sanitizer::AllocatorStats*, unsigned long, unsigned long) sanitizer_allocator.h:1011
    #4 0x41fbd5 in Reallocate asan_allocator2.cc:518
    #5 0x461046 in __interceptor_realloc _asan_rtl_
    #6 0x7f431981db01 in Realloc nsSubstring.cpp:246
    #7 0x7f431982504e in ReplacePrepInternal nsTSubstring.cpp:169
    #8 0x7f43198280ab in ReplacePrep nsTSubstring.h:1010
    #9 0x7f431b52f335 in Append nsTSubstring.h:524
    #10 0x7f431b5326a6 in ConsumeToken nsExpatDriver.cpp:1184
    #11 0x7f431b542429 in Tokenize nsParser.cpp:1943
    #12 0x7f431b53dc5d in ResumeParse nsParser.cpp:1464
    #13 0x7f431b5439ba in OnDataAvailable nsParser.cpp:1841
    #14 0x7f4319c5ae11 in do_OnDataAvailable nsHTTPCompressConv.cpp:356
    #15 0x7f4319f3e525 in OnDataAvailable nsHttpChannel.cpp:5785
    #16 0x7f4319b0c967 in OnStateTransfer nsInputStreamPump.cpp:607
    #17 0x7f4319b0b727 in OnInputStreamReady nsInputStreamPump.cpp:436
    #18 0x7f4319924109 in Run nsStreamUtils.cpp:91
    #19 0x7f43199602c4 in ProcessNextEvent nsThread.cpp:868
    #20 0x7f43199bdaea in NS_ProcessNextEvent nsThreadUtils.cpp:265
    #21 0x7f431a338a59 in Run MessagePump.cpp:95
    #22 0x7f431a289c6c in RunInternal message_loop.cc:233
    #23 0x7f431f61cce7 in Run nsBaseAppShell.cpp:165
    #24 0x7f43212a7468 in Run nsAppStartup.cpp:280
    #25 0x7f43213a8b2c in XRE_mainRun nsAppRunner.cpp:4071
    #26 0x7f43213a9b4c in XRE_main nsAppRunner.cpp:4151
    #27 0x7f43213aa9c5 in XRE_main nsAppRunner.cpp:4240
    #28 0x47b07a in do_main nsBrowserApp.cpp:214
    #29 0x7f432a8faec4 in __libc_start_main libc-start.c:287
    #30 0x47a54a in _start ??:?

Eric, is this related or something completely different?
Flags: needinfo?(erahm)
This is somewhat different, I don't think we're seeing a buffer overrun in this case. The stack looks a little off though...

ConsumeToken nsExpatDriver.cpp:1184 [1]:
>   HandleError();
so not a string call. My guess is it's really the append call a few lines up [2]:
>   mLastLine.Append(Substring(buffer, buffer + endOffset));

Which indeed could be rather large given the size of the input. TBH, it's not clear to me *why* we're doing this "find the next newline and append" logic at all (I guess it's for the error message?)

[1] https://hg.mozilla.org/mozilla-central/annotate/a0787486ecf5/parser/htmlparser/nsExpatDriver.cpp#l1184
[2] https://hg.mozilla.org/mozilla-central/annotate/a0787486ecf5/parser/htmlparser/nsExpatDriver.cpp#l1175
Flags: needinfo?(erahm)
If this looks somewhat different than the original buffer overrun that was reported, I'll create a new bug to address comment #25 and continue going through verifications on the other channels.

Eric, let me know if this sounds reasonable :)
Flags: needinfo?(erahm)
(In reply to Kamil Jozwiak [:kjozwiak] from comment #27)
> If this looks somewhat different than the original buffer overrun that was
> reported, I'll create a new bug to address comment #25 and continue going
> through verifications on the other channels.
> 
> Eric, let me know if this sounds reasonable :)

Yes a separate follow up sounds fine. In general we need to do a better job of handling large XML streams in nsExpatDriver:
  - We should limit the amount of data we feed to libexpat (something like 20MiB seems reasonable) and just hand it over in chunks
  - Any sort of logging should have a size limitation. This includes debug logs and error messages.
Flags: needinfo?(erahm)
Blocks: 1161062
Whiteboard: [adv-main38+][adv-esr31.7+]
status-firefox40: fixed → verified
Alias: CVE-2015-2716
I reproduced the initial issue using an Asan build from 2015-04-14 using steps from comment 4. To verify the fix I used Firefox 38.0 ESR build 1 and 31.7.0 ESR build 2 and did not encounter any crashes, only hangs on Linux and on Mac the browser also hangs if stay on the tab I loaded localhost, and also I get this in that tab: 

> XML Parsing Error: Out of memory
> Location: http://localhost/
> Line Number 1, Column 1:

Is this expected?
Flags: needinfo?(erahm)
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> I reproduced the initial issue using an Asan build from 2015-04-14 using
> steps from comment 4. To verify the fix I used Firefox 38.0 ESR build 1 and
> 31.7.0 ESR build 2 and did not encounter any crashes, only hangs on Linux
> and on Mac the browser also hangs if stay on the tab I loaded localhost, and
> also I get this in that tab: 
> 
> > XML Parsing Error: Out of memory
> > Location: http://localhost/
> > Line Number 1, Column 1:
> 
> Is this expected?

Yes, per comment 11 and comment 13 an out of memory error page is expected.
Flags: needinfo?(erahm)
(In reply to Eric Rahm [:erahm] from comment #30)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #29)
> > I reproduced the initial issue using an Asan build from 2015-04-14 using
> > steps from comment 4. To verify the fix I used Firefox 38.0 ESR build 1 and
> > 31.7.0 ESR build 2 and did not encounter any crashes, only hangs on Linux
> > and on Mac the browser also hangs if stay on the tab I loaded localhost, and
> > also I get this in that tab: 
> > 
> > > XML Parsing Error: Out of memory
> > > Location: http://localhost/
> > > Line Number 1, Column 1:
> > 
> > Is this expected?
> 
> Yes, per comment 11 and comment 13 an out of memory error page is expected.

Right, thanks Eric. Marking this as verified on 31.7.0 ESR and 38.0 ESR.
status-firefox-esr31: fixed → verified
status-firefox-esr38: fixed → verified
(In reply to Eric Rahm [:erahm] from comment #18)
 
> Note: as mentioned by Peter, this is a bug in libexpat which is not fixed
> upstream. It could effect all users of libexpat.

Has anyone reported this upstream to the maintainers of libexpat, whomever they are?
(In reply to Al Billings [:abillings] from comment #32)
> (In reply to Eric Rahm [:erahm] from comment #18)
>  
> > Note: as mentioned by Peter, this is a bug in libexpat which is not fixed
> > upstream. It could effect all users of libexpat.
> 
> Has anyone reported this upstream to the maintainers of libexpat, whomever
> they are?

I have not, a cursory look seems to indicate it's not being actively developed.

The last release was in 2012 [1], which does seem to indicate there were security fixes. They do seem to have a bug page [2] where some folks have commented on bugs more recently. The best I can tell is that someone named 'Karl Waclawek' is the current maintainer.

[1] http://www.libexpat.org/
[2] http://sourceforge.net/p/expat/bugs/?source=navbar&page=0
Because of Bug # 1161062 (I'm assuming that's the cause), you'll eventually crash fx if you reload the poc a few times in the same session. The unfortunate part is that it doesn't generate a crash report :( I've added more details in Bug # 1161062 comment # 1. There's also an image of the crash message that fx displays.

Went through verification using the following builds:
- https://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/38.0-candidates/build2/linux-x86_64/en-US/
- https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-05-08-00-40-08-mozilla-aurora/

Receive the "XML Parsing Error: out of memory" error rather than crashing.
status-firefox38: fixed → verified
status-firefox39: fixed → verified
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+

Updated

2 years ago
Group: core-security → core-security-release
I think we can open this bug since the issue is public now:
https://codereview.chromium.org/1224303003
http://sourceforge.net/p/expat/bugs/528/

Perhaps hide any STR / PoC first though...
(Reporter)

Comment 36

2 years ago
Think so, even POC is published by chromium dev. http://code.google.com/p/chromium/issues/detail?id=492052
But it's interesting how reporter got same poc, as i attached here.

(In reply to Mats Palmgren (:mats) from comment #35)
> I think we can open this bug since the issue is public now:
> https://codereview.chromium.org/1224303003
> http://sourceforge.net/p/expat/bugs/528/
> 
> Perhaps hide any STR / PoC first though...
(In reply to Ucha Gobejishvili from comment #36)
> But it's interesting how reporter got same poc, as i attached here.

That is odd. The test case is exactly the same, character for character.
(Reporter)

Comment 38

2 years ago
(In reply to Andrew McCreight [:mccr8] from comment #37)
> (In reply to Ucha Gobejishvili from comment #36)
> > But it's interesting how reporter got same poc, as i attached here.
> 
> That is odd. The test case is exactly the same, character for character.

Agree! is it related to bugzilla breach?
It might be. It appears our compromised account loaded this bug and that testcase once, on May 14, 2015.
(Reporter)

Comment 40

2 years ago
(In reply to Daniel Veditz [:dveditz] from comment #39)
> It might be. It appears our compromised account loaded this bug and that
> testcase once, on May 14, 2015.

Well from my side, i can confirm that only place i submitted testcase, it's here in this thread.
False alarm. We released the fix for this bug on May 12. It was reported to Google after that date by the RedHat Firefox package maintainer, Huzaifa, who had legitimate access to this testcase.
It's a bit unfortunate that information that we haven't disclosed (a PoC in a non-public bug
in this case) is posted in a public forum (a Chromium bug in this case).

Anyway, I guess we can open this bug now?
Indeed, to both points.
Group: core-security-release

Comment 44

a year ago
For the sake of anyone reading through to comment 44, I should point out that the fix at https://bug1140537.bmoattachments.org/attachment.cgi?id=8592473 relies on signed arithmetic overflows producing 2's complement result. This is fine for the few people who compile with, say, “gcc -fwrapv”, but not for the majority.

Here is a copy of the message I also sent to Sebastian Pipping, with whom I had been discussing expat:

____

Please look at the following link. It illustrates how counter-intuitive compiler optimizations have become since the late nineties:

https://godbolt.org/g/Zl8gdF

Starting from a positive x, GCC knows that multiplying x by 2, x can only stay positive or overflow. Signed overflow is undefined behavior, so the compiler is allowed to fly daemons out of your nose if that happens. In this case, the compiler reserves the daemons but decides to compile the condition “x > 0” to always true and the test “x <= 0” to always false.

The last two chunks in the patch that was supposed to fix CVE-2015-1283 (https://sourceforge.net/p/expat/bugs/528/ ) assume that overflows in adding two positives numbers or doubling a positive number can be recognized by testing for a negative result. The last chunk in particular is almost exactly like my example. 

The patch to CVE-2015-1283 is extremely dangerous, as a sufficiently smart compiler will recognize that these conditions are only true or only false for undefined signed overflow, and assume in consequence that they do not happen. This has been reported as a compiler bug over and over (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=30475 https://gcc.gnu.org/bugzilla/show_bug.cgi?id=49820 ). Compiler authors do not treat it as a bug and will not change this behavior. This applies to both GCC and Clang.

The only thing that possibly still prevents current compilers from applying the same transformation in the case of the last chunk, for instance, is if they are not able to infer that bufferSize starts positive. I'm not sure that they aren't able to infer this, and this is subject to change.

A quick fix to these two chunks might look like:

diff --git a/expat/lib/xmlparse.c b/expat/lib/xmlparse.c
index 7586b24..620a820 100644
--- a/expat/lib/xmlparse.c
+++ b/expat/lib/xmlparse.c
@@ -1730,7 +1730,8 @@ XML_GetBuffer(XML_Parser parser, int len)
 #ifdef XML_CONTEXT_BYTES
     int keep;
 #endif  /* defined XML_CONTEXT_BYTES */
-    int neededSize = len + (int)(bufferEnd - bufferPtr);
+    /* Do not invoke signed arithmetic overflow: */
+    int neededSize = (int) ((unsigned)len + (unsigned)(bufferEnd - bufferPtr));
     if (neededSize < 0) {
       errorCode = XML_ERROR_NO_MEMORY;
       return NULL;
@@ -1761,7 +1762,8 @@ XML_GetBuffer(XML_Parser parser, int len)
       if (bufferSize == 0)
         bufferSize = INIT_BUFFER_SIZE;
       do {
-        bufferSize *= 2;
+        /* Do not invoke signed arithmetic overflow: */
+        bufferSize = (int) (2U * (unsigned) bufferSize);
       } while (bufferSize < neededSize && bufferSize > 0);
       if (bufferSize <= 0) {
         errorCode = XML_ERROR_NO_MEMORY;
Depends on: 1272020
Thanks for the explanation. I filed bug 1272020 based on your initial report. We have a CheckedInt class that should probably used liberally in this file.
You need to log in before you can comment on or make changes to this bug.