use-after-poison with tables, -moz-perspective and transform [@ OverflowChangedTracker::Flush]

RESOLVED FIXED in Firefox 22

Status

()

Core
Layout
--
critical
RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: miaubiz, Assigned: mattwoodrow)

Tracking

(5 keywords)

Trunk
mozilla22
crash, csectype-framepoisoning, regression, sec-other, testcase
Points:
---
Bug Flags:
sec-bounty -

Firefox Tracking Flags

(firefox21 unaffected, firefox22 fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [asan][adv-main22-])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Created attachment 724421 [details]
repro case

this: 

<html>
  <head>
    <style>
      #el0 {
        display: table-cell;
      }
      #el2 { 
        transform: skew(0);
        display: table;
        float: left;
      }
      #el1 { 
        float: left;
      } 
      .c0 { 
        float: left;
      } 
    </style>
    <script>
      onload = function() {
        el0=document.createElement('div')
        el0.setAttribute('id','el0')
        document.body.appendChild(el0)
        el1=document.createElement('div')
        el1.setAttribute('id','el1')
        document.body.appendChild(el1)
        el2=document.createElement('div')
        el2.setAttribute('id','el2')
        document.body.appendChild(el2)
        document.body.offsetTop
        document.styleSheets[0].insertRule("#el2 { -moz-perspective: 1px; } ", 0)
        el0.setAttribute('class', 'c0') 
        document.body.offsetTop
        }</script>
      </head><body>
  </body></html>


gives this:

==70831== ERROR: AddressSanitizer: use-after-poison on address 0x00011f429b98 at pc 0x104207bfb bp 0x7fff5fbeb570 sp 0x7fff5fbeb568
READ of size 8 at 0x00011f429b98 thread T0
    #0 0x104207bfa in nsIFrame::StyleContext() const (in XUL) + 42
    #1 0x104207af8 in nsIFrame::PresContext() const (in XUL) + 8
    #2 0x1042b3671 in nsIFrame::Properties() const (in XUL) + 113

0x00011f429b98 is located 7000 bytes inside of 8192-byte region [0x00011f428040,0x00011f42a040)
allocated by thread T0 here:
    #0 0x10000ec7c in 0x20000ec7c
    #1 0x7fff93066152 in _asl_msg_test_expression (in libsystem_c.dylib) + 82
    #2 0x7fff93066ba6 in _asl_msg_new_key_val_op (in libsystem_c.dylib) + 570
    #3 0x1025a4e9a in PL_ArenaAllocate (in libplds4.dylib) + 682
(Reporter)

Comment 1

5 years ago
Created attachment 724422 [details]
asan log osx
(Reporter)

Comment 2

5 years ago
Created attachment 724426 [details]
asan log linux
mOverflowChangedTracker contains a destroyed frame.  We do get a call to
nsCSSFrameConstructor::NotifyDestroyingFrame for that frame while it is in
the mOverflowChangedTracker list, but it appears the RemoveFrame call there
fails to remove it for some reason.
Severity: normal → critical
Keywords: crash, csec-framepoisoning, sec-other, testcase
OS: Mac OS X → All
Hardware: x86 → All
Summary: use-after-poison with tables, -moz-perspective and transform → use-after-poison with tables, -moz-perspective and transform [@ OverflowChangedTracker::Flush]
Whiteboard: [asan]
Flags: sec-bounty?
Assignee: nobody → matt.woodrow
Assignee: matt.woodrow → matspal
Well, I got as far as OverflowChangedTracker::RemoveFrame
http://hg.mozilla.org/mozilla-central/annotate/b2636816c7fd/layout/base/RestyleTracker.h#l59
Is depth==0 supposed to find the entry for the frame anywhere in the SplayTree?

I think Matt is a better owner for this bug, he wrote all of this code afaict.
Assignee: matspal → matt.woodrow
Our current understanding is that frame-poisoning is an effective mitigation so not awarding a bug bounty
Flags: sec-bounty? → sec-bounty-
(Assignee)

Comment 6

5 years ago
Created attachment 727019 [details] [diff] [review]
Fix RemoveFrame

Good catch Mats, that is exactly the issue.

This is a regression from bug 849263 I guess, since before that we were pretty much ignoring the depth parameter.
Attachment #727019 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Blocks: 849263
Entry(aFrame, false) expands to Entry(aFrame, aFrame->GetDepthInFrameTree(), false).
Would it be worth trying to avoid doing GetDepthInFrameTree() twice?
Also, this is called unconditionally on every destroyed frame so it might be
worth doing an early return if mEntryList is empty? (to avoid doing
GetDepthInFrameTree() at all).
(In reply to Mats Palmgren [:mats] from comment #7)
> Entry(aFrame, false) expands to Entry(aFrame, aFrame->GetDepthInFrameTree(),
> false).
> Would it be worth trying to avoid doing GetDepthInFrameTree() twice?

Yes please

> Also, this is called unconditionally on every destroyed frame so it might be
> worth doing an early return if mEntryList is empty? (to avoid doing
> GetDepthInFrameTree() at all).

Yes!
(Assignee)

Comment 9

5 years ago
Created attachment 727035 [details] [diff] [review]
Fix RemoveFrame

Good call. I took the liberty of fixing AddFrame in the same way too
Attachment #727019 - Attachment is obsolete: true
Attachment #727019 - Flags: review?(roc)
Attachment #727035 - Flags: review?(roc)
Attachment #727035 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b9f77eff4526
https://hg.mozilla.org/mozilla-central/rev/b9f77eff4526
Status: NEW → RESOLVED
Last Resolved: 5 years ago
status-firefox22: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
status-b2g18: --- → unaffected
status-firefox21: --- → unaffected
status-firefox-esr17: --- → unaffected
Keywords: regression
Whiteboard: [asan] → [asan][adv-main22-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.