Fanolian Reporter
	Description • 8 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20160904030201

Steps to reproduce:

1. In a new profile, go to http://web.whatsapp.com/. The first ever load should redirect to https://web.whatsapp.com/ properly, i.e. green lock is present.
2. Load http://web.whatsapp.com/ again.


Actual results:

On the second try, it redirects to https://web.whatsapp.com/ but the green lock is missing. It says the connection is not secure. (see attachment)


Expected results:

The connection should show as secure on every redirects.

Note:
I guess this bug maybe related to HSTS cache. Only the first try in a new profile redirects properly unless I forget the HSTS settings [1] for WhatsApp Web. Deleting history or cache alone does not work.

[1]: https://support.mozilla.org/en-US/questions/919498

	Fanolian Reporter
	Comment 1 • 8 years ago

From mozregression:

Last good Nightly: 2016-01-24
First bad Nightly: 2016-01-25

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d6d81655dd9e146c300a64c0fcaeb04ca3300a19&tochange=3f41d7d0f544ebd98273e39bd945c28878a47427

	Fanolian Reporter
	Comment 2 • 8 years ago

It cannot be reproduced in Firefox 48.0.2 Release.
Nightly 2016-04-24 (last nightly build of 48.0a1) is affected though.

	Alice0775 White
	Comment 3 • 8 years ago

[Tracking Requested - why for this release]: The connection is not secure if e10s is enabled

I can reproduce the problem on Nightly51.0a1 with e10s.

However it works as expected on Nightly51.0a1 if e10s is disabled.

	Fanolian Reporter
	Comment 4 • 8 years ago

Thanks Alice. I can confirm 48.0.2 Release is also affected if e10s is force-enabled.

	Alice0775 White
	Comment 5 • 8 years ago

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:

Regression window(force enabled e10s and UA spoof):
user_pref("browser.tabs.remote", true);
user_pref("browser.tabs.remote.autostart", true);
user_pref("browser.tabs.remote.autostart.1", true);
user_pref("browser.tabs.remote.autostart.2", true);
user_pref("browser.tabs.remote.force-enable", true);
user_pref("extensions.e10sBlocksEnabling", false);
user_pref("extensions.e10sBlockedByAddons", false);
user_pref("general.useragent.override", "Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0");

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=642a536b741a77cde34ef22789b4cffcaf42b17f&tochange=344d31ef4cc6ed5546ee4c604af41eddc2016d15

Regressed by: Bug 1214305

	(no longer active)
	Comment 6 • 8 years ago

Andrew, can you please find a Necko owner for this?  I'm busy with bug 1297687 and friends.  Thanks!

	Jason Duell
	Comment 7 • 8 years ago

I'm hoping Honza can jump in here.

	Daniel Stenberg [:bagder]
	Comment 8 • 8 years ago

I don't know why, but when I tried to bisect this to find the offending commit (it is a fairly limited number of commits), *all* builds I tried failed (and I tried 10 or so). I can't recall anything in particular since that has changed since back in January that would explain that. What did I forget?

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 9 • 8 years ago

Tracking for 49+.  We are heading into the RC2 build for the 49 release. 
We are shipping e10s to more users in the last week or so of 48, and with 49. 
Should we ship with this issue or should it be a release blocker?

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 8 years ago

Wait, the connection is secure, but shows as not secure? That's not ideal, but I don't think I would block 49 on that. 
We could still take a patch for 50 though.

	Honza Bambas (:mayhemer)
	Comment 11 • 8 years ago

There is no securityInfo object on the https redirected child channel.  Why?  Because ServiceWorkers interfere and load the channel w/o any IPC (that would carry the secInfo object from parent down to child).  HttpChannelChild::BeginNonIPCRedirect is being called for the redirected channel.  

Leads to:

2016-09-07 14:01:10.036000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10780900: remember securityInfo 0
2016-09-07 14:01:10.037000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10780900: UpdateSecurityState:  old-new  0 - 0
2016-09-07 14:01:10.037000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10780900: UpdateSecurityState: calling OnSecurityChange
2016-09-07 14:01:10.044000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10780900: OnSecurityChange: (4) https://web.whatsapp.com/

Correct has to look like:

2016-09-07 14:00:29.120000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10783480: remember securityInfo 10a3f800
2016-09-07 14:00:29.120000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10783480: UpdateSecurityState:  old-new  0 - 3
2016-09-07 14:00:29.120000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10783480: UpdateSecurityState: calling OnSecurityChange
2016-09-07 14:00:29.126000 UTC - [Main Thread]: D/nsSecureBrowserUI SecureUI:10783480: OnSecurityChange: (40002) https://web.whatsapp.com/

	Josh Matthews [:jdm] Assignee
	Comment 12 • 8 years ago

This would be fixed by bug 1231222. It's not clear if we should prioritize fixing this separately.

	Jim Mathies [:jimm]
	Comment 13 • 8 years ago

We would very much like to get this fixed asap, Josh can you prioritize your work in the parent bug?

	Josh Matthews [:jdm] Assignee
	Comment 14 • 8 years ago

Is there a particular release you want to guarantee this is fixed by? Bug 1231222 has turned out to be much more complicated to implement than originally anticipated.

	Jim Mathies [:jimm]
	Comment 15 • 8 years ago

(In reply to Josh Matthews [:jdm] from comment #14)
> Is there a particular release you want to guarantee this is fixed by? Bug
> 1231222 has turned out to be much more complicated to implement than
> originally anticipated.

Well I guess that's more up to you based on your current work load. We have a bug here where we falsely advertise a page as being insecure when in fact it isn't. Pretty serious bug but not sec critical.

	(no longer active)
	Comment 16 • 8 years ago

Josh, in InterceptedChannelContent::FinishSynthesizedResponse() before calling BeginNonIPCRedirect, couldn't we grab mChannel's security info and pass that to BeginNonIPCRedirect and set that on the redirect channel when SetupRedirect() returns there?  That seems fairly simple, and from what I can tell it should fix this bug.  It also kinda mirrors what we already do in InterceptedChannelChrome::FinishSynthesizedResponse() to attach the security info to the synthesized cache entry before performing the redirect...

	(no longer active)
	Comment 17 • 8 years ago

(In reply to Jim Mathies [:jimm] from comment #15)
> (In reply to Josh Matthews [:jdm] from comment #14)
> > Is there a particular release you want to guarantee this is fixed by? Bug
> > 1231222 has turned out to be much more complicated to implement than
> > originally anticipated.
> 
> Well I guess that's more up to you based on your current work load. We have
> a bug here where we falsely advertise a page as being insecure when in fact
> it isn't. Pretty serious bug but not sec critical.

This is extremely terrible since we have spent years educating people to look at the lock icon to determine whether a site is "secure".  Especially since it's completely non-obvious what the site can do in order to fix this on their side.  :(

	Josh Matthews [:jdm] Assignee
	Comment 18 • 8 years ago

Not sure how or whether automated testing is possible for these changes. Waiting to ask for review until I settle that question.

	Josh Matthews [:jdm] Assignee
	Comment 19 • 8 years ago

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

	Josh Matthews [:jdm] Assignee
	Comment 20 • 8 years ago

I have ideas for how to test this. I've also stepped through precisely what's happening and understand the problem better now - we have a special setup in e10s HTTP channels where we take channels that _would_ be upgraded via HSTS and do our ServiceWorker interception setup as if they already have been upgraded. If the SW chooses to provide a response, we then initiate an internal redirection from the http URL to the https URL, then fill the resulting channel with the expected response headers and body that the SW provided. So HSTS is actually the reason that a redirection is involved here at all; we're being provided with a secure response from the SW, but that response's security info is not being propagated to the upgraded channel that results.

	Josh Matthews [:jdm] Assignee
	Comment 22 • 8 years ago

Comment on attachment 8790422 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

This is a problem that affects any e10s HTTP channel that is intercepted and provided with a response with a different URL than the original channel URL. Examples:
* HSTS upgrade where the upgraded URL is intercepted by a ServiceWorker
* HTTP request that is replaced with any HTTPS response
* HTTPS response that is replaced with the result of a fetch() to some other URL
* HTTPS response that is replaced with the result of a DOM Cache match

When a SW provides a response for an intercepted request, the security info on the channel is updated to match the security info associated with the response source (eg. a channel for fetch(), serialized DB field for DOM cache). However, when a redirect is initiated that does not use the usual e10s IPC redirection mechanism (which occurs in order to make the final channel URI match the URI of the response that was provided), the security info is not propagated to the new channel.

	Honza Bambas (:mayhemer)
	Comment 23 • 8 years ago

Comment on attachment 8790422 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1348,5 @@
> +HttpChannelChild::OverrideSecurityInfo(nsISupports* securityInfo)
> +{
> +  MOZ_ASSERT(!mSecurityInfo);
> +  mSecurityInfo = securityInfo;
> +}

we already have OverrideSecurityInfo on HttpChannelBase which HttpChannelChild derives from.  I don't think you need this one, which is also pretty unsafe, btw.  The existing implementation has a lot of checks.

If those don't fit, we may think of a new method (or fixing the existing one), but that needs to be discussed first.

::: netwerk/protocol/http/InterceptedChannel.cpp
@@ +454,5 @@
>    bool equal = false;
>    originalURI->Equals(responseURI, &equal);
>    if (!equal) {
>      mChannel->ForceIntercepted(mSynthesizedInput);
> +    mChannel->BeginNonIPCRedirect(responseURI, *mSynthesizedResponseHead.ptr();

probably a mistaken change?

	Josh Matthews [:jdm] Assignee
	Comment 25 • 8 years ago

Comment on attachment 8792053 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Cancelling until I get a good try run.

	Josh Matthews [:jdm] Assignee
	Comment 26 • 8 years ago

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

	Josh Matthews [:jdm] Assignee
	Comment 27 • 8 years ago

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

	Josh Matthews [:jdm] Assignee
	Comment 28 • 8 years ago

Comment on attachment 8792053 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Try push looks good.

	Honza Bambas (:mayhemer)
	Comment 29 • 8 years ago

Comment on attachment 8792053 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

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

Thanks!

	Pulsebot
	Comment 30 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b56ad4440b05
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected. r=mayhemer

	Josh Matthews [:jdm] Assignee
	Comment 31 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/dddb857bd3fd2e12e4c11d1456cb586f68e87aa3
Bug 1300464 - Avoid overriding security info with null. r=orange

	Josh Matthews [:jdm] Assignee
	Comment 32 • 8 years ago

Forgot to upload the patch version that null-checked mSecurityInfo before overriding it.

	Iris Hsiao [:ihsiao]
	Comment 33 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b56ad4440b05
https://hg.mozilla.org/mozilla-central/rev/dddb857bd3fd

	Gerry Chang [:gchang]
	Comment 34 • 8 years ago

Hi :jdm,
Since this bug is a regression, do you consider to uplift this for 51 at least if this patch is not too risky?

	Josh Matthews [:jdm] Assignee
	Comment 35 • 8 years ago

Comment on attachment 8794295 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Approval Request Comment
[Feature/regressing bug #]: ServiceWorkers + e10s
[User impact if declined]: Green lock icon does not appear for certain sites using ServiceWorkers that should otherwise be considered secure.
[Describe test coverage new/current, TreeHerder]: New test covering this case has been added.
[Risks and why]: Very low. Code changes are targeted and conservative.
[String/UUID change made/needed]:

	Gerry Chang [:gchang]
	Comment 36 • 8 years ago

Comment on attachment 8794295 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Fix a regression related to serviceworkers + e10s. Take it in 51 aurora.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 37 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/835338db2670
https://hg.mozilla.org/releases/mozilla-aurora/rev/eab6700676c1

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 38 • 8 years ago

Hi Josh, should we uplift this fix to Beta50?

	Josh Matthews [:jdm] Assignee
	Comment 39 • 8 years ago

Comment on attachment 8794295 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Same reasoning as before. Small change, with implications for how people trust the sites they use in certain cases.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 40 • 8 years ago

Hi Fanolian, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 41 • 8 years ago

Comment on attachment 8794295 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Regression fix, Beta50+

	Ryan VanderMeulen [:RyanVM]
	Comment 42 • 8 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/c4c9d891c4f9

	Fanolian Reporter
	Comment 43 • 8 years ago

(In reply to Ritu Kothari (:ritu) from comment #40)
> Hi Fanolian, could you please verify this issue is fixed as expected on a
> latest Nightly build? Thanks!

This issue is fixed in latest Nightly build. Thanks a lot.

I just find that Developer Tool has a similar issue regarding service workers (see attachment). Is it already covered by other bugs, e.g. bug 1231222? Or should I file a new one? Thanks.

	Josh Matthews [:jdm] Assignee
	Comment 44 • 8 years ago

That issue is worth filing separately.

	Fanolian Reporter
	Comment 45 • 8 years ago

(In reply to Josh Matthews [:jdm] from comment #44)
> That issue is worth filing separately.

I filed bug 1307784.