Closed Bug 358797 Opened 18 years ago Closed 18 years ago

range.createContextualFragment() crash when range node is DocType

Categories

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

1.8 Branch
defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: dveditz, Assigned: dveditz)

References

()

Details

(Keywords: crash, verified1.8.0.9, verified1.8.1.1)

Attachments

(3 files)

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
Flags: in-testsuite?
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.
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 
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.
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?
Attached patch 1.8 branch patchSplinter Review
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)
Flags: blocking1.8.1.1?
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: traversal-range → dveditz
Flags: blocking1.8.1.1? → blocking1.8.1.1+
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.
Comment on attachment 245475 [details]
mochittest compatible testcase

r=dveditz
Attachment #245475 - Flags: review?(dveditz) → review+
Attachment #244371 - Flags: approval1.8.1.1?
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+
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+
Whiteboard: needs landing
Flags: blocking1.8.0.9+
Whiteboard: needs landing
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
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 :)

verified fixed because of comment #19 and #21
Status: RESOLVED → VERIFIED
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.