Closed Bug 1286472 Opened 3 years ago Closed 3 years ago

Replace |nsiSupports owner| with |nsIPrincipal triggeringPrincipal| within nsIDocShell

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files, 3 obsolete files)

Currently docshell uses the term 'owner' but the loadInfo uses the term TriggeringPrincipal. Would it make sense to update |nsISupports owner;| to become |nsIPrincipal triggeringPrincipal;| [1]?

I crafted a first patch and it seems owner is always an nsIPrincipal so I guess there is no need that it remains an nsISupports, or am I forgetting about something?

[1] https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsIDocShell.idl#164
Assignee: nobody → ckerschb
Status: NEW → ASSIGNED
Whiteboard: [domsecurity-active]
Blocks: 1182569
Boris, any objections on replacing |nsiSupports owner;| with |nsIPrincipal triggeringPrincipal| for docshell loads?
Flags: needinfo?(bzbarsky)
No, makes total sense to me.
Flags: needinfo?(bzbarsky)
Blocks: 1286838
Boris, replacing the owner within nsIDocShell.idl goes hand in hand with also updating:
* nsIDocShellLoadInfo.idl
* nsIWebNavigation.idl
* nsISHEntry.idl

As a follow up I filed Bug 1286838 within which we can then deprecate 'owner' on nsIChannel once we have converted docshell loads to rely on asyncOpen2().


https://treeherder.mozilla.org/#/jobs?repo=try&revision=5673030830f7
Attachment #8770974 - Flags: review?(bzbarsky)
Comment on attachment 8770974 [details] [diff] [review]
bug_1286472_replace_owner_with_triggeringprincipal.patch

>+++ b/docshell/base/nsDocShell.cpp
>+  // (4) we pass a null principal into the channel, and a principal will be

This is easy to confuse with a NullPrincipal.  Maybe "we don't pass a principal into the channel"?  This occurs in two places.

>+  // NOTE: This all only works because the only thing the triggeringPrincipal
>+  //       is used for in InternalLoad is data:, javascript:, and about:blank

This comment is no longer true, right?  Those are the only things that the triggeringPrincipal is _inherited_ by, but we _use_ it for everything, by putting it in the loadinfo.

>+  nsCOMPtr<nsIPrincipal> ownerPrincipal = triggeringPrincipal;

You don't need the ownerPrincipal thing anymore.  Just use triggeringPrincipal, since you no longer need to QI.

+++ b/docshell/base/nsIWebNavigation.idl
>-  const unsigned long LOAD_FLAGS_DISALLOW_INHERIT_OWNER = 0x40000;

This will break addons.  See https://dxr.mozilla.org/addons/search?q=DISALLOW_INHERIT_OWNER

You probably want to just have both constants here, with the same value and a comment.

r=me with that.  Thank you for doing this!
Attachment #8770974 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #4)
> >+  nsCOMPtr<nsIPrincipal> ownerPrincipal = triggeringPrincipal;
> 
> You don't need the ownerPrincipal thing anymore.  Just use
> triggeringPrincipal, since you no longer need to QI.

That part within nsDocShell::LoadURI() seems fragile, e.g. further down is this part:

>  if (aLoadFlags & LOAD_FLAGS_DISALLOW_INHERIT_PRINCIPAL) {
>    inheritPrincipal = false;
>    triggeringPrincipal = ownerPrincipal ?
>                            nsNullPrincipal::CreateWithInheritedAttributes(ownerPrincipal) :
>                            nsNullPrincipal::CreateWithInheritedAttributes(this);
>   }

I am too scared to refactor those bits because it seems not very robust and I think we should rather keep the tmp variable 'ownerPrincipal' within this 'renaming' patch and really don't change any logic. If you really think this part is worth updating, then I am happy to file a follow up.
Attachment #8770974 - Attachment is obsolete: true
Attachment #8771959 - Flags: review+
Boris, when running
> browser/components/sessionstore/test/browser_911547.js
I realized we don't serialize correctly because code within SessionHistory was relying on entry.owner which has now become .triggeringPrincipal.

Sorry, I missed that the first time around!
Attachment #8771960 - Flags: review?(bzbarsky)
> e.g. further down is this part

Ah, indeed.  Can you please rename ownerPrincipal to originalTriggeringPrincipal or principalToInheritAttributesFrom, then?  That's what we're really using it for, after all.
Comment on attachment 8771960 [details] [diff] [review]
bug_1286472_serialize_triggeringprincipal_not_owner.patch

>+    if (entry.triggeringPrincipal_b64) {

No, this is wrong.  The reason it's wrong is consider what happens during the update from a version of Firefox without this change to a version with.  The serialized data will have entry.owner_b64 but here we'll look for entry.triggeringPrincipal_b64.  We need to look for both, probably preferring the latter.

There's a similar issue in the Android code.

Also, if someone _downgrades_ Firefox this could lose parts of the session, afaict: the serialization will have entry.triggeringPrincipal_b64 but the downgraded deserialization code will look for entry.owner_b64.

It might sense to leave the property named "owner_b64" in the serialization... Please get review from actual owners of this code on this stuff.
Attachment #8771960 - Flags: review?(ttaubert)
Attachment #8771960 - Flags: review?(s.kaspari)
Attachment #8771960 - Flags: review?(bzbarsky)
Comment on attachment 8771960 [details] [diff] [review]
bug_1286472_serialize_triggeringprincipal_not_owner.patch

Not my area of expertise. @snorp: Can you review this or find a reviewer? :)
Attachment #8771960 - Flags: review?(s.kaspari) → review?(snorp)
Comment on attachment 8771960 [details] [diff] [review]
bug_1286472_serialize_triggeringprincipal_not_owner.patch

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

I don't really know this code, but if it's mostly s/owner/triggeringPrincipal/ I guess it seems fine?
Attachment #8771960 - Flags: review?(snorp) → review+
Except see comment 9.
Flags: needinfo?(snorp)
Redirecting to Mike who is responsible for sessionstore nowadays. I unfortunately course can't do this properly because he blocked review/needinfo requests.

Mike, please check comment #9, Boris makes some really good points especially about what happens when loading an old session after upgrading. I personally wouldn't be too worried about the downgrade scenario but that's your call to make now :)
Comment on attachment 8771960 [details] [diff] [review]
bug_1286472_serialize_triggeringprincipal_not_owner.patch

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

Just sent Mike an email with a link to the previous comment.
Attachment #8771960 - Flags: review?(ttaubert)
Comment on attachment 8771960 [details] [diff] [review]
bug_1286472_serialize_triggeringprincipal_not_owner.patch

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

I can't r+ this just yet, because I'd like to hear your opinion on my comment regarding supporting owner_b64 for older sessionstore.js reads.
Other than that this looks good.

::: browser/components/sessionstore/SessionHistory.jsm
@@ +378,5 @@
>          childDocIdents = matchingEntry.childDocIdents;
>        }
>      }
>  
> +    if (entry.triggeringPrincipal_b64) {

Well, I think there's a possibility that we'll get sessionstore.js entries with `entry.owner_b64` in there still. So we'll need to be backward compatible for a while by doing something like:
```js
// This field was called 'owner_b64', so read that field if it's still there.
if (entry.owner_b64) {
  entry.triggeringPricipal_b64 = entry.owner_b64;
  delete entry.owner_b64;
}
if (entry.triggeringPrincipal_b64) {
...
```

::: mobile/android/components/SessionStore.js
@@ +1251,5 @@
>      }
>  
> +    if (aEntry.triggeringPrincipal_b64) {
> +      let triggeringPrincipalInput = Cc["@mozilla.org/io/string-input-stream;1"].
> +                                     createInstance(Ci.nsIStringInputStream);

nit: appreciate the formatting, but can you write it like:

``js
let triggeringPrincipalInput = Cc["@mozilla.org/io/string-input-stream;1"]
                                 .createInstance(Ci.nsIStringInputStream);
```
?

@@ +1255,5 @@
> +                                     createInstance(Ci.nsIStringInputStream);
> +      let binaryData = atob(aEntry.triggeringPrincipal_b64);
> +      triggeringPrincipalInput.setData(binaryData, binaryData.length);
> +      let binaryStream = Cc["@mozilla.org/binaryinputstream;1"].
> +                         createInstance(Ci.nsIObjectInputStream);

nit: same here.

@@ +1263,1 @@
>        } catch (ex) { dump(ex); }

While you're here, can you change the 'dump' here to `Cu.reportError("SessionStore: " + ex);`?
Attachment #8771960 - Flags: review?(mdeboer) → review-
Depends on: 1289785
Just renamed ownerPrincipal to principalToInheritAttributesFrom.
Carrying over r+ from bz.
Attachment #8771959 - Attachment is obsolete: true
Attachment #8775150 - Flags: review+
Mike, thanks for the feedback. I incorporated all you suggestions and nits.
Attachment #8771960 - Attachment is obsolete: true
Attachment #8775151 - Flags: review?(mdeboer)
Comment on attachment 8775151 [details] [diff] [review]
bug_1286472_serialize_triggeringprincipal_not_owner.patch

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

I like it, thanks!

::: browser/components/sessionstore/SessionHistory.jsm
@@ +378,5 @@
>          childDocIdents = matchingEntry.childDocIdents;
>        }
>      }
>  
> +    // The field entry.owner_b64 got renamed to entry.triggeringPrincipal within

nit: entry.triggeringPrincipal_b64 and s/within/in/
Attachment #8775151 - Flags: review?(mdeboer) → review+
Boris, I think you and Mike got it all covered and we are still backward compatible by providing
> if (entry.owner_b64) {
>  entry.triggeringPricipal_b64 = entry.owner_b64;
>  delete entry.owner_b64;
> }

Do we need to wait for snorp's feedback, or can we land those changesets?
Flags: needinfo?(bzbarsky)
>+    // The field entry.owner_b64 got renamed to entry.triggeringPrincipal within

No, it got renamed to entry.triggeringPricipal_b64.

If Mike is ok with this, and in particular if he feels like he can make this call for Fennec, we can probably just land this.
Flags: needinfo?(bzbarsky) → needinfo?(mdeboer)
(In reply to Boris Zbarsky [:bz] from comment #20)
> >+    // The field entry.owner_b64 got renamed to entry.triggeringPrincipal within
> 
> No, it got renamed to entry.triggeringPricipal_b64.

Oh, indeed. Thanks for catching this. I will update that in the final patch.
(In reply to Boris Zbarsky [:bz] from comment #20)
> No, it got renamed to entry.triggeringPricipal_b64.

The review comments I made before for SessionHistory.jsm also apply here, so please apply them, Christoph. Thanks!

> If Mike is ok with this, and in particular if he feels like he can make this
> call for Fennec, we can probably just land this.

Hmmm, I don't know if my newly acquired module ownership extends to Fennec as well, but I think it's fine in this case.
Flags: needinfo?(mdeboer)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/edb10ed57ed4
Replace owner with triggeringPrincipal within docshell. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/5edd4f667589
Replace serializing nsISHEntry.owner with nsISHEntry.triggeringPrincipal. r=bz,mikedeboer
https://hg.mozilla.org/mozilla-central/rev/edb10ed57ed4
https://hg.mozilla.org/mozilla-central/rev/5edd4f667589
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.