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

RESOLVED FIXED in Firefox 21

Status

()

Core
DOM: Core & HTML
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: juho.roine@gmail.com, Assigned: Gijs)

Tracking

(4 keywords)

19 Branch
mozilla23
crash, regression, reproducible, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox20 affected, firefox21+ fixed, firefox22+ fixed, firefox23+ verified)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

Comment 1

5 years ago
Created attachment 741227 [details]
Reporter's testcase

Comment 2

5 years ago
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
(Assignee)

Comment 3

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

Updated

5 years ago
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox22: --- → affected
status-firefox23: --- → unaffected
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
(Assignee)

Comment 4

5 years ago
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

Updated

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

Comment 5

5 years ago
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

Comment 6

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

Comment 7

5 years ago
(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
(Assignee)

Comment 9

5 years ago
Created attachment 741246 [details] [diff] [review]
Simple nullcheck

(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)
(Assignee)

Comment 10

5 years ago
Created attachment 741247 [details]
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+
(Assignee)

Comment 12

5 years ago
(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...
(Assignee)

Comment 13

5 years ago
Created attachment 741249 [details] [diff] [review]
Patch with test

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+

Updated

5 years ago
tracking-firefox21: --- → ?
tracking-firefox22: --- → ?
tracking-firefox23: --- → ?
(Assignee)

Comment 15

5 years ago
Checked into inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ba1bd4eccd7c
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
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23

Updated

5 years ago
tracking-firefox22: ? → +
tracking-firefox23: ? → +

Updated

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

Updated

5 years ago
Keywords: checkin-needed
(Assignee)

Updated

5 years ago
status-firefox22: affected → fixed
status-firefox23: affected → fixed
Keywords: checkin-needed
(Assignee)

Comment 18

5 years ago
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+
(Assignee)

Comment 22

5 years ago
(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 .
(Assignee)

Updated

5 years ago
status-firefox21: affected → fixed

Updated

5 years ago
tracking-firefox21: ? → +
Verified fixed on: Mac OSX 10.7.5, Ubuntu 12.10 32bit and Windows XP, using both the reporter and trunk testcases.

Also, very few crashes reported on Socorro, within last month, for Fx 23 beta 2 and 3:

https://crash-stats.mozilla.com/report/list?signature=nsContentUtils%3A%3AContentIsDescendantOf%28nsINode+const%2A%2C+nsINode+const%2A%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A21.0&hang_type=any&date=2013-07-16+12%3A00%3A00&range_value=4
status-firefox23: fixed → verified

Updated

4 years ago
QA Contact: manuela.muntean
You need to log in before you can comment on or make changes to this bug.