Closed Bug 876092 Opened 7 years ago Closed 6 years ago

out of bounds stack read in mozilla::DisplayItemClip::IntersectWith

Categories

(Core :: Layout, defect, critical)

Product:
Core
Component:
Layout
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla24
Tracking Flags:
Tracking Status
firefox22 --- unaffected
firefox23 - fixed
firefox24 - fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g18-v1.0.0 --- unaffected
b2g18-v1.0.1 --- unaffected
b2g-v1.1hd --- unaffected

People

(Reporter: miaubiz, Assigned: roc)

References

Details

(6 keywords, Whiteboard: [asan][adv-main23-])

Attachments

(6 files)

repro case
459 bytes, text/plain
Details
asan log osx
15.52 KB, text/plain
Details
asan log linux
2.31 KB, text/plain
Details
trace + frame dump
29.05 KB, text/html
Details
fix
3.40 KB, patch
mats
: review+
lsblakk
: approval-mozilla-aurora+
Details | Diff | Splinter Review
fix failing reftest
3.65 KB, patch
mats
: review+
Details | Diff | Splinter Review
Attached file repro case — 
opening:

<html>
  <head>
    <style>
      #s2::before { 
        content: "a";
        position: absolute; 
      }
      #s1 {
        overflow: -moz-hidden-unscrollable;
      }
      #s3 {
        position: relative;
      }
      #s5 {
        position: absolute;
      }
    </style>
  </head>
  <body>
    <strike id="s1">
    <strike id="s2">
    <small id="s3">
      <div>
        <div id="s4"></div>
        </strike>
      </div>
      </body>
    </html>

crashes asan build on osx and linux like so:

==23010== ERROR: AddressSanitizer: stack-buffer-overflow on address 0x7fff5fbec8a0 at pc 0x1044d89c3 bp 0x7fff5fbe8d30 sp 0x7fff5fbe8d28
READ of size 1 at 0x7fff5fbec8a0 thread T0
    #0 0x1044d89c2 in mozilla::DisplayItemClip::IntersectWith(mozilla::DisplayItemClip const&) (in XUL) + 178
    #1 0x1044dc007 in mozilla::DisplayListClipState::GetCurrentCombinedClip(nsDisplayListBuilder*) (in XUL) + 343
    #2 0x1045ac194 in nsDisplayItem::nsDisplayItem(nsDisplayListBuilder*, nsIFrame*) (in XUL) + 116
    #3 0x10482cfed in nsCharClipDisplayItem::nsCharClipDisplayItem(nsDisplayListBuilder*, nsIFrame*) (in XUL) + 13


Address 0x7fff5fbec8a0 is located at offset 256 in frame <_ZN12nsBlockFrame16BuildDisplayListEP20nsDisplayListBuilderRK6nsRectRK16nsDisplayListSet> of T0's stack:
  This frame has 10 object(s):
    [32, 36) 'drawnLines'
    [96, 112) 'ca'
    [160, 168) 'textOverflow'
Attached file asan log osx —
Attached file asan log linux —
Blocks: 841192
ASan detects accessing a stack allocated DisplayItemClip (that has gone
out of scope), the address to it is stored on the
OutOfFlowDisplayDataProperty frame property.

Here's a trace of what happens, with the frame tree at the end.

nsIFrame::BuildDisplayListForChild aChild=0x7fffda316d50 (cyan)
creates the AutoClipMultiple on the stack then calls
MarkOutOfFlowFrameForDisplay which stores the address (pink) of
AutoClipMultiple::mExtraClip in the abs.pos. child 0x7fffda3171f8 (red)
OutOfFlowDisplayDataProperty->mContainingBlockClip.  Then we return
mkaing that property have a dangling pointer.

Later we call BuildDisplayListForChild on the placeholder (blue)
which looks up the property on the out-of-flow and try to use
its mContainingBlockClip (see call stack).
DisplayItemClip is a simple POD-type so I think the worst that can happen
is that we read random memory (likely from the stack) and use that to
intersect a nsRect or something.  So marking sec-low for now, until there's
evidence that we WRITE to that object.
Severity: normal → critical
Keywords: crash, csec-wildptr, regression, sec-low, testcase
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [asan]
roc, can you take this?

I don't like that we both create DisplayItemClips on the stack and that we have
a frame property with a DisplayItemClip*.  That seems like *really* fragile design
to put it mildly ;-)
Assignee: nobody → roc
Blocks: 876221
We don't track sec-low, and we only uplift sec-low bugs when there's something interesting going on.
tracking-firefox23: ? → -
tracking-firefox24: ? → -
It's kinda broken to have the abs-pos placeholder not be a descendant of the parent of its abs-pos frame. But OK...
status-b2g18: unaffected → ---
status-b2g18-v1.0.0: unaffected → ---
status-b2g18-v1.0.1: unaffected → ---
status-b2g-v1.1hd: unaffected → ---
status-firefox21: unaffected → ---
status-firefox22: unaffected → ---
status-firefox23: affected → ---
status-firefox24: affected → ---
status-firefox-esr17: unaffected → ---
tracking-firefox23: - → ---
tracking-firefox24: - → ---
Attached patch fix — — Splinter Review
Attachment #762539 - Flags: review?(matspal)
Comment on attachment 762539 [details] [diff] [review]
fix

r=mats
Attachment #762539 - Flags: review?(matspal) → review+
sec-low, only on Aurora, so I think we can just land this.
Comment on attachment 762539 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 841192
User impact if declined: possible crash or weird rendering in odd cases
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): very low risk
String or IDL/UUID changes made by this patch: none
Attachment #762539 - Flags: approval-mozilla-aurora?
Looks like this caused the regression.
The test image looks like it's been overdrawn many times, so clipping is probably failing to work somewhere.
This is an existing bug that was somehow not visibly failing due to this bug.
Attachment #762539 - Flags: approval-mozilla-aurora?
Comment on attachment 763530 [details] [diff] [review]
fix failing reftest

r=mats
Attachment #763530 - Flags: review?(matspal) → review+
https://hg.mozilla.org/mozilla-central/rev/7fc64b1eeebd
https://hg.mozilla.org/mozilla-central/rev/194268f3bd8a
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox24: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Comment on attachment 762539 [details] [diff] [review]
fix

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 841192
User impact if declined: possible crash or weird rendering in odd cases
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): very low risk
String or IDL/UUID changes made by this patch: none

(This approval request is for both patches)
Attachment #762539 - Flags: approval-mozilla-aurora?
Attachment #762539 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Flags: sec-bounty?
Whiteboard: [asan] → [asan][adv-main23-]
Does not qualify for the security bug bounty
Flags: sec-bounty? → sec-bounty-
Keywords: csec-bounds
Summary: stack buffer overflow with in mozilla::DisplayItemClip::IntersectWith → out of bounds stack read in mozilla::DisplayItemClip::IntersectWith
Group: core-security
Landed the crashtest:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77127ac8cc9f
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.