Last Comment Bug 798867 - (CVE-2013-0778) Out-of-bounds read in ClusterIterator::NextCluster
(CVE-2013-0778)
: Out-of-bounds read in ClusterIterator::NextCluster
Status: RESOLVED FIXED
[adv-main19+]
: csectype-bounds, sec-low
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla19
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-07 00:14 PDT by Abhishek Arya
Modified: 2014-11-19 19:35 PST (History)
8 users (show)
mats: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix
fixed
-
wontfix
wontfix
wontfix


Attachments
Testcase (441 bytes, text/html)
2012-10-07 00:14 PDT, Abhishek Arya
no flags Details
stack where we hit first assert (5.19 KB, text/plain)
2012-11-07 11:03 PST, Andrew McCreight [:mccr8]
no flags Details
fix (1.71 KB, patch)
2012-11-07 22:18 PST, Mats Palmgren (:mats)
roc: review+
akeybl: approval‑mozilla‑aurora-
Details | Diff | Splinter Review

Description Abhishek Arya 2012-10-07 00:14:21 PDT
Created attachment 668884 [details]
Testcase

Reproduces on trunk by double-clicking anywhere in the content window.

=================================================================
==11059== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f0f8a78828b at pc 0x7f0fb1376d36 bp 0x7fffe7937850 sp 0x7fffe7937848
READ of size 1 at 0x7f0f8a78828b thread T0
    #0 0x7f0fb1376d35 in ClusterIterator::NextCluster() layout/generic/nsTextFrameThebes.cpp:6659
    #1 0x7f0fb137ab2a in nsTextFrame::PeekOffsetWord(bool, bool, bool, int*, nsIFrame::PeekWordState*) layout/generic/nsTextFrameThebes.cpp:6733
    #2 0x7f0fb0fa0b57 in nsIFrame::PeekOffset(nsPeekOffsetStruct*) layout/generic/nsFrame.cpp:6226
    #3 0x7f0fb0f5b56e in nsFrame::PeekBackwardAndForward(nsSelectionAmount, nsSelectionAmount, int, nsPresContext*, bool, unsigned int) layout/generic/nsFrame.cpp:2954
    #4 0x7f0fb0f5a4f5 in nsFrame::SelectByTypeAtPoint(nsPresContext*, nsPoint const&, nsSelectionAmount, nsSelectionAmount, unsigned int) layout/generic/nsFrame.cpp:2862
    #5 0x7f0fb0f5d56b in nsFrame::HandleMultiplePress(nsPresContext*, nsGUIEvent*, nsEventStatus*, bool) layout/generic/nsFrame.cpp:2911
    #6 0x7f0fb0f559d9 in nsFrame::HandlePress(nsPresContext*, nsGUIEvent*, nsEventStatus*) layout/generic/nsFrame.cpp:2710
    #7 0x7f0fb0f4f9b7 in nsFrame::HandleEvent(nsPresContext*, nsGUIEvent*, nsEventStatus*) layout/generic/nsFrame.cpp:2401
    #8 0x7f0fb0c2a70e in nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor&) layout/base/nsPresShell.cpp:491
    #9 0x7f0fb338abd3 in nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) content/events/src/nsEventDispatcher.cpp:362
    #10 0x7f0fb338fe79 in nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) content/events/src/nsEventDispatcher.cpp:629
    #11 0x7f0fb0be882c in PresShell::HandleEventInternal(nsEvent*, nsEventStatus*) layout/base/nsPresShell.cpp:6437
    #12 0x7f0fb0be2ea3 in PresShell::HandlePositionedEvent(nsIFrame*, nsGUIEvent*, nsEventStatus*) layout/base/nsPresShell.cpp:6208
    #13 0x7f0fb0bdaed6 in PresShell::HandleEvent(nsIFrame*, nsGUIEvent*, bool, nsEventStatus*) layout/base/nsPresShell.cpp:6007
    #14 0x7f0fb454c39c in nsViewManager::DispatchEvent(nsGUIEvent*, nsIView*, nsEventStatus*) view/src/nsViewManager.cpp:767
    #15 0x7f0fb4537f37 in nsView::HandleEvent(nsGUIEvent*, bool) view/src/nsView.cpp:1062
    #16 0x7f0fb453818d in non-virtual thunk to nsView::HandleEvent(nsGUIEvent*, bool) :?
    #17 0x7f0fb9ca5e0a in nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) widget/gtk2/nsWindow.cpp:458
    #18 0x7f0fb9cd9541 in nsWindow::OnButtonPressEvent(_GtkWidget*, _GdkEventButton*) widget/gtk2/nsWindow.cpp:2729
    #19 0x7f0fb9cf04d9 in button_press_event_cb(_GtkWidget*, _GdkEventButton*) widget/gtk2/nsWindow.cpp:5256
    #20 0x7f0fab655dd7 in ?? ??:0
0x7f0f8a78828b is located 1 bytes to the right of 10-byte region [0x7f0f8a788280,0x7f0f8a78828a)
allocated by thread T0 here:
    #0 0x4c3640 in malloc ??:?
    #1 0x7f0fc9f7255a in moz_xmalloc memory/mozalloc/mozalloc.cpp:57
    #2 0x7f0faef6a566 in nsTArrayInfallibleAllocator::Malloc(unsigned long) ../../dist/include/nsTArray.h:60
    #3 0x7f0faef6849e in nsTArray_base<nsTArrayDefaultAllocator>::EnsureCapacity(unsigned int, unsigned int) ../../dist/include/nsTArray-inl.h:119
    #4 0x7f0fb1379494 in nsTArray<bool, nsTArrayDefaultAllocator>::AppendElements(unsigned int) ../../../dist/include/nsTArray.h:902
    #5 0x7f0fb1377eb7 in ClusterIterator layout/generic/nsTextFrameThebes.cpp:6683
    #6 0x7f0fb137ab1d in nsTextFrame::PeekOffsetWord(bool, bool, bool, int*, nsIFrame::PeekWordState*) layout/generic/nsTextFrameThebes.cpp:6731
    #7 0x7f0fb0fa0b57 in nsIFrame::PeekOffset(nsPeekOffsetStruct*) layout/generic/nsFrame.cpp:6226
    #8 0x7f0fb0f5b56e in nsFrame::PeekBackwardAndForward(nsSelectionAmount, nsSelectionAmount, int, nsPresContext*, bool, unsigned int) layout/generic/nsFrame.cpp:2954
    #9 0x7f0fb0f5a4f5 in nsFrame::SelectByTypeAtPoint(nsPresContext*, nsPoint const&, nsSelectionAmount, nsSelectionAmount, unsigned int) layout/generic/nsFrame.cpp:2862
    #10 0x7f0fb0f5d56b in nsFrame::HandleMultiplePress(nsPresContext*, nsGUIEvent*, nsEventStatus*, bool) layout/generic/nsFrame.cpp:2911
    #11 0x7f0fb0f559d9 in nsFrame::HandlePress(nsPresContext*, nsGUIEvent*, nsEventStatus*) layout/generic/nsFrame.cpp:2710
    #12 0x7f0fb0f4f9b7 in nsFrame::HandleEvent(nsPresContext*, nsGUIEvent*, nsEventStatus*) layout/generic/nsFrame.cpp:2401
    #13 0x7f0fb0c2a70e in nsPresShellEventCB::HandleEvent(nsEventChainPostVisitor&) layout/base/nsPresShell.cpp:491
    #14 0x7f0fb338abd3 in nsEventTargetChainItem::HandleEventTargetChain(nsEventChainPostVisitor&, unsigned int, nsDispatchingCallback*, bool, nsCxPusher*) content/events/src/nsEventDispatcher.cpp:362
    #15 0x7f0fb338fe79 in nsEventDispatcher::Dispatch(nsISupports*, nsPresContext*, nsEvent*, nsIDOMEvent*, nsEventStatus*, nsDispatchingCallback*, nsCOMArray<nsIDOMEventTarget>*) content/events/src/nsEventDispatcher.cpp:629
    #16 0x7f0fb0be882c in PresShell::HandleEventInternal(nsEvent*, nsEventStatus*) layout/base/nsPresShell.cpp:6437
    #17 0x7f0fb0be2ea3 in PresShell::HandlePositionedEvent(nsIFrame*, nsGUIEvent*, nsEventStatus*) layout/base/nsPresShell.cpp:6208
    #18 0x7f0fb0bdaed6 in PresShell::HandleEvent(nsIFrame*, nsGUIEvent*, bool, nsEventStatus*) layout/base/nsPresShell.cpp:6007
    #19 0x7f0fb454c39c in nsViewManager::DispatchEvent(nsGUIEvent*, nsIView*, nsEventStatus*) view/src/nsViewManager.cpp:767
    #20 0x7f0fb4537f37 in nsView::HandleEvent(nsGUIEvent*, bool) view/src/nsView.cpp:1062
    #21 0x7f0fb453818d in non-virtual thunk to nsView::HandleEvent(nsGUIEvent*, bool) :?
    #22 0x7f0fb9ca5e0a in nsWindow::DispatchEvent(nsGUIEvent*, nsEventStatus&) widget/gtk2/nsWindow.cpp:458
    #23 0x7f0fb9cd9541 in nsWindow::OnButtonPressEvent(_GtkWidget*, _GdkEventButton*) widget/gtk2/nsWindow.cpp:2729
    #24 0x7f0fb9cf04d9 in button_press_event_cb(_GtkWidget*, _GdkEventButton*) widget/gtk2/nsWindow.cpp:5256
Shadow byte and word:
  0x1fe1f14f1051: 2
  0x1fe1f14f1050: 00 02 fb fb fb fb fb fb
More shadow bytes:
  0x1fe1f14f1030: fa fa fa fa fa fa fa fa
  0x1fe1f14f1038: fa fa fa fa fa fa fa fa
  0x1fe1f14f1040: fa fa fa fa fa fa fa fa
  0x1fe1f14f1048: fa fa fa fa fa fa fa fa
=>0x1fe1f14f1050: 00 02 fb fb fb fb fb fb
  0x1fe1f14f1058: fb fb fb fb fb fb fb fb
  0x1fe1f14f1060: fa fa fa fa fa fa fa fa
  0x1fe1f14f1068: fa fa fa fa fa fa fa fa
  0x1fe1f14f1070: fa fa fa fa fa fa fa fa
Stats: 247M malloced (270M for red zones) by 396757 calls
Stats: 42M realloced by 23311 calls
Stats: 223M freed by 275477 calls
Stats: 90M really freed by 187533 calls
Stats: 480M (122969 full pages) mmaped in 120 calls
  mmaps   by size class: 8:294894; 9:32764; 10:8190; 11:14329; 12:2048; 13:1536; 14:1280; 15:256; 16:1152; 17:1280; 18:144; 19:40; 20:20;
  mallocs by size class: 8:330117; 9:32695; 10:8737; 11:16289; 12:2501; 13:1727; 14:1555; 15:317; 16:1287; 17:1317; 18:154; 19:40; 20:21;
  frees   by size class: 8:224898; 9:24420; 10:5483; 11:13159; 12:1629; 13:1507; 14:1392; 15:267; 16:1231; 17:1301; 18:134; 19:38; 20:18;
  rfrees  by size class: 8:164815; 9:8613; 10:2103; 11:8730; 12:630; 13:557; 14:541; 15:144; 16:1042; 17:328; 18:25; 19:4; 20:1;
Stats: malloc large: 1532 small slow: 2246
==11059== ABORTING
Comment 1 Daniel Veditz [:dveditz] 2012-10-10 10:27:51 PDT
What is this reading, and would it keep reading if asan hadn't stopped it? Off by one reading character data wouldn't be too bad; reading lots of random memory as text that we're going to put up on screen would be an info leak/privacy bug (moderate to high); reading objects that we're going to call or write into would be potentially critical.
Comment 2 Andrew McCreight [:mccr8] 2012-11-07 10:57:09 PST
In a debug build, after clicking I get a bunch of assertion failures:

###!!! ASSERTION: aOffset out of range: 'aOffset && *aOffset <= contentLength', file /Users/amccreight/mz/cent/layout/generic/nsTextFrameThebes.cpp, line 6546
###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/amccreight/mz/cent/gfx/thebes/gfxSkipChars.cpp, line 60
###!!! ASSERTION: aOffset out of range: 'aOffset && *aOffset <= contentLength', file /Users/amccreight/mz/cent/layout/generic/nsTextFrameThebes.cpp, line 6718
###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/amccreight/mz/cent/gfx/thebes/gfxSkipChars.cpp, line 60
###!!! ASSERTION: Invalid offset: 'aOffset <= mSkipChars->mCharCount', file /Users/amccreight/mz/cent/gfx/thebes/gfxSkipChars.cpp, line 60
Assertion failure: i < Length() (invalid array index), at ../../../dist/include/nsTArray.h:537
Comment 3 Andrew McCreight [:mccr8] 2012-11-07 11:03:17 PST
Created attachment 679262 [details]
stack where we hit first assert
Comment 4 Andrew McCreight [:mccr8] 2012-11-07 15:57:43 PST
Okay, well, I couldn't make any sense out of it.  It looks like an invalid offsets.offset is being generated: somehow it is 3, but the content length is only 1.
Comment 5 Mats Palmgren (:mats) 2012-11-07 22:16:17 PST
nsFrame::SelectByTypeAtPoint calls nsFrameSelection::GetFrameForNodeOffset
with aNode:

html@0x7fffda0d2500 ...
  head@0x7fffd808e060 ...
  body@0x7fffda035280 ...
    script@0x7fffd808f450 ...
      Text@0x7fffda4d7f80 flags=[00000000] primaryframe=(nil) refcount=1<...>
    >
    Text@0x7fffda4d8000 flags=[02000000] primaryframe=0x7fffd9fba748 refcount=2<&gt;>
  >
  Text@0x7fffd8138a00 flags=[02000000] primaryframe=0x7fffd9fbc1c0 refcount=2<\u000a>
>

and aOffset = 3 which is after the last of its child nodes (seems correct).

In GetFrameForNodeOffset we fall through to this:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1849
where 'theNode' is the last (text node) so we set "*aReturnOffset" to 1.

Back in SelectByTypeAtPoint we discard this value (offset) and instead use
the original value (offsets.offset) in the call to PeekBackwardAndForward:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2880
Comment 6 Mats Palmgren (:mats) 2012-11-07 22:18:26 PST
Created attachment 679549 [details] [diff] [review]
fix

Use offset instead of offsets.offset

https://tbpl.mozilla.org/?tree=Try&rev=6634dff0df81
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-11-08 17:05:19 PST
https://hg.mozilla.org/mozilla-central/rev/ac1fdaa261f2
Comment 9 Jonathan Kew (:jfkthame) 2012-11-09 01:34:40 PST
This looks like it would be relevant to older branches as well, no?
Comment 10 Jonathan Kew (:jfkthame) 2012-11-09 01:37:36 PST
From code inspection (not actual testing), this seems likely to affect all branches. The faulty code is present in all of esr10/beta/aurora, AFAICS.
Comment 11 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-09 14:31:33 PST
(In reply to Jonathan Kew (:jfkthame) from comment #10)
> From code inspection (not actual testing), this seems likely to affect all
> branches. The faulty code is present in all of esr10/beta/aurora, AFAICS.

The faulty code might be present, but without a sec-rating we don't have an indication of the severity of this issue to know whether to track & request uplift or not.  Can someone fill in that information and nominate patches for uplift if they apply cleanly to branches (esr10 specifically) and are low risk to take for our last beta?
Comment 12 Daniel Veditz [:dveditz] 2012-11-09 21:01:18 PST
As far as I can tell from the point we get the wrong offset and on this value is only used in reading data and the results used in calculating how to display it. Nothing is written back into frames based on the incorrect offset. The results ought to be limited to rendering incorrectly and/or crashing on a read access violation.

Might be nice to get it into Aurora but no need to cram it into Firefox 17 at the last minute.
Comment 13 Daniel Veditz [:dveditz] 2012-11-09 21:04:54 PST
That said, I have only a passing acquaintance with this code. It'd be nice if Mats or Jonathan could concur that this is how we use those objects..
Comment 14 Mats Palmgren (:mats) 2012-11-18 03:26:35 PST
Comment on attachment 679549 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: crash
Testing completed (on m-c, etc.): on m-c since 2012-11-08
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none
Comment 15 Alex Keybl [:akeybl] 2012-11-18 20:34:00 PST
Thanks Mats - I don't think there's any reason to uplift this though (clearing out the flags). sec-low, been around forever, and little user impact. Let's let this ride the trains.

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