Last Comment Bug 358797 - range.createContextualFragment() crash when range node is DocType
: range.createContextualFragment() crash when range node is DocType
Status: VERIFIED FIXED
: crash, verified1.8.0.9, verified1.8.1.1
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: 1.8 Branch
: All All
: -- critical (vote)
: ---
Assigned To: Daniel Veditz [:dveditz]
: Hixie (not reading bugmail)
:
Mentors:
http://lists.grok.org.uk/pipermail/fu...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-30 11:57 PST by Daniel Veditz [:dveditz]
Modified: 2013-04-04 13:53 PDT (History)
20 users (show)
dveditz: blocking1.8.1.1+
dveditz: blocking1.8.0.9+
sayrer: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
modified original testcase (won't crash on open) (522 bytes, text/html)
2006-11-01 13:46 PST, Daniel Veditz [:dveditz]
no flags Details
1.8 branch patch (1.56 KB, patch)
2006-11-01 15:31 PST, Daniel Veditz [:dveditz]
jonas: review+
jonas: superreview+
dveditz: approval1.8.1.1+
Details | Diff | Splinter Review
mochittest compatible testcase (978 bytes, text/plain)
2006-11-13 11:33 PST, Adam Guthrie
dveditz: review+
Details

Description Daniel Veditz [:dveditz] 2006-10-30 11:57:31 PST
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
Comment 1 Daniel Veditz [:dveditz] 2006-10-30 13:05:50 PST
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)
Comment 2 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-10-30 15:32:50 PST
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.
Comment 3 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-10-30 15:42:55 PST
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 Hermann Schwab 2006-11-01 09:47:17 PST
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 
Comment 5 Daniel Veditz [:dveditz] 2006-11-01 13:46:44 PST
Created attachment 244356 [details]
modified original testcase (won't crash on open)
Comment 6 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-01 14:04:15 PST
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 Alex 2006-11-01 14:47:52 PST
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?
Comment 8 Daniel Veditz [:dveditz] 2006-11-01 15:31:59 PST
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.
Comment 9 Daniel Veditz [:dveditz] 2006-11-03 09:23:05 PST
Where do you put the try/catch? doesn't seem to help me any on FF2.
Comment 10 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-03 16:24:20 PST
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
Comment 11 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-03 16:27:47 PST
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.
Comment 12 Adam Guthrie 2006-11-13 11:33:32 PST
Created attachment 245475 [details]
mochittest compatible testcase
Comment 13 Adam Guthrie 2006-11-13 11:56:26 PST
Comment on attachment 245475 [details]
mochittest compatible testcase

dveditz, can you take a quick look at this to make sure it's correct?
Comment 14 Reed Loden [:reed] (use needinfo?) 2006-11-27 00:08:56 PST
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?
Comment 15 Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-11-27 02:19:35 PST
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.
Comment 16 Daniel Veditz [:dveditz] 2006-11-27 10:15:13 PST
Comment on attachment 245475 [details]
mochittest compatible testcase

r=dveditz
Comment 17 Robert Sayre 2006-11-27 16:04:01 PST
Checking in tests/test_bug358797.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug358797.html,v  <--  test_bug358797.html
initial revision: 1.1
done
Comment 18 Daniel Veditz [:dveditz] 2006-11-27 16:58:24 PST
Comment on attachment 244371 [details] [diff] [review]
1.8 branch patch

approved for 1.8 branch, a=dveditz for drivers
Comment 19 [:Aleksej] 2006-12-01 10:38:53 PST
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 [:Aleksej] 2006-12-01 10:40:02 PST
The verification above was with the modified original testcase.
Comment 21 Carsten Book [:Tomcat] 2006-12-01 11:10:51 PST
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 :)

Comment 23 Carsten Book [:Tomcat] 2006-12-01 13:03:41 PST
verified fixed because of comment #19 and #21

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