Closed Bug 1295309 Opened 3 years ago Closed 3 years ago

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

Categories

(DevTools :: JSON Viewer, defect)

51 Branch
defect
Not set

Tracking

(firefox48 unaffected, firefox49 unaffected, firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox48 --- unaffected
firefox49 --- unaffected
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: jerri.rice.001, Assigned: kershaw)

References

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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.
Version: 48 Branch → 51 Branch
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.)
Flags: needinfo?(odvarko)
Component: Untriaged → Developer Tools: JSON Viewer
I do remember seeing code in place that attempts to limit the loading of this to a top level window.  It's broken obviously.
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.
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.
Attached file 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.
Oh yeah forgot, open web console as before.  Man this is tedious from a chromebook ;-)
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? :-)
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
(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? :-)
Flags: needinfo?(wmccloskey)
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.
Flags: needinfo?(wmccloskey) → needinfo?(kechang)
Sorry for causing this side effect.

Could you take a look at this patch? Thanks.
Assignee: nobody → kechang
Flags: needinfo?(kechang)
Attachment #8782783 - Flags: review?(wmccloskey)
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...
I can verify that the patch solves the issue. JSON viewer isn't loaded
inside an iframe anymore.

Honza
Flags: needinfo?(odvarko)
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 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."
Attachment #8782783 - Flags: review?(wmccloskey) → review+
(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
Carry r+ and rebase.
Attachment #8782783 - Attachment is obsolete: true
Attachment #8783316 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/64f4ea57b6fd
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Please request Aurora approval on this when you get a chance.
Flags: needinfo?(kechang)
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.
Flags: needinfo?(kechang)
Attachment #8783316 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
(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?
Flags: needinfo?(odvarko)
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.
(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
Flags: needinfo?(odvarko) → needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(bobbyholley)
(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).
Flags: needinfo?(bobbyholley)
(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
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(bobbyholley)
(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.
(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?
Flags: needinfo?(ryanvm)
It was an oversight, sorry.
Flags: needinfo?(ryanvm)
(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?
Flags: needinfo?(bobbyholley)
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.
Flags: needinfo?(bobbyholley)
(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.
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
(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
Flags: needinfo?(tanvi)
Flags: needinfo?(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
Flags: needinfo?(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!
Flags: needinfo?(kechang)
> 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?
(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?
Flags: needinfo?(kechang)
(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.
(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.
Flags: needinfo?(ckerschb)
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.
Flags: needinfo?(bzbarsky)
Neither.  You should be looking in nsLoadInfo.h and nsILoadInfo.idl.
Flags: needinfo?(bzbarsky)
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.
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.
(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.
Flags: needinfo?(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.
(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) ?
Flags: needinfo?(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.
Flags: needinfo?(ckerschb)
Group: firefox-core-security → core-security-release
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.
Flags: needinfo?(kechang)
(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?
Flags: needinfo?(kechang) → needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8783316 [details] [diff] [review]
Add isTopLevelLoad attribute in nsILoadInfo, r=billm

Fixes a recent regression, Aurora50+
Attachment #8783316 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Clearing my needinfo since it looks like Christoph has answered your questions.  Please file a followup bug.
Flags: needinfo?(tanvi) → needinfo?(kechang)
(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.
Flags: needinfo?(kechang)
(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.
Flags: needinfo?(kechang)
(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
Flags: needinfo?(kechang)
Group: core-security-release
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.