1456742 - if a channel doesn't have a load group, its origin attributes aren't honored

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler]]

This may just be a misunderstanding about the API, but it doesn't seem like the intended behavior. It appears that if a channel doesn't have its own load group set, any origin attributes that are set on the channel's load info aren't honored. See for example the OCSP request code:

https://searchfox.org/mozilla-central/rev/8f06c1b9a080b84435a2906e420fe102e1ed780b/security/manager/ssl/nsNSSCallbacks.cpp#129

  if (mRequestSession->mOriginAttributes != OriginAttributes()) {
    OriginAttributes attrs;
    attrs.mFirstPartyDomain =
      mRequestSession->mOriginAttributes.mFirstPartyDomain;
    attrs.mPrivateBrowsingId =
      mRequestSession->mOriginAttributes.mPrivateBrowsingId;

    nsCOMPtr<nsILoadInfo> loadInfo = chan->GetLoadInfo();
    if (loadInfo) {
      rv = loadInfo->SetOriginAttributes(attrs);
      NS_ENSURE_SUCCESS(rv, rv);
    }
  }

  // Create a loadgroup for this new channel.  This way if the channel
  // is redirected, we'll have a way to cancel the resulting channel.
  nsCOMPtr<nsILoadGroup> lg = do_CreateInstance(NS_LOADGROUP_CONTRACTID);
  chan->SetLoadGroup(lg);

If the given origin attributes aren't the default, we set some custom origin attributes on the channel's load info. We then set a load group. If the load group isn't set (basically, comment out references to 'lg' or 'mLoadGroup' in that file), the test xpcshell test security/manager/ssl/tests/unit/test_ocsp_private_caching.js will fail because we make an OCSP request in a private context that gets cached as non-private.

Obviously the workaround is clear (just set a load group), but it's less clear that this is the intended behavior. It also seems like a pitfall for any new code that relies on separating requests by origin attributes.

Comment 1

[Michal Novotny [:michal]]

Honza, can you have a look at this?

Comment 2

[Daniel Stenberg [:bagder]]

(giving it a prio 1, feel free to change)

Comment 3

[Honza Bambas (:mayhemer)]

Seems like a regressions (or a feature? :)) of bug 1199841.  

The problem is that UpdatePrivateBrowsing() is not called by the channel on itself.  We only call it when notification callbacks or a load group is set on the channel (as it affects the is-private state).  

The UpdatePrivateBrowsing() method is responsible for updating PrivateBrowsingChannel->mPrivateBrowsing member that is used to _sync_ PB state on the channel's origin attributes when we want to be used by external consumers.

So, I think one way to fix this is to call UpdatePrivateBrowsing() from inside AsyncOpen or some later point when the channel has no load group and also doesn't have notification callbacks.  That's a good fix.

I don't understand why there is this weird mechanism in the first place.  When we want to tell the cache which jar to use the privacy state is determined here:
	xul.dll!mozilla::net::PrivateBrowsingChannel<mozilla::net::HttpBaseChannel>::GetIsChannelPrivate(0x0000000a415f2760) Line 51	C++
 	xul.dll!NS_GetOriginAttributes(0x000001e3dc199050, {...}) Line 2038	C++
 	xul.dll!mozilla::net::GetLoadContextInfo(0x000001e3dc199050) Line 131	C++
 	xul.dll!mozilla::net::nsHttpChannel::OpenCacheEntryInternal(true, 0x0000000000000000, true) Line 3723	C++

instead of just directly returning the OAs of the channel.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

I think the patch is enough self-explanatory.  Locally tested for an ocsp request made for a load in a PB window.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73d26c2c475f
Make sure to call UpdatePrivateBrowsing() in http channel to properly set private browsing state of the channel when no load group or callbacks have been set on it before AsyncOpen. r=jdm

Comment 6

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/73d26c2c475f

Comment 7

[Ryan VanderMeulen [:RyanVM]]

What's the severity of this issue? Given how old it is, can the fix just ride the trains or should we consider backport?

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #7)
> What's the severity of this issue? Given how old it is, can the fix just
> ride the trains or should we consider backport?

I think the only affected consumer is now PSM and they have a workaround.  Hence, I think this can (should) ride.

Thanks.