771942 - [FIX] DataDocumentContentPolicy should apply to chrome documents

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 3

[Henri Sivonen (:hsivonen)]

(2) makes sense to me.

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

#2 sounds fine to me.

Comment 6

[Daniel Veditz [:dveditz]]

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

Comment 7

[Daniel Veditz [:dveditz]]

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

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

...still on my todo list.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: WIP

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

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: tiny bit simpler

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 11

[Boris Zbarsky [:bzbarsky]]

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

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

[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.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

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

Comment 16

[Alex Keybl [:akeybl]]

(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 17

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 690491 [details] [diff] [review]
v3

sec-approval+ for landing on 1/7/2013 or later.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

So, ok to land now?

Comment 19

[Alex Keybl [:akeybl]]

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

yep!

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/6bf0c1bcdba1

Comment 21

[Alex Keybl [:akeybl]]

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 22

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8d59b8f96c24

Comment 24

[Alex Keybl [:akeybl]]

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

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

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 27

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 28

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-esr17/rev/dd8f1f98a631