Closed Bug 1236923 (CVE-2016-0718) Opened 9 years ago Closed 8 years ago

Heap read out-of-bound and crash in expat 2.1.0

Categories

(Core :: XML, defect)

Product:
Core
Component:
XML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox46 --- wontfix
firefox47 + wontfix
firefox48 + fixed
firefox49 + fixed
firefox-esr45 --- wontfix

People

(Reporter: gustavo.grieco, Assigned: erahm)

References

Details

(Keywords: csectype-intoverflow, reporter-external, sec-moderate, Whiteboard: [adv-main48+] RedHat CVE-2016-0718 Embargo until May 17, 2016 21:00 UTC+2 (noon PDT))

Attachments

(6 files, 1 obsolete file)

overflow.xml
615 bytes, text/xml
Details
CVE-2016-0718 Advisory
792 bytes, text/plain
Details
CVE-2016-0718 patch against v2-1-1-1
25.00 KB, patch
Details | Diff | Splinter Review
overflow testcases
1.64 KB, application/x-bzip2
Details
id:000000,sig:06,src:001773+001086,op:splice,rep:2
615 bytes, application/xml
Details
Check int for overflow
1.78 KB, patch
peterv
: review+
gchang
: approval-mozilla-beta+
Details | Diff | Splinter Review
Attached file overflow.xml — 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151210085006

Steps to reproduce:

A read out-of-bound in the heap was detected in expat 2.1.0 (tested in Ubuntu 14.04). There is no crash in Firefox opening the xml, but I don't know if this affect the expat code in the Mozilla repositories anyway.


Actual results:

$ xmlwf overflow.xml

==16291== ERROR: AddressSanitizer: heap-buffer-overflow on address 0xb5a03e80 at pc 0x81021d0 bp 0xbfffeb48 sp 0xbfffeb3c
READ of size 1 at 0xb5a03e80 thread T0
    #0 0x81021cf (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x81021cf)
    #1 0x8068791 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x8068791)
    #2 0x80a1f61 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x80a1f61)
    #3 0x80c88a2 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x80c88a2)
    #4 0x80edd44 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x80edd44)
    #5 0x8051f89 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x8051f89)
    #6 0x805299d (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x805299d)
    #7 0x8052327 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x8052327)
    #8 0x804a6df (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x804a6df)
    #9 0xb6851a82 (/lib/i386-linux-gnu/libc-2.19.so+0x19a82)
    #10 0x804b755 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x804b755)
0xb5a03e80 is located 0 bytes to the right of 2048-byte region [0xb5a03680,0xb5a03e80)
allocated by thread T0 here:
    #0 0xb69fc854 (/usr/lib/i386-linux-gnu/libasan.so.0.0.0+0x16854)
    #1 0x80eee06 (/home/vagrant/afl-tests/progs/expat-2.1.0/xmlwf/xmlwf+0x80eee06)
Shadow bytes around the buggy address:
  0x36b40780: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x36b40790: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x36b407a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x36b407b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x36b407c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x36b407d0:[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b407e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b407f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b40800: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b40810: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x36b40820: 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 righ 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
==16291== ABORTING

Program received signal SIGABRT, Aborted.
0xb7fdd428 in __kernel_vsyscall ()
(gdb) bt
#0  0xb7fdd428 in __kernel_vsyscall ()
#1  0xb6866607 in __GI_raise (sig=sig@entry=6) at ../nptl/sysdeps/unix/sysv/linux/raise.c:56
#2  0xb6869a33 in __GI_abort () at abort.c:89
#3  0xb6a042e4 in ?? () from /usr/lib/i386-linux-gnu/libasan.so.0
#4  0xb69f858a in ?? () from /usr/lib/i386-linux-gnu/libasan.so.0
#5  0xb6a00f4b in ?? () from /usr/lib/i386-linux-gnu/libasan.so.0
#6  0xb69ffd3a in __asan_report_error () from /usr/lib/i386-linux-gnu/libasan.so.0
#7  0xb69f88ff in __asan_report_load1 () from /usr/lib/i386-linux-gnu/libasan.so.0
#8  0x081021d0 in little2_toUtf8 (enc=<optimized out>, fromP=<optimized out>, fromLim=<optimized out>, toP=<optimized out>, toLim=<optimized out>)
    at lib/xmltok.c:620
#9  0x08068792 in poolAppend (end=0xb5a038e7 "", ptr=0xb5a038d4 "a", enc=0x816caa0 <little2_encoding>, pool=0xb6003f90) at lib/xmlparse.c:6170
#10 poolStoreString (pool=0xb6003f90, enc=0x816caa0 <little2_encoding>, ptr=<optimized out>, end=0xb5a038e7 "") at lib/xmlparse.c:6220
#11 0x080a1f62 in doProlog (parser=parser@entry=0xb6603c80, enc=0x816caa0 <little2_encoding>, s=0xb5a038d4 "a", s@entry=0xb5a03680 "\377\376<",
    end=end@entry=0xb5a038e7 "", tok=<optimized out>, tok@entry=14, next=<optimized out>, nextPtr=nextPtr@entry=0xb6603c98, haveMore=<optimized out>)
    at lib/xmlparse.c:4293
#12 0x080c88a3 in prologProcessor (nextPtr=0xb6603c98, end=0xb5a038e7 "", s=0xb5a03680 "\377\376<", parser=0xb6603c80) at lib/xmlparse.c:3758
#13 prologInitProcessor (parser=0xb6603c80, s=0xb5a03680 "\377\376<", end=0xb5a038e7 "", nextPtr=0xb6603c98) at lib/xmlparse.c:3575
#14 0x080edd45 in XML_ParseBuffer (parser=0xb6603c80, len=615, isFinal=1) at lib/xmlparse.c:1651
#15 0x080efbd8 in XML_Parse (parser=parser@entry=0xb6603c80, s=0x267 <error: Cannot access memory at address 0x267>, s@entry=0xb7fd2000 "\377\376<",
    len=1, len@entry=615, isFinal=isFinal@entry=1) at lib/xmlparse.c:1617
#16 0x08051f8a in processFile (data=data@entry=0xb7fd2000, size=size@entry=615, filename=filename@entry=0xbffff829 "overflow.xml", args=0xbffff540)
    at xmlwf/xmlfile.c:82
#17 0x0805299e in filemap (name=name@entry=0xbffff829 "overflow.xml", processor=processor@entry=0x8051ea0 <processFile>, arg=arg@entry=0xbffff540)
    at xmlwf/unixfilemap.c:61
#18 0x08052328 in XML_ProcessFile (parser=parser@entry=0xb6603c80, filename=0xbffff829 "overflow.xml", flags=flags@entry=1) at xmlwf/xmlfile.c:238
#19 0x0804a6e0 in main (argc=2, argv=0xbffff6c4) at xmlwf/xmlwf.c:847

If you try to continue the execution of xmlwf, it will usually crash. 


Expected results:

Parsing XML shouldn't perform any memory unsafe operations.
What version of expat do we have in our tree?
Group: core-security → dom-core-security
Flags: needinfo?(erahm)
We have a moderately patched version of 2.0.0. I believe some unused files have been removed as well.
Flags: needinfo?(erahm)
If this ends up affecting us RedHat assigned CVE-2016-0718
Whiteboard: RedHat CVE-2016-0718
Peter, as XML module owner (congrats, I never knew!) do you want to take a look at this?
Flags: needinfo?(peterv)
(In reply to Andrew Overholt [:overholt] from comment #4)
> Peter, as XML module owner (congrats, I never knew!) do you want to take a
> look at this?

I just want to reiterate that this does not affect Firefox. This looks like an off by 1 issue w/ character conversion (it's similar to a previous bug), but we always pass in guaranteed 2-byte wide chars (char16).
(In reply to Eric Rahm [:erahm] from comment #5)
> (In reply to Andrew Overholt [:overholt] from comment #4)
> > Peter, as XML module owner (congrats, I never knew!) do you want to take a
> > look at this?
> 
> I just want to reiterate that this does not affect Firefox. This looks like
> an off by 1 issue w/ character conversion (it's similar to a previous bug),
> but we always pass in guaranteed 2-byte wide chars (char16).

I was thinking of bug 1217616, comment 4.
(In reply to Eric Rahm [:erahm] from comment #5)
> (In reply to Andrew Overholt [:overholt] from comment #4)
> > Peter, as XML module owner (congrats, I never knew!) do you want to take a
> > look at this?
> 
> I just want to reiterate that this does not affect Firefox. This looks like
> an off by 1 issue w/ character conversion (it's similar to a previous bug),
> but we always pass in guaranteed 2-byte wide chars (char16).

Oh, great! Sorry for missing that from your earlier comments.

Do we just wait for a fixed expat to be imported before we close this?
Flags: needinfo?(peterv)
Is it the same with the other bug?

https://bugzilla.mozilla.org/show_bug.cgi?id=1237229
Keywords: sec-other
Alias: CVE-2016-0718
Whiteboard: RedHat CVE-2016-0718 → RedHat CVE-2016-0718 Embargo until May 17, 2016 21:00 UTC+2 (noon PDT)
Attached file CVE-2016-0718 Advisory — 
Advisory for this issue from upstream
Attached file overflow testcases — 
Contains two additional testcases in addition to the original overflow.xml
Eric: given the patch and the two additional testcases, does that change your assessment that this does not affect Firefox?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(erahm)
Keywords: sec-other
As it's isolated to character conversion this still does not affect Firefox. I checked the new test cases and we present an XML parsing error as expected.

Cherry picking the changes to |poolGrow| might make sense, but I'm fairly certain that's only used in character conversion. I'll double-check that now.
Flags: needinfo?(erahm)
Alright it seems like poolGrow is sometimes used (maybe w/ XUL?). We should probably cherry-pick that portion of the patch, but given that it's limited to XUL I'm not super worried about it. Patch forthcoming.
It appears that one source is RDF, digging deeper now.
Attached patch Check int for overflow (obsolete) — — Splinter Review 
This cherry-picks the integer overflow checks in poolGrow. We don't need to
worry about conversion, we're always UTF-16.
Attachment #8751038 - Flags: review?(peterv)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Marking as tracking:? for 46 now that we know it may be a problem.
tracking-firefox46: --- → ?
So, is it possible to trigger the integer overflow in poolGrow parsing a RDF feed?
(In reply to Gustavo Grieco from comment #19)
> So, is it possible to trigger the integer overflow in poolGrow parsing a RDF
> feed?

I'm not sure, all I know is that when loading xul documents we pass through an RDF parser that is using expat in a way that calls poolGrow. Loading the provided XML documents did not (nor did a sampling of other large XML files).
Attachment #8751038 - Flags: review?(peterv) → review+
poolGrow does seem to be used in a couple of places, looks like mostly entity code. So we probably can end up there even for non-RDF/XUL files. But I don't know if it's possible to trigger the overflow in that way.
Just to be sure, i put a breakpoint in poolGrow and then try to load a corrupted xml file. I got several calls to poolGrow, but this is the most interesting one:

Breakpoint 1, poolGrow (pool=0x7fffcb59d348) at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:6261
6261	in /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c
(gdb) bt
#0  poolGrow (pool=0x7fffcb59d348) at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:6261
#1  0x00007fffeb71cb2f in poolAppend (pool=pool@entry=0x7fffcb59d348, enc=0x7fffedcc6360 <little2_encoding_ns>, ptr=0x7fffcb59b43e "1", 
    end=0x7fffcb59b444 "\"") at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:6198
#2  0x00007fffeb71cb79 in poolStoreString (pool=pool@entry=0x7fffcb59d348, enc=<optimized out>, ptr=<optimized out>, end=<optimized out>)
    at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:6251
#3  0x00007fffeb71cd18 in processXmlDecl (parser=parser@entry=0x7fffcb59d000, isGeneralTextEntity=<optimized out>, s=0x7fffcb59b420 "<", 
    next=0x7fffcb59b44a "\n") at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:3510
#4  0x00007fffeb720734 in doProlog (parser=parser@entry=0x7fffcb59d000, enc=0x7fffedcc6360 <little2_encoding_ns>, s=s@entry=0x7fffcb59b420 "<", 
    end=end@entry=0x7fffcb59b684 "\345\345\345", <incomplete sequence \345>, tok=12, next=<optimized out>, nextPtr=nextPtr@entry=0x7fffffffc3e8, 
    haveMore=1 '\001') at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:3911
#5  0x00007fffeb721d8e in prologProcessor (parser=0x7fffcb59d000, s=0x7fffcb59b420 "<", 
    end=0x7fffcb59b684 "\345\345\345", <incomplete sequence \345>, nextPtr=0x7fffffffc3e8)
    at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:3784
#6  0x00007fffeb7239d0 in MOZ_XML_Parse (parser=0x7fffcb59d000, s=0x7fffcb59b420 "<", len=612, isFinal=0)
    at /build/firefox-meWOta/firefox-46.0+build5/parser/expat/lib/xmlparse.c:1528
#7  0x00007fffea2f6c3d in nsExpatDriver::ParseBuffer (this=this@entry=0x7fffcb4967a0, 
    aBuffer=aBuffer@entry=0x7fffcb59b420 u"<?xml version=\"1.0\"?>\n\n<!DOCTYPE spec SYSTEM \"spec.dtd\" [\n\n\n\n<!-- 日本語訳のための解析対象実体（ここから） -->\n\n\n\n<!ENTITY TR-or-Rec", ' ' <repeats 13 times>, "\"仕様書\">\n\n<!-- <!ENTITY TR-or-Rec", ' ' <repeats 13 times>, "\"標準情報(TR)\"> -->\n\n<!ENTITY eTR-o"..., aLength=aLength@entry=306, aIsFinal=aIsFinal@entry=false, 
    aConsumed=aConsumed@entry=0x7fffffffc4ac) at /build/firefox-meWOta/firefox-46.0+build5/parser/htmlparser/nsExpatDriver.cpp:1014
#8  0x00007fffea2faec5 in nsExpatDriver::ConsumeToken (this=0x7fffcb4967a0, aScanner=..., aFlushTokens=<optimized out>)
    at /build/firefox-meWOta/firefox-46.0+build5/parser/htmlparser/nsExpatDriver.cpp:1110
#9  0x00007fffea2f98a5 in nsParser::Tokenize (this=this@entry=0x7fffd2980480, aIsFinalChunk=aIsFinalChunk@entry=false)
    at /build/firefox-meWOta/firefox-46.0+build5/parser/htmlparser/nsParser.cpp:1945
#10 0x00007fffea2fa249 in nsParser::ResumeParse (this=this@entry=0x7fffd2980480, allowIteration=allowIteration@entry=true, 
    aIsFinalChunk=aIsFinalChunk@entry=false, aCanInterrupt=aCanInterrupt@entry=true)
    at /build/firefox-meWOta/firefox-46.0+build5/parser/htmlparser/nsParser.cpp:1464
#11 0x00007fffea2fab81 in nsParser::OnDataAvailable (this=0x7fffd2980480, request=0x7fffc5024848, aContext=<optimized out>, pIStream=<optimized out>, 
    sourceOffset=<optimized out>, aLength=<optimized out>) at /build/firefox-meWOta/firefox-46.0+build5/parser/htmlparser/nsParser.cpp:1842
#12 0x00007fffe9f3b9f3 in mozilla::net::nsHttpChannel::OnDataAvailable (this=0x7fffc5024800, request=<optimized out>, ctxt=<optimized out>, 
    input=0x7fffc5f83da0, offset=<optimized out>, count=615) at /build/firefox-meWOta/firefox-46.0+build5/netwerk/protocol/http/nsHttpChannel.cpp:6092
#13 0x00007fffe9e53cfc in nsInputStreamPump::OnStateTransfer (this=this@entry=0x7fffd415e470)
    at /build/firefox-meWOta/firefox-46.0+build5/netwerk/base/nsInputStreamPump.cpp:603
#14 0x00007fffe9e53e63 in nsInputStreamPump::OnInputStreamReady (this=0x7fffd415e470, stream=<optimized out>)
    at /build/firefox-meWOta/firefox-46.0+build5/netwerk/base/nsInputStreamPump.cpp:430
#15 0x00007fffe9de1d4e in nsInputStreamReadyEvent::Run (this=0x7fffd96aa100)
    at /build/firefox-meWOta/firefox-46.0+build5/xpcom/io/nsStreamUtils.cpp:94
#16 0x00007fffe9df43d3 in nsThread::ProcessNextEvent (this=0x7ffff6bd5f80, aMayWait=<optimized out>, aResult=0x7fffffffca1f)
---Type <return> to continue, or q <return> to quit---
    at /build/firefox-meWOta/firefox-46.0+build5/xpcom/threads/nsThread.cpp:995
#17 0x00007fffe9e0bde9 in NS_ProcessNextEvent (aThread=<optimized out>, aMayWait=aMayWait@entry=false)
    at /build/firefox-meWOta/firefox-46.0+build5/xpcom/glue/nsThreadUtils.cpp:297
#18 0x00007fffe9ff48f6 in mozilla::ipc::MessagePump::Run (this=0x7ffff6bf8fc0, aDelegate=0x7ffff6b8af00)
    at /build/firefox-meWOta/firefox-46.0+build5/ipc/glue/MessagePump.cpp:95
#19 0x00007fffe9fe0b87 in RunHandler (this=0x7ffff6b8af00) at /build/firefox-meWOta/firefox-46.0+build5/ipc/chromium/src/base/message_loop.cc:227
#20 MessageLoop::Run (this=0x7ffff6b8af00) at /build/firefox-meWOta/firefox-46.0+build5/ipc/chromium/src/base/message_loop.cc:201
#21 0x00007fffeafa8669 in nsBaseAppShell::Run (this=0x7fffde744400) at /build/firefox-meWOta/firefox-46.0+build5/widget/nsBaseAppShell.cpp:156
#22 0x00007fffeb5557e2 in nsAppStartup::Run (this=0x7fffde708100)
    at /build/firefox-meWOta/firefox-46.0+build5/toolkit/components/startup/nsAppStartup.cpp:281
#23 0x00007fffeb5889e9 in XREMain::XRE_mainRun (this=this@entry=0x7fffffffcc98)
    at /build/firefox-meWOta/firefox-46.0+build5/toolkit/xre/nsAppRunner.cpp:4370
#24 0x00007fffeb588ca3 in XREMain::XRE_main (this=this@entry=0x7fffffffcc98, argc=argc@entry=4, argv=argv@entry=0x7fffffffe1a8, 
    aAppData=aAppData@entry=0x7fffffffcea8) at /build/firefox-meWOta/firefox-46.0+build5/toolkit/xre/nsAppRunner.cpp:4467
#25 0x00007fffeb588f07 in XRE_main (argc=4, argv=0x7fffffffe1a8, aAppData=0x7fffffffcea8, aFlags=<optimized out>)
    at /build/firefox-meWOta/firefox-46.0+build5/toolkit/xre/nsAppRunner.cpp:4569
#26 0x00005555555590b1 in do_main (argc=4, argv=0x7fffffffe1a8, xreDirectory=0x7ffff6b6b900)
    at /build/firefox-meWOta/firefox-46.0+build5/browser/app/nsBrowserApp.cpp:212
#27 0x000055555555880a in main (argc=4, argv=0x7fffffffe1a8) at /build/firefox-meWOta/firefox-46.0+build5/browser/app/nsBrowserApp.cpp:352


This confirms that you can have a call to poolGrow directly using a XML file (part of the parsed XML can be seen in nsExpatDriver::ParseBuffer). Also you can see here how the stack trace looks like in xmlwf when you parse on of the same file (also stopping at poolGrow):

#0  poolGrow (pool=pool@entry=0x62ea80) at lib/xmlparse.c:6230
#1  0x0000000000403e50 in poolAppend (pool=pool@entry=0x62ea80, enc=enc@entry=0x4258c0 <little2_encoding>, ptr=0x63013c "", ptr@entry=0x62f1c4 "a", 
    end=0x62f1d7 "") at lib/xmlparse.c:6173
#2  0x0000000000403ea9 in poolStoreString (pool=pool@entry=0x62ea80, enc=enc@entry=0x4258c0 <little2_encoding>, ptr=ptr@entry=0x62f1c4 "a", 
    end=<optimized out>) at lib/xmlparse.c:6220
#3  0x0000000000408b6c in doProlog (parser=parser@entry=0x62e010, enc=0x4258c0 <little2_encoding>, s=0x62f1c4 "a", s@entry=0x62ef70 "\377\376<", 
    end=end@entry=0x62f1d7 "", tok=18, next=<optimized out>, nextPtr=0x62e040, haveMore=0 '\000') at lib/xmlparse.c:4293
#4  0x000000000040a728 in prologProcessor (parser=0x62e010, s=0x62ef70 "\377\376<", end=0x62f1d7 "", nextPtr=0x62e040) at lib/xmlparse.c:3758
#5  0x000000000040c460 in XML_ParseBuffer (parser=0x62e010, len=<optimized out>, isFinal=1) at lib/xmlparse.c:1651
#6  0x00000000004029e0 in processFile (data=data@entry=0x7ffff7ff7000, size=size@entry=615, 
    filename=filename@entry=0x7fffffffe4a4 "../c2/id:000000,sig:06,src:001773+001086,op:splice,rep:2", args=args@entry=0x7fffffffdfd0)
    at xmlwf/xmlfile.c:82
#7  0x0000000000402ea8 in filemap (name=name@entry=0x7fffffffe4a4 "../c2/id:000000,sig:06,src:001773+001086,op:splice,rep:2", 
    processor=processor@entry=0x4029c0 <processFile>, arg=arg@entry=0x7fffffffdfd0) at xmlwf/unixfilemap.c:61
#8  0x0000000000402d76 in XML_ProcessFile (parser=parser@entry=0x62e010, 
    filename=0x7fffffffe4a4 "../c2/id:000000,sig:06,src:001773+001086,op:splice,rep:2", flags=flags@entry=1) at xmlwf/xmlfile.c:238
#9  0x00000000004013d5 in main (argc=<optimized out>, argv=<optimized out>) at xmlwf/xmlwf.c:847

In the case of xmlwf, poolGrow is called a few times (5 o 6) and it will overflow blockSize. 
Still, i don't have a test case to be sure that an actual integer overflow is possible. Maybe in Firefox you can't force poolGrow to be called enough times to trigger it..
Critsmash triage: we're trying to figure out a security rating on this. Can you suggest one? Given the scope, this seems like a likely sec-moderate until we can see a clear path to the overflow. So far, people don't seem sure that this affects us in an effective way.
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → ?
Flags: needinfo?(erahm)
Flags: needinfo?(peterv)
The provided test cases do not cause a buffer overflow for us. Gustavo, can you provide the corrupted xml file you used in comment 22?
Flags: needinfo?(erahm)
Sure thing.
In this case a call to |poolGrow| that enters the if branches with possible overflow is called 4 times, each time on a new pool.

Each call happens when initializing a pool of size 0 to add a new string. The allocation size will be the default of 1024 due to the pool size being zero.

#1 - copies the encoding name ("UTF-16") into temp pool A
#2 - copies the xml ns (u"xml=http://www.w3.org/XML/1998/namespace") into temp pool B
#3 - copies the xml version ("1") into temp pool C
#4 - copies the first entity value (u"仕様書") into temp pool D

We then encounter a malformed entity name (u"applica污ാ", no allocation is made) and present an XML parsing error page as expected:

> XML Parsing Error: no element found
> Location: http://localhost:8000/exploit.xml
> Line Number 19, Column 19:<!ENTITY applica污ാ
> ------------------^

In this case we have already sanitized the input and the malformed input is handled properly. I don't think it's possible to cause an overflow using malformed entity names as we don't use expat's character conversion utilities.

I do still think we should land this patch as it certainly is possible that some other malicious input not related to character conversion flaws could cause an issue. I'll leave it to the experts to decide the severity.
Oh also I think we were supposed to land this today. Not sure the process for that.
Flags: needinfo?(abillings)
(In reply to Eric Rahm [:erahm] from comment #27)
> Oh also I think we were supposed to land this today. Not sure the process
> for that.

"no earlier than" today. If we don't need to chemspill then we can land when we're confident in it.
This has at least been announced now:
https://marc.ttias.be/oss-security/2016-05/msg00183.php

No details in the text, but maybe there are some in the attachment that doesn't show up in that archive.
Great, I'm glad we can just land this for 47 (target release date June 7). Tracking for 47+ for now since we aren't sure of a rating.
status-firefox46: --- → wontfix
tracking-firefox46: ? → ---
tracking-firefox47: --- → +
tracking-firefox48: --- → +
tracking-firefox49: --- → +
(In reply to Eric Rahm [:erahm] from comment #27)
> Oh also I think we were supposed to land this today. Not sure the process
> for that.

https://wiki.mozilla.org/Security/Bug_Approval_Process

It needs a rating. If the rating is sec-high or sec-critical, it needs sec-approval+ on the patch before it can land. If the rating of this is lower than those (sec-moderate, sec-low or not a security bug) then it can land on trunk as normal though it isn't clear if we'll backport it to branches.
Flags: needinfo?(abillings)
Comment on attachment 8751038 [details] [diff] [review]
Check int for overflow

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

Unsure, the current exploits for this don't work in Firefox.

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

It's fairly obvious, but there is no specific wording.

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?

Patch should apply cleanly.

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

Not likely.
Attachment #8751038 - Flags: sec-approval?
(In reply to Al Billings [:abillings] from comment #31)
> (In reply to Eric Rahm [:erahm] from comment #27)
> > Oh also I think we were supposed to land this today. Not sure the process
> > for that.
> 
> https://wiki.mozilla.org/Security/Bug_Approval_Process
> 
> It needs a rating. If the rating is sec-high or sec-critical, it needs
> sec-approval+ on the patch before it can land. If the rating of this is
> lower than those (sec-moderate, sec-low or not a security bug) then it can
> land on trunk as normal though it isn't clear if we'll backport it to
> branches.

I was primarily concerned about the chemspill aspect of landing on multiple branches at once. Sounds like we're not going that route.
Comment on attachment 8751038 [details] [diff] [review]
Check int for overflow

Making this a sec-moderate. This doesn't need sec-approval as such.
Attachment #8751038 - Flags: sec-approval?
Well, as it turns out one of the overflow checks from the original patch looks not quite thorough enough. Patch forthcoming.
This adds one more overflow check that the CVE patch did not contain.
Attachment #8753579 - Flags: review?(peterv)
Attachment #8751038 - Attachment is obsolete: true
Comment on attachment 8753579 [details] [diff] [review]
Check int for overflow

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

::: parser/expat/lib/xmlparse.c
@@ +6311,5 @@
>      else
>        blockSize *= 2;
> +
> +    if (blockSize < 0)
> +      return XML_FALSE;

This being the missing check.
> This being the missing check.

Good catch. We patched only the integer overflow triggering in our test cases, but it is better to be safe!. After this patch lands in Mozilla code, can you share this improvement in oss-security?
Attachment #8753579 - Flags: review?(peterv) → review+
(In reply to Gustavo Grieco from comment #38)
> > This being the missing check.
> 
> Good catch. We patched only the integer overflow triggering in our test
> cases, but it is better to be safe!. After this patch lands in Mozilla code,
> can you share this improvement in oss-security?

Will do.
Flags: needinfo?(peterv)
Keywords: checkin-needed
Also, the Android security team asked to fix this (obvious?) integer overflow in XML_Parse:

https://github.com/mozilla/gecko-dev/blob/7b9ea8afc579378606ecf3593b04fc2aaba7daec/parser/expat/lib/xmlparse.c

I don't know if Firefox is using such function. Should i open create another issue for that or you want to add it in this patch?
(In reply to Gustavo Grieco from comment #40)
> Also, the Android security team asked to fix this (obvious?) integer
> overflow in XML_Parse:
> 
> https://github.com/mozilla/gecko-dev/blob/
> 7b9ea8afc579378606ecf3593b04fc2aaba7daec/parser/expat/lib/xmlparse.c
> 
> I don't know if Firefox is using such function. Should i open create another
> issue for that or you want to add it in this patch?

I suppose you mean this line: https://github.com/mozilla/gecko-dev/blob/7b9ea8afc579378606ecf3593b04fc2aaba7daec/parser/expat/lib/xmlparse.c#L1568

Yes please file a follow up and we'll get that patched.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef37bc36547c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Group: dom-core-security → core-security-release
Are we going to uplift this to aurora and beta? Thanks
Flags: needinfo?(erahm)
Comment on attachment 8753579 [details] [diff] [review]
Check int for overflow

Looks like the patch applies cleanly.

Approval Request Comment
[Feature/regressing bug #]: N/A
[User impact if declined]: Possible buffer overflow.
[Describe test coverage new/current, TreeHerder]: Baked on central / aurora.
[Risks and why]: Low, been live for a while, just adds integer overflow checks.
[String/UUID change made/needed]: N/A
Flags: needinfo?(erahm)
Attachment #8753579 - Flags: approval-mozilla-beta?
Hi Dan, is this something we'd want to uplift to ESR45? Based on comment 32, this doesn't seem exploitable as such.
Flags: needinfo?(dveditz)
(In reply to Ritu Kothari (:ritu) from comment #46)
> Hi Dan, is this something we'd want to uplift to ESR45? Based on comment 32,
> this doesn't seem exploitable as such.

Yeah I don't think it's really worth pushing to ESR45, we haven't seen any exploitable examples for Firefox and it was deemed a sec-moderate.

On the other hand it's a reasonably simple defensive patch so I still think uplifting to beta might make sense.
Comment on attachment 8753579 [details] [diff] [review]
Check int for overflow

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

Looks like this will fix buffer overflow. Take it in 48 beta 8 and aurora.
Attachment #8753579 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
status-firefox47: affected → wontfix
Whiteboard: RedHat CVE-2016-0718 Embargo until May 17, 2016 21:00 UTC+2 (noon PDT) → [adv-main48+] RedHat CVE-2016-0718 Embargo until May 17, 2016 21:00 UTC+2 (noon PDT)
Flags: sec-bounty?
Because we can't find a path to trigger the vulnerability in Firefox we are not awarding a Bug Bounty for this issue.
Flags: sec-bounty?
Flags: sec-bounty-
Flags: needinfo?(dveditz)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: