out-of-bounds read at nsFontVariantTextRunFactory::RebuildTextRun

VERIFIED FIXED in Firefox 15

Status

()

defect
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: attekett, Assigned: jfkthame)

Tracking

({crash, csectype-dos, regression})

15 Branch
mozilla15
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 unaffected, firefox15 fixed, firefox16 unaffected, firefox17 unaffected, firefox-esr10 unaffected)

Details

(Whiteboard: [advisory-tracking-][asan], crash signature)

Attachments

(3 attachments, 2 obsolete attachments)

Posted file Reprofile
Reprofile as attachment.

ASAN report:

==9109== ERROR: AddressSanitizer heap-buffer-overflow on address 0x7f785d2a3a90 at pc 0x7f78882553fb bp 0x7fffcab62c90 sp 0x7fffcab62c88
READ of size 8 at 0x7f785d2a3a90 thread T0
    #0 0x7f78882553fb in nsFontVariantTextRunFactory::RebuildTextRun(nsTransformedTextRun*, gfxContext*) /home/attekett/firefox/src/../../dist/include/nsAutoPtr.h:1037
    #1 0x7f7888257656 in nsCaseTransformTextRunFactory::RebuildTextRun(nsTransformedTextRun*, gfxContext*) /home/attekett/firefox/src/layout/generic/nsTextRunTransformations.cpp:961
    #2 0x7f7888213c08 in nsTArray_base<nsTArrayDefaultAllocator>::Length() const /home/attekett/firefox/src/../../dist/include/nsTArray.h:224
    #3 0x7f788820fa2d in BuildTextRunsScanner::FlushFrames(bool, bool) /home/attekett/firefox/src/layout/generic/nsTextFrameThebes.cpp:1408
    #4 0x7f7888218841 in ~BuildTextRunsScanner /home/attekett/firefox/src/layout/generic/nsTextFrameThebes.cpp:795
    #5 0x7f788823e2f8 in nsTextFrame::ReflowText(nsLineLayout&, int, nsRenderingContext*, bool, nsHTMLReflowMetrics&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsTextFrameThebes.cpp:7412
    #6 0x7f78881c11ae in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) /home/attekett/firefox/src/layout/generic/nsLineLayout.cpp:870
    #7 0x7f78881b63d9 in nsInlineFrame::ReflowInlineFrame(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsIFrame*, unsigned int&) /home/attekett/firefox/src/layout/generic/nsInlineFrame.cpp:713
    #8 0x7f78881b5836 in nsInlineFrame::ReflowFrames(nsPresContext*, nsHTMLReflowState const&, nsInlineFrame::InlineReflowState&, nsHTMLReflowMetrics&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsInlineFrame.cpp:575
    #9 0x7f78881b4da6 in nsInlineFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsInlineFrame.cpp:429
    #10 0x7f78881c10f1 in nsLineLayout::ReflowFrame(nsIFrame*, unsigned int&, nsHTMLReflowMetrics*, bool&) /home/attekett/firefox/src/layout/generic/nsLineLayout.cpp:863
    #11 0x7f78880ef319 in nsBlockFrame::ReflowInlineFrame(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:3834
    #12 0x7f78880ee0ce in nsBlockFrame::DoReflowInlineFrames(nsBlockReflowState&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:3630
    #13 0x7f78880eb35a in nsBlockFrame::ReflowInlineFrames(nsBlockReflowState&, nsLineList_iterator, bool*) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:3482
    #14 0x7f78880e5cc3 in nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:2570
    #15 0x7f78880dfef0 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:2020
    #16 0x7f78880dab59 in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /home/attekett/firefox/src/layout/generic/nsBlockFrame.cpp:1069
    #17 0x7f78880fd63a in nsBlockReflowContext::ReflowBlock(nsRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) /home/attekett/firefox/src/layout/generic/nsBlockReflowContext.cpp:295
0x7f785d2a3a90 is located 0 bytes to the right of 16-byte region [0x7f785d2a3a80,0x7f785d2a3a90)
allocated by thread T0 here:
    #0 0x41b722 in malloc ??:0
    #1 0x7f788f189398 in moz_xmalloc /home/attekett/firefox/src/memory/mozalloc/mozalloc.cpp:87
==9109== ABORTING
Stats: 141M malloced (146M for red zones) by 300926 calls
Stats: 43M realloced by 18278 calls
Stats: 104M freed by 182253 calls
Stats: 0M really freed by 0 calls
Stats: 312M (79917 full pages) mmaped in 78 calls
  mmaps   by size class: 8:229362; 9:49146; 10:16380; 11:18423; 12:3072; 13:2048; 14:1536; 15:384; 16:384; 17:128; 18:160; 19:48; 20:12;
  mallocs by size class: 8:220142; 9:42496; 10:15462; 11:16463; 12:2208; 13:1672; 14:1475; 15:322; 16:352; 17:118; 18:156; 19:48; 20:12;
  frees   by size class: 8:120175; 9:32460; 10:12053; 11:13298; 12:1391; 13:797; 14:1307; 15:271; 16:285; 17:103; 18:57; 19:46; 20:10;
  rfrees  by size class:
Stats: malloc large: 334 small slow: 1634
Shadow byte and word:
  0x1fef0ba54752: fb
  0x1fef0ba54750: 00 00 fb fb fb fb fb fb
More shadow bytes:
  0x1fef0ba54730: 00 04 fb fb fb fb fb fb
  0x1fef0ba54738: fb fb fb fb fb fb fb fb
  0x1fef0ba54740: fa fa fa fa fa fa fa fa
  0x1fef0ba54748: fa fa fa fa fa fa fa fa
=>0x1fef0ba54750: 00 00 fb fb fb fb fb fb
  0x1fef0ba54758: fb fb fb fb fb fb fb fb
  0x1fef0ba54760: fa fa fa fa fa fa fa fa
  0x1fef0ba54768: fa fa fa fa fa fa fa fa
  0x1fef0ba54770: 00 00 fb fb fb fb fb fb
Component: General → Layout: Text
Product: Firefox → Core
QA Contact: general → layout.fonts-and-text
Most likely, yes. I'll take a look - given a reproducible testcase, it should be debuggable without too much trouble.
This was a bug in the patch for bug 307039 - it added the local |styleContext| variable to avoid repeatedly looking up the current element in the textRun's mStyles[] array, but it's not valid to do this for the last iteration of the loop, when i==length and we're beyond the end of the array. In this case |styleContext| is not used anyway, so we shouldn't try to set it in the first place.
Assignee: nobody → jfkthame
Attachment #621306 - Flags: review?(smontagu)
Blocks: 307039
Duplicate of this bug: 752168
Keywords: regression
Version: Trunk → 15 Branch
Duplicate of this bug: 752236
Attachment #621416 - Flags: review?(smontagu)
OK, one more update - this is the same code as the previous version, but has added comments to clarify what's actually going on with the loop here.
Attachment #621306 - Attachment is obsolete: true
Attachment #621416 - Attachment is obsolete: true
Attachment #621306 - Flags: review?(smontagu)
Attachment #621416 - Flags: review?(smontagu)
Attachment #621419 - Flags: review?(smontagu)
Comment on attachment 621419 [details] [diff] [review]
patch with added comments to clarify the tricky loop

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

The comments help, but there's still something a bit clunky about this loop. However, I can't see a way to improve it.
Attachment #621419 - Flags: review?(smontagu) → review+
https://hg.mozilla.org/mozilla-central/rev/e6228decd18e
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Crash Signature: [@ nsFontVariantTextRunFactory::RebuildTextRun(nsTransformedTextRun*, gfxContext*)]
Verified fixed using testcase and ASAN build from 5/10.
Status: RESOLVED → VERIFIED
Does this bug have any security impact?
This does not look like a security issue -- the previous code read a possibly invalid style[i] but did not use it unless i was safely < length. A time bomb: might have triggered an access violation (denial of service) and a slight chance some future coder might have started using that handy (and usually correct) styleContext variable without checking for the i==length case. But not actually vulnerable in the pre-fixed code.

Thanks for catching and reporting this case
Whiteboard: [advisory-tracking-]
Group: core-security
Summary: ASAN: heap-buffer-overflow at nsFontVariantTextRunFactory::RebuildTextRun → out-of-bounds read at nsFontVariantTextRunFactory::RebuildTextRun
Whiteboard: [advisory-tracking-] → [advisory-tracking-][asan]
You need to log in before you can comment on or make changes to this bug.