1286838 - Deprecate nsIChannel->owner and replace with nsILoadInfo->triggeringPrincipal

Status: NEW

(Core :: DOM: Security, defect, P3)

Description

[Christoph Kerschbaumer [:ckerschb]]

Similar to what we are doing within Bug 1286472 we should also completely deprecate nsIChannel owner [1] and replace with nsILoadInfo triggeringPrincipal [2].

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIChannel.idl#62
[2] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsILoadInfo.idl#218

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1286838_deprecate_owner.patch

Boris, now that we have a loadInfo on all channels I think we should be able to remove the owner of a channel, right? Unfortunately TRY turns into a nice christmas tree after implementing that change. Could you provide some feedback? Am I missing something completely?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Well, most obviously that patch breaks chrome://, no?  I see nothing replacing the code that was removed from nsChromeProtocolHandler::NewChannel2.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3)
> Well, most obviously that patch breaks chrome://, no?  I see nothing
> replacing the code that was removed from
> nsChromeProtocolHandler::NewChannel2.

Indeed.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Boris, in order to have this bug landed we need to make some adjustments to GetChannelResultPrincipal and I was wondering about your opinion. Consider for example the following case:

>  channelURI: chrome://global/content/bindings/general.xml
>  ownerPrincipal: SystemPrincipal
>  loadingPrincipal: SystemPrincipal
>  triggeringPrincipal: SystemPrincipal
>  principalToInherit: nullptr
>  contentPolicyType: 9
>  securityMode: SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS, SEC_ALLOW_CHROME,

In the old world the ChromeProtocolHandler sets the owner of the channel to a SystemPrincipal [1]. When calling GetChannelResultPrincipal we then used to check for the owner first [2] and used to return SystemPrincipal.

In the new world we enter the clause for SEC_REQUIRE_SAME_ORIGIN_DATA_INHERITS but then only return the principalToInherit in case ChannelShouldInheritPrincipal returns true [3]. What do you think is the best way forward to compensate for the lack of the owner semantics?

[1] https://dxr.mozilla.org/mozilla-central/source/chrome/nsChromeProtocolHandler.cpp#182
[2] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#288
[3] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#358

Comment 7

[Boris Zbarsky [:bzbarsky]]

Well, we need to add something to the loadinfo that will let the protocol handler communicate the right behavior to GetChannelResultPrincipal.

The question is really how to do this in the simplest way possible, that interacts in the best possible way with all the other things that try to communicate that information already, right?  Bug 1319111 is adding such a thing, but as a URI, not principal.

What we should probably do is write down all the sources of principal information we might have and how they can/should interact, then try to figure out what a non-insane API for it would look like.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1286838_deprecate_owner.patch

Boris, within this patch I added a loadinfo.channelResultPrincipal which we can manually set for certain cases, e.g. within ChromeProtocolHandler (similar idea as in Bug 1319111).

Within ScriptSecurityManager::GetChannelResultPrincipal we first check if loadinfo.channelResultPrincipal is non-null, and if non-null then we return that principal (basically the previous owner case). For all other cases we can proceed as we do right now.

This patch would allow a parity implementation with the previous owner but with the benefit that we can remove the overruleOwner-case as well as having all information semantically bundled within the loadinfo. Would that be acceptable? (Please note that I still have to clean up the patch slightly to cover all cases).

Comment 9

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

I would recommend WONTFIXing this bug.

There's unfortunately too many places where we rely on the owner and don't have a decent other solution.

I don't recall all of the places where we use owner, but I recall two problematic areas in particular:

* The RSS-feed viewer creates a channel to load the viewer UI, but then sets the owner of the channel to be the URL of the
  RSS feed. This ensures that the viewer runs with the same privileges as the feed. This is good since the viewer renders
  a lot of complex content from the feed.
* The PDF viewer does some crazy stuff with owners. When I asked the pdf.js developers that had written the code in question
  they didn't really know. The answer was basically that they had changed things until it seemed to work. There's also no tests
  for this code so making changes means high risk of security problems as well as broken functionality.

Unless at the very least both these uses of owners go away, I don't think it's worth trying to remove nsIChannel.owner. There are far higher cleanup projects that would provide more bang for the buck.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #9)
> I would recommend WONTFIXing this bug.
> 
> There's unfortunately too many places where we rely on the owner and don't
> have a decent other solution.

Jonas, to me it feels semantically wrong that we have an owner on the channel but all the other security decisions are based on information we store in the loadInfo. By adding a channelResultPrincipal to the loadInfo we could deprecate the owner and solely rely on the loadInfo. I know that for the RSS-feeds that works, for PDF.js I am not sure, we would have to check.

Obviously we have other cleanup projects, but on the other hand we already have code for deprecating the owner with this patch which would just need a little cleanup to get it ready for landing. If both Boris and you strongly feel we should WONTFIX that bug, than obviously I will not argue against that in the end.

Comment 11

[Boris Zbarsky [:bzbarsky]]

I don't have strong feelings about this, apart from agreeing that it's not a super-high priority compared to the rest of the work we have, even in related code.

What I do think would be useful, if it's possible, is simplifying and rationalizing the final principal determination as much as we can, now that we have more implementation experience with it and a better idea of what the various moving pieces are.  If that involves/requires removing owner (and I suspect it does), great.  But just removing owner on its own, especially if we just replace it with an equivalent thing on the loadinfo without simplifying the overall model at all, is not very compelling.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #11)
> I don't have strong feelings about this, apart from agreeing that it's not a
> super-high priority compared to the rest of the work we have, even in
> related code.

Ok, agreed. Not giving up on this bug yet, but deprioritizing it for now -> backlog.