1295309 - In the newest nightly builds, its possible to partially load dev tools at content level.

Status: RESOLVED FIXED

(DevTools :: JSON Viewer, defect)

Description

[Jerri Rice (rehash pending)]

Attachment: partialDevtoolLoad-testcase-new.html

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160728205137

Steps to reproduce:

Inserted an iframe into a document with the src attribute pointed to a data:application/json URL.


Actual results:

In the iframe is loaded a panel that is part of the developer tools.  This window also provides a function called postChromeMessage, which appears atm to have been safely exported to content scope.  I was working on ways to exploit this combined with certain was to capture chrome level exceptions/errors at content level.


Expected results:

This result is fine as long as it is here in the newer nightly builds for testing.  I have not yet tried to get this panel to load in a release build.

Comment 1

[:Gijs (out for now; he/him)]

Honza, I thought we only loaded the JSON viewer for toplevel documents?

(The JSON viewer is disabled by default on non-nightly/aurora builds, behind the devtools.jsonview.enabled pref, so this indeed doesn't affect release.)

Comment 2

[Jerri Rice (rehash pending)]

I do remember seeing code in place that attempts to limit the loading of this to a top level window.  It's broken obviously.

Comment 3

[Jerri Rice (rehash pending)]

I thought I should mention that not only do you gain full access to the window loaded from a resource:// URI, through passing objects or arguments into the postChromeMessage function, you can get warnings about xraywrapper access denial and such.  One warning even indicates that passed in arguments directly interact with a chrome level object.

I'll put together smaller testcases later through today to show some of this off, as well as how this also allows capturing chrome level errors at content level again.

Comment 4

[Jerri Rice (rehash pending)]

Attachment: devtoolLoadChromeObject-testcase-new.html

Slightly modified testcase.  After it runs, open your content level web console for a warning showing that arguments did in fact interact with a chrome level object.

I did have an end goal here guys, sadly with so many things going on I'm not going to have time.  I'll get as much filed and documented as I can though.

Comment 5

[Jerri Rice (rehash pending)]

Attachment: Capturing chrome error

This shows a "chrome level" error being returned from interaction with this function.

It's almost identical to the previous testcase yet shows that the chrome object may be the error throw.

Comment 6

[Jerri Rice (rehash pending)]

Oh yeah forgot, open web console as before.  Man this is tedious from a chromebook ;-)

Comment 7

[:Gijs (out for now; he/him)]

Seems like in the code here: http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/network-helper.js#275

for this testcase, the outer window ID is always 4294967297 and it matches the parent outer window ID. Because the data URIs are loaded in frames, AIUI from the documentation this shouldn't be the case. Olli, is this a bug in the loadinfo code or the documentation or my understanding of it? :-)

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Sorry, I'm not familiar with window IDs in nsILoadInfo. Would need to read all the relevant code.

Looks like that stuff is coming from bug 1163861, so
billm should know more.

Comment 9

[:Gijs (out for now; he/him)]

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #7)
> Seems like in the code here:
> http://searchfox.org/mozilla-central/source/devtools/shared/webconsole/
> network-helper.js#275
> 
> for this testcase, the outer window ID is always 4294967297 and it matches
> the parent outer window ID. Because the data URIs are loaded in frames, AIUI
> from the documentation this shouldn't be the case. Olli, is this a bug in
> the loadinfo code or the documentation or my understanding of it? :-)

(In reply to Olli Pettay [:smaug] from comment #8)
> Sorry, I'm not familiar with window IDs in nsILoadInfo. Would need to read
> all the relevant code.
> 
> Looks like that stuff is coming from bug 1163861, so
> billm should know more.

--> Bill? :-)

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

It looks like this is a side effect of bug 1256595. We should have updated isTopLevelLoad to do something like this:
https://hg.mozilla.org/mozilla-central/rev/fa6552880a34#l6.14

Kershaw, do you mind taking this since you worked on the other bug? I think it should be a simple fix.

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Add isTopLevelLoad attribute in nsILoadInfo

Sorry for causing this side effect.

Could you take a look at this patch? Thanks.

Comment 12

[:Gijs (out for now; he/him)]

Ah, that explains why I couldn't reproduce on 48 and 49 when I flipped the pref for the json viewer in about:config. I initially thought maybe the tool was not available on those version at all, but this is just a regression in 50. Thanks for clarifying Bill!

I do wonder if other consumers are affected by that change...

Comment 13

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I can verify that the patch solves the issue. JSON viewer isn't loaded
inside an iframe anymore.

Honza

Comment 14

[Jerri Rice (rehash pending)]

To those who know me and my situation, it's been a pleasure absolutely.  I'll probably still do what I can like this.  For some reason finding these issues right as they're introduced recently has made me feel great in regards to things overall.

Something like feeling my skills have at least been upped a notch, and that's never a small thing.

Cheers to all.

Comment 15

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8782783 [details] [diff] [review]
Add isTopLevelLoad attribute in nsILoadInfo

Review of attachment 8782783 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsILoadInfo.idl
@@ +609,5 @@
>    }
>  %}
> +
> +  /**
> +   * Returns true if this load is for top level document.

Please add to this comment:
"Note that the load for a sub-frame's document will return false here."

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Jerri Rice from comment #14)
> To those who know me and my situation, it's been a pleasure absolutely. 
> I'll probably still do what I can like this.  For some reason finding these
> issues right as they're introduced recently has made me feel great in
> regards to things overall.
This is just awesome!

Honza

Comment 17

[Kershaw Chang [:kershaw]]

Attachment: Add isTopLevelLoad attribute in nsILoadInfo, r=billm

Carry r+ and rebase.

Comment 18

[Kershaw Chang [:kershaw]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/64f4ea57b6fd

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/64f4ea57b6fd

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 22

[Kershaw Chang [:kershaw]]

Comment on attachment 8783316 [details] [diff] [review]
Add isTopLevelLoad attribute in nsILoadInfo, r=billm

Approval Request Comment
[Feature/regressing bug #]: bug 1256595
[User impact if declined]: The JSON viewer will be loaded inside an iframe.
[Describe test coverage new/current, TreeHerder]: Manually tested.
[Risks and why]: I think it's low risk. It seems that only JSON viewer is affected.
[String/UUID change made/needed]: None.

Comment 23

[Andrew McCreight [:mccr8]]

Security bugs should get ratings before landing. (Though it isn't a huge deal here because this is only on Aurora and Nightly.) Gijs, what are the security implications here? Do you have a suggested rating?

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Andrew McCreight [:mccr8] from comment #23)
> Gijs, what are the
> security implications here? Do you have a suggested rating?

Not really... it's hard to know to what degree structured clone and the wrappers save our butt here. resource:// really isn't particularly safe and we shouldn't let things get a handle to those kind of windows. Like, as-is the demonstrable impact is sec-low, but that's just because the testcases haven't bothered to try to exploit this to the extent that you might be able to. There's basically 3 vectors here:

- the resource:// window ref. You shouldn't be able to get that ref and the methods in the window should not be content-accessible. It's not clear to me why they are in this case. If you use window.open() instead the window ref gets nulled out. Note that resource:// URIs generally are... not safe to have content access. In theory they're not quite as unsafe as chrome://, but I believe that practically they kind of are right now.
- the JSON view API stuff which comes with this particular window ref. AFAICT they just allow you to copy text to the clipboard and provide an arbitrary string of data which the user then gets shown a "save file as" viewer for, both of which are capabilities the web generally has at this point. But I've not tried to see if this is exploitable directly in the way that things were before security wrappers (the "bad old days", let's say).
- the chrome exceptions you get if you send the json view API bad data (attachment 8781716 [details]). I haven't really poked at those and I don't know if they are evil and/or how evil they are.

So I would expect that it would end up at least sec-moderate, maybe higher. Bobby would be a better assessor than me.

Comment 25

[Bobby Holley (:bholley)]

Is the principal of the window with postChromeMessage actually resource://? If so, that would imply XSS, which would be a big deal and would need to be addressed. If the devtools pane is being loaded with content principal and _then_ exporting postChromeMessage to it, that also seems problematic.

In general, it seems like there is at least one additional issue that needs fixing beyond the patch in this bug.

Comment 26

[:Gijs (out for now; he/him)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> If the devtools pane is being loaded with content principal and
> _then_ exporting postChromeMessage to it, that also seems problematic.

This.

> In general, it seems like there is at least one additional issue that needs
> fixing beyond the patch in this bug.

Fair. Honza, can you file a followup for not exporting things and just adding listeners etc. from chrome instead?

Comment 27

[Bobby Holley (:bholley)]

Are we intentionally loading page UI with the same principal as the content? That seems suboptimal. I'm all in favor of not giving them a privileged principal, but making them same-origin with the page gives up a very useful layer of defense-in-depth.

Comment 28

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to :Gijs Kruitbosch from comment #26)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> > If the devtools pane is being loaded with content principal and
> > _then_ exporting postChromeMessage to it, that also seems problematic.
> 
> This.
> 
> > In general, it seems like there is at least one additional issue that needs
> > fixing beyond the patch in this bug.
> 
> Fair. Honza, can you file a followup for not exporting things and just
> adding listeners etc. from chrome instead?
bug 1297361

Gijs, why using Cu.exportFunction is problematic in this case?

Honza

Comment 29

[:Gijs (out for now; he/him)]

(In reply to Jan Honza Odvarko [:Honza] from comment #28)
> (In reply to :Gijs Kruitbosch from comment #26)
> > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> > > If the devtools pane is being loaded with content principal and
> > > _then_ exporting postChromeMessage to it, that also seems problematic.
> > 
> > This.
> > 
> > > In general, it seems like there is at least one additional issue that needs
> > > fixing beyond the patch in this bug.
> > 
> > Fair. Honza, can you file a followup for not exporting things and just
> > adding listeners etc. from chrome instead?
> bug 1297361
> 
> Gijs, why using Cu.exportFunction is problematic in this case?
> 
> Honza

This is a question for Bobby. In any case, it already seems like I misunderstood his point earlier, per comment 27, which suggests the problem is the principal we (re)use for the json viewer, rather than exactly how we insert APIs into that page.

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #27)
> Are we intentionally loading page UI with the same principal as the content?
> That seems suboptimal. I'm all in favor of not giving them a privileged
> principal, but making them same-origin with the page gives up a very useful
> layer of defense-in-depth.

I don't know how hard it is to change the principal we use for the document the jsonview converter generates (or what the principal should be - unique null principal? Something else?). Bobby, what specifically do you think needs to be done here?

Comment 30

[Bobby Holley (:bholley)]

(In reply to :Gijs Kruitbosch from comment #29)
> (In reply to Jan Honza Odvarko [:Honza] from comment #28)
> > (In reply to :Gijs Kruitbosch from comment #26)
> > > (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #25)
> > > > If the devtools pane is being loaded with content principal and
> > > > _then_ exporting postChromeMessage to it, that also seems problematic.
> > > 
> > > This.
> > > 
> > > > In general, it seems like there is at least one additional issue that needs
> > > > fixing beyond the patch in this bug.
> > > 
> > > Fair. Honza, can you file a followup for not exporting things and just
> > > adding listeners etc. from chrome instead?
> > bug 1297361
> > 
> > Gijs, why using Cu.exportFunction is problematic in this case?
> > 
> > Honza
> 
> This is a question for Bobby. In any case, it already seems like I
> misunderstood his point earlier, per comment 27, which suggests the problem
> is the principal we (re)use for the json viewer, rather than exactly how we
> insert APIs into that page.
> 
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #27)
> > Are we intentionally loading page UI with the same principal as the content?
> > That seems suboptimal. I'm all in favor of not giving them a privileged
> > principal, but making them same-origin with the page gives up a very useful
> > layer of defense-in-depth.
> 
> I don't know how hard it is to change the principal we use for the document
> the jsonview converter generates (or what the principal should be - unique
> null principal? Something else?). Bobby, what specifically do you think
> needs to be done here?

My suggestion is that we use a unique null principal for the JSON viewer, unless it specifically needs access to objects in the page (in which case we should give it an nsExpandedPrincipal).

Comment 31

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #30)
> My suggestion is that we use a unique null principal for the JSON viewer,
> unless it specifically needs access to objects in the page (in which case we
> should give it an nsExpandedPrincipal).
How can I specify null principal for the JSON Viewer window?

The Converter component (converting the original JSON HTTP response into HTML page -> JSON Viewer) generates the HTML within `onStopRequest` [1] and exports `postChromeMessage` when `DOMContentLoad` event is fired [2].

Do we have any API to set null principal to an existing window? 
Or when exactly it should be set?

Honza

[1] https://dxr.mozilla.org/mozilla-central/rev/052656fc513c05da969590ac5934abd67271a897/devtools/client/jsonview/converter-child.js#168

[2] https://dxr.mozilla.org/mozilla-central/rev/052656fc513c05da969590ac5934abd67271a897/devtools/client/jsonview/converter-child.js#142

Comment 32

[Bobby Holley (:bholley)]

(In reply to Jan Honza Odvarko [:Honza] from comment #31)
> Do we have any API to set null principal to an existing window? 
> Or when exactly it should be set?

It should be set when creating the window. How is the window being created, exactly? If it's an iframe, you can just mark the iframe as <iframe sandbox> and it will Just Work.

Comment 33

[Jerri Rice (rehash pending)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #32)
> (In reply to Jan Honza Odvarko [:Honza] from comment #31)
> > Do we have any API to set null principal to an existing window? 
> > Or when exactly it should be set?
> 
> It should be set when creating the window. How is the window being created,
> exactly? If it's an iframe, you can just mark the iframe as <iframe sandbox>
> and it will Just Work.

The top level window case should be remembered.  It only takes a click to open a top level window.

Comment 34

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

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> https://hg.mozilla.org/mozilla-central/rev/64f4ea57b6fd

Why did this land without sec-approval or a security rating?

Comment 35

[Ryan VanderMeulen [:RyanVM]]

It was an oversight, sorry.

Comment 36

[:Gijs (out for now; he/him)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #32)
> (In reply to Jan Honza Odvarko [:Honza] from comment #31)
> > Do we have any API to set null principal to an existing window? 
> > Or when exactly it should be set?
> 
> It should be set when creating the window. How is the window being created,
> exactly? If it's an iframe, you can just mark the iframe as <iframe sandbox>
> and it will Just Work.

AIUI the idea is that we show this viewer when a request with the JSON mimetype is received at the toplevel. So the viewer doesn't control the docshell or frame, if that's what you mean. So the tool is *meant* to be used in the "normal" browsing area. Would it / the converter still be able to change its own principal as you suggest, and if so how?

Comment 37

[Bobby Holley (:bholley)]

Ok, so this stuff?

http://searchfox.org/mozilla-central/rev/44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/devtools/client/jsonview/converter-child.js#59

Some ideas:
* Add a CSP header to the request to give the sandbox directive.
* Modify the request somehow so that the channel owner is a new null principal. I don't really know the specifics of how to do this, but Boris could probably tell you.
* Hoist all the interesting content inside a toplevel iframe sandbox.

Comment 38

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #37)
> Ok, so this stuff?
> 
> http://searchfox.org/mozilla-central/rev/
> 44f6964ba95b8ddd8ebf70c55b34cd2323afeef4/devtools/client/jsonview/converter-
> child.js#59
> 
> Some ideas:
> * Add a CSP header to the request to give the sandbox directive.
> * Modify the request somehow so that the channel owner is a new null
> principal. I don't really know the specifics of how to do this, but Boris
> could probably tell you.
@Boris, any pointers?

Honza

> * Hoist all the interesting content inside a toplevel iframe sandbox.

Comment 39

[Boris Zbarsky [:bzbarsky]]

At the moment you can set the .owner of the channel and that will affect what nsScriptSecurityManager::GetChannelResultPrincipal does.  But that may not keep working going into the future as we move more stuff to nsILoadInfo; please talk to Tanvi and Christoph about what _is_ guaranteed to work going forward, if anything.  We may need to add a loadinfo-based API of some sort for this.

Of course you can also synthesize your own channel object with whatever security properties you want and pass _that_ along, but that's even less forward-compatible.

Comment 40

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Boris Zbarsky [:bz] from comment #39)
> At the moment you can set the .owner of the channel and that will affect
> what nsScriptSecurityManager::GetChannelResultPrincipal does.  But that may
> not keep working going into the future as we move more stuff to nsILoadInfo;
> please talk to Tanvi and Christoph about what _is_ guaranteed to work going
> forward, if anything.  We may need to add a loadinfo-based API of some sort
> for this.
Tanvi, Christoph any tips how to move forward with this bug?
Honza

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jan Honza Odvarko [:Honza] from comment #40)
> (In reply to Boris Zbarsky [:bz] from comment #39)
> > At the moment you can set the .owner of the channel and that will affect
> > what nsScriptSecurityManager::GetChannelResultPrincipal does.  But that may
> > not keep working going into the future as we move more stuff to nsILoadInfo;
> > please talk to Tanvi and Christoph about what _is_ guaranteed to work going
> > forward, if anything.  We may need to add a loadinfo-based API of some sort
> > for this.
> Tanvi, Christoph any tips how to move forward with this bug?
> Honza

Honza, all of our nsIChannel creations should by now go through NewChannelFromURIWithProxyFlagsInternal which sets the owner to a nullptr if sandboxed [1], which in turn causes GetChannelResultPrincipal() to rely on the loadInfo set on the channel [2].

Once we have converted docshell to rely on asyncOpen2() [Bug 1182569] we have plans to completely deprecate channel.owner [Bug 1286838].

[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIOService.cpp#810
[2] https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#343

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

:kershaw, just had a quick look at the patch, wouldn't it have been sufficient to just check the contentPolicyType of that load in the loadInfo?

if (loadinfo.contentPolicyType == TYPE_DOCUMENT) { // we know it's a toplevel load }

Not a big deal in the end, just curious!

Comment 43

[Boris Zbarsky [:bzbarsky]]

> Honza, all of our nsIChannel creations

That doesn't answer Honza's question.  His question is: given a channel that docshell created sometime in the past, and given that he's implementing a stream converter that modifies what data that channel is returning, how can he affect the outcome of getChannelResultPrincipal for the channel to properly reflect the data being returned, since it's no longer returning the original data from the server?

Comment 44

[Kershaw Chang [:kershaw]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #42)
> :kershaw, just had a quick look at the patch, wouldn't it have been
> sufficient to just check the contentPolicyType of that load in the loadInfo?
> 
> if (loadinfo.contentPolicyType == TYPE_DOCUMENT) { // we know it's a
> toplevel load }
> 
> Not a big deal in the end, just curious!

Sorry, I have no idea about what contentPolicyType is. I am also not sure can we use it to determine whether it is a top level load.

BTW, it seems that there is only externalContentPolicyType in nsILoadInfo is available to script. Is externalContentPolicyType equal to the contentPolicyType you mentioned above?

Comment 45

[Jerri Rice (rehash pending)]

(In reply to Kershaw Chang [:kershaw] from comment #44)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #42)
> > :kershaw, just had a quick look at the patch, wouldn't it have been
> > sufficient to just check the contentPolicyType of that load in the loadInfo?
> > 
> > if (loadinfo.contentPolicyType == TYPE_DOCUMENT) { // we know it's a
> > toplevel load }
> > 
> > Not a big deal in the end, just curious!
> 
> Sorry, I have no idea about what contentPolicyType is. I am also not sure
> can we use it to determine whether it is a top level load.
> 
> BTW, it seems that there is only externalContentPolicyType in nsILoadInfo is
> available to script. Is externalContentPolicyType equal to the
> contentPolicyType you mentioned above?

I believe the idea was to confirm its a top level load then use CSP to have the top level load be treated like an <iframe sandbox> load which would give you unique null principal IIRC.  I could be wrong though.

Comment 46

[:Gijs (out for now; he/him)]

(In reply to Boris Zbarsky [:bz] from comment #43)
> > Honza, all of our nsIChannel creations
> 
> That doesn't answer Honza's question.  His question is: given a channel that
> docshell created sometime in the past, and given that he's implementing a
> stream converter that modifies what data that channel is returning, how can
> he affect the outcome of getChannelResultPrincipal for the channel to
> properly reflect the data being returned, since it's no longer returning the
> original data from the server?

--> ni ckerschb for this. I think we need a better defense in depth story here.

Comment 47

[Jerri Rice (rehash pending)]

Ok, I thought I might take a stab at this and from what I've been looking through I was wondering if I should be looking at the class definition for nsILoadInfo in nsIProxiedProtocolHandler.h(my first choice) or nsIProtocolHandler.h

For some reason I believe bz will probably have the answer. :-)

Anyone who feels I'm stepping on toes or has work in progress just let me know and I'll back off.

Comment 48

[Boris Zbarsky [:bzbarsky]]

Neither.  You should be looking in nsLoadInfo.h and nsILoadInfo.idl.

Comment 49

[Jerri Rice (rehash pending)]

Already had both loaded with the others, just followed my instincts based on pieces from here.  Thanks, I'll look this over pretty in depth and see if I can come up with anything.

Comment 50

[Jerri Rice (rehash pending)]

For anyone else following along, it's LoadInfo.h with the same WebIDL file.  Thanks again, I think it's going to be interesting seeing if I can get the source to build with this crazy chroot secondary distro setup I have on this machine.

  ahh headaches.

Comment 51

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #46)
> > That doesn't answer Honza's question.  His question is: given a channel that
> > docshell created sometime in the past, and given that he's implementing a
> > stream converter that modifies what data that channel is returning, how can
> > he affect the outcome of getChannelResultPrincipal for the channel to
> > properly reflect the data being returned, since it's no longer returning the
> > original data from the server?

Well, with our current setup that is basically not possible, because the LoadingPrincipal as well as the TriggeringPrincipal get frozen at creation time and can't be modified thereafter. I don't even think that we want to modify those two principals. Potentially we could add another Principal to nsILoadInfo, which is settable (please note that loadingPrincipal and triggeringPrincipal are readonly). Further we would have to add another flag (e.g. SEC_USE_MODIFIED_RESULT_PRINCIPAL) to the loadInfo which indicates that the data in the channel was modified (e.g. by the streamconverter). Within GetChannelResultPrincipal we would then have to check for that flag and return the new principal. What scares me though is that that mechanism can be abused from other parts in the code. But I suppose that is the only way forward to accomplish such a mechanism.

Comment 52

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kershaw Chang [:kershaw] from comment #44)
> BTW, it seems that there is only externalContentPolicyType in nsILoadInfo is
> available to script. Is externalContentPolicyType equal to the
> contentPolicyType you mentioned above?

Yes, that is what you would want to use.

Comment 53

[:Gijs (out for now; he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #51)
> Potentially we could add another Principal to
> nsILoadInfo, which is settable (please note that loadingPrincipal and
> triggeringPrincipal are readonly). Further we would have to add another flag
> (e.g. SEC_USE_MODIFIED_RESULT_PRINCIPAL) to the loadInfo which indicates
> that the data in the channel was modified (e.g. by the streamconverter).
> Within GetChannelResultPrincipal we would then have to check for that flag
> and return the new principal. What scares me though is that that mechanism
> can be abused from other parts in the code. But I suppose that is the only
> way forward to accomplish such a mechanism.

Would it be simpler if we restricted the new principal to being a new null principal (could just have a single setter that force-generates a new null principal) ?

Comment 54

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs Kruitbosch from comment #53)
> Would it be simpler if we restricted the new principal to being a new null
> principal (could just have a single setter that force-generates a new null
> principal) ?

Yes, I suppose that would work. In fact I would prefer that, assuming this new principal always needs to be a NullPrincipal.

Comment 55

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Kershaw, based on the discussion in this email it's unclear to me whether this fix is ready to be uplifted to Aurora as is or is a secondary fix coming along? Please let me know.

Comment 56

[Kershaw Chang [:kershaw]]

(In reply to Ritu Kothari (:ritu) from comment #55)
> Hi Kershaw, based on the discussion in this email it's unclear to me whether
> this fix is ready to be uplifted to Aurora as is or is a secondary fix
> coming along? Please let me know.

I also have no idea about how to proceed.
Maybe Gijs could give us some advice?

Comment 57

[:Gijs (out for now; he/him)]

(In reply to Kershaw Chang [:kershaw] from comment #56)
> (In reply to Ritu Kothari (:ritu) from comment #55)
> > Hi Kershaw, based on the discussion in this email it's unclear to me whether
> > this fix is ready to be uplifted to Aurora as is or is a secondary fix
> > coming along? Please let me know.
> 
> I also have no idea about how to proceed.
> Maybe Gijs could give us some advice?

This should be uplifted. Followup fixes can happen in other bugs. The patch as-is fixes Real Issues.

Comment 58

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8783316 [details] [diff] [review]
Add isTopLevelLoad attribute in nsILoadInfo, r=billm

Fixes a recent regression, Aurora50+

Comment 59

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/249bd8812e3a

Comment 60

[Tanvi Vyas[:tanvi]]

Clearing my needinfo since it looks like Christoph has answered your questions.  Please file a followup bug.

Comment 61

[Kershaw Chang [:kershaw]]

(In reply to Tanvi Vyas [:tanvi] from comment #60)
> Clearing my needinfo since it looks like Christoph has answered your
> questions.  Please file a followup bug.

It seems that the followup bug has been filed, which is bug 1297361.

Comment 62

[:Gijs (out for now; he/him)]

(In reply to Kershaw Chang [:kershaw] from comment #61)
> (In reply to Tanvi Vyas [:tanvi] from comment #60)
> > Clearing my needinfo since it looks like Christoph has answered your
> > questions.  Please file a followup bug.
> 
> It seems that the followup bug has been filed, which is bug 1297361.

No, we need a separate followup bug to downgrade the channel principal to a separate null principal, per comment 37, comment 43, comment 51, comment 54.

Comment 63

[Kershaw Chang [:kershaw]]

(In reply to :Gijs Kruitbosch from comment #62)
> (In reply to Kershaw Chang [:kershaw] from comment #61)
> > (In reply to Tanvi Vyas [:tanvi] from comment #60)
> > > Clearing my needinfo since it looks like Christoph has answered your
> > > questions.  Please file a followup bug.
> > 
> > It seems that the followup bug has been filed, which is bug 1297361.
> 
> No, we need a separate followup bug to downgrade the channel principal to a
> separate null principal, per comment 37, comment 43, comment 51, comment 54.

bug 1305012