1275059 - heap-buffer-overflow at CanBreakLineBefore

Status: RESOLVED FIXED

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

Description

[Aki Helin]

Attachment: 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
[...]

Comment 1

[:Gijs (he/him)]

Jonathan, not 100% sure this is the right place, can you redirect if necessary and/or ping the right people? Thanks!

Comment 2

[Jonathan Kew [:jfkthame]]

Xidorn, this has text-combine-upright in it... can you take a look?

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

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.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 5

[Xidorn Quan [:xidorn] UTC+10]

Looks like switching to MOZ_ASSERT is fine so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a1bb81cf6cef

Comment 6

[Jonathan Kew [:jfkthame]]

Comment on attachment 8755642 [details] [diff] [review]
patch

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

Looks OK. We should add the example as a crashtest, too.

Comment 7

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: followup patch to use MOZ_ASSERT

Probably could make this kind of issues more visible.

Comment 9

[Jonathan Kew [: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.

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2c2fdd7ea7ff410a7e22293d7bb3caccb58fa4
Bug 1275059 - Simple fix for this bug. r=jfkthame

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8be8ae4cf532b21e261faf43b4360c534286bbf
Bug 1275059 followup - Use MOZ_ASSERT for methods of gfxTestRun. r=jfkthame

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8755642 [details] [diff] [review]
patch

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

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch for crashtest

We may want to land this patch after the bug gets disclosed.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cb2c2fdd7ea7

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8be8ae4cf53

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8755642 [details] [diff] [review]
patch

Taking it to fix a crash.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f2a42f8eda64
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba38537fbb5a

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e23a24b7a75
Crashtest for this bug. DONTBUILD

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e23a24b7a75