Closed Bug 818454 (CVE-2013-1676) Opened 11 years ago Closed 11 years ago

Out of Bounds Read in SelectionIterator::GetNextSegment

Categories

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

x86_64
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox17 --- affected
firefox18 - affected
firefox19 - affected
firefox20 - affected
firefox21 --- fixed
firefox-esr10 --- unaffected
firefox-esr17 - wontfix
b2g18 --- wontfix
b2g18-v1.0.1 --- wontfix

People

(Reporter: inferno, Assigned: smontagu)

References

Details

(Keywords: regression, sec-moderate, Whiteboard: [asan][adv-main21+])

Attachments

(4 files, 1 obsolete file)

Attached file 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
Component: General → Graphics
Product: Firefox → Core
This is probably bad. Assigning to Milan for help with triage and assignment.
Assignee: nobody → milan
Actually it looks like text runs. Johnathan can you take a look and rate this one?
Assignee: milan → jfkthame
Component: Graphics → Layout: Text
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.
Summary: Heap-buffer-overflow in SelectionIterator::GetNextSegment → Out of Bounds Read in SelectionIterator::GetNextSegment
Whiteboard: [asan]
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.
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.
Given that this first showed up in FF12 and is still present on trunk, marking all current branches except esr10 as affected.
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?
Attachment #690364 - Flags: review?(smontagu)
Longstanding bug, unclear how dangerous this is, no need to track for release.
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.
Keywords: sec-moderate
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.
Attachment #690364 - Flags: review?(smontagu) → review+
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?
Flags: needinfo?(dveditz)
https://hg.mozilla.org/mozilla-central/rev/8c749372d5a7

Leaving open for needinfo etc.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla20
(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.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: needinfo?(dveditz)
Resolution: --- → FIXED
Reopening since I proposed backing this out to fix bug 826163 and bug 827192.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch New patch (obsolete) — Splinter Review
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.
Attachment #708548 - Flags: review?(roc)
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?
Not sure. Could nextParent be parent's next-in-flow if they weren't both line participants?
Sure, if a paragraph is in a block that has broken across a column boundary.
Attached patch New patch v.2Splinter Review
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.
Assignee: jfkthame → smontagu
Attachment #708548 - Attachment is obsolete: true
Attachment #710093 - Flags: review?(roc)
Depends on: CVE-2013-1677
No longer depends on: CVE-2013-1677
Depends on: CVE-2013-1677
https://hg.mozilla.org/mozilla-central/rev/d48d6344a9bd
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: mozilla20 → mozilla21
Batch edit: Bugs still affected on b2g18 after 2/13 merge to v1.0.1 branch are affected on v1.0.1 branch.
Depends on: 827192
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
Depends on: 827192
(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
For reference, attaching the testcase I described in comment 20
Whiteboard: [asan] → [asan][adv-main21+]
Alias: CVE-2013-1676
Group: core-security
Checked in testcase as crashtest: https://hg.mozilla.org/integration/mozilla-inbound/rev/937a7c7b2767
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.