1198559 - Wrong requestingNode passed to NS_NewChannelInternal (Tor 16855)

Status: RESOLVED DUPLICATE of bug 1113196

(Core :: DOM: Security, defect)

Description

[Arthur Edelstein [:arthur]]

In bug 1038756, comment 176 it says "I think using GetFrameElementInternal in all cases here is wrong.  That will be the <xul:browser> for a toplevel tab, no?  Do we not have tests for that?"

The last attached version of that patch, attachment 8486695 [details] [diff] [review], has the use of GetFrameElementInternal removed as requested. But in the committed version, GetFrameElementInternal is back: https://hg.mozilla.org/mozilla-central/rev/7fc6512d34b1

I ran into the problem described above in Tor Browser, which attempts to use the loading document to obtain the first party domain name. This attempt fails because it gets browser.xul from the channel's LoadInfo instead of the content document.

For reference, here is the Tor Browser ticket: https://trac.torproject.org/projects/tor/ticket/16855

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: wip.patch

(In reply to Arthur Edelstein from comment #0)
> In bug 1038756, comment 176 it says "I think using GetFrameElementInternal
> in all cases here is wrong.  That will be the <xul:browser> for a toplevel
> tab, no?  Do we not have tests for that?"
> 
> The last attached version of that patch, attachment 8486695 [details] [diff] [review]
> [review], has the use of GetFrameElementInternal removed as requested. But
> in the committed version, GetFrameElementInternal is back:
> https://hg.mozilla.org/mozilla-central/rev/7fc6512d34b1

Thanks Arthur for pointing that out. At the moment, I can't really find out why that change didn't make into the codebase. Fact is that loadingPrincipal and loadingNode for docshell created channels can not be completely trusted until we have landed Bug 1066868. This bug has been on the backlog for quite some time now, but now Tanvi is working on that. [CC'ed Tanvi on the bug]. I will mark this bug as blocking bug 1066868.

I attached a WIP patch of the version that didn't make it into the codebase for easier comparison.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Tanvi, please also consider the WIP patch here when working on Bug 1066868.

Comment 3

[Tanvi Vyas[:tanvi]]

Thank you Arthur for catching this!  I will look into it along with bug 1105556 and 1066868.

Comment 4

[Tanvi Vyas[:tanvi]]

Keeping my needinfo for now.

Comment 5

[Georg Koppen]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #1)
> I attached a WIP patch of the version that didn't make it into the codebase
> for easier comparison.

FWIW this patch does not compile when applied to an ESR 38 branch:

/home/ubuntu/build/tor-browser/docshell/base/nsDocShell.cpp: In member function 'nsresult nsDocShell::DoURILoad(nsIURI*, nsIURI*, bool, uint32_t, nsISupports*, const char*, const nsAString_internal&, nsIInputStream*, nsIInputStream*, bool, nsIDocShell**, nsIRequest**, bool, bool, bool, const nsAString_internal&, nsIURI*, nsContentPolicyType)':

/home/ubuntu/build/tor-browser/docshell/base/nsDocShell.cpp:10543:73: error: 'createPrincipalFromReferrer' was not declared in this scope
                                      getter_AddRefs(requestingPrincipal));

I was trying to find out whether your idea is really fixing our issue...

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8652684 [details] [diff] [review]
wip.patch

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

::: docshell/base/nsDocShell.cpp
@@ +10368,5 @@
> +  nsCOMPtr<nsINode> requestingNode = mScriptGlobal->GetExtantDoc();
> +  nsCOMPtr<nsIPrincipal> requestingPrincipal = do_QueryInterface(aOwner);
> +  if (!requestingPrincipal && aReferrerURI) {
> +    rv = createPrincipalFromReferrer(aReferrerURI,
> +                                     getter_AddRefs(requestingPrincipal));

Yeah sorry, it's a WIP patch, if you wanna try it out you would have to use
CreatePrincipalFromReferrer(...) [Upper Case]

Comment 7

[Georg Koppen]

That worked and fixed the problem, thanks.

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Georg Koppen from comment #7)
> That worked and fixed the problem, thanks.

Just to make sure, it fixed the compile problem or it fixed the problem that Arthur was describing in Comment 0? If it fixed Arthur's problem, then we could maybe convert this WIP patch into a real one and land even before Tanvi has fixed Bug 1066868, because I suspect that to be more difficult and it might take a while till we get it landed.

Comment 9

[Georg Koppen]

It fixed both and, yes, having this landed earlier would be really good. Thanks!

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1198559_wrong_loadingNode_in_docshell.patch

Smaug or Boris, as Arthur pointed out in Comment 0 we are setting the wrong loadingNode within docshell. Boris already commented on that in the inital review [1] and I honestly don't know why it didn't made it into the codebase. Probably I lost track within Bug 1038756, which has something like 30+ patches attached. Anyhow, no excuses, my bad. So far this went unnoticed because the loadingNode in loadInfo for top level loads is pretty much unused, and we also filed Bug 1066868 and Bug 1105556 to make sure we use the right loadingPrincipal, triggeringPrincipal and LoadingNode in docshell before we move securityChecks into asyncOpen2() for docshell loads. Tanvi is on it, but it might still take a while till Bug 1066868 and Bug 1105556 are done. Probably it makes sense to accept this patch right now since it apparently helps Tor folks to move forward.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1038756#c176

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8653582 [details] [diff] [review]
bug_1198559_wrong_loadingNode_in_docshell.patch

Given that comment 0 refers to bz' comment, perhaps he could take a look at this.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8653582 [details] [diff] [review]
bug_1198559_wrong_loadingNode_in_docshell.patch

What are the intended semantics of requestingNode in this case?

The use described in comment 0 doesn't make sense to me if the extant doc is used: if the user types a new URL in the URL bar, it will treat the old document as "requesting node", but I have a hard time believing that's what Tor wants here.

Past that, the important part of bug 1038756 comment 176 that's not reflected in this patch is this:

  The right thing to do is to use GetFrameElementInternal() only when aContentPolicyType is
  subdocument, I would think.

That would use the <iframe> as the requesting node for loads in an <iframe> but would treat toplevel document loads as having the previous document as the requesting node...  That's assuming we can't have a null requesting node, of course.  If we can, then a load from the URL bar should presumably do that, right?

In all cases, why is the mScriptGlobal null-check no longer needed?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8653582 [details] [diff] [review]
> bug_1198559_wrong_loadingNode_in_docshell.patch
> 
> What are the intended semantics of requestingNode in this case?

We haven't really settled on the toplevel load semantics as of now. Probably we will have to reevaluate once we have more insights into Bug 1066868 and Bug 1105556 where we will find the right loadingPrincipal, triggeringPrincipal and loadingNode for docshell loads.

> Past that, the important part of bug 1038756 comment 176 that's not
> reflected in this patch is this:
> 
>   The right thing to do is to use GetFrameElementInternal() only when
> aContentPolicyType is
>   subdocument, I would think.
> 
> That would use the <iframe> as the requesting node for loads in an <iframe>
> but would treat toplevel document loads as having the previous document as
> the requesting node...  That's assuming we can't have a null requesting
> node, of course.  If we can, then a load from the URL bar should presumably
> do that, right?

That sounds reasonable; however I think I'll leave this bug untouched until Tanvi brings more insights about all the different load cases we need to inspect. Unless we want the change to use GetFrameElementInternal() for subdocument loads updated right away!

> In all cases, why is the mScriptGlobal null-check no longer needed?

That is still needed indeed.

Comment 14

[Arthur Edelstein [:arthur]]

(In reply to Boris Zbarsky [:bz] from comment #12)
> Comment on attachment 8653582 [details] [diff] [review]
> bug_1198559_wrong_loadingNode_in_docshell.patch
> 
> What are the intended semantics of requestingNode in this case?
> 
> The use described in comment 0 doesn't make sense to me if the extant doc is
> used: if the user types a new URL in the URL bar, it will treat the old
> document as "requesting node", but I have a hard time believing that's what
> Tor wants here.

Thanks for pointing this out.

Here's a demo of the original problem that prompted this ticket:
https://arthuredelstein.github.io/tordemos/download-blob.html

A Blob URL is created and we use its URL to prepare an element like
<a href="blob:..." download>Download<a>
which, when clicked, results in the downloading of a file containing the blob's contents.

When the user clicks the link, an nsIChannel for the blob URI is created in nsDocShell::DoURILoad, and Tor Browser will subsequently want to know the first party loading document for that channel. In this blob-downloading case, I think the first party document should be the top-level document for the link that was clicked. (Tor Browser uses this information to determine if the code that is attempting to accessing the blob URI has the same first party domain as the script that created the blob URI.)

So would it make sense to use something like the following?

+ if (mScriptGlobal && !aFileName.IsVoid()) {
+   nsCOMPtr<nsINode> requestingNode = mScriptGlobal->GetExtantDoc();
+ }

Here (as far as I can tell) !aFileName.IsVoid() indicates that a file is to be downloaded.

Or is there a more general case in nsDocShell::DoURILoad() where the extant doc is appropriate?

Comment 15

[Tanvi Vyas[:tanvi]]

The plan for docshell loads is outlined here:
https://bugzilla.mozilla.org/show_bug.cgi?id=1105556#c12

The loadingNode will always be null for top level loads in the future.  For frames, we should get a loadingNode of the frame's parent.

I'm not sure about the blob test case above.  The channel that loads https://arthuredelstein.github.io/tordemos/download-blob.html will load it with the null principal (in the future).  When I click the link to download the blob url, I get a download prompt asking me if I want to save or download.  Is the download considered a top level load or a subresource?  If it is a subresource, it shoudl get a loadingNode for the github page.  But since it is an a href, which implies navigation, it may be considered a top level load and hence get a null principal (in the future).  Boris should know more.

I would avoid making changes to nsDocShell here and instead wait for 1105556, which is in progress.  There is a lot of testing to be done for that bug even before we change the code, and I'm doing that now.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> In this blob-downloading case, I think the first party document should be the top-level
> document for the link that was clicked.

That doesn't seem unreasonable.  Note that in the case of <a target> this may have nothing whatsoever to do with the docshell where the load is happening, right?

> Here (as far as I can tell) !aFileName.IsVoid() indicates that a file is to be downloaded.

All it means is that the <a> element had an attribute named "download".  There are cases in which the file will be downloaded when aFileName.IsVoid() tests true (e.g. when the server responds with a MIME type we decide to download).

> Is the download considered a top level load or a subresource? 

Mostly doesn't matter, since you don't know whether it's a download or not until after you've gotten the server response, the @download case aside.

I really do think we should differentiate page-triggered navigations (which should, imo, be blamed on the page that triggered them) and UI-triggered navigations (which should not be blamed on any web page at all);.

Comment 17

[Tanvi Vyas[:tanvi]]

Hi Arthur,

This is what happens in nsDocShell currently when you load https://arthuredelstein.github.io/tordemos/download-blob.html and click on the link:
https://pastebin.mozilla.org/8852656

The click on the blob link is treated as a new top level navigation, with TYPE_DOCUMENT.  If you are looking for the page that caused the "navigation" you should look for the triggeringPrincipal or the referrer.  loadingPrincipal will not be https://arthuredelstein.github.io/tordemos/download-blob.html, since the blob download is not considered a subresource in this case.  If it were embedded in a way where it is considered a subresource, then you may want to look at the loadingPrincipal.

Comment 18

[Tanvi Vyas[:tanvi]]

We no longer use GetExtantDoc in nsDocShell.  This bug can be fixed duplicate to either bug 1113196 or 1105556.  Using 1113196 because that was the patch that removed GetExtantDoc.  Arthur, please let us know if there is still another bug here.  Thanks!