Closed Bug 850672 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: Layout, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla22
Tracking Flags:
Tracking Status
firefox21 --- unaffected
firefox22 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: miaubiz, Assigned: mattwoodrow)

References

Details

(5 keywords, Whiteboard: [asan][adv-main22-])

Attachments

(4 files, 1 obsolete file)

repro case
911 bytes, text/plain
Details
asan log osx
9.31 KB, text/plain
Details
asan log linux
5.83 KB, text/plain
Details
Fix RemoveFrame
1.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached file 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
Attached file asan log osx — 

    
    

    
  

      
      
      
      

        Attached file
          
        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]
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-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix RemoveFrame (obsolete)
        — 
          — Splinter Review
      

    


  
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)

    
  
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!

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Fix RemoveFrame
        — 
          — Splinter Review
      

    


  
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)

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/b9f77eff4526
Status: NEW → RESOLVED
Closed: 11 years ago

          status-firefox22:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Whiteboard: [asan] → [asan][adv-main22-]
Group: core-security

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: