Jesse Ruderman Reporter
	Description • 10 years ago

###!!! ASSERTION: Wrong scope, this is really bad!: 'js::GetGlobalForObjectCrossCompartment(obj) == newScope', file content/base/src/nsDocument.cpp, line 4458

	Peter Van der Beken [:peterv]
	Comment 2 • 10 years ago

mTemplateContentsOwner::mWillReparent is not set to true when calling SetScriptGlobalObject here: http://hg.mozilla.org/mozilla-central/annotate/18c21532848a/content/base/src/nsDocument.cpp#l4507. Maybe it should be set to the same value as the caller's mWillReparent?

	William Chen [:wchen] Assignee
	Comment 3 • 10 years ago

Following your advice I've set mTemplateContentsOwner::mWillReparent to true and added some code to reparent the contents owner document.

Some alternatives could be to not change the global of mTemplateContentsOwner except for setting it to nullptr. We could also just set the global to nullptr when the document is created and keep it that way. I'm not sure too sure about the consequences of these options given that the template contents owner is a data document.

	Peter Van der Beken [:peterv]
	Comment 4 • 10 years ago

Comment on attachment 8439550 [details] [diff] [review]
Reparent template contents owner document.

Review of attachment 8439550 [details] [diff] [review]:
-----------------------------------------------------------------

We'll need an automated testcase. Can the template content's document be accessed by script? It needs a global if so. And we need a testcase that does that after document.open() too. r+ assuming the document is exposed to script.

	William Chen [:wchen] Assignee
	Comment 5 • 10 years ago

Testcase as requested.

	William Chen [:wchen] Assignee
	Comment 6 • 10 years ago

Comment on attachment 8439550 [details] [diff] [review]
Reparent template contents owner document.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The comment suggests what the problem is.

Which older supported branches are affected by this flaw?
aurora

If not all supported branches, which bug introduced the flaw?
bug 1017896

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Patches should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause a regression, opening the attached testcase in debug build without assertion and passing the attached mochitest testcase should be sufficient.

	Al Billings [:abillings - ex-MoCo]
	Comment 7 • 10 years ago

Comment on attachment 8439550 [details] [diff] [review]
Reparent template contents owner document.

sec-approval+ for trunk. Once it is on trunk and trunk is green, please nominate an Aurora patch so we can avoid shipping this issue.

	William Chen [:wchen] Assignee
	Comment 9 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a5e4f7ea2cb

	Carsten Book [:Tomcat]
	Comment 10 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/0a5e4f7ea2cb

	William Chen [:wchen] Assignee
	Comment 11 • 10 years ago

Comment on attachment 8439550 [details] [diff] [review]
Reparent template contents owner document.

Approval Request Comment
[Feature/regressing bug #]: Bug 1017896
[User impact if declined]: Malicious scripts might be able to access objects in an old document global.
[Describe test coverage new/current, TBPL]: Code has a debug assertion that is executed by the included mochitest.
[Risks and why]: No risk to taking this patch.
[String/UUID change made/needed]: None

	Ryan VanderMeulen [:RyanVM]
	Comment 12 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/ff12bd072d73

	Matt Wobensmith [:mwobensmith][:matt:]
	Comment 13 • 10 years ago

Confirmed assert on Fx32, 2014-06-10
Verified fixed on Fx32, Fx33, 2014-07-14.