1292450 - Check mPrivateBrowsingId is correct in LoadInfo and LoadContext

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Bug 1269361 added mPrivateBrowsingId into OriginAttributes, however when Jonas asked me to add assertion to check that value I found lots of failures on try.

https://bugzilla.mozilla.org/show_bug.cgi?id=1264231#c27
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7480f005031c
https://bugzilla.mozilla.org/show_bug.cgi?id=1264231#c30

So in this bug, I'll make sure mPrivateBrowsingId is correct, and it's matched between LoadInfo and LoadContext.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I thought that we had gotten rid of the SEC_FORCE_PRIVATE_BROWSING flag and replaced it with the mPrivateBrowsingId OriginAttribute. But it appears that is not the case. So based on that I assume that we have not yet fully implemented mPrivateBrowsingId yet? Ehsan, do you know?

The goal in this bug should be to add assertions to make sure that when bug 1291652 lands, that we don't break private browsing.

I don't really know how the cookie code determines that whether to use private-browsing cookies or not. So maybe asserting that mPrivateBrowsingId matches between the loadContext and the loadInfo is not how we should accomplish that.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

I found that in bug 1278664 we made mOriginAttributes.mPrivateBrowsingId cannot be set on chrome docshell, while the docshell.mPrivateBrowsingId still can be set.

This is part of the reason of mismatch PB (loadInfo.UsePrivateBrowsing() and loadInfo.GetOriginAttributes().mPrivateBrowsingId) in LoadInfo, because the loadInfo will inherit OriginAttributes from the docshell (if there's no loadingPrincipa), which won't have the mOriginAttributes.mPrivateBrowsingId set(mOriginAttributes.mPrivateBrowsingId = 0) if its ItemType is chrome, meanwhile its securityFlags still has SEC_FORCE_PRIVATE_BROWSING set, so loadInfo.UsePrivateBrowsing() will be set.

Also LoadContext.cpp and JS-implemented nsILoadContext don't have the PB value sync.

Comment 3

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8778825 [details] [diff] [review]
Patch.

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

Hi Ehsan and jdm
Since you reviewed mPrivateBrowsingId previously, could you review this patch for me?

Hi Smaug,
since you reviewed bug 1278664, could you help to review this?
I'd like to make sure loadInfo and loadContext bahave the same in PB.

Thank you all.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8778825 [details] [diff] [review]
Patch.

>+++ b/docshell/base/LoadContext.cpp
>@@ -48,23 +48,23 @@ LoadContext::LoadContext(nsIPrincipal* aPrincipal,
>   , mUsePrivateBrowsing(false)
>   , mUseRemoteTabs(false)
> #ifdef DEBUG
>   , mIsNotNull(true)
> #endif
> {
>   PrincipalOriginAttributes poa = BasePrincipal::Cast(aPrincipal)->OriginAttributesRef();
>   mOriginAttributes.InheritFromDocToChildDocShell(poa);
>-  mOriginAttributes.SyncAttributesWithPrivateBrowsing(mUsePrivateBrowsing);
Whaat is this. We always set mOriginAttribute's pb flag to false here? Isn't this very broken. Do we need to fix this on branches?
Shouldn't get *get* mUsePrivateBrowsing, not set?




>   if (!aOptionalBase) {
>     return;
>   }
> 
>   MOZ_ALWAYS_SUCCEEDS(aOptionalBase->GetIsContent(&mIsContent));
>   MOZ_ALWAYS_SUCCEEDS(aOptionalBase->GetUsePrivateBrowsing(&mUsePrivateBrowsing));
>+  mOriginAttributes.SyncAttributesWithPrivateBrowsing(mUsePrivateBrowsing);
Why do we need Get/Sync here at all. Shouldn't we be able to get pb flag from OA?
aOptionalBase was added in Bug 1118845, and I hope it already passes sane principal.


>+  // Check if mPrivateBrowsingId is matched between LoadInfo and LoadContext.
>+  nsCOMPtr<nsILoadContext> loadContext;
>+  NS_QueryNotificationCallbacks(this, loadContext);
>+  // For addons it's possible that mLoadInfo is null.
>+  if (mLoadInfo && loadContext) {
>+      DocShellOriginAttributes docShellAttrs;
>+      loadContext->GetOriginAttributes(docShellAttrs);
>+      MOZ_ASSERT(mLoadInfo->GetOriginAttributes().mPrivateBrowsingId == docShellAttrs.mPrivateBrowsingId,
>+                 "PrivateBrowsingId values are not the same between LoadInfo and LoadContext.");
>+      MOZ_ASSERT(loadContext->UsePrivateBrowsing() == (docShellAttrs.mPrivateBrowsingId != 0),
>+                 "PrivateBrowsing is mismatch in LoadContext.");
>+  }
This should be inside #ifdef DEBUG


> 
>+    // Check if mPrivateBrowsingId is matched between LoadInfo and LoadContext.
>+    nsCOMPtr<nsILoadContext> loadContext;
>+    NS_QueryNotificationCallbacks(this, loadContext);
>+    // For addons it's possible that mLoadInfo is null.
>+    if (mLoadInfo && loadContext) {
>+        DocShellOriginAttributes docShellAttrs;
>+        loadContext->GetOriginAttributes(docShellAttrs);
>+        MOZ_ASSERT(mLoadInfo->GetOriginAttributes().mPrivateBrowsingId == docShellAttrs.mPrivateBrowsingId,
>+                   "PrivateBrowsingId values are not the same between LoadInfo and LoadContext.");
>+    }
#ifdef DEBUG

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Comment on attachment 8778825 [details] [diff] [review]
> Patch.
> 
> >+++ b/docshell/base/LoadContext.cpp
> >@@ -48,23 +48,23 @@ LoadContext::LoadContext(nsIPrincipal* aPrincipal,
> >   , mUsePrivateBrowsing(false)
> >   , mUseRemoteTabs(false)
> > #ifdef DEBUG
> >   , mIsNotNull(true)
> > #endif
> > {
> >   PrincipalOriginAttributes poa = BasePrincipal::Cast(aPrincipal)->OriginAttributesRef();
> >   mOriginAttributes.InheritFromDocToChildDocShell(poa);
> >-  mOriginAttributes.SyncAttributesWithPrivateBrowsing(mUsePrivateBrowsing);
> Whaat is this. We always set mOriginAttribute's pb flag to false here? Isn't
> this very broken. Do we need to fix this on branches?
>
The test case failure I met is dom/workers/test/test_sharedWorker_privateBrowsing.html. (from bug 1177621)
And it's loadContext doesn't have the correct PB, loadInfo has correct value.

The LoadContext here is the mLoadContext in dom/workers/WorkerPrivate.cpp,
and in WorkerPrivate I saw the mPrivateBrowsing are all related to nsContentUtils::IsInPrivateBrowsing, which should return the correct PB value from the docshell, so I guess it's fine ?

I agree the mLoadContext doesn't have PB value set, but I am not sure how much risk do we have here.
But if you do see we need to uplift it, I'll split this fix into a standalone patch.

> Shouldn't get *get* mUsePrivateBrowsing, not set?
> 
Sorry, but I don't get this part :P
but my fix would be like https://hg.mozilla.org/try/diff/ac562308b43c/docshell/base/LoadContext.cpp

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v2

Addressed smaug's comments.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a78bc062a99&selectedJob=25367399

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Yoshi Huang[:allstars.chh] from comment #6)
> 
> > Shouldn't get *get* mUsePrivateBrowsing, not set?
> > 
> Sorry, but I don't get this part :P
Oops. 

"Should we get mUsePrivateBrowsing from OA, not set"

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

and yes, your patch seem to do that.
Though I don't know why you need 
mOriginAttributes.SyncAttributesWithPrivateBrowsing(mUsePrivateBrowsing)

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779293 [details] [diff] [review]
Patch v2

> LoadContext::LoadContext(nsIPrincipal* aPrincipal,
>                          nsILoadContext* aOptionalBase)
>   : mTopFrameElement(nullptr)
>   , mNestedFrameId(0)
>   , mIsContent(true)
>-  , mUsePrivateBrowsing(false)
>   , mUseRemoteTabs(false)
> #ifdef DEBUG
>   , mIsNotNull(true)
> #endif
> {
>   PrincipalOriginAttributes poa = BasePrincipal::Cast(aPrincipal)->OriginAttributesRef();
>   mOriginAttributes.InheritFromDocToChildDocShell(poa);
>+  mUsePrivateBrowsing = (poa.mPrivateBrowsingId != 0);
>   mOriginAttributes.SyncAttributesWithPrivateBrowsing(mUsePrivateBrowsing);
mOriginAttributes.SyncAttributesWithPrivateBrowsing(mUsePrivateBrowsing);
should be now useless, right? So remove it



>   InheritOriginAttributes(mLoadingPrincipal, mOriginAttributes);
>+
>+  // For chrome docshell, the mPrivateBrowsingId remains 0 even its
>+  // UsePrivateBrowsing() is true, so we only update the mPrivateBrowsingId in
>+  // origin attributes if the type of the docshell is content.
>+  if (aLoadingContext) {
>+    nsCOMPtr<nsIDocShell> docShell = aLoadingContext->OwnerDoc()->GetDocShell();
>+    if (docShell && (docShell->ItemType() == nsIDocShellTreeItem::typeContent)) {
>+      mOriginAttributes.SyncAttributesWithPrivateBrowsing(GetUsePrivateBrowsing());
I see, so OA has pb flag only in content context, but GetUsePrivateBrowsing may return true even in chrome.


This needs to be documented in nsILoadInfo, both in originAttributes and usePrivateBrowsing

Comment 11

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Attachment: Patch v3

remove the extra call of SyncPrivateBrowsing and add some documentation in nsILoadInfo.

smaug, if my documentation on nsILoadInfo.idl doesn't do well, feel free to correct me.

Thanks

Comment 12

[(no longer active)]

Bug 1282124 has been filed to remove SEC_FORCE_PRIVATE_BROWSING.

Yoshi, how do you expect that bug landing to change things here?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to :Ehsan Akhgari from comment #12)
> Bug 1282124 has been filed to remove SEC_FORCE_PRIVATE_BROWSING.

\o/

That's great!

> Yoshi, how do you expect that bug landing to change things here?

I would expect that that means that we can simply assert that LoadInfo.GetOriginAttributes().mPrivateBrowsingId == LoadContext.GetOriginAttributes().mPrivateBrowsingId
in the cookie code.

If that assertion fires, that means that bug 1291652 would change behavior. If that's the case we need to figure out which of the LoadInfo and the LoadContext contains the right information and make sure to adjust the other one before we land bug 1291652.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to :Ehsan Akhgari from comment #12)
> Bug 1282124 has been filed to remove SEC_FORCE_PRIVATE_BROWSING.
> 
> Yoshi, how do you expect that bug landing to change things here?

In chrome code, the originAttributes.mPrivateBrowsingId of nsILoadInfo and nsILoadContext will always be 0,
so in your bug if you tried to remove usePrivateBrowsing from nsILoadInfo, I worried that in chrome code, nsILoadInfo doesn't have enough information to indicate that this loadInfo is for PrivateBrowsing.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

My vision was that with bug 1282124, chrome code that wanted to use private-browsing would create an nsILoadContext which had loadContext.originAttributes.privateBrowsingId == 1 *and* set channel.loadInfo.originAttributes = { privateBrowsingId = 1 }

And then in this bug we'd assert that the two match.

And then at some point in the future, once we feel confident that the switch in bug 1291652 didn't cause any regressions, we can remove this assertion, and change any chrome code that wants to use private browsing to only set channel.loadInfo.originAttributes.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

(In reply to Jonas Sicking (:sicking) PTO Until July 5th from comment #15)
> My vision was that with bug 1282124, chrome code that wanted to use
> private-browsing would create an nsILoadContext which had
> loadContext.originAttributes.privateBrowsingId == 1 *and* set
> channel.loadInfo.originAttributes = { privateBrowsingId = 1 }
> 
I am a little confused now, it seems this contradicts bug 1278664.
If bug 1278664 is not correct, so my patch is not correct either.

See the conclusion made by that bug.
http://searchfox.org/mozilla-central/source/docshell/base/nsDocShell.cpp#3714

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi smaug,
Can you check Jonas' comment 15?
I think you two have different opinions on this, :P

Thanks

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

I wouldn't have the pb flag set in origin attributes if chrome wants to do pb request, since
chrome stuff just shouldn't have any OA flags set (because system principal is singleton).

But I admit this is a tricky case.

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Hi Ehsan
I think bug 1282124 and this bug are trying to solve difffernt problems, in this bug I just make sure the information between loadInfo and loadContext are consistent. So I'd like to remove the dependency on it here, also this is because this bug is already blocking other bug, which also blocking other bugs, and so on. In fact, this bug is in the 4th or 5th level of the dependency tree :P.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 8779642 [details] [diff] [review]
Patch v3

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

I realized ehsan is the owner of PB, (I thought it was jdm before).
so r? ehsan should be enough.

Comment 21

[(no longer active)]

Comment on attachment 8779642 [details] [diff] [review]
Patch v3

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

r=me with the comments below addressed.

::: netwerk/base/LoadInfo.cpp
@@ +194,5 @@
> +  // origin attributes if the type of the docshell is content.
> +  if (aLoadingContext) {
> +    nsCOMPtr<nsIDocShell> docShell = aLoadingContext->OwnerDoc()->GetDocShell();
> +    if (docShell && (docShell->ItemType() == nsIDocShellTreeItem::typeContent)) {
> +      mOriginAttributes.SyncAttributesWithPrivateBrowsing(GetUsePrivateBrowsing());

Can you please add an assertion in the chrome case here ensuring that mPrivateBrowsingId is 0?

@@ +252,5 @@
>      nsDocShell::Cast(docShell)->GetOriginAttributes();
> +
> +  if (docShell->ItemType() == nsIDocShellTreeItem::typeContent) {
> +    MOZ_ASSERT(GetUsePrivateBrowsing() == (attrs.mPrivateBrowsingId != 0),
> +               "docshell and mSecurityFlags have different value for PrivateBrowsing().");

Please add an assertion similar to the above for the chrome case.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1759,5 @@
>  
>    LOG(("HttpChannelChild::AsyncOpen [this=%p uri=%s]\n", this, mSpec.get()));
>  
> +  MOZ_ASSERT(mLoadInfo->GetUsePrivateBrowsing() == (mLoadInfo->GetOriginAttributes().mPrivateBrowsingId != 0),
> +             "PrivateBrowsing is mismatch in LoadInfo.");

Nit: "mPrivateBrowsingId mismatch on LoadInfo"

@@ +1762,5 @@
> +  MOZ_ASSERT(mLoadInfo->GetUsePrivateBrowsing() == (mLoadInfo->GetOriginAttributes().mPrivateBrowsingId != 0),
> +             "PrivateBrowsing is mismatch in LoadInfo.");
> +
> +#ifdef DEBUG
> +  // Check if mPrivateBrowsingId is matched between LoadInfo and LoadContext.

Nit: s/is matched/matches/

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5471,5 @@
> +        DocShellOriginAttributes docShellAttrs;
> +        loadContext->GetOriginAttributes(docShellAttrs);
> +        MOZ_ASSERT(mLoadInfo->GetOriginAttributes().mPrivateBrowsingId == docShellAttrs.mPrivateBrowsingId,
> +                   "PrivateBrowsingId values are not the same between LoadInfo and LoadContext.");
> +    }

Nit: in order to avoid code duplication, I'd appreciate if you can refactor this out into a method on HttpBaseChannel which you call in both places.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

To clarify, I guess I have three goals that I really care about:

1. That we get rid of all private-browsing flags, and make private browsing simply an OriginAttribute
2. That necko uses the OriginAttributes on the nsILoadInfo to determine which set of cookies to use
3. That we don't accidentally regress private browsing while working towards 1 and 2.

The goal of this bug was to help with 3. I.e. to add assertions that make sure that while we work on 1 and 2, that we don't change behavior of when PB cookies are used, and when they are not used.

But anything that accomplishes 1-3 above is great with me!

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

It is unclear to me whether we want (1) (and then also (2)). Maybe we do, maybe we don't.
system privileged stuff doesn't really have OA, and shouldn't have since system principal is singleton.

nsILoadInfo does indicate that its OA might not map to OA of the principal. That is bit error prone.
When does one use OA from principal, when from loadinfo? At least loadinfo's documeentation need some updates.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe562e66e9b9
Check mPrivateBrowsingId is correct in LoadInfo and LoadContext. r=smaug, ehsan

Comment 25

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fe562e66e9b9

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Olli Pettay [:smaug] from comment #23)
> It is unclear to me whether we want (1) (and then also (2)). Maybe we do,
> maybe we don't.
> system privileged stuff doesn't really have OA, and shouldn't have since
> system principal is singleton.

System privileged stuff needs to be able to select which "cookie jar" to use when making requests. For example when the awesomebar code makes a network request to the search provider in order to get search-related autocomplete suggestions, it needs to set the userContextId based on the currently focused tab.

This is currently a real pain to do due that the nsILoadContext affects both the OA of the network request, and a host of other things. That's why we want 2.

The reason we want 1is that we currently have two cookie jar mechanisms, which is error prone. One is better. Especially since the PB flag has the downside of not being available in nsIPrincipal. Having just OA makes it more likely that things like containers and first-party-separation will work correctly since we do lots of testing of PB which gets leveraged. 


> nsILoadInfo does indicate that its OA might not map to OA of the principal.
> That is bit error prone.

The intent is that the nsILoadInfo OA can be overridden in order to affect which cookies a request uses. This is useful for for example the awesomebar, or (in the future) for private-related addons, or for changing cookie jars when a topple bel navigation network request is redirected cross-domain and we do first-party cookie separation.

I suggest you talk to Tanvi and Huseby about these projects. 

I've also sent better docs to Chistoph which is hopefully making its way into the tree.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Jonas Sicking (:sicking) No longer reading bugmail consistently from comment #26)
> (In reply to Olli Pettay [:smaug] from comment #23)
> > It is unclear to me whether we want (1) (and then also (2)). Maybe we do,
> > maybe we don't.
> > system privileged stuff doesn't really have OA, and shouldn't have since
> > system principal is singleton.
> 
> System privileged stuff needs to be able to select which "cookie jar" to use
> when making requests. For example when the awesomebar code makes a network
> request to the search provider in order to get search-related autocomplete
> suggestions, it needs to set the userContextId based on the currently
> focused tab.
Good point.


> The reason we want 1is that we currently have two cookie jar mechanisms,
> which is error prone. One is better. Especially since the PB flag has the
> downside of not being available in nsIPrincipal.
pb flag is normally available in principal's OA.
So it is error prone that usually nsILoadInfo's principal's OA is reasonable and could be used to select 'cookie jar', but in some cases it is not. And doesn't help that currently nsILoadInfo's documentation is rather broken. But good that it is being fixed.


> Having just OA makes it
> more likely that things like containers and first-party-separation will work
> correctly since we do lots of testing of PB which gets leveraged. 
right. So we need to make somehow sure that one uses the right OA, since there aren't just one OA around, there are up to 3:
loadinfo's, loadingPrincipal's and triggeringPrincipal's.
If we could somehow prevent one to access the principals of nsILoadInfo, and add more methods to it to do whatever principal related checks one does with triggering and loading principals. But looks like that isn't really possible.