785753 - (CVE-2012-4185) Global-buffer-overflow in nsCharTraits::length

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Atte Kettunen]

Attachment: Original repro-file.

Tested with build from 

http://people.mozilla.org/~choller/firefox/asan/20120826-mozilla-central-linux64-debug-b3cce81fef1a+asan.html

ASAN-report from the original repro-file:

###!!! ASSERTION: Decoder returned an error but filled the output buffer! Should not happen.: '0 < capacity - haveRead', file /builds/slave/try-lnx64-dbg/build/netwerk/base/src/nsUnicharStreamLoader.cpp, line 225
=================================================================
==1146== ERROR: AddressSanitizer global-buffer-overflow on address 0x7f80ca470002 at pc 0x7f80c2dce5ce bp 0x7ffff1b53d20 sp 0x7ffff1b53d18
READ of size 2 at 0x7f80ca470002 thread T0
    #0 0x7f80c2dce5ce in nsCharTraits<unsigned short>::length(unsigned short const*) /builds/slave/try-lnx64-dbg/build/../../../dist/include/nsCharTraits.h:345
    #1 0x7f80c52900bc in nsAString_internal::Assign(unsigned short const*, unsigned int, mozilla::fallible_t const&) /builds/slave/try-lnx64-dbg/build/xpcom/string/src/nsTSubstring.cpp:304
    #2 0x7f80c5290019 in nsAString_internal::Assign(unsigned short const*, unsigned int) /builds/slave/try-lnx64-dbg/build/xpcom/string/src/nsTSubstring.cpp:290
    #3 0x7f80c2e723c3 in nsString::operator=(unsigned short const*) /builds/slave/try-lnx64-dbg/build/../../dist/include/nsTString.h:65
    #4 0x7f80c46c11a7 in nsDocShell::SetTitle(unsigned short const*) /builds/slave/try-lnx64-dbg/build/docshell/base/nsDocShell.cpp:5236
    #5 0x7f80c46c16b0 in non-virtual thunk to nsDocShell::SetTitle(unsigned short const*) /builds/slave/try-lnx64-dbg/build/media/libvpx/vp8/encoder/x86/quantize_mmx.asm:0
    #6 0x7f80c35ecf4a in nsDocument::DoNotifyPossibleTitleChange() /builds/slave/try-lnx64-dbg/build/content/base/src/nsDocument.cpp:5288
    #7 0x7f80c360a34a in nsRunnableMethodImpl<void (nsDocument::*)(), false>::Run() /builds/slave/try-lnx64-dbg/build/../../../dist/include/nsThreadUtils.h:349
    #8 0x7f80c5248003 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/try-lnx64-dbg/build/xpcom/threads/nsThread.cpp:624
    #9 0x7f80c5199a93 in NS_ProcessNextEvent_P(nsIThread*, bool) /builds/slave/try-lnx64-dbg/build/obj-firefox/xpcom/build/nsThreadUtils.cpp:220
    #10 0x7f80c4ecd749 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/try-lnx64-dbg/build/ipc/glue/MessagePump.cpp:82
    #11 0x7f80c52cdca2 in MessageLoop::RunInternal() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:209
    #12 0x7f80c52cdb9f in MessageLoop::Run() /builds/slave/try-lnx64-dbg/build/ipc/chromium/src/base/message_loop.cc:176
    #13 0x7f80c4c70c82 in nsBaseAppShell::Run() /builds/slave/try-lnx64-dbg/build/widget/xpwidgets/nsBaseAppShell.cpp:165
    #14 0x7f80c482cde1 in nsAppStartup::Run() /builds/slave/try-lnx64-dbg/build/toolkit/components/startup/nsAppStartup.cpp:273
    #15 0x7f80c2abca75 in XREMain::XRE_mainRun() /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3800
    #16 0x7f80c2abdb43 in XREMain::XRE_main(int, char**, nsXREAppData const*) /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3877
    #17 0x7f80c2abe4e2 in XRE_main /builds/slave/try-lnx64-dbg/build/toolkit/xre/nsAppRunner.cpp:3953
    #18 0x408dec in do_main(int, char**) /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:174
    #19 0x4085db in main /builds/slave/try-lnx64-dbg/build/browser/app/nsBrowserApp.cpp:279
    #20 0x7f80cd06d76d in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:258
0x7f80ca470002 is located 0 bytes to the right of global variable 'gNullChar (/builds/slave/try-lnx64-dbg/build/xpcom/string/src/nsSubstring.cpp)' (0x7f80ca470000) of size 2
==1146== ABORTING
Stats: 235M malloced (239M for red zones) by 324882 calls
Stats: 30M realloced by 21484 calls
Stats: 202M freed by 199576 calls
Stats: 68M really freed by 103398 calls
Stats: 436M (111696 full pages) mmaped in 109 calls
  mmaps   by size class: 8:196596; 9:32764; 10:20475; 11:16376; 12:3072; 13:1536; 14:1280; 15:256; 16:448; 17:1248; 18:192; 19:40; 20:16;
  mallocs by size class: 8:236195; 9:35940; 10:24728; 11:19418; 12:2929; 13:1796; 14:1466; 15:270; 16:632; 17:1255; 18:195; 19:44; 20:14;
  frees   by size class: 8:131552; 9:24822; 10:20131; 11:16183; 12:1854; 13:1549; 14:1301; 15:227; 16:520; 17:1244; 18:140; 19:41; 20:12;
  rfrees  by size class: 8:69590; 9:9871; 10:11815; 11:9747; 12:640; 13:470; 14:487; 15:107; 16:301; 17:358; 18:6; 19:5; 20:1;
Stats: malloc large: 1508 small slow: 1931
Shadow byte and word:
  0x1ff01948e000: 2
  0x1ff01948e000: 02 f9 f9 f9 f9 f9 f9 f9
More shadow bytes:
  0x1ff01948dfe0: 00 00 f9 f9 f9 f9 f9 f9
  0x1ff01948dfe8: 00 f9 f9 f9 f9 f9 f9 f9
  0x1ff01948dff0: 00 00 f9 f9 f9 f9 f9 f9
  0x1ff01948dff8: 00 f9 f9 f9 f9 f9 f9 f9
=>0x1ff01948e000: 02 f9 f9 f9 f9 f9 f9 f9
  0x1ff01948e008: 00 00 00 f9 f9 f9 f9 f9
  0x1ff01948e010: 00 f9 f9 f9 f9 f9 f9 f9
  0x1ff01948e018: 00 f9 f9 f9 f9 f9 f9 f9
  0x1ff01948e020: 00 f9 f9 f9 f9 f9 f9 f9

Comment 1

[Atte Kettunen]

Attachment: Contains two lines that both can reproduce this issue.

This issue can be reproduced with both lines from the second attachment. I couldn't paste those directly into the bugzilla.

Smaller testcases have slightly different asan-output so those should be checked also. 

###!!! ASSERTION: Decoder returned an error but filled the output buffer! Should not happen.: '0 < capacity - haveRead', file /builds/slave/try-lnx64-dbg/build/netwerk/base/src/nsUnicharStreamLoader.cpp, line 225
WARNING: Subdocument container has no frame: file /builds/slave/try-lnx64-dbg/build/layout/base/nsDocumentViewer.cpp, line 2401
++DOMWINDOW == 13 (0x7f87f4491100) [serial = 15] [outer = 0x7f87f4131080]
=================================================================
==1193== ERROR: AddressSanitizer global-buffer-overflow on address 0x7f881d444002 at pc 0x42e1b9 bp 0x7fff128db2b0 sp 0x7fff128db290
READ of size 1 at 0x7f881d444002 thread T0
    #0 0x42e1b9 in strlen ??:0
    #1 0x7f88182688fc in nsACString_internal::Assign(char const*, unsigned int, mozilla::fallible_t const&) /builds/slave/try-lnx64-dbg/build/xpcom/string/src/nsTSubstring.cpp:304
    #2 0x7f8818268859 in nsACString_internal::Assign(char const*, unsigned int) /builds/slave/try-lnx64-dbg/build/xpcom/string/src/nsTSubstring.cpp:290
    #3 0x7f8815a7fe63 in nsCString::operator=(char const*) /builds/slave/try-lnx64-dbg/build/../../dist/include/nsTString.h:65
    #4 0x7f8815b4e7f1 in nsStandardURL::Init(unsigned int, int, nsACString_internal const&, char const*, nsIURI*) /builds/slave/try-lnx64-dbg/build/netwerk/base/src/nsStandardURL.cpp:2628
    #5 0x7f8815e0d8e5 in nsJARURI::SetSpecWithBase(nsACString_internal const&, nsIURI*) /builds/slave/try-lnx64-dbg/build/modules/libjar/nsJARURI.cpp:264
    #6 0x7f8815e03783 in nsJARProtocolHandler::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) /builds/slave/try-lnx64-dbg/build/modules/libjar/nsJARProtocolHandler.cpp:134
    #7 0x7f8815b0f1e3 in nsIOService::NewURI(nsACString_internal const&, char const*, nsIURI*, nsIURI**) /builds/slave/try-lnx64-dbg/build/netwerk/base/src/nsIOService.cpp:552
    #8 0x7f8815b2720d in NS_NewURI(nsIURI**, nsACString_internal const&, char const*, nsIURI*, nsIIOService*) /builds/slave/try-lnx64-dbg/build/../../dist/include/nsNetUtil.h:142
    #9 0x7f881818a327 in nsChromeRegistry::ConvertChromeURL(nsIURI*, nsIURI**) /builds/slave/try-lnx64-dbg/build/chrome/src/nsChromeRegistry.cpp:310
    #10 0x7f881819454e in nsChromeProtocolHandler::NewChannel(nsIURI*, nsIChannel**) /builds/slave/try-lnx64-dbg/build/chrome/src/nsChromeProtocolHandler.cpp:151
.
.
.

Comment 3

[Mats Palmgren (inactive)]

nsUnicharStreamLoader::DetermineCharset() calls WriteSegmentFun with
aSegment="p" and aCount=1.
http://hg.mozilla.org/mozilla-central/annotate/8af2ff9c6018/netwerk/base/src/nsUnicharStreamLoader.cpp#l191

nsUTF16ToUnicodeBase::GetMaxLength return zero:
http://hg.mozilla.org/mozilla-central/annotate/8af2ff9c6018/intl/uconv/ucvlatin/nsUCS2BEToUnicode.cpp#l173
(mState is STATE_FIRST_CALL, so 1 / 2 => 0)

WriteSegmentFun calls self->mBuffer.SetCapacity(0)

nsUTF16BEToUnicode::Convert returns NS_ERROR_ILLEGAL_INPUT
and set *aSrcLength=0, *aDestLength=0
http://hg.mozilla.org/mozilla-central/annotate/8af2ff9c6018/intl/uconv/ucvlatin/nsUCS2BEToUnicode.cpp#l187

WriteSegmentFun overwrites the buffer with 0xFFFD at line 226.
This code was added in bug 541496 (part 4).

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

I'm not sure what this error correction stuff is trying
to accomplish, but we shouldn't let it write past the
buffer end anyway.

Comment 5

[Zack Weinberg (:zwol)]

I wrote this code long enough ago that I don't remember what I was thinking exactly.  It surprises me that GetMaxLength can set dstLen to zero when srcLen is nonzero.

The quick fix would be to replace the assertion with a buffer resize.  I am not in a position to write code right now (in the middle of a cross-country move) -- I can do it in about a week, but since this is a genuine buffer overrun, perhaps someone else should do it first :)

Comment 6

[Zack Weinberg (:zwol)]

For "what this error correction stuff is trying to accomplish", see the language about REPLACEMENT CHARACTER in http://dev.w3.org/csswg/css3-syntax/#the-input-byte-stream .  I thought there was language to the same effect somewhere in CSS 2.1 but can't find it at the moment.

Comment 7

[Zack Weinberg (:zwol)]

Comment on attachment 656001 [details] [diff] [review]
fix

It's not clear to me whether we can hit this case anywhere but at EOF.  If we can, then breaking out of the loop would cause an otherwise-valid style sheet to be truncated at the invalid byte sequence, which would be a conformance violation on our part.

Comment 8

[Mats Palmgren (inactive)]

Attachment: fix v2

OK, like this then?

Comment 9

[Zack Weinberg (:zwol)]

Comment on attachment 656034 [details] [diff] [review]
fix v2

This looks right to me, but let's run it by bz as well.

Test cases would be nice :)

Comment 10

[Mats Palmgren (inactive)]

> Test cases would be nice :)

Sure, but I intentionally never include tests together with
the fix for security bugs to avoid unintentionally exposing
them when pushing to Try or when landing the fix.
Tests should only land after the bug is public, generally.

Comment 11

[Mats Palmgren (inactive)]

Attachment: crash tests (DO NOT LAND BEFORE BUG IS PUBLIC)

Comment 12

[Mats Palmgren (inactive)]

I suspect this is exploitable, though it looks hard to do so to me.

Comment 13

[Zack Weinberg (:zwol)]

That's all very well, but what I had in mind for tests was something a bit more thorough: reftests, probably, making use of otherwise-well-formed style sheets with intruded invalid byte sequences, in several different supported encodings, placed at threshold points in the input stream to tickle all of the boundary conditions.

(This becomes a good deal less important if we can determine that the bad assertion can only happen for an invalid byte sequence immediately followed by EOF; I am then content to assume it doesn't occur in non-malicious code.)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 656034 [details] [diff] [review]
fix v2

r=me

Comment 15

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5e5d1e9a4a76

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5e5d1e9a4a76

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 656034 [details] [diff] [review]
fix v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 541496 (part 4)
User impact if declined: possibly exploitable buffer overflow
Testing completed (on m-c, etc.): on m-c since 2012-08-30
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: none

Comment 18

[Mats Palmgren (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1dd7f8d9061d
https://hg.mozilla.org/releases/mozilla-beta/rev/4a4ec8fa0cf4
https://hg.mozilla.org/releases/mozilla-esr10/rev/75f39b23569a

ESR only have a fallible SetCapacity method so I made this change:
(intra-diff between reviewed patch and what I pushed)

< +        if (!self->mBuffer.SetCapacity(haveRead + 1, fallible_t())) {
---
> +        if (!self->mBuffer.SetCapacity(haveRead + 1)) {

Comment 19

[Jesse Ruderman]

Mats, I think you can land the crashtest now.

Comment 20

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old

Comment 21

[Mats Palmgren (inactive)]

Landed the crashtests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85b064039fc

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d85b064039fc