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!

Dave, which user context id is right? The loadinfo one or the loadcontext one?

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?

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.

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.

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.

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.

Comment on attachment 8744260 [details] [diff] [review] Part 2: fix for NS_NULLPRINCIPAL_CONTRACTID Review of attachment 8744260 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, looks fine.

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

Comment on attachment 8744261 [details] [diff] [review] Part 3: fix for nsNullPrincipal::Create Review of attachment 8744261 [details] [diff] [review]: ----------------------------------------------------------------- LGTM

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.

addressed Bobby's comment.

addressed Bobby's comment

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

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.

Bobby, sorry, my bad. This should be what you want. Also I've removed the checks after calling nsNullPrincipal::Create*

Comment on attachment 8745836 [details] [diff] [review] Part 3: fix for nsNullPrincipal::Create v3 Review of attachment 8745836 [details] [diff] [review]: ----------------------------------------------------------------- lgtm. r=bholley

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b08a5ce65ae https://hg.mozilla.org/integration/mozilla-inbound/rev/6b8afcb75d33 https://hg.mozilla.org/integration/mozilla-inbound/rev/bc40974a002f

https://hg.mozilla.org/mozilla-central/rev/0b08a5ce65ae https://hg.mozilla.org/mozilla-central/rev/6b8afcb75d33 https://hg.mozilla.org/mozilla-central/rev/bc40974a002f