The default bug view has changed. See this FAQ.

range.createContextualFragment() crash when range node is DocType

VERIFIED FIXED

Status

()

Core
DOM: Core & HTML
--
critical
VERIFIED FIXED
11 years ago
4 years ago

People

(Reporter: dveditz, Assigned: dveditz)

Tracking

({crash, verified1.8.0.9, verified1.8.1.1})

1.8 Branch
crash, verified1.8.0.9, verified1.8.1.1
Points:
---
Bug Flags:
blocking1.8.1.1 +
blocking1.8.0.9 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments)

(Assignee)

Description

11 years ago
Carlos Barros reported a DoS crash calling Mozilla-specific createContextualFragment on a range whose selected node is the DocType. This leads to a null-dereference crash in FF1.5.0.x and FF2

http://www.gotfault.net/research/advisory/gadv-firefox.txt
http://lists.grok.org.uk/pipermail/full-disclosure/2006-October/050416.html

According to the DOM spec trying to select the DocType should raise an exception

http://www.w3.org/TR/2000/REC-DOM-Level-2-Traversal-Range-20001113/ranges.html#Level2-Range-method-selectNode

Updated

11 years ago
Flags: in-testsuite?
(Assignee)

Comment 1

11 years ago
This appears to have been fixed on trunk on Oct 20. bug 357445 is the only possibly relevant patch, and it's a biggie.

I initially suspected bug 336381 of fixing this since it added nsRange::IsValidBoundary that throws the right exception, but that was checked in May 2006 so clearly not. Looks like that's only called from SetStart/SetEnd but not selectNode.

selectNode does check for some bad node types, but doctype isn't one of those.

The existing validity checks only check the specified node itself, doesn't look like there are any "or ancestor" checks as mentioned in the DOM spec.

Would be simple to add nsIDOMNode::DOCUMENT_TYPE_NODE: at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsRange.cpp&rev=1.211&mark=707#694

Would that be sufficient? Probably leaves all sorts of variants given the inconsistent checks (and some missing on the 1.8 branch)
Like you said, bug 357445 was a pretty hard whack to the range code, so I'm not sure what fixed it. The stack in the report seems very odd and not related to the described crash.
Adding DOCUMENT_TYPE_NODE there would be wrong. You are allowed to select *around* a doctype, you're just not allowed to put a range endpoint *inside* the doctype.

Checking for a doctype up the parent chain is not needed since there is no way to give doctypes any children.

Comment 4

11 years ago
Talkback records on testcase http://werterxyz.altervista.org/Firefox2Range.htm from http://www.derkeiler.com/Mailing-Lists/securityfocus/bugtraq/2006-10/msg00509.html

TB25358411M Firefox 1.5.0.8 
TB25349295K Firefox 2.0 
(Assignee)

Comment 5

11 years ago
Created attachment 244356 [details]
modified original testcase (won't crash on open)
I suspect we should fix this two ways on branch. One problem is that selectNode(docType) will create a range in an invalid state since we'll actually put the endpoints *inside* the doctype (due to the ugly hack where selectNode on a node whos parent is a document acts as selectNodeContents).

But we should also be able to deal with the parser being null rather than crashing.

Comment 7

11 years ago
I don't know if this is of any relevance, but if I wrap this POC in a try/catch exception handler, Firefox doesn't crash here.

Why?
(Assignee)

Comment 8

11 years ago
Created attachment 244371 [details] [diff] [review]
1.8 branch patch

This stops the crash, but what bad things could happen if we propagate the bad state?

The null-check on the sink is extra, but looked like it could lead to the same kinds of problems. I'll happily put it back if you think I should.
Attachment #244371 - Flags: review?(bugmail)

Updated

11 years ago
Flags: blocking1.8.1.1?
(Assignee)

Comment 9

11 years ago
Where do you put the try/catch? doesn't seem to help me any on FF2.
Comment on attachment 244371 [details] [diff] [review]
1.8 branch patch

>Index: parser/htmlparser/src/nsParser.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/parser/htmlparser/src/nsParser.cpp,v
>retrieving revision 3.370.4.4
>diff -u -p -6 -r3.370.4.4 nsParser.cpp
>--- parser/htmlparser/src/nsParser.cpp	13 Jul 2006 17:28:08 -0000	3.370.4.4
>+++ parser/htmlparser/src/nsParser.cpp	1 Nov 2006 23:28:14 -0000
>@@ -1870,19 +1870,27 @@ nsParser::ParseFragment(const nsAString&
>   if (NS_FAILED(result)) {
>     mFlags |= NS_PARSER_FLAG_OBSERVERS_ENABLED;
>     return result;
>   }
> 
>   nsCOMPtr<nsIFragmentContentSink> fragSink = do_QueryInterface(mSink);
>-  NS_ASSERTION(fragSink, "ParseFragment requires a fragment content sink");
>+  if (!fragSink) {
>+    NS_ERROR("ParseFragment requires a fragment content sink");
>+    mFlags |= NS_PARSER_FLAG_OBSERVERS_ENABLED;
>+    return kUnknownError;
>+  }

Just return NS_ERROR_HTMLPARSER_UNKNOWN instead.

>   if (!aXMLMode) {
>     // First, we have to flush any tags that don't belong in the head if there
>     // was no <body> in the context.
>     // XXX This is extremely ugly. Maybe CNavDTD should have FlushMisplaced()?
>-    NS_ASSERTION(mParserContext, "Parsing didn't create a parser context?");
>+    if (!mParserContext) {
>+      NS_ERROR("Parsing didn't create a parser context?");
>+      mFlags |= NS_PARSER_FLAG_OBSERVERS_ENABLED;
>+      return kInvalidParserContext;
>+    }

Same thing here, simply return NS_ERROR_HTMLPARSER_INVALIDPARSERCONTEXT

r/sr=sicking with that
Attachment #244371 - Flags: superreview+
Attachment #244371 - Flags: review?(bugmail)
Attachment #244371 - Flags: review+
I just checked, and on branch and on trunk up until today it has been possible to get into the same state by calling range.selectNodeContents(doctype), so I don't think we need to add extra checks for that on branch.

On trunk it should be impossible to get into that state starting today.
(Assignee)

Updated

11 years ago
Assignee: traversal-range → dveditz
Flags: blocking1.8.1.1? → blocking1.8.1.1+

Comment 12

11 years ago
Created attachment 245475 [details]
mochittest compatible testcase

Comment 13

11 years ago
Comment on attachment 245475 [details]
mochittest compatible testcase

dveditz, can you take a quick look at this to make sure it's correct?
Attachment #245475 - Flags: review?(dveditz)
As this is blocking1.8.1.1+, please request approval1.8.1.1 on the current patch.

Also, does this patch need to be committed to the trunk, or is it already
there?
Status: NEW → ASSIGNED
We may actually want to land this on trunk too even tough there are other things stopping crash there already. It's always good to have multiple levels of protection.
(Assignee)

Comment 16

11 years ago
Comment on attachment 245475 [details]
mochittest compatible testcase

r=dveditz
Attachment #245475 - Flags: review?(dveditz) → review+
Attachment #244371 - Flags: approval1.8.1.1?

Comment 17

11 years ago
Checking in tests/test_bug358797.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug358797.html,v  <--  test_bug358797.html
initial revision: 1.1
done
Flags: in-testsuite? → in-testsuite+
(Assignee)

Comment 18

11 years ago
Comment on attachment 244371 [details] [diff] [review]
1.8 branch patch

approved for 1.8 branch, a=dveditz for drivers
Attachment #244371 - Flags: approval1.8.1.1? → approval1.8.1.1+
(Assignee)

Updated

11 years ago
Whiteboard: needs landing
(Assignee)

Updated

11 years ago
Flags: blocking1.8.0.9+
Keywords: fixed1.8.0.9, fixed1.8.1.1
(Assignee)

Updated

11 years ago
Whiteboard: needs landing

Comment 19

11 years ago
Verified:
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9pre) Gecko/20061201 Firefox/1.5.0.9pre
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.1pre) Gecko/20061201 BonEcho/2.0.0.1pre

Comment 20

11 years ago
The verification above was with the modified original testcase.
Works for me on Windows XP x64 Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.1.1pre) Gecko/20061129 BonEcho/2.0.0.1pre
Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.9pre) Gecko/20061130 Firefox/1.5.0.9pre

Also tested with Vista and also no crash on testcase from comment #5. I leave this Bug as assigned, because i can`t find a checkin for this patch :)

Keywords: fixed1.8.0.9, fixed1.8.1.1 → verified1.8.0.9, verified1.8.1.1
(Assignee)

Comment 22

11 years ago
This was checked in to the branches on Nov 28

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=&branchtype=match&dir=mozilla%2Fparser&file=&filetype=match&who=dveditz%25cruzio.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-11-28&maxdate=2006-11-29&cvsroot=%2Fcvsroot

Marking fixed since it's not a trunk problem
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
verified fixed because of comment #19 and #21
Status: RESOLVED → VERIFIED

Updated

7 years ago
Component: DOM: Traversal-Range → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.