Closed Bug 1275059 Opened 6 years ago Closed 6 years ago

heap-buffer-overflow at CanBreakLineBefore


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

Not set



Tracking Status
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- fixed
firefox49 --- fixed
firefox-esr45 --- unaffected


(Reporter: aki.helin, Assigned: xidorn)



(Keywords: regression, sec-low)


(4 files)

Attached file repro.html
My tinderbox asan-build from a few days ago spots the following heap buffer overflow (read) every time when the attached file is opened.

==7900==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60f000090504 at pc 0x7f1c7bba9486 bp 0x7ffca1934d70 sp 0x7ffca1934d68
READ of size 4 at 0x60f000090504 thread T0 (Web Content)
    #0 0x7f1c7bba9485 in CanBreakLineBefore /builds/slave/m-aurora-l64-asan-000000000000/build/src/obj-firefox/dist/include/gfxFont.h:834
    #1 0x7f1c7bba9485 in AddInlineMinISizeForFlow /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsTextFrame.cpp:7917
    #2 0x7f1c7bbaa4c1 in AddInlineMinISize /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsTextFrame.cpp:8050
    #3 0x7f1c7b9a117a in DoInlineIntrinsicISize /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsContainerFrame.cpp:892
    #4 0x7f1c7b92b6de in GetMinISize /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsBlockFrame.cpp:738
    #5 0x7f1c7b9a1934 in ShrinkWidthToFit /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsFrame.cpp:4849
    #6 0x7f1c7b9a1934 in ComputeAutoSize /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsContainerFrame.cpp:937
    #7 0x7f1c7b9a97d7 in ComputeSize /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsFrame.cpp:4601
    #8 0x7f1c7babc9cf in InitConstraints /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsHTMLReflowState.cpp:2377
    #9 0x7f1c7bab3935 in Init /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsHTMLReflowState.cpp:396
    #10 0x7f1c7b94a377 in ComputeCollapsedBStartMargin /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsBlockReflowContext.cpp:166
    #11 0x7f1c7b943043 in ReflowBlockFrame /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsBlockFrame.cpp:3229
    #12 0x7f1c7b936c58 in ReflowLine /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsBlockFrame.cpp:2750
    #13 0x7f1c7b936c58 in ReflowDirtyLines /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsBlockFrame.cpp:2289
    #14 0x7f1c7b92f897 in Reflow /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsBlockFrame.cpp:1189
    #15 0x7f1c7b98cbfb in ReflowChild /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsContainerFrame.cpp:1022
    #16 0x7f1c7b98b353 in Reflow /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsCanvasFrame.cpp:643
    #17 0x7f1c7ba298a7 in ReflowChild /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsContainerFrame.cpp:1022
    #18 0x7f1c7ba298a7 in ReflowScrolledFrame /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsGfxScrollFrame.cpp:541
    #19 0x7f1c7ba2ac19 in ReflowContents /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsGfxScrollFrame.cpp:653
    #20 0x7f1c7ba2d007 in Reflow /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsGfxScrollFrame.cpp:887
    #21 0x7f1c7b9a203e in ReflowChild /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsContainerFrame.cpp:1065
    #22 0x7f1c7bbc91cf in Reflow /builds/slave/m-aurora-l64-asan-000000000000/build/src/layout/generic/nsViewportFrame.cpp:313
Jonathan, not 100% sure this is the right place, can you redirect if necessary and/or ping the right people? Thanks!
Flags: needinfo?(jfkthame)
Group: firefox-core-security → core-security
Component: General → Layout: Text
Flags: needinfo?(jfkthame)
Product: Firefox → Core
Xidorn, this has text-combine-upright in it... can you take a look?
Flags: needinfo?(bugzilla)
Group: core-security → layout-core-security
Assignee: nobody → bugzilla
Flags: needinfo?(bugzilla)
Attached patch patchSplinter Review
This is easy to fix.

The page actually hits an NS_ASSERTION. I wonder whether we want to convert all of the NS_ASSERTION there to MOZ_ASSERT so that they have a better visiblility, if we do think heap overflow is a security issue.
Attachment #8755642 - Flags: review?(jfkthame)
Given that we have being treating non-fatal assertions as test failure for reftests, I guess changing them to fatal ones doesn't make things much different, though.
Looks like switching to MOZ_ASSERT is fine so far:
Comment on attachment 8755642 [details] [diff] [review]

Review of attachment 8755642 [details] [diff] [review]:

Looks OK. We should add the example as a crashtest, too.
Attachment #8755642 - Flags: review?(jfkthame) → review+
So in this bug, at worst, we read one byte from some arbitrary memory location as a boolean value, and either crash safely with a SEGV or record whether we have an optional break according to that value. The "aData" we are calling in the branch just operates some in-stack fields, so shouldn't causes extra harm. And even if the memory location contains sensitive information, it can only have one byte, and information is reduced to a boolean. Given this, I think this isn't exploitable, so sec-low.

Let me know if I missed something.
Keywords: sec-low
Probably could make this kind of issues more visible.
Attachment #8755785 - Flags: review?(jfkthame)
Comment on attachment 8755785 [details] [diff] [review]
followup patch to use MOZ_ASSERT

Review of attachment 8755785 [details] [diff] [review]:

Yes, I'm OK with doing this.

Although an out-of-bounds read in these accessors isn't likely to be a security vulnerability in itself (beyond the possibility of a SEGV-type crash), it definitely indicates a bug, and sometimes it'll be the first indication of a deeper problem such as using a stale textrun that should have been reconstructed (or maybe has been reconstructed, and we're using a stale pointer to a freed object). So we definitely want to be aware of any cases where we hit these assertions; increased visibility sounds good.
Attachment #8755785 - Flags: review?(jfkthame) → review+
Comment on attachment 8755642 [details] [diff] [review]

Approval Request Comment
[Feature/regressing bug #]: bug 1097499
[User impact if declined]: will crash with the given testcase
[Describe test coverage new/current, TreeHerder]: n/a, a crashtest may be landed later after this patch gets disclosed.
[Risks and why]: low risk, it just adds a bound check
[String/UUID change made/needed]: n/a
Attachment #8755642 - Flags: approval-mozilla-aurora?
We may want to land this patch after the bug gets disclosed.
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Group: layout-core-security → core-security-release
Comment on attachment 8755642 [details] [diff] [review]

Taking it to fix a crash.
Attachment #8755642 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 1276012
Duplicate of this bug: 1274579
Group: core-security-release
Keywords: regression
You need to log in before you can comment on or make changes to this bug.