Closed Bug 798867 (CVE-2013-0778) Opened 12 years ago Closed 12 years ago

Out-of-bounds read in ClusterIterator::NextCluster

Categories

(Core :: Layout: Text and Fonts, defect)

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox17 - wontfix
firefox18 - wontfix
firefox19 --- fixed
firefox-esr10 - wontfix
firefox-esr17 --- wontfix
b2g18 --- wontfix

People

(Reporter: inferno, Assigned: MatsPalmgren_bugz)

Details

(Keywords: csectype-bounds, sec-low, Whiteboard: [adv-main19+])

Attachments

(3 files)

Attached file 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
Component: General → Layout
Product: Firefox → Core
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.
Flags: needinfo?
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
Flags: needinfo?
Component: Layout → Layout: Text
Assignee: nobody → continuation
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.
Assignee: continuation → nobody
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
Attached patch fixSplinter Review
Use offset instead of offsets.offset

https://tbpl.mozilla.org/?tree=Try&rev=6634dff0df81
Assignee: nobody → matspal
Attachment #679549 - Flags: review?(roc)
https://hg.mozilla.org/mozilla-central/rev/ac1fdaa261f2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
This looks like it would be relevant to older branches as well, no?
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.
(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?
Flags: needinfo?(matspal)
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.
Keywords: csec-bounds, sec-low
Summary: Heap-buffer-overflow in ClusterIterator::NextCluster → Out-of-bounds read in ClusterIterator::NextCluster
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 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
Attachment #679549 - Flags: approval-mozilla-aurora?
Flags: needinfo?(matspal)
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.
Attachment #679549 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
Whiteboard: [adv-main19+]
Alias: CVE-2013-0778
Group: core-security
You need to log in before you can comment on or make changes to this bug.