Last Comment Bug 785753 - (CVE-2012-4185) Global-buffer-overflow in nsCharTraits::length
(CVE-2012-4185)
: Global-buffer-overflow in nsCharTraits::length
Status: RESOLVED FIXED
[asan][advisory-tracking+]
: assertion, crash, csectype-bounds, reproducible, sec-critical, testcase
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86_64 Linux
: -- critical (vote)
: mozilla18
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-08-26 15:09 PDT by Atte Kettunen
Modified: 2014-07-24 13:44 PDT (History)
8 users (show)
dveditz: sec‑bounty+
mats: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
fixed
fixed
16+
fixed


Attachments
Original repro-file. (13.52 KB, text/plain)
2012-08-26 15:09 PDT, Atte Kettunen
no flags Details
Contains two lines that both can reproduce this issue. (185 bytes, text/plain)
2012-08-26 15:22 PDT, Atte Kettunen
no flags Details
fix (1.17 KB, patch)
2012-08-28 07:58 PDT, Mats Palmgren (:mats)
zackw: feedback-
Details | Diff | Splinter Review
fix v2 (1.44 KB, patch)
2012-08-28 09:18 PDT, Mats Palmgren (:mats)
zackw: review+
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑esr10+
Details | Diff | Splinter Review
crash tests (DO NOT LAND BEFORE BUG IS PUBLIC) (15.66 KB, patch)
2012-08-28 10:06 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review

Description Atte Kettunen 2012-08-26 15:09:06 PDT
Created attachment 655460 [details]
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 2012-08-26 15:22:43 PDT
Created attachment 655463 [details]
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 (:mats) 2012-08-28 07:51:58 PDT
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 (:mats) 2012-08-28 07:58:51 PDT
Created attachment 656001 [details] [diff] [review]
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) 2012-08-28 08:23:36 PDT
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) 2012-08-28 08:27:28 PDT
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) 2012-08-28 08:30:19 PDT
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 (:mats) 2012-08-28 09:18:10 PDT
Created attachment 656034 [details] [diff] [review]
fix v2

OK, like this then?
Comment 9 Zack Weinberg (:zwol) 2012-08-28 09:25:08 PDT
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 (:mats) 2012-08-28 10:05:17 PDT
> 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 (:mats) 2012-08-28 10:06:20 PDT
Created attachment 656067 [details] [diff] [review]
crash tests (DO NOT LAND BEFORE BUG IS PUBLIC)
Comment 12 Mats Palmgren (:mats) 2012-08-28 10:12:33 PDT
I suspect this is exploitable, though it looks hard to do so to me.
Comment 13 Zack Weinberg (:zwol) 2012-08-28 11:10:40 PDT
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 [:bz] 2012-08-28 23:21:58 PDT
Comment on attachment 656034 [details] [diff] [review]
fix v2

r=me
Comment 16 Ed Morley [:emorley] 2012-08-30 04:01:44 PDT
https://hg.mozilla.org/mozilla-central/rev/5e5d1e9a4a76
Comment 17 Mats Palmgren (:mats) 2012-08-30 06:01:10 PDT
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 (:mats) 2012-08-30 17:28:20 PDT
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 2013-02-20 12:02:33 PST
Mats, I think you can land the crashtest now.
Comment 20 Tracy Walker [:tracy] 2014-01-10 10:41:58 PST
mass remove verifyme requests greater than 4 months old
Comment 21 Mats Palmgren (:mats) 2014-07-13 06:07:27 PDT
Landed the crashtests.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d85b064039fc
Comment 22 Carsten Book [:Tomcat] - PTO-back Sept 4th 2014-07-14 05:54:30 PDT
https://hg.mozilla.org/mozilla-central/rev/d85b064039fc

Note You need to log in before you can comment on or make changes to this bug.