Create null principal with correct origin attributes

RESOLVED FIXED in Firefox 49

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: allstars.chh, Assigned: allstars.chh)

Tracking

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(firefox48 affected, firefox49 fixed)

Details

(Whiteboard: [OA])

Attachments

(3 attachments, 4 obsolete attachments)

So far when a null is created, most of them don't inherit any origin attributes.

Jonas and Bobby had a discussion wheter we should create null principal with inherited origin attributes.
Bobby listed a number of places that we need to fix.

<quote from Bobby>
These are the callers that already inherit OriginAttributes into null principals:
* https://dxr.mozilla.org/mozilla-central/search?q=%2Bcallers%3A%22nsNullPrincipal%3A%3ACreateWithInheritedAttributes%28class+nsIPrincipal+%2A%29%22

These are the callers that don't:
* http://searchfox.org/mozilla-central/search?q=%40mozilla.org%2Fnullprincipal%3B1&redirect=true
* http://searchfox.org/mozilla-central/search?q=NS_NULLPRINCIPAL_CONTRACTID&redirect=true
* http://searchfox.org/mozilla-central/search?q=createNullPrincipal&redirect=true
* http://searchfox.org/mozilla-central/search?q=nsNullPrincipal%3A%3ACreate%28&redirect=true

I recommend propagating OriginAttributes to null principals in the following places, all of which would be one-line or two-line patches:
* https://hg.mozilla.org/mozilla-central/file/494289c72ba3/ipc/glue/BackgroundUtils.cpp#l61
* https://hg.mozilla.org/mozilla-central/file/494289c72ba3/caps/BasePrincipal.cpp#l558
* https://hg.mozilla.org/mozilla-central/file/494289c72ba3/caps/BasePrincipal.cpp#l549
* https://hg.mozilla.org/mozilla-central/file/494289c72ba3/docshell/base/nsDocShell.cpp#l1491
* https://hg.mozilla.org/mozilla-central/file/494289c72ba3/docshell/base/nsDocShell.cpp#l1502
* https://hg.mozilla.org/mozilla-central/file/494289c72ba3/media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp#l511
* https://hg.mozilla.org/mozilla-central/file/494289c72ba3/dom/media/MediaManager.cpp#l983
https://hg.mozilla.org/mozilla-central/file/494289c72ba3/netwerk/protocol/http/nsCORSListenerProxy.cpp#l701

</quote from Bobby>
Thanks Yoshi!
Can you link to the discussion?  If a null principal is supposed to represent a "unique opaque origin" in the spec, why do the origin attributes matter?
irc log(but partial):
http://logs.glob.uno/?c=mozilla#developers&s=24+Feb+2016&e=24+Feb+2016&h=sicking%23c1384125

The discussion ends in the middle, so I sent mail to ask them privately.
I hope they don't mind I copy the discussion here.

Jonas' point is 
"I would imagine that it is always doable to find the correct OAs. It is just software after all. But it might be a lot of work, so we should make sure the advantages are as high as the costs. We have a lot of important things to work on so we should prioritize accordingly. "
...
"My question is why in cases when we'll run arbitrary web script using
a principal, why does the OriginAttributes matter if that principal is
a nullprincipal.

As far as I can think of, we currently use principals for two things:

1) Check if a principal is same-origin with another principal. For
example to see if page A can reach into the object graph of page B.
2) Use the principal as a key for various persisted and non-persisted
databases. Like cookies, permissions, and IndexedDB.

For 1, nullprincipals are always unique, so when comparing two
different principals, if one is a nullprincipal they are never
same-origin, and so OriginAttributes don't matter. And if they are the
same principal, then they would have had the same OriginAttributes as
well.

For 2, we never allow nullprincipals to access any of these databases,
and so the OriginAttributes doesn't matter. And even if we did,
nullprincipals have unique origins, so it doesn't really matter what
string we append at the end of the key, the key that we use into these
databases would be unique anyway."

Bobby's point
"My concern with giving up on this issue _entirely_ is that it provides a lot of stop-energy to fixing bugs in the future. If the story is "we mostly made OAs work with nullprincipals, but we may have missed a caller or two", we can fix OA-propagation bugs with patches to propagate them. If the story is "we gave up on that", the inertia is much higher, because the assumption is that there are lots of other broken things as well."
Thanks Yoshi!

And to complete the outline of my position: we already use OriginAttributes in various ways other than the principal equality check (for example, determining whether the principal is associated with an addon), and it seems plausible to me that we will eventually use it for more such things, as well as private browsing. So I'd rather at least try to make it work in the obvious cases (like sandboxed subframes) to decrease the likelihood or running into corner-case bugs in the future, as well as the inertia to fixing them.

The recommendations in comment 0 shouldn't take more than an hour to do, I wouldn't think.
Hi Bobby
Sorry I was busy last week, I start to look into this until now.
I have some questions when I went through the list
Could you help to explain it?
(In reply to Yoshi Huang[:allstars.chh] from comment #0)
> 
> I recommend propagating OriginAttributes to null principals in the following
> places, all of which would be one-line or two-line patches:
> *
> https://hg.mozilla.org/mozilla-central/file/494289c72ba3/ipc/glue/
> BackgroundUtils.cpp#l61
This one could be replaced by nsNullPrincipal::Create(), right?

> *
> https://hg.mozilla.org/mozilla-central/file/494289c72ba3/docshell/base/
> nsDocShell.cpp#l1491
I am not sure what to do for this, as the principal to be inherited here is a ExpandedPrincipal, which shouldn't have origin attributes, right?

> *
> https://hg.mozilla.org/mozilla-central/file/494289c72ba3/docshell/base/
> nsDocShell.cpp#l1502
The original code uses default OA, should we inherit OA from ownerPrincipal even the flag is already LOAD_FLAGS_DISALLOW_INHERIT_OWNER?

> *
> https://hg.mozilla.org/mozilla-central/file/494289c72ba3/dom/media/
> MediaManager.cpp#l983
This one already uses default OA by using nsNullPrincipal::Create, should we inherit OA from document?

Thanks
(In reply to Yoshi Huang[:allstars.chh] from comment #5)
> Hi Bobby
> Sorry I was busy last week, I start to look into this until now.
> I have some questions when I went through the list
> Could you help to explain it?
> (In reply to Yoshi Huang[:allstars.chh] from comment #0)
> > 
> > I recommend propagating OriginAttributes to null principals in the following
> > places, all of which would be one-line or two-line patches:
> > *
> > https://hg.mozilla.org/mozilla-central/file/494289c72ba3/ipc/glue/
> > BackgroundUtils.cpp#l61
> This one could be replaced by nsNullPrincipal::Create(), right?

Yes. We'll also need to pass the attributes via http://mxr.mozilla.org/mozilla-central/source/ipc/glue/PBackgroundSharedTypes.ipdlh#20 , and also write those out in PrincipalToPrincipalInfo.

> > *
> > https://hg.mozilla.org/mozilla-central/file/494289c72ba3/docshell/base/
> > nsDocShell.cpp#l1491
> I am not sure what to do for this, as the principal to be inherited here is
> a ExpandedPrincipal, which shouldn't have origin attributes, right?

You're right. Let's skip that one.

> > *
> > https://hg.mozilla.org/mozilla-central/file/494289c72ba3/docshell/base/
> > nsDocShell.cpp#l1502
> The original code uses default OA, should we inherit OA from ownerPrincipal
> even the flag is already LOAD_FLAGS_DISALLOW_INHERIT_OWNER?

I think so, yes. The principal will still be cross-origin, but we'll just be propagating contextual information (for example, if the owner is part of an addon or rendered in Private Browsing Mode, we probably want that for the new content as well).

> 
> > *
> > https://hg.mozilla.org/mozilla-central/file/494289c72ba3/dom/media/
> > MediaManager.cpp#l983
> This one already uses default OA by using nsNullPrincipal::Create, should we
> inherit OA from document?

Correct.

Thanks for working on this!
Whiteboard: [OA]
Depends on: 1125916
Dave, which user context id is right? The loadinfo one or the loadcontext one?
Flags: needinfo?(huseby)
oops!  Wrong bug.
(In reply to Tanvi Vyas [:tanvi] from comment #7)
> Dave, which user context id is right? The loadinfo one or the loadcontext
> one?
No longer depends on: 1125916
Flags: needinfo?(huseby)
Attachment #8744259 - Flags: review?(bobbyholley)
Attachment #8744260 - Flags: review?(bobbyholley)
Comment on attachment 8744261 [details] [diff] [review]
Part 3: fix for nsNullPrincipal::Create

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

Hi Bobby
Some callsites don't have the principal (or the principal is null)
so I just updated those parts I think I should, but not entirely sure,
you might check this part in more detail :P
But anyway, the try looks good.
Attachment #8744261 - Flags: review?(bobbyholley)
Comment on attachment 8744259 [details] [diff] [review]
Part 1: fix for @mozilla.org/nullprincipal;1

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

::: browser/components/feeds/FeedWriter.js
@@ +892,5 @@
>                 QueryInterface(Ci.nsIDocShell).currentDocumentChannel;
>  
> +    let attrs = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                       .getInterface(Ci.nsIWebNavigation)
> +                       .QueryInterface(Ci.nsIDocShell).getOriginAttributes();

Grab the docshell as a local variable to avoid the duplicated code with the above?

I guess we should really be calling InheritFromDocShellToDoc here, but it probably doesn't matter enough to make a way to call that from JS. Maybe make a comment to that effect though?

@@ +1193,5 @@
> +                               .getInterface(Ci.nsIWebNavigation)
> +                               .QueryInterface(Ci.nsIDocShell);
> +    let usePrivateBrowsing = docShell.QueryInterface(Ci.nsILoadContext)
> +                                     .usePrivateBrowsing;
> +    let attrs = docShell.getOriginAttributes();

Add a similar comment here.

::: devtools/client/inspector/rules/rules.js
@@ +120,5 @@
>    });
>    let docShell = getDocShell(frame);
>    let eventTarget = docShell.chromeEventHandler;
> +  let ssm = Services.scriptSecurityManager;
> +  let nullPrincipal = ssm.createNullPrincipal(docShell.getOriginAttributes());

and here.
Attachment #8744259 - Flags: review?(bobbyholley) → review+
Comment on attachment 8744260 [details] [diff] [review]
Part 2: fix for NS_NULLPRINCIPAL_CONTRACTID

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

f? mt on the webrtc change. I'm 99% sure it's fine so don't let this block you from landing, but I just thought he should be aware.

r=me with those changes.

::: caps/nsJSPrincipals.cpp
@@ +235,5 @@
> +        nsAutoCString suffix;
> +        nullInfo.attrs().CreateSuffix(suffix);
> +        return JS_WriteUint32Pair(aWriter, SCTAG_DOM_NULL_PRINCIPAL, 0) &&
> +               JS_WriteUint32Pair(aWriter, suffix.Length(), 0) &&
> +               JS_WriteBytes(aWriter, suffix.get(), suffix.Length());

Can you abstract this stuff out into static ReadSuffix/WriteSuffix helpers which share code with the ContentPrincipalInfo case?

::: ipc/glue/BackgroundUtils.cpp
@@ +60,5 @@
>      case PrincipalInfo::TNullPrincipalInfo: {
> +      const NullPrincipalInfo& info =
> +        aPrincipalInfo.get_NullPrincipalInfo();
> +      principal = nsNullPrincipal::Create(info.attrs());
> +      NS_ENSURE_TRUE(principal, nullptr);

Ugh, I think we should just make null principal creation infallible, and MOZ_RELEASE_ASSERT against failure in nsNullPrincipal::Init (it doesn't appear to me that it can ever fail in practice). That should let us remove the NS_ENSURE_TRUE here.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1907,5 @@
>          // we're either certain that we need isolation for the streams, OR
>          // we're not sure and we can fix the stream in SetDtlsConnected
> +        principal = doc ?
> +                      nsNullPrincipal::CreateWithInheritedAttributes(doc->NodePrincipal()) :
> +                      nsNullPrincipal::Create();

I'm pretty sure doc can never be null here. I think we can move the MOZ_ASSERT out of the |if| and get rid of the conditional here.
Attachment #8744260 - Flags: review?(bobbyholley)
Attachment #8744260 - Flags: review+
Attachment #8744260 - Flags: feedback?(martin.thomson)
Comment on attachment 8744261 [details] [diff] [review]
Part 3: fix for nsNullPrincipal::Create

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

r=me with those changes.

::: docshell/base/nsDocShell.cpp
@@ +1493,5 @@
> +      // We didn't inherit OriginAttributes here as ExpandedPrincipal doesn't
> +      // have origin attributes.
> +      PrincipalOriginAttributes attrs;
> +      attrs.InheritFromDocShellToDoc(GetOriginAttributes(), nullptr);
> +      owner = nsNullPrincipal::Create(attrs);

How about we add an overload of CreateWithInheritedAttributes that takes a docshell argument and does this for us? Then we could use it here...

@@ +1509,5 @@
> +      owner = nsNullPrincipal::CreateWithInheritedAttributes(ownerPrincipal);
> +    } else {
> +      PrincipalOriginAttributes attrs;
> +      attrs.InheritFromDocShellToDoc(GetOriginAttributes(), nullptr);
> +      owner = nsNullPrincipal::Create(attrs);

...and here...

@@ +12258,5 @@
>        // pick it up from the about:blank page we just loaded, and we
>        // don't really want even that in this case.
> +      PrincipalOriginAttributes attrs;
> +      attrs.InheritFromDocShellToDoc(GetOriginAttributes(), nullptr);
> +      owner = nsNullPrincipal::Create(attrs);

...and here...

@@ +13942,5 @@
>    if (!print || !print->IsInitializedForPrintPreview()) {
>      Stop(nsIWebNavigation::STOP_ALL);
> +    PrincipalOriginAttributes attrs;
> +    attrs.InheritFromDocShellToDoc(GetOriginAttributes(), nullptr);
> +    nsCOMPtr<nsIPrincipal> principal = nsNullPrincipal::Create(attrs);

...and here.

::: dom/base/DOMParser.cpp
@@ +350,5 @@
>      mOriginalPrincipal = mPrincipal;
>      if (nsContentUtils::IsSystemPrincipal(mPrincipal)) {
>        // Don't give DOMParsers the system principal.  Use a null
>        // principal instead.
> +      mPrincipal = nsNullPrincipal::CreateWithInheritedAttributes(mPrincipal);

Inheriting OAs from the system principal doesn't make sense. Please remove this.

::: dom/media/MediaManager.cpp
@@ +909,5 @@
>        };
>  
>        nsCOMPtr<nsIPrincipal> principal;
>        if (mPeerIdentity) {
> +        principal = nsNullPrincipal::CreateWithInheritedAttributes(window->GetExtantDoc()->NodePrincipal());

Similar f? from mt.

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +699,5 @@
> +          // Spec says to set our source origin to a unique origin.
> +          mOriginHeaderPrincipal =
> +            nsNullPrincipal::CreateWithInheritedAttributes(oldChannelPrincipal);
> +          if (!mOriginHeaderPrincipal) {
> +            rv = NS_ERROR_OUT_OF_MEMORY;

We should be able to remove this null check with the change mentioned in the other patch.
Attachment #8744261 - Flags: review?(bobbyholley) → review+
Comment on attachment 8744260 [details] [diff] [review]
Part 2: fix for NS_NULLPRINCIPAL_CONTRACTID

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

Yeah, looks fine.
Attachment #8744260 - Flags: feedback?(martin.thomson) → feedback+
(In reply to Bobby Holley (busy) from comment #14)
> ::: caps/nsJSPrincipals.cpp
> @@ +235,5 @@
> > +        nsAutoCString suffix;
> > +        nullInfo.attrs().CreateSuffix(suffix);
> > +        return JS_WriteUint32Pair(aWriter, SCTAG_DOM_NULL_PRINCIPAL, 0) &&
> > +               JS_WriteUint32Pair(aWriter, suffix.Length(), 0) &&
> > +               JS_WriteBytes(aWriter, suffix.get(), suffix.Length());
> 
> Can you abstract this stuff out into static ReadSuffix/WriteSuffix helpers
> which share code with the ContentPrincipalInfo case?
> 
Hi Bobby,
maybe I do this only for ReadSuffix helper, and not for WriteSuffix?
Because for writing suffix, we still need to write the length of the suffix first.
And for ContentPrincipalInfo it also needs to write another 'specLength'.

For example, if I move JS_WriteUint32Pair(aWriter, suffix.Length(), 0) into WriteSuffix helper,
then we need another argument for specLength, (it will be 0 for nullPrincipalInfo), which makes this helper a litter nasty as it not only writes suffix and also the length of spec.

Or if I just do JS_WriteBytes(aWriter, suffix.get(), suffix.Length()) inside the helper, that seems not worth the effort as it's just a one line call.
Comment on attachment 8744261 [details] [diff] [review]
Part 3: fix for nsNullPrincipal::Create

Hi Martin
Sorry for bothering again.
Bobby said the dom/media/MediaManager.cpp change should ask feedback from you.(Comment 15)
Attachment #8744261 - Flags: feedback?(martin.thomson)
Comment on attachment 8744261 [details] [diff] [review]
Part 3: fix for nsNullPrincipal::Create

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

LGTM
Attachment #8744261 - Flags: feedback?(martin.thomson) → feedback+
I'm not quite sure what the difficulty is. It seems like we could just make two functions:

ReadSuffixAndSpec(nsACString& aSuffix, nsACString& aSpec);
WriteSuffixAndSpec(const nsACString& aSuffix, const nsACString& aSpec);

In the nullprincipal case, we would just use an empty string for aSpec. The lengths can be derived by the helper using .Length().

Does that all seem reasonable?
(In reply to Bobby Holley (busy) from comment #20)
> I'm not quite sure what the difficulty is. It seems like we could just make
> two functions:
> 
> ReadSuffixAndSpec(nsACString& aSuffix, nsACString& aSpec);
> WriteSuffixAndSpec(const nsACString& aSuffix, const nsACString& aSpec);
> 
> In the nullprincipal case, we would just use an empty string for aSpec. The
> lengths can be derived by the helper using .Length().
> 
> Does that all seem reasonable?

Yeah, ReadSuffixAndSpec makes sense.
Thanks
addressed Bobby's comment.
Attachment #8744259 - Attachment is obsolete: true
Attachment #8745225 - Flags: review+
addressed Bobby's comment.
Attachment #8744260 - Attachment is obsolete: true
Attachment #8745226 - Flags: review+
addressed Bobby's comment
Attachment #8744261 - Attachment is obsolete: true
Comment on attachment 8745227 [details] [diff] [review]
Part 3: fix for nsNullPrincipal::Create v2

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

Hi Bobby
I added another nsDocShell::Cast for const nsIDocShell*, and it's for PrincipalOriginAttributes::InheritFromDocShell(const nsIDocShell* aDocShell)
I could have used const_cast but I remember you said we could add another Cast for const* in somewhere on bugzilla.

Would you mind reviewing nsDocShell.h change again?

Thanks
Attachment #8745227 - Flags: review?(bobbyholley)
Comment on attachment 8745227 [details] [diff] [review]
Part 3: fix for nsNullPrincipal::Create v2

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

I think you may have misinterpreted my suggestion in comment 15.

I was suggesting that we add a CreateWithInheritedAttributes overload for docshell so that we could do something like:

if (ownerPrincipal) {
  owner = nsNullPrincipal::CreateWithInheritedAttributes(ownerPrincipal);
} else {
  owner = nsNullPrincipal::CreateWithInheritedAttributes(this);
}

and avoid all the repeated lines in nsDocShell.cpp.

Whereas it looks like the change you made is to add an inherit overload to PrincipalOriginAttributes that takes an nsIDocShell instead of a DocShellOriginAttributes. I'm not sure we want that.
Attachment #8745227 - Flags: review?(bobbyholley) → review-
Status: NEW → ASSIGNED
Bobby, sorry, my bad.

This should be what you want.
Also I've removed the checks after calling nsNullPrincipal::Create*
Attachment #8745227 - Attachment is obsolete: true
Attachment #8745836 - Flags: review?(bobbyholley)
Comment on attachment 8745836 [details] [diff] [review]
Part 3: fix for nsNullPrincipal::Create v3

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

lgtm. r=bholley
Attachment #8745836 - Flags: review?(bobbyholley) → review+
https://hg.mozilla.org/mozilla-central/rev/0b08a5ce65ae
https://hg.mozilla.org/mozilla-central/rev/6b8afcb75d33
https://hg.mozilla.org/mozilla-central/rev/bc40974a002f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.