1300464 - [e10s] WhatsApp Web (HTTP) redirects to HTTPS but shows as not secure

Status: VERIFIED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Fanolian]

Attachment: Whatsapp Web shows as Not Secure

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

Comment 1

[Fanolian]

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

Comment 2

[Fanolian]

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

Comment 3

[Alice0775 White]

[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.

Comment 4

[Fanolian]

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

Comment 5

[Alice0775 White]

[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

Comment 6

[(no longer active)]

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

Comment 7

[Jason Duell]

I'm hoping Honza can jump in here.

Comment 8

[Daniel Stenberg [:bagder]]

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?

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

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?

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 11

[Honza Bambas (:mayhemer)]

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/

Comment 12

[Josh Matthews [:jdm]]

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

Comment 13

[Jim Mathies [:jimm]]

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

Comment 14

[Josh Matthews [:jdm]]

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.

Comment 15

[Jim Mathies [:jimm]]

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

Comment 16

[(no longer active)]

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...

Comment 17

[(no longer active)]

(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.  :(

Comment 18

[Josh Matthews [:jdm]]

Attachment: Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

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

Comment 19

[Josh Matthews [:jdm]]

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

Comment 20

[Josh Matthews [:jdm]]

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.

Comment 21

[Josh Matthews [:jdm]]

Attachment: Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Comment 22

[Josh Matthews [:jdm]]

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.

Comment 23

[Honza Bambas (:mayhemer)]

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?

Comment 24

[Josh Matthews [:jdm]]

Attachment: Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

Comment 25

[Josh Matthews [:jdm]]

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.

Comment 26

[Josh Matthews [:jdm]]

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

Comment 27

[Josh Matthews [:jdm]]

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

Comment 28

[Josh Matthews [:jdm]]

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.

Comment 29

[Honza Bambas (:mayhemer)]

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!

Comment 30

[Pulsebot]

Pushed by [e-mail]:
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

Comment 31

[Josh Matthews [:jdm]]

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

Comment 32

[Josh Matthews [:jdm]]

Attachment: Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected

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

Comment 33

[Iris Hsiao [:ihsiao]]

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

Comment 34

[Gerry Chang [:gchang]]

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?

Comment 35

[Josh Matthews [:jdm]]

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]:

Comment 36

[Gerry Chang [:gchang]]

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.

Comment 37

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

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

Comment 38

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Josh, should we uplift this fix to Beta50?

Comment 39

[Josh Matthews [:jdm]]

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.

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 41

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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+

Comment 42

[Ryan VanderMeulen [:RyanVM]]

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

Comment 43

[Fanolian]

Attachment: Dev_tool_service_worker.PNG

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

Comment 44

[Josh Matthews [:jdm]]

That issue is worth filing separately.

Comment 45

[Fanolian]

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

I filed bug 1307784.