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

VERIFIED FIXED in Firefox 50

Status

()

Core
DOM: Service Workers
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: Fanolian, Assigned: jdm)

Tracking

({regression, reproducible, ux-consistency})

47 Branch
mozilla52
regression, reproducible, ux-consistency
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(e10s+, firefox48 wontfix, firefox49+ wontfix, firefox50+ fixed, firefox51+ fixed, firefox52 fixed)

Details

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8788057 [details]
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
(Reporter)

Comment 1

2 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
Has STR: --- → yes
Keywords: regression, regressionwindow-wanted, reproducible
(Reporter)

Comment 2

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

Comment 3

2 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.
Blocks: 516752
Status: UNCONFIRMED → NEW
status-firefox48: --- → wontfix
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
tracking-e10s: --- → ?
Ever confirmed: true
Keywords: ux-consistency
(Reporter)

Comment 4

2 years ago
Thanks Alice. I can confirm 48.0.2 Release is also affected if e10s is force-enabled.
(Reporter)

Updated

2 years ago
Summary: WhatsApp Web (HTTP) redirects to HTTPS but shows as not secure → [e10s] WhatsApp Web (HTTP) redirects to HTTPS but shows as not secure

Comment 5

2 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
Blocks: 1214305
tracking-firefox49: --- → ?
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
Flags: needinfo?(ehsan)
Keywords: regressionwindow-wanted

Comment 6

2 years ago
Andrew, can you please find a Necko owner for this?  I'm busy with bug 1297687 and friends.  Thanks!
Flags: needinfo?(ehsan) → needinfo?(overholt)
I'm hoping Honza can jump in here.
Flags: needinfo?(honzab.moz)
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?
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?
tracking-firefox49: ? → +
tracking-firefox50: ? → +
tracking-firefox51: ? → +
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.
status-firefox49: affected → fix-optional
Flags: needinfo?(overholt)
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/
Component: Networking → DOM: Service Workers
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 12

2 years ago
This would be fixed by bug 1231222. It's not clear if we should prioritize fixing this separately.
(Assignee)

Updated

2 years ago
Depends on: 1231222

Comment 13

2 years ago
We would very much like to get this fixed asap, Josh can you prioritize your work in the parent bug?
tracking-e10s: ? → +
Flags: needinfo?(josh)
Priority: -- → P1
(Assignee)

Comment 14

2 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.
Flags: needinfo?(josh)

Comment 15

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

Comment 16

2 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...
Flags: needinfo?(josh)

Comment 17

2 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.  :(
(Assignee)

Comment 18

2 years ago
Created attachment 8789552 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Assignee: nobody → josh
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Flags: needinfo?(josh)
(Assignee)

Comment 20

2 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.
(Assignee)

Comment 21

2 years ago
Created attachment 8790422 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected
(Assignee)

Updated

2 years ago
Attachment #8789552 - Attachment is obsolete: true
(Assignee)

Comment 22

2 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.
Attachment #8790422 - Flags: review?(honzab.moz)
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?
Attachment #8790422 - Flags: review?(honzab.moz) → feedback+
(Assignee)

Comment 24

2 years ago
Created attachment 8792053 [details] [diff] [review]
Propagate redirecting channel's security information to new HTTP channels when intercepted channels are redirected
Attachment #8792053 - Flags: review?(honzab.moz)
(Assignee)

Updated

2 years ago
Attachment #8790422 - Attachment is obsolete: true
(Assignee)

Comment 25

2 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.
Attachment #8792053 - Flags: review?(honzab.moz)
(Assignee)

Comment 28

2 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.
Attachment #8792053 - Flags: review?(honzab.moz)
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!
Attachment #8792053 - Flags: review?(honzab.moz) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 30

2 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
Keywords: checkin-needed
(Assignee)

Comment 32

2 years ago
Created attachment 8794295 [details] [diff] [review]
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.
(Assignee)

Updated

2 years ago
Attachment #8792053 - Attachment is obsolete: true

Comment 33

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b56ad4440b05
https://hg.mozilla.org/mozilla-central/rev/dddb857bd3fd
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
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?
Flags: needinfo?(josh)
(Assignee)

Comment 35

2 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]:
Flags: needinfo?(josh)
Attachment #8794295 - Flags: approval-mozilla-aurora?
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.
Attachment #8794295 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Hi Josh, should we uplift this fix to Beta50?
Flags: needinfo?(josh)
(Assignee)

Comment 39

2 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.
Flags: needinfo?(josh)
Attachment #8794295 - Flags: approval-mozilla-beta?
Hi Fanolian, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(Fanolian+bugzilla)
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+
Attachment #8794295 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 42

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/c4c9d891c4f9
status-firefox50: affected → fixed
Flags: in-testsuite+
(Reporter)

Comment 43

2 years ago
Created attachment 8797981 [details]
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.
Flags: needinfo?(Fanolian+bugzilla)
(Reporter)

Updated

2 years ago
Status: RESOLVED → VERIFIED
(Assignee)

Comment 44

2 years ago
That issue is worth filing separately.
(Reporter)

Comment 45

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

I filed bug 1307784.
status-firefox49: fix-optional → wontfix
You need to log in before you can comment on or make changes to this bug.