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)
Tracking
()
VERIFIED
FIXED
People
(Reporter: dveditz, Assigned: dveditz)
References
()
Details
(Keywords: crash, verified1.8.0.9, verified1.8.1.1)
Attachments
(3 files)
522 bytes,
text/html
|
Details | |
1.56 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
dveditz
:
approval1.8.1.1+
|
Details | Diff | Splinter Review |
978 bytes,
text/plain
|
dveditz
:
review+
|
Details |
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•18 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 1•18 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•18 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•18 years ago
|
||
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?
Assignee | ||
Comment 8•18 years ago
|
||
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•18 years ago
|
Flags: blocking1.8.1.1?
Assignee | ||
Comment 9•18 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•18 years ago
|
Assignee: traversal-range → dveditz
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Comment 12•18 years ago
|
||
Comment 13•18 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)
Comment 14•18 years ago
|
||
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•18 years ago
|
||
Comment on attachment 245475 [details]
mochittest compatible testcase
r=dveditz
Attachment #245475 -
Flags: review?(dveditz) → review+
Updated•18 years ago
|
Attachment #244371 -
Flags: approval1.8.1.1?
Comment 17•18 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•18 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•18 years ago
|
Whiteboard: needs landing
Assignee | ||
Updated•18 years ago
|
Flags: blocking1.8.0.9+
Keywords: fixed1.8.0.9,
fixed1.8.1.1
Assignee | ||
Updated•18 years ago
|
Whiteboard: needs landing
Comment 19•18 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•18 years ago
|
||
The verification above was with the modified original testcase.
Comment 21•18 years ago
|
||
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 :)
Assignee | ||
Comment 22•18 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
Closed: 18 years ago
Resolution: --- → FIXED
See Also: → https://launchpad.net/bugs/69719
Updated•11 years ago
|
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•