1188642 - Use channel->ascynOpen2 in dom/base/nsObjectLoadingContent.cpp

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1188642_nsobjectloadingcontent.patch

Putting up a WIP patch since this bug is still blocked by others, but the question is: What do we do about NS_CheckContentProcessPolicy in general?

Comment 2

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

Regarding NS_CheckContentProcessPolicy, I would say that we can either leave the checks as-is, or try to move them into the channel. I don't know if the checks are called at a consistent enough place that it makes sense to move into the channel. I.e. if the checks are always happening in OnStartRequest or if they always happen before the channel is opened.

If they are too inconsistent I would say leave them as-is.

Comment 3

[Tanvi Vyas[:tanvi]]

Some old notes I took on ShouldProcess() can be found on line 59 onwards here: https://etherpad.mozilla.org/ContentPolicies

I don't fully understand the use case for it and am not convinced that our implementation is sound.

Comment 4

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

I share Tanvi's concern that our ShouldProcess implementation is probably not sound. All the more reason to try to fix/remove it as a separate effort in separate bugs.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> I share Tanvi's concern that our ShouldProcess implementation is probably
> not sound. All the more reason to try to fix/remove it as a separate effort
> in separate bugs.

I agree with both of you. What I remember though, addons are still using them, so I think it's just safest if we leave them as-is for now. Once we are done with moving security checks into AsyncOpen2, we can probably start to deprecate them in a separate effort.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1188642_asyncopen2_nsobjectloadingcontent.patch

Hey Olli, since :bz is on PTO I was wondering if you feel comfortable reviewing a change within nsObjectLoadingContent or if you could suggest someone else who might be willing to review that change.

We have *not* converted nsURILoader to use asyncOpen2() yet (see Bug 1182569), so we can't remove all relevant security checks within nsObjectLoadingContent, because as you can see there is a call to uriloader->OpenChannel() within nsObjectLoadingContent(). Hence I would like to keep this changeset as self contained as possible so that security checks are only skipped at the callsite and enforced within .asyncOpen2() if we are in fact hitting that branch where we create a new channel and call asyncOpen2() on it. Happy to answer more questions if that description does not provide sufficient information.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b82d682f2a5c

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bae08c08118
Use channel->ascynOpen2 in dom/base/nsObjectLoadingContent.cpp r=smaug

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2bae08c08118