Extend loadURI within nsIWebNavigation.idl by a triggeringPrincipal argument

RESOLVED FIXED in Firefox 55

Status

()

Core
DOM: Security
P2
normal
RESOLVED FIXED
8 months ago
8 months ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Depends on: 2 bugs, Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 attachment, 2 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

8 months ago
I think we need to make our way (small patches) till we can finally assert that loadURIWithOptions recceives a non null triggeringPrincipal within Bug 1333030.
Assignee: nobody → ckerschb
Blocks: 1333030
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
(Assignee)

Comment 2

8 months ago
Created attachment 8861142 [details] [diff] [review]
bug_1359092_extend_loaduri_with_triggeringprincipal.patch

Boris, ultimately we would like to add an assertion that LoadURI[WithOptions]() always receives a valid triggeringPrincipal which initiated the load (See Bug 1333030).

This patch adds the groundwork by extending loadURI with an optional argument triggeringPrincipal. Since all those principal decisions are critical I would like to land a bunch of smaller sized patches, each blocking Bug 1333030 so we can focus on making sure we pass the right principal for each callsite.

Please note that this patch really only contains C++ callsites of loadURI. Acceptable?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97cc9722ece94fb5c0186e3261be1257954615c2
Attachment #8861142 - Flags: review?(bzbarsky)
Comment on attachment 8861142 [details] [diff] [review]
bug_1359092_extend_loaduri_with_triggeringprincipal.patch

>@@ -444,17 +445,18 @@ nsDSURIContentListener::CheckFrameOption

This should probably use a nullprincipal or something instead of system.  Otherwise we're relying on our various complicated "oh, but don't inherit from triggering principal if it's system, etc" logic.  There's no need for that here.

Followup OK for this part.

>@@ -999,17 +1000,18 @@ nsDocShellTreeOwner::HandleEvent(nsIDOME

This one seems pretty fishy to me too...  I guess this is just preserving behavior for now?

>@@ -7875,17 +7875,18 @@ nsGlobalWindow::HomeOuter(ErrorResult& a
>+                           GetPrincipal());

It would be better to change the IDL here to pass in the caller principal and use that as the triggering principal.

r=me with the HomeOuter thing fixed, and followups filed as needed to investigate some of the other callsites.
Attachment #8861142 - Flags: review?(bzbarsky) → review+
(Assignee)

Updated

8 months ago
Depends on: 1359191
(Assignee)

Comment 4

8 months ago
Created attachment 8861156 [details] [diff] [review]
bug_1359092_extend_loaduri_with_triggeringprincipal.patch

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3)
> >@@ -444,17 +445,18 @@ nsDSURIContentListener::CheckFrameOption
> 
> This should probably use a nullprincipal

Updated to nullPrincipal.

> 
> >@@ -999,17 +1000,18 @@ nsDocShellTreeOwner::HandleEvent(nsIDOME
> 
> This one seems pretty fishy to me too...  I guess this is just preserving
> behavior for now?

Yes - I filed Bug 1359191 to investigate.

> >@@ -7875,17 +7875,18 @@ nsGlobalWindow::HomeOuter(ErrorResult& a
> >+                           GetPrincipal());
> 
> It would be better to change the IDL here to pass in the caller principal
> and use that as the triggering principal.

done.

> r=me with the HomeOuter thing fixed
Carrying over r+ from bz.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4897698e169d06eb0bc14fce2be78d13d0929776
Attachment #8861142 - Attachment is obsolete: true
Attachment #8861156 - Flags: review+

Comment 5

8 months ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e42f84996ea
Extend loadURI within nsIWebNavigation.idl by a triggeringPrincipal argument. r=bz

Comment 6

8 months ago
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f609798e5007
Backed out changeset 3e42f84996ea for causing assertion failures
Hi Christoph, seems this push cause assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=93949697&repo=mozilla-inbound - also it seems the try run from comment #4 confirms this since there are 3 failures with this assertion failure. Could you take a look, thanks!
Flags: needinfo?(ckerschb)
(Assignee)

Updated

8 months ago
Depends on: 1359358
(Assignee)

Comment 8

8 months ago
Created attachment 8861349 [details] [diff] [review]
bug_1359092_extend_loaduri_with_triggeringprincipal.patch

Ah, wrong Origin attributes, that problem didn't occur with the systemPrincipal in the initial patch that was r+ed by Boris. In fact I took a closer look at this callsite again and I think it's best if we reuse the triggeringPrincipal from the cancelled channel and fall back to using the SystemPrincipal as we had in the initial patch. I filed Bug 1359358 to investigate whether we should fall back to systemPrincipal or rather to a new principal (using the right OA).

+++ b/docshell/base/nsDSURIContentListener.cpp
@@ -444,20 +444,23 @@ nsDSURIContentListener::CheckFrameOption
         if (webNav) {
+          nsCOMPtr<nsILoadInfo> loadInfo = httpChannel->GetLoadInfo();
+          nsCOMPtr<nsIPrincipal> triggeringPrincipal = loadInfo
+            ? loadInfo->TriggeringPrincipal()
+            : nsContentUtils::GetSystemPrincipal();
           webNav->LoadURI(u"about:blank",
                           0, nullptr, nullptr, nullptr,
+                          triggeringPrincipal);
Attachment #8861156 - Attachment is obsolete: true
Flags: needinfo?(ckerschb)
Attachment #8861349 - Flags: review+
(Assignee)

Comment 9

8 months ago
Now everything should be fine, but let's make sure:
   https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae84b2b4fad5b209640f5dc60bfe218d113b8067

Comment 10

8 months ago
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb4dc8b2f6af
Extend loadURI within nsIWebNavigation.idl by a triggeringPrincipal argument. r=bz

Comment 11

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fb4dc8b2f6af
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.