Last Comment Bug 818454 - (CVE-2013-1676) Out of Bounds Read in SelectionIterator::GetNextSegment
(CVE-2013-1676)
: Out of Bounds Read in SelectionIterator::GetNextSegment
Status: RESOLVED FIXED
[asan][adv-main21+]
: regression, sec-moderate
Product: Core
Classification: Components
Component: Layout: Text (show other bugs)
: Trunk
: x86_64 All
: -- normal (vote)
: mozilla21
Assigned To: Simon Montagu :smontagu
:
Mentors:
Depends on: CVE-2013-1677 827192
Blocks: 722137
  Show dependency treegraph
 
Reported: 2012-12-05 05:31 PST by Abhishek Arya
Modified: 2014-11-20 17:38 PST (History)
10 users (show)
smontagu: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
-
affected
-
affected
-
affected
fixed
unaffected
-
wontfix
wontfix
wontfix


Attachments
Testcase (771 bytes, text/html)
2012-12-05 05:31 PST, Abhishek Arya
no flags Details
patch, backout the change from bug 722137 (1.44 KB, patch)
2012-12-10 06:43 PST, Jonathan Kew (:jfkthame)
smontagu: review+
Details | Diff | Review
New patch (1.88 KB, patch)
2013-01-31 05:59 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
New patch v.2 (2.03 KB, patch)
2013-02-05 01:44 PST, Simon Montagu :smontagu
roc: review+
Details | Diff | Review
Variation on the testcase (768 bytes, text/html)
2013-04-29 06:13 PDT, Simon Montagu :smontagu
no flags Details

Description Abhishek Arya 2012-12-05 05:31:12 PST
Created attachment 688730 [details]
Testcase

==31621== ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7ff3ebc86f00 at pc 0x7ff40dbe4966 bp 0x7fff6b40d150 sp 0x7fff6b40d148
READ of size 4 at 0x7ff3ebc86f00 thread T0
    #0 0x7ff40dbe4965 in gfxShapedWord::CompressedGlyph::IsClusterStart() const src/gfx/thebes/gfxFont.h:1920
    #1 0x7ff40db2dfbc in gfxTextRun::IsClusterStart(unsigned int) src/gfx/thebes/gfxFont.h:2383
    #2 0x7ff40db5c58c in SelectionIterator::GetNextSegment(double*, unsigned int*, unsigned int*, double*, short*, nsTextRangeStyle*) src/layout/generic/nsTextFrameThebes.cpp:5287
    #3 0x7ff40db6c52d in nsTextFrame::PaintTextSelectionDecorations(gfxContext*, gfxPoint const&, gfxPoint const&, gfxRect const&, PropertyProvider&, unsigned int, unsigned int, nsTextPaintStyle&, SelectionDetails*, short, nsTextFrame::DrawPathCallbacks*) src/layout/generic/nsTextFrameThebes.cpp:5581
    #4 0x7ff40db707ab in nsTextFrame::PaintTextWithSelection(gfxContext*, gfxPoint const&, gfxPoint const&, gfxRect const&, PropertyProvider&, unsigned int, unsigned int, nsTextPaintStyle&, nsCharClipDisplayItem::ClipEdges const&, nsTextFrame::DrawPathCallbacks*) src/layout/generic/nsTextFrameThebes.cpp:5636
    #5 0x7ff40db4d125 in nsTextFrame::PaintText(nsRenderingContext*, nsPoint, nsRect const&, nsCharClipDisplayItem const&, nsTextFrame::DrawPathCallbacks*) src/layout/generic/nsTextFrameThebes.cpp:5860
    #6 0x7ff40db4b2d3 in nsDisplayText::Paint(nsDisplayListBuilder*, nsRenderingContext*) src/layout/generic/nsTextFrameThebes.cpp:4572
    #7 0x7ff40cdde2f6 in mozilla::FrameLayerBuilder::DrawThebesLayer(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*) src/layout/base/FrameLayerBuilder.cpp:3282
    #8 0x7ff41a3d6a0f in mozilla::layers::BasicThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*) src/gfx/layers/basic/BasicThebesLayer.h:94
    #9 0x7ff41a3d60be in mozilla::layers::BasicShadowableThebesLayer::PaintBuffer(gfxContext*, nsIntRegion const&, nsIntRegion const&, nsIntRegion const&, bool, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*) src/gfx/layers/basic/BasicThebesLayer.cpp:402
    #10 0x7ff41a3cec7b in mozilla::layers::BasicThebesLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicThebesLayer.cpp:189
    #11 0x7ff41a3d371a in mozilla::layers::BasicShadowableThebesLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicThebesLayer.cpp:303
    #12 0x7ff41a3d4103 in non-virtual thunk to mozilla::layers::BasicShadowableThebesLayer::PaintThebes(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicThebesLayer.cpp:314
    #13 0x7ff41a35d495 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) src/gfx/layers/basic/BasicLayerManager.cpp:827
    #14 0x7ff41a35a8e9 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicLayerManager.cpp:942
    #15 0x7ff41a35da75 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) src/gfx/layers/basic/BasicLayerManager.cpp:843
    #16 0x7ff41a35a8e9 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicLayerManager.cpp:942
    #17 0x7ff41a35da75 in mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintContext&, gfxContext*) src/gfx/layers/basic/BasicLayerManager.cpp:843
    #18 0x7ff41a35a8e9 in mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::ReadbackProcessor*) src/gfx/layers/basic/BasicLayerManager.cpp:942
    #19 0x7ff41a355d01 in mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) src/gfx/layers/basic/BasicLayerManager.cpp:589
    #20 0x7ff41a354447 in mozilla::layers::BasicLayerManager::EndTransaction(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) src/gfx/layers/basic/BasicLayerManager.cpp:508
    #21 0x7ff41a3677c3 in mozilla::layers::BasicShadowLayerManager::EndTransaction(void (*)(mozilla::layers::ThebesLayer*, gfxContext*, nsIntRegion const&, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) src/gfx/layers/basic/BasicLayerManager.cpp:1142
    #22 0x7ff40d0d70cc in nsDisplayList::PaintForFrame(nsDisplayListBuilder*, nsRenderingContext*, nsIFrame*, unsigned int) const src/layout/base/nsDisplayList.cpp:1141
    #23 0x7ff40d0d4227 in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) const src/layout/base/nsDisplayList.cpp:1003
    #24 0x7ff40d276425 in nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, unsigned int) src/layout/base/nsLayoutUtils.cpp:1872
    #25 0x7ff40d3cd0a9 in PresShell::Paint(nsIView*, nsRegion const&, unsigned int) src/layout/base/nsPresShell.cpp:5329
    #26 0x7ff41140e63a in nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) src/view/src/nsViewManager.cpp:433
    #27 0x7ff411421a2c in nsViewManager::ProcessPendingUpdates() src/view/src/nsViewManager.cpp:1208
    #28 0x7ff40d44c1e7 in nsRefreshDriver::Notify(nsITimer*) src/layout/base/nsRefreshDriver.cpp:432
    #29 0x7ff419ad5f63 in nsTimerImpl::Fire() src/xpcom/threads/nsTimerImpl.cpp:485
    #30 0x7ff419ad7301 in nsTimerEvent::Run() src/xpcom/threads/nsTimerImpl.cpp:565
    #31 0x7ff419a99ffe in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:627
    #32 0x7ff41970f08f in NS_ProcessNextEvent_P(nsIThread*, bool) src/objdir-ff-asan-sym/xpcom/build/nsThreadUtils.cpp:221
    #33 0x7ff417c8d2b6 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:82
    #34 0x7ff419d82ade in MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:215
    #35 0x7ff419d82925 in MessageLoop::RunHandler() src/ipc/chromium/src/base/message_loop.cc:208
    #36 0x7ff419d8280b in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:182
    #37 0x7ff4170a55f4 in nsBaseAppShell::Run() src/widget/xpwidgets/nsBaseAppShell.cpp:163
    #38 0x7ff415c06af2 in nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290
    #39 0x7ff40b126c44 in XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:3823
    #40 0x7ff40b12c919 in XREMain::XRE_main(int, char**, nsXREAppData const*) src/toolkit/xre/nsAppRunner.cpp:3890
    #41 0x7ff40b12f690 in XRE_main src/toolkit/xre/nsAppRunner.cpp:4084
    #42 0x40c0c6 in do_main(int, char**) src/browser/app/nsBrowserApp.cpp:174
    #43 0x409900 in main src/browser/app/nsBrowserApp.cpp:279
    #44 0x7ff42b5fa76c in
0x7ff3ebc86f00 is located 8 bytes to the right of 184-byte region [0x7ff3ebc86e40,0x7ff3ebc86ef8)
allocated by thread T0 here:
    #0 0x4c3810 in malloc
    #1 0x7ff429188855 in moz_malloc src/memory/mozalloc/mozalloc.cpp:64
    #2 0x7ff41a10d1eb in gfxTextRun::AllocateStorageForTextRun(unsigned long, unsigned int) src/gfx/thebes/gfxFont.cpp:4491
    #3 0x7ff41a0f0441 in gfxTextRun::Create(gfxTextRunFactory::Parameters const*, unsigned int, gfxFontGroup*, unsigned int) src/gfx/thebes/gfxFont.cpp:4508
    #4 0x7ff41a0f3d33 in gfxFontGroup::MakeTextRun(unsigned char const*, unsigned int, gfxTextRunFactory::Parameters const*, unsigned int) src/gfx/thebes/gfxFont.cpp:3553
    #5 0x7ff40db1473a in gfxTextRun* MakeTextRun<unsigned char>(unsigned char const*, unsigned int, gfxFontGroup*, gfxTextRunFactory::Parameters const*, unsigned int) src/layout/generic/nsTextFrameThebes.cpp:553
    #6 0x7ff40db0963b in BuildTextRunsScanner::BuildTextRunForFrames(void*) src/layout/generic/nsTextFrameThebes.cpp:2050
    #7 0x7ff40dafe5f5 in BuildTextRunsScanner::FlushFrames(bool, bool) src/layout/generic/nsTextFrameThebes.cpp:1409
    #8 0x7ff40db0e12d in BuildTextRunsScanner::ScanFrame(nsIFrame*) src/layout/generic/nsTextFrameThebes.cpp:1601
Shadow byte and word:
  0x1ffe7d790de0: fa
  0x1ffe7d790de0: fa fa fa fa fa fa fa fa
More shadow bytes:
  0x1ffe7d790dc0: fa fa fa fa fa fa fa fa
  0x1ffe7d790dc8: 00 00 00 00 00 00 00 00
  0x1ffe7d790dd0: 00 00 00 00 00 00 00 00
  0x1ffe7d790dd8: 00 00 00 00 00 00 00 fb
=>0x1ffe7d790de0: fa fa fa fa fa fa fa fa
  0x1ffe7d790de8: 00 00 00 00 00 00 00 00
  0x1ffe7d790df0: 00 00 00 00 00 00 fb fb
  0x1ffe7d790df8: fa fa fa fa fa fa fa fa
  0x1ffe7d790e00: fa fa fa fa fa fa fa fa
Stats: 262M malloced (245M for red zones) by 451453 calls
Stats: 47M realloced by 19228 calls
Stats: 228M freed by 226481 calls
Stats: 193M really freed by 173783 calls
Stats: 245M (62800 full pages) mmaped in 467 calls
  mmaps   by size class: 7:217035; 8:47081; 9:14322; 10:5621; 11:7650; 12:1280; 13:1088; 14:512; 15:224; 16:720; 17:464; 18:34; 19:35; 20:21;
  mallocs by size class: 7:304827; 8:87290; 9:24303; 10:8218; 11:17377; 12:2304; 13:2068; 14:1626; 15:415; 16:1440; 17:1449; 18:73; 19:41; 20:22;
  frees   by size class: 7:125017; 8:55922; 9:17553; 10:4893; 11:15407; 12:1512; 13:1639; 14:1437; 15:293; 16:1261; 17:1430; 18:60; 19:38; 20:19;
  rfrees  by size class: 7:95680; 8:43221; 9:14163; 10:3105; 11:11615; 12:1100; 13:1032; 14:1283; 15:230; 16:933; 17:1315; 18:50; 19:37; 20:19;
Stats: malloc large: 3440 small slow: 5269
==31621== ABORTING
Comment 1 David Bolter [:davidb] 2012-12-05 10:05:41 PST
This is probably bad. Assigning to Milan for help with triage and assignment.
Comment 2 David Bolter [:davidb] 2012-12-05 10:12:09 PST
Actually it looks like text runs. Johnathan can you take a look and rate this one?
Comment 3 Daniel Veditz [:dveditz] 2012-12-05 10:16:38 PST
An OoBR in IsClusterStart() isn't particularly dangerous looking, just checking some flags against some random memory value. But it does mean we've got the length of the run wrong and if we call any of the Set* methods on gfxShapedWord then we'll be overwriting something: much worse.
Comment 4 Jonathan Kew (:jfkthame) 2012-12-08 06:42:35 PST
Right, it looks like we have a mismatch between the length of the textrun we're using and the amount of text the textframe thinks it should cover. It's not clear to me quite how dangerous this is, besides the obvious possibility of crashing when we try to read glyph data from somewhere beyond the actual size of the run; once textruns have been created, they're largely (though not completely) treated as read-only, so there aren't all that many situations where we'd be *writing* outside the allocated run. Setting line-breaks would be one case, I think, so if a testcase were constructed such that we get into this mismatched situation (whatever it is, exactly), and then do line-breaking on the text concerned, we might corrupt other stuff.

Note that although this was found through ASAN, the testcase here will crash a regular Firefox build; ASAN is not required to reproduce it. In a debug build, a bunch of assertions are triggered before it actually crashes.

I notice that Firefox 10esr and Firefox 11 do -not- crash on this testcase, but Firefox 12 and later releases do crash. So it seems something regressed, or at least became more fragile, between FF11 and FF12.
Comment 6 Jonathan Kew (:jfkthame) 2012-12-08 09:55:03 PST
Backing out changeset 7b5995e5d551 (bug 722137) from trunk appears to prevent the crash with the testcase here (and all the assertion spew that precedes the actual crash).

Interestingly, even with that patch backed out, I do -not- get assertions from bug 722137's testcase, so I wonder if other (bidi-related?) changes in the meantime have also affected behavior here.
Comment 7 Jonathan Kew (:jfkthame) 2012-12-08 09:58:25 PST
Given that this first showed up in FF12 and is still present on trunk, marking all current branches except esr10 as affected.
Comment 8 Jonathan Kew (:jfkthame) 2012-12-10 06:43:25 PST
Created attachment 690364 [details] [diff] [review]
patch, backout the change from bug 722137

Backing out bug 722137 appears to resolve the assertions and crash here, and does -not- reintroduce a crash or assertion with the testcase from that bug.

Is this at all reasonable, or should we be doing something different here?
Comment 9 Alex Keybl [:akeybl] 2012-12-10 16:36:13 PST
Longstanding bug, unclear how dangerous this is, no need to track for release.
Comment 10 Daniel Veditz [:dveditz] 2012-12-12 10:43:51 PST
I guess if we leave the testcase from bug 722137 in the tree and it continues to pass then we could back out the patch.

Going to call this sec-moderate based on comment 4 unless we find a case where we do write back into the character run.
Comment 11 Simon Montagu :smontagu 2012-12-12 11:39:08 PST
Comment on attachment 690364 [details] [diff] [review]
patch, backout the change from bug 722137

Review of attachment 690364 [details] [diff] [review]:
-----------------------------------------------------------------

It's certainly possible that some other change made the checked-in fix to bug 722137 not necessary. I'd love to know which change and why exactly, but fixing this bug doesn't have to depend on that.
Comment 12 Jonathan Kew (:jfkthame) 2012-12-12 13:06:23 PST
Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c749372d5a7

We should also land the testcase here as a crashtest, but I'm a bit hesitant to do that while the crash remains open on older branches.

dveditz, do you want us to look into taking the patch here on aurora/beta/esr (first we'd need to test whether it's safe there, or reintroduces the bug 722137 crash), or do you not consider it worthwhile to backport?
Comment 13 Ed Morley [:emorley] 2012-12-13 07:58:20 PST
https://hg.mozilla.org/mozilla-central/rev/8c749372d5a7

Leaving open for needinfo etc.
Comment 14 Daniel Veditz [:dveditz] 2013-01-17 09:49:51 PST
(In reply to Jonathan Kew (:jfkthame) from comment #12)
> dveditz, do you want us to look into taking the patch here on
> aurora/beta/esr (first we'd need to test whether it's safe there, or
> reintroduces the bug 722137 crash), or do you not consider it worthwhile to
> backport?

The affected releases would be Firefox 19 and Firefox 17-ESR. Unless we learn something that makes us understand this as a worse problem than it is it's probably better to just leave this alone on those branches.
Comment 15 Simon Montagu :smontagu 2013-01-31 05:52:03 PST
Reopening since I proposed backing this out to fix bug 826163 and bug 827192.
Comment 16 Simon Montagu :smontagu 2013-01-31 05:59:54 PST
Created attachment 708548 [details] [diff] [review]
New patch

I think the problem with the patch from bug 722137 was not that it was wrong but that it didn't go far enough, and left the frame tree in an inconsistent state.

The patch there checked whether a text frame at the end of a directional run had a fluid continuation (from the last time through bidi resolution) and converted it to a non-fluid continuation. This addition walks up the parent chain of the frame and the continuation making the same check.
Comment 17 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-01-31 13:09:30 PST
Comment on attachment 708548 [details] [diff] [review]
New patch

Review of attachment 708548 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsBidiPresUtils.cpp
@@ +813,5 @@
> +              while (parent && nextParent) {
> +                if (parent == nextParent ||
> +                    nextParent != parent->GetNextInFlow()) {
> +                  break;
> +                }

Actually ... does this need to stop also when parent/nextParent is not a line participant?
Comment 18 Simon Montagu :smontagu 2013-01-31 22:35:33 PST
Not sure. Could nextParent be parent's next-in-flow if they weren't both line participants?
Comment 19 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2013-02-01 01:39:41 PST
Sure, if a paragraph is in a block that has broken across a column boundary.
Comment 20 Simon Montagu :smontagu 2013-02-05 01:44:05 PST
Created attachment 710093 [details] [diff] [review]
New patch v.2

So like this? 

I managed to reproduce a hang (infinite loop with assertions like bug 827192) with the original patch but not with this version, but didn't manage to capture it with an automated testcase yet.
Comment 22 Ed Morley [:emorley] 2013-02-12 10:54:12 PST
https://hg.mozilla.org/mozilla-central/rev/d48d6344a9bd
Comment 23 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-13 11:05:14 PST
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
Comment 24 Daniel Veditz [:dveditz] 2013-02-21 16:53:29 PST
Does the exchange in comment 18 and 19 mean this bug might be worse than "sec-moderate"? I ask because that's when the ESR branch status was changed from wontfix back to affected. If it's still just a moderate issue we won't take it on the ESR branches, but if it's worse let's fix the rating and then renominate for ESR and b2g18
Comment 25 Simon Montagu :smontagu 2013-02-21 22:15:30 PST
(In reply to Daniel Veditz [:dveditz] from comment #24)
> Does the exchange in comment 18 and 19 mean this bug might be worse than
> "sec-moderate"? I ask because that's when the ESR branch status was changed
> from wontfix back to affected.

No, that was accidental
Comment 26 Simon Montagu :smontagu 2013-04-29 06:13:57 PDT
Created attachment 743037 [details]
Variation on the testcase

For reference, attaching the testcase I described in comment 20
Comment 27 Simon Montagu :smontagu 2014-11-20 05:52:06 PST
Checked in testcase as crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/937a7c7b2767
Comment 28 Wes Kocher (:KWierso) 2014-11-20 17:38:43 PST
https://hg.mozilla.org/mozilla-central/rev/937a7c7b2767

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