Closed Bug 865147 Opened 11 years ago Closed 11 years ago

Crash in nsContentUtils::ContentIsDescendantOf if select.add() is called with incorrect parameters

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Version:
19 Branch
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla23
Tracking Flags:
Tracking Status
firefox20 --- affected
firefox21 + fixed
firefox22 + fixed
firefox23 + verified

People

(Reporter: juho.roine, Assigned: Gijs)

References

Details

(4 keywords)

Crash Data

Attachments

(3 files, 1 obsolete file)

Reporter's testcase
494 bytes, text/html
Details
Trunk testcase
495 bytes, text/html
Details
Patch with test
2.04 KB, patch
bzbarsky
: review+
Gijs
: feedback+
bajaj
: approval-mozilla-aurora+
bajaj
: approval-mozilla-beta+
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:20.0) Gecko/20100101 Firefox/20.0
Build ID: 20130329030352

Steps to reproduce:

Call select.add() with incorrect parameters -> crash firefox.
Exploit follows. Should work with 20.0.1 (Windows) and 20.0 (Ubuntu):

<html>
<head>
<script language="javascript">
function crashFirefox()
{
        selectItem=document.getElementById('crashSelect');
        var newOption = document.createElement('option');
        selectItem.add(selectItem, newOption); //Call select.add() with incorrect parameters -> crash
}
</script>
</head>

<body>
<form>
<select id="crashSelect"></select>
<input type="button" value="Crash!" onClick="crashFirefox();">
WARNING: WILL CRASH FIREFOX!
</form>
</body>
</html>



Actual results:

Firefox crash


Expected results:

Error message and script execution stop
Attached file Reporter's testcase — 

    
    

    
  
It crashes FF20 but not FF23, so I guess it's already fixed.
Severity: normal → critical
Crash Signature: [@ nsContentUtils::ContentIsDescendantOf(nsINode const*, nsINode const*) ]
Keywords: crash, 
          
            reproducible, 
          
            testcase

    
    

    
  
Crash report: https://crash-stats.mozilla.com/report/index/bp-2639fb96-0746-4e29-a9c7-88fc32130424
Status: UNCONFIRMED → NEW
Ever confirmed: true

    
  

          status-firefox20:
          --- → affected

          status-firefox21:
          --- → affected

          status-firefox22:
          --- → affected

          status-firefox23:
          --- → unaffected
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core

    
    

    
  
The issue is that in http://mxr.mozilla.org/mozilla-aurora/source/content/html/content/src/nsHTMLSelectElement.cpp#599, aBefore's parentNode is null, which makes nsContentUtils::ContentIsDescendantOf crash. As far as I can tell this is caused by the fixes for bug 807075. Because this is fixed on trunk (Firefox 23) by the new(er?) bindings, I'm not sure if the DOM people want to fix this on 21/22... ?
Depends on: 807075
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All

    
  
Blocks: 807075
No longer depends on: 807075
Version: 20 Branch → 19 Branch

    
    

    
  
I don't obtain the same regression range:
bad=2013-04-12
good=2013-04-13
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=7b8ed29c6bc0&tochange=24a6b5ed51e3
No longer blocks: 807075
Depends on: 807075
Summary: Firefox crashes if select.add() is called with incorrect parameters → Crash in nsContentUtils::ContentIsDescendantOf if select.add() is called with incorrect parameters
Version: 19 Branch → 20 Branch

    
    

    
  
Sorry for the mess, my regression range range is about the fix of the crash.
Version: 20 Branch → 19 Branch

    
    

    
  
(In reply to Loic from comment #6)
> Sorry for the mess, my regression range range is about the fix of the crash.
It's named a working range in that case.
Blocks: 807075
No longer depends on: 807075

    
    

    
  
With the following function:

function crashFirefox()
{
        selectItem=document.getElementById('crashSelect');
        var newOption = document.createElement('option');
        selectItem.add(newOption, newOption); //Call select.add() with incorrect parameters -> crash
}

I believe we do still crash.

          status-firefox23:
          unaffected → affected

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Simple nullcheck (obsolete)
        — 
          — Splinter Review
      

    


  
(In reply to :Ms2ger from comment #8)
> With the following function:
> 
> function crashFirefox()
> {
>         selectItem=document.getElementById('crashSelect');
>         var newOption = document.createElement('option');
>         selectItem.add(newOption, newOption); //Call select.add() with
> incorrect parameters -> crash
> }
> 
> I believe we do still crash.

Fascinating. This is true. This patch fixes trunk, and AFAICT should fix all the branches.
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

        Attachment #741246 -
        Flags: review?(khuey)

        Attachment #741246 -
        Flags: feedback?(Ms2ger)

    
    

    
  

      
      
      
      

        Attached file
          
        Trunk testcase
        — 
      

    


  
Testcase from comment #8 that crashes on trunk

    
    

    
  
Comment on attachment 741246 [details] [diff] [review]
Simple nullcheck

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

Yep. (I guess this matches the spec?)

Also add a test :)

        Attachment #741246 -
        Flags: feedback?(Ms2ger) → feedback+

    
    

    
  
(In reply to :Ms2ger from comment #11)
> (I guess this matches the spec?)

"NOT_FOUND_ERR: Raised if before is not a descendant of the SELECT element."

So I guess so. Also, this is what we did before bug 807075.

> Also add a test :)

I figured as much, was about to ask where, now just used my brain and looked. One crashtest coming up...

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch with test
        — 
          — Splinter Review
      

    


  
Patch with a test, carrying over feedback+.

        Attachment #741246 -
        Attachment is obsolete: true

        Attachment #741246 -
        Flags: review?(khuey)

        Attachment #741249 -
        Flags: review?(khuey)

        Attachment #741249 -
        Flags: feedback+

    
    

    
  
Comment on attachment 741249 [details] [diff] [review]
Patch with test

r=me

        Attachment #741249 -
        Flags: review?(khuey) → review+

    
  

          tracking-firefox21:
          --- → ?

          tracking-firefox22:
          --- → ?

          tracking-firefox23:
          --- → ?

    
    

    
  
Comment on attachment 741249 [details] [diff] [review]
Patch with test

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 807075 
User impact if declined: sites may cause a null-deref crash
Testing completed (on m-c, etc.): on inbound, passes automated tests
Risk to taking this patch (and alternatives if risky): very small
String or IDL/UUID changes made by this patch: none

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

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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/ba1bd4eccd7c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

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

    
    

    
  
Apologies, forgot to include the push URL: https://hg.mozilla.org/releases/mozilla-aurora/rev/b7bd03c0b3e5

    
    

    
  
(In reply to :Gijs Kruitbosch from comment #15)
> Checked into inbound:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1bd4eccd7c

:Gijs, considering we have only 2 beta's left before Fx21 goes out the door, and since this has been there since Fx19  , I would give it time to bake and let ride it on Fx22 instead unless there is a strong reason we should consider it for our final two beta's ? Just worried about any new fallout's this may have(due to baketime) and a chance to have worser regression out there for Fx21.

    
    

    
  
(In reply to bhavana bajaj [:bajaj] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #15)
> > Checked into inbound:
> > https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1bd4eccd7c
> 
> :Gijs, considering we have only 2 beta's left before Fx21 goes out the door,
> and since this has been there since Fx19  , I would give it time to bake and
> let ride it on Fx22 instead unless there is a strong reason we should
> consider it for our final two beta's ? Just worried about any new fallout's
> this may have(due to baketime) and a chance to have worser regression out
> there for Fx21.

It's just a null check that makes us throw an exception where we would otherwise crash, so I think this is perfectly safe to take. On the other hand, we already shipped this since 19, and this is the first bug about it, so I suspect it's fine to let it ride the trains as well.

    
    

    
  
Comment on attachment 741249 [details] [diff] [review]
Patch with test

Approving because its low risk.PLease land before tomorrow to get this into ur next beta.

        Attachment #741249 -
        Flags: approval-mozilla-beta? → approval-mozilla-beta+

    
    

    
  
(In reply to bhavana bajaj [:bajaj] from comment #21)
> Comment on attachment 741249 [details] [diff] [review]
> Patch with test
> 
> Approving because its low risk.PLease land before tomorrow to get this into
> ur next beta.

I'm in CEST, so only saw this just now. Landed: https://hg.mozilla.org/releases/mozilla-beta/rev/27b9c37360a9 .

    
  

          status-firefox21:
          affected → fixed
QA Contact: manuela.muntean

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: