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

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Boris, any objections on replacing |nsiSupports owner;| with |nsIPrincipal triggeringPrincipal| for docshell loads?

Comment 2

[Boris Zbarsky [:bzbarsky]]

No, makes total sense to me.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1286472_replace_owner_with_triggeringprincipal.patch

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

Comment 4

[Boris Zbarsky [: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!

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1286472_replace_owner_with_triggeringprincipal.patch

(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.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1286472_serialize_triggeringprincipal_not_owner.patch

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!

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 8

[Boris Zbarsky [: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 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

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? :)

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

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?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Except see comment 9.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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 14

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 15

[Mike de Boer [:mikedeboer]]

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);`?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1286472_replace_owner_with_triggeringprincipal.patch

Just renamed ownerPrincipal to principalToInheritAttributesFrom.
Carrying over r+ from bz.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1286472_serialize_triggeringprincipal_not_owner.patch

Mike, thanks for the feedback. I incorporated all you suggestions and nits.

Comment 18

[Mike de Boer [:mikedeboer]]

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/

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 20

[Boris Zbarsky [: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.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

(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.

Comment 22

[Mike de Boer [:mikedeboer]]

(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.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
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

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/edb10ed57ed4
https://hg.mozilla.org/mozilla-central/rev/5edd4f667589