range.createContextualFragment() crash when range node is DocType

Bug #69719 reported by José Paulo Matafome Oleiro
256
Affects Status Importance Assigned to Milestone
Mozilla Firefox
Fix Released
Critical
firefox (Ubuntu)
Fix Released
Critical
Mozilla Team

Bug Description

I'm was openning the following address http://zugzug.homelinux.org/teste.html and Firefox suddenly crashed and my computer begins to work very slow

CVE References

Revision history for this message
In , Dveditz (dveditz) wrote :

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)

Revision history for this message
In , Jonas-sicking (jonas-sicking) wrote :

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.

Revision history for this message
In , Jonas-sicking (jonas-sicking) wrote :

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.

Revision history for this message
José Paulo Matafome Oleiro (matafomeoleiro) wrote : Firefox crash

I'm was openning the following address http://zugzug.homelinux.org/teste.html and Firefox suddenly crashed and my computer begins to work very slow

Revision history for this message
José Paulo Matafome Oleiro (matafomeoleiro) wrote :

This is the relatory of the crash

Revision history for this message
In , Hhschwab (hhschwab) wrote :
Revision history for this message
Manuel López-Ibáñez (manuellopezibanez) wrote : Re: Firefox crash

Confirmed.

Could you test with other versions of Firefox and/or from other distributions, windows or mac os?

Changed in firefox:
status: Unconfirmed → Confirmed
Revision history for this message
José Paulo Matafome Oleiro (matafomeoleiro) wrote : Re: [Bug 69719] Re: Firefox crash

I will check it with windows!

On Wednesday, 1 de November de 2006 18:14, manu wrote:
> Confirmed.
>
> Could you test with other versions of Firefox and/or from other
> distributions, windows or mac os?
>
> ** Changed in: firefox (Ubuntu)
> Status: Unconfirmed => Confirmed

Revision history for this message
Manuel López-Ibáñez (manuellopezibanez) wrote : Re: Firefox crash

This seems like a security vulnerability, however, I am not sure whether it is know or not. If you have time, perhaps you could try to look it up in https://launchpad.net/distros/ubuntu/+source/firefox/+bugs

or upstream in https://bugzilla.mozilla.org

Try using parts of the testcase to find it:

range = document.createRange();
range.selectNode(document.firstChild);
range.createContextualFragment('<span></span>');

Revision history for this message
In , Dveditz (dveditz) wrote :

Created attachment 244356
modified original testcase (won't crash on open)

Revision history for this message
In , Jonas-sicking (jonas-sicking) wrote :

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.

Revision history for this message
In , Super-fakeman (super-fakeman) wrote :

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?

Revision history for this message
In , Dveditz (dveditz) wrote :

Created attachment 244371
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.

Revision history for this message
José Paulo Matafome Oleiro (matafomeoleiro) wrote : Re: Firefox crash

This seems to crash in Firefox 2.0 for Windows, and crash Galeon too. This is a security bug I think. Not sure since I'm not a programmer, only and advanced user, anyone can help get more details about this? Only Internet Explorer had opened the site without problems.

Revision history for this message
José Paulo Matafome Oleiro (matafomeoleiro) wrote :

Details that I had obtained using Internet Explorer
Security bug! Malicious java script code. Confirmed!

<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN">
<html>
<head>
<script type="text/javascript">
function do_crash()
{
var range;

range = document.createRange();
range.selectNode(document.firstChild);
range.createContextualFragment('<span></span>');
}
</script>
</head>
<body onload="do_crash()">
<p>Good bye Firefox!</p>
</body>
</html>

Revision history for this message
José Paulo Matafome Oleiro (matafomeoleiro) wrote :

I think this is a buffer overflow exploit.

Revision history for this message
Manuel López-Ibáñez (manuellopezibanez) wrote :

This is known upstream.

Changed in firefox:
status: Unknown → Confirmed
Revision history for this message
In , Dveditz (dveditz) wrote :

Where do you put the try/catch? doesn't seem to help me any on FF2.

Revision history for this message
In , Jonas-sicking (jonas-sicking) wrote :

Comment on attachment 244371
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

Revision history for this message
In , Jonas-sicking (jonas-sicking) wrote :

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.

Revision history for this message
In , Adam Guthrie (ispiked) wrote :

Created attachment 245475
mochittest compatible testcase

Revision history for this message
In , Adam Guthrie (ispiked) wrote :

Comment on attachment 245475
mochittest compatible testcase

dveditz, can you take a quick look at this to make sure it's correct?

Revision history for this message
In , Reed Loden (reed) wrote :

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?

Revision history for this message
In , Jonas-sicking (jonas-sicking) wrote :

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.

Revision history for this message
In , Dveditz (dveditz) wrote :

Comment on attachment 245475
mochittest compatible testcase

r=dveditz

Revision history for this message
In , RobertSayre (sayrer) wrote :

Checking in tests/test_bug358797.html;
/cvsroot/mozilla/testing/mochitest/tests/test_bug358797.html,v <-- test_bug358797.html
initial revision: 1.1
done

Revision history for this message
In , Dveditz (dveditz) wrote :

Comment on attachment 244371
1.8 branch patch

approved for 1.8 branch, a=dveditz for drivers

Changed in firefox:
status: Confirmed → In Progress
Revision history for this message
In , Aleksej (aleksejrs) wrote :

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

Revision history for this message
In , Aleksej (aleksejrs) wrote :

The verification above was with the modified original testcase.

Revision history for this message
In , Cbook (cbook) wrote :

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 :)

Revision history for this message
In , Dveditz (dveditz) wrote :
Revision history for this message
In , Cbook (cbook) wrote :

verified fixed because of comment #19 and #21

Changed in firefox:
status: In Progress → Fix Released
David Farning (dfarning)
Changed in firefox:
assignee: nobody → mozillateam
importance: Undecided → Critical
Revision history for this message
Alexander Sack (asac) wrote :

According to upstream, this bug should be fixed in 2.0.0.1/1.5.0.9. Can you confirm that those versions fixed the initial test-case?

BTW, I cannot test the url of your initial report, as the page seems to be unavailable.

Thanks,

Alexander

Revision history for this message
Alex Latchford (alex.latchford) wrote :

I have put the code José Paulo Matafome Oleiro into a file on my localhost, accessed it and nothing appeared to happen, I am marking this as Fix Released unless anyone else can say otherwise.

Thanks, Alex.

Changed in firefox:
status: Confirmed → Fix Released
Changed in firefox:
importance: Unknown → Critical
To post a comment you must log in.
This report contains Public Security information  
Everyone can see this security related information.

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.