It is very strange that data documents may load stuff if they are chrome documents. This lead to bug 770684 (though, I'll fix that bug in some other way). As far as I see, we have similar problems as bug 770684 if someone does something like document.implementation.createHTMLDocument("").body.innerHTML = "stuff". If that stuff contains images or scripts, they are loaded, I think.

Another option is to handle loads coming from data documents in some other way.

Should we (1) special case DataDocumentContentPolicy, so that it gets called even for chrome docs, or (2) do data document checks before any normal content policy checks and remove DataDocumentContentPolicy or (3) do all content policy checks for chrome documents? (3) would be the best from consistency point of view, but not from performance point of view. I think (2) should be ok. In that case ContentPolicy would really mean *Content*Policy. (2) would mean that CHECK_PRINCIPAL in nsContentPolicyUtils should do the data document checks. I prefer (2).

(2) makes sense to me.

I don't think we can do (3) since I believe we removed those checks for performance reasons. There's also the risk that it'd break existing addons not expecting chrome URLs to be passed in. 2 sounds good to me since content policies is an API that should be completely rewritten anyway.

#2 sounds fine to me.

I'm assuming at least some apps and addons are going to do this and might not correctly sanitize user-supplied content.

Looks like we've got two votes for approach #2, assigning back to Olli for next steps.

...still on my todo list.

Adds one QI to all chrome loads, and if the load is coming from data/image/resource doc, also getService call.

So I think we could do this and then bug 820029 as a followup. (Bug 820029 would let us use contentutils etc.) But feel free to disagree :) do_GetService call isn't exactly nice one, but at least it happens only on data/resource/docAsImage documents.

Comment on attachment 690476 [details] [diff] [review] tiny bit simpler I think I can live with this, with a few nits: 1) Document around nsDataDocumentContentPolicy::ShouldLoad that the "still valid" bit includes the fact that the caller doesn't actually pass all the arguments and such. 2) Do we really need to null-check OwnerDoc()? If we do, add a comment as to why. r=me with that

We don't need to null check OwnerDoc(). It was just handy to write the code that way. But I can change it.

Nah, it's fine as it is. Just comment that it makes the code simpler.

[Security approval request comment] How easily can the security issue be deduced from the patch? It is not so easy to see the security problem, but it is easy to see the behavior the patch is changing. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. The commit message will be about preventing loads from chrome data documents, and IMO that as such doesn't _sound_ like a security problem. Which older supported branches are affected by this flaw? All (Everything since bug 388597) How likely is this patch to cause regressions; how much testing does it need? Addons may break if they expect stuff to be loaded even on data documents. So some amount testing is needed.

Adding Alex so we can discuss when we want to take this since there is some risk.

(In reply to Al Billings [:abillings] from comment #15) > Adding Alex so we can discuss when we want to take this since there is some > risk. Given the fact that this is internally reported, a longstanding bug, it doesn't have a low risk evaluation, and the holidays are coming up (low feedback volume), I'd prefer that this landed at the beginning of the next cycle (1/7/2013).

Comment on attachment 690491 [details] [diff] [review] v3 sec-approval+ for landing on 1/7/2013 or later.

So, ok to land now?

(In reply to Olli Pettay [:smaug] from comment #18) > So, ok to land now? yep!

Given the higher than normal risk profile but low visibility, let's compromise and uplift to Aurora 20 and then to the corresponding ESR.

Comment on attachment 690491 [details] [diff] [review] v3 [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 388597 User impact if declined: chrome may accidentally load stuff from network it wasn't going to Testing completed (on m-c, etc.): landed two days ago Risk to taking this patch (and alternatives if risky): Addons may break if they expect stuff to be loaded even on data documents. So some amount testing is needed. String or UUID changes made by this patch: NA

Do we think that this could impact B2G? It's unclear what we're discussing here w/r/t chrome.

In theory b2g chrome code could accidentally load something from the network. But I'm not sure how much there is chrome code.

Comment on attachment 690491 [details] [diff] [review] v3 [Approval Request Comment] User impact if declined: chrome may accidentally load stuff from network it wasn't going to Fix Landed on Version: FF21, FF20 Risk to taking this patch (and alternatives if risky): may affect to some addons, but there are really no alternatives String or UUID changes made by this patch: NA Landed to m-c 2013-01-08 and Aurora 2013-01-12

Comment on attachment 690491 [details] [diff] [review] v3 Supporting addons on ESR branch is not the priority, so if there is fallout from taking this we'll have to manage that in another fashion rather than leave this unfixed on ESR.