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

RESOLVED FIXED in Firefox 50

Status

()

Core
DOM: Security
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks: 1 bug)

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Updated

2 years ago
Assignee: nobody → mozilla
Blocks: 1182535
(Assignee)

Updated

2 years ago
Depends on: 1206961, 1182569
(Assignee)

Comment 1

2 years ago
Created attachment 8664046 [details] [diff] [review]
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?
Flags: needinfo?(jonas)
Status: NEW → ASSIGNED
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.
Flags: needinfo?(jonas)
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.
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.
(Assignee)

Comment 5

2 years ago
(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.
(Assignee)

Updated

a year ago
Whiteboard: [domsecurity-active]
Blocks: 1269814
(Assignee)

Comment 6

a year ago
Created attachment 8765571 [details] [diff] [review]
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.
Attachment #8664046 - Attachment is obsolete: true
Attachment #8765571 - Flags: review?(bugs)
(Assignee)

Comment 7

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

Updated

a year ago
Attachment #8765571 - Flags: review?(bugs) → review+

Comment 8

a year ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2bae08c08118
Use channel->ascynOpen2 in dom/base/nsObjectLoadingContent.cpp r=smaug

Comment 9

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2bae08c08118
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Depends on: 1317641
You need to log in before you can comment on or make changes to this bug.