Bug 785753 (CVE-2012-4185)

Global-buffer-overflow in nsCharTraits::length

RESOLVED FIXED in Firefox 16



7 years ago
5 years ago


(Reporter: attekett, Assigned: mats)


(6 keywords)

Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(firefox16+ fixed, firefox17+ fixed, firefox18 fixed, firefox-esr1016+ fixed)


(Whiteboard: [asan][advisory-tracking+])


(4 attachments, 1 obsolete attachment)

Tested with build from 


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
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
Whiteboard: [asan]
nsUnicharStreamLoader::DetermineCharset() calls WriteSegmentFun with
aSegment="p" and aCount=1.

nsUTF16ToUnicodeBase::GetMaxLength return zero:
(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

WriteSegmentFun overwrites the buffer with 0xFFFD at line 226.
This code was added in bug 541496 (part 4).
Severity: normal → critical
Component: General → Networking
Product: Firefox → Core
Posted patch fix (obsolete) — Splinter Review
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.
Attachment #656001 - Flags: review?(bzbarsky)
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 :)
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 on attachment 656001 [details] [diff] [review]

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.
Attachment #656001 - Flags: feedback-
Posted patch fix v2Splinter Review
OK, like this then?
Attachment #656034 - Flags: review?(zackw)
Attachment #656001 - Attachment is obsolete: true
Attachment #656001 - Flags: review?(bzbarsky)
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 :)
Attachment #656034 - Flags: review?(zackw)
Attachment #656034 - Flags: review?(bzbarsky)
Attachment #656034 - Flags: review+
> 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.
I suspect this is exploitable, though it looks hard to do so to me.
Keywords: sec-critical
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 on attachment 656034 [details] [diff] [review]
fix v2

Attachment #656034 - Flags: review?(bzbarsky) → review+
Assignee: nobody → matspal
Flags: in-testsuite?
Target Milestone: --- → mozilla18
Closed: 7 years ago
Resolution: --- → FIXED
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
Attachment #656034 - Flags: approval-mozilla-release?
Attachment #656034 - Flags: approval-mozilla-esr10?
Attachment #656034 - Flags: approval-mozilla-beta?
Attachment #656034 - Flags: approval-mozilla-aurora?
Attachment #656034 - Flags: approval-mozilla-release?
Attachment #656034 - Flags: approval-mozilla-esr10?
Attachment #656034 - Flags: approval-mozilla-esr10+
Attachment #656034 - Flags: approval-mozilla-beta?
Attachment #656034 - Flags: approval-mozilla-beta+
Attachment #656034 - Flags: approval-mozilla-aurora?
Attachment #656034 - Flags: approval-mozilla-aurora+

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)) {
Whiteboard: [asan] → [asan][advisory-tracking+]
Alias: CVE-2012-4185
Group: core-security
Flags: sec-bounty+
Mats, I think you can land the crashtest now.
Flags: needinfo?(matspal)
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Landed the crashtests.
Flags: needinfo?(mats)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.