1055238 - [B2G][Browser] Self-Signed/Untrusted/Expired SSL certificate warnings are not displaying

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Jordan de Geus(JordanD)]

Attachment: logcat_20140818_1332.txt

Description:
When navigating to a website that has a untrusted/expired SSL certificate, users will observe the page loads to a blank screen and not display the SSL certificate.


Repro Steps:
1) Update a Flame to 20140818040201
2) Navigate to https://tv.eurosport.com
3) Observe lack of SSL warning

Actual:
Expired SSL certificate webpage is not displaying

Expected:
Webpage with warnings for the expired SSL certificate displays

Environmental Variables:
Device: Flame Master (319mb)
Build ID: 20140818040201
Gaia: aa8aace12d65956dd9525da5dac66e0d3b28597f
Gecko: 0aaa2d3d15cc
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Repro frequency: 3/3
Link to failed test case: https://moztrap.mozilla.org/manage/case/6757/
See attached: Screenshot and logcat

Comment 1

[Jordan de Geus(JordanD)]

Attachment: 2014-08-18-13-42-25.png

This issue DOES occur on Flame 2.1 (512mb) and Buri 2.1

Actual: Expired/untrusted SSL certificate warning page is not displaying

Flame 2.1 (512mb)

Environmental Variables:
Device: Flame Master
BuildID: 20140818040201
Gaia: aa8aace12d65956dd9525da5dac66e0d3b28597f
Gecko: 0aaa2d3d15cc
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Buri 2.1

Environmental Variables:
Device: Buri 2.1 Master
BuildID: 20140818073016
Gaia: ba1992f2addc5a84afc2eab426f222a6bf2962ba
Gecko: bf27e27c994d
Version: 34.0a1 (2.1 Master)
Firmware: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0


--------------------------------------------------------------------------------

This issue DOES NOT occur on Flame 2.0 (319mb), Flame 1.4 (319mb), Buri 2.1, Buri 2.0, Buri 1.4, Open C 2.0, and Open C 1.4

Actual: Expired SSL Certificate displays correctly

Flame 2.0 (319mb)

Device: Flame 2.0
BuildID: 20140818000201
Gaia: fb2dd31abed2803eb7ad67eb4c52abb48de1e0f7
Gecko: 09f7a7184c71
Version: 32.0 (2.0)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Buri 2.0

Environmental Variables:
Device: Buri 2.0
Build ID: 20140818063008
Gaia: 640ce38ca03f1e26a4524ff4215b8b3f7731e2f0
Gecko: 692c93509dc9
Version: 32.0 (2.0)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 2

[KTucker [:KTucker][Inactive 3/4/2016]]

Please correct your branch checks. You are missing environmental variables.

Comment 3

[Jordan de Geus(JordanD)]

Please ignore comment 1 as my environmental variables were not correct. 

This DOES OCCUR on Flame 2.1(512mb), Buri 2.1, Open C 2.1

Actual: Expired/untrusted SSL certificate warning page is not displaying

Flame 2.1 (512mb)

Environmental Variables:
Device: Flame Master 
BuildID: 20140820040203193
Gaia: df39c463259d348396ef7f143c2c780eeb8f02d8
Gecko: ffdd1a398105
Version: 34.0a1 (Master)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Buri 2.1

Environmental Variables:
Device: Buri Master
Build ID: 20140820073027
Gaia: 33d4b999f464fbad1c23d488da4689c5de9967ec
Gecko: cbbc380f1e1c
Version: 34.0a1 (Master)
Firmware Version: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Open C 2.1

Environmental Variables:
Device: Open_C Master
Build ID: 20140820040203
Gaia: df39c463259d348396ef7f143c2c780eeb8f02d8
Gecko: ffdd1a398105
Version: 34.0a1 (Master)
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

---------------------------------------------------------------------

This does NOT occur on Flame 2.0(319mb), Flame 1.4 (319mb), Buri 2.0, Open C 2.0

Flame 2.0 (319mb)

Environmental Variables:
Device: Flame 2.0 
BuildID: 20140820000201
Gaia: 88db39a0826086024631049d83ae6aa397f0918d
Gecko: 2092ac87eceb
Version: 32.0 (2.0)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Flame 1.4 (319mb)

Environmental Variables:
Device: Flame 1.4
Build ID: 20140820003001
Gaia: 4f92950e6d96326785a249e8acb704da3647616b
Gecko: e1de5a959089
Version: 30.0 (1.4)
Firmware Version: v123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Buri 2.0

Environmental Variables:
Device: Buri 2.0
BuildID: 20140819183013
Gaia: 88db39a0826086024631049d83ae6aa397f0918d
Gecko: 2092ac87eceb
Version: 32.0 (2.0)
Firmware: v1.2device.cfg
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Open C 2.0

Environmental Variables:
Device: Open_C 2.0
BuildID: 20140820000201
Gaia: 88db39a0826086024631049d83ae6aa397f0918d
Gecko: 2092ac87eceb
Version: 32.0 (2.0)
Firmware Version: P821A10V1.0.0B06_LOG_DL
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Comment 4

[KTucker [:KTucker][Inactive 3/4/2016]]

[Blocking Requested - why for this release]:

Possibly a dupe of bug 1055328

This is a regression from 2.0 and the page just loads to a blank page so nominating to block 2.1?

Comment 5

[Chris Kreinbring [:CKreinbring]]

Regression window
Last working
BuildID: 20140816143114
Gaia: 325f68045e7abacdd80f28cce53315d469e82469
Gecko: c3eb1b5ad4e4
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First broken
BuildID: 20140816144415
Gaia: 325f68045e7abacdd80f28cce53315d469e82469
Gecko: 94ba78a42305
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Working Gaia / Broken Gecko = Repro
Gaia: 325f68045e7abacdd80f28cce53315d469e82469
Gecko: 94ba78a42305
Broken Gaia / Working Gecko = No repro
Gaia: 325f68045e7abacdd80f28cce53315d469e82469
Gecko: c3eb1b5ad4e4
Gecko pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c3eb1b5ad4e4&tochange=94ba78a42305


Mozilla inbound
Last working
BuildID: 20140815111214
Gaia: d0d773c277a9105288ee35da2121f4ae62709be8
Gecko: fd08e61066de
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

First broken
BuildID: 20140815112915
Gaia: d0d773c277a9105288ee35da2121f4ae62709be8
Gecko: 229181c88a3c
Platform Version: 34.0a1
Firmware Version: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Working Gaia / Broken Gecko = Repro
Gaia: d0d773c277a9105288ee35da2121f4ae62709be8
Gecko: 229181c88a3c
Broken Gaia / Working Gecko = No repro
Gaia: d0d773c277a9105288ee35da2121f4ae62709be8
Gecko: fd08e61066de
Gecko pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=fd08e61066de&tochange=229181c88a3c

Comment 6

[Joshua Mitchell (Inactive)]

Broken by bug 1029155 - Garrett?

Comment 7

[Garrett Robinson [:grobinson]]

First of all, initial comment is incorrect - the test case URL's cert is not expired, it is invalid for the given domain.

Note: this test case wfm on desktop, and passed try when it landed a few weeks ago. I see a MozTrap test is failing - what are those, and why aren't they included in the results from try?

Anyway, I would guess the problem is here, where you guys re-implement certificate checks for error pages: http://dxr.mozilla.org/mozilla-central/source/b2g/components/ErrorPage.jsm#75 While this code has the potential to be incorrect (no guarantee that you'll get the same cert on a subsequent request to one that served a bad one), it should work most of the time. Specifically, you are checking whether the channel can provide an nsISSLStatus, and using that as the signal as to whether your connection succeeded or not. When I wrote the patch for bug 1029155, I took specific care not to disturb that (unfortunate) assumption - when I did, it broke unit tests in PSM. It seems strange that those tests would pass but the problem would occur elsewhere in the code. Perhaps there are some other differences in cert handling on B2G.

Anyway, I'll try to look into this more later but I am leaving Mozilla and today is my last day, so there are no guarantees. I'm going to punt this to keeler, who is familiar with PSM (sorry keeler).

Comment 8

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

Hmmm - looks like ErrorPage.jsm needs to be re-written in the style of bug 1025332 (i.e. pass in the failed channel and get its sslStatus - don't attempt an xhr to get the sslStatus). In general, I would be wary of modeling any new code on old code in security/manager/pki - there's a lot that needs to be updated/replaced there, and if we add new code that behaves like it, that just means more work.

Comment 9

[Jason Smith [:jsmith]]

Josh - Can you finish triaging the QAnalyst-Triage flag here?

Comment 10

[Gregor Wagner [:gwagner]]

doug, can you help with triage here and an assignee if we have to fix this for 2.1?

Comment 11

[Doug Turner (:dougt)]

See comment #7.

Comment 12

[Gregor Wagner [:gwagner]]

Thinker, can you guys take care of this bug?

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

I can take care after two days.

Comment 14

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

Actually, I was wrong about what was going on here. When loading a page, the parent notifies the child of events and includes a serialized security info structure. The child deserializes this. When this happens, if the serialization includes an nsIX509Cert, a different object is created than if it were in the parent process (see nsNSSCertificate vs nsNSSCertificateFakeTransport). This is because we don't want to initialize NSS in the child process (any more than we already do due to webrtc, etc., but that's another story), and so we basically create a fake certificate object. It can't do anything, really, but at least it won't cause NSS initialization (or, rather, cause assertion failures because it attempts to do so in the child process). When we added the failedCertChain (backed by nsIX509CertList/nsNSSCertList), we neglected to include a fake nsNSSCertList that could be created in the child process that would prevent assertions as a result of attempting NSS initialization.

Comment 15

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

(Note that this can be reproduced with e10s on desktop.)

Comment 16

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

Attachment: patch 1/2: some clean-up

This is just some clean-up I thought it would be nice to do while we're here.

Comment 17

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

Attachment: patch 2/2: nsNSSCertListFakeTransport

This is the actual fix.

Comment 18

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

https://tbpl.mozilla.org/?tree=Try&rev=b27955438b46

Comment 19

[Richard Barnes [:rbarnes]]

Comment on attachment 8487573 [details] [diff] [review]
patch 1/2: some clean-up

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

::: security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
@@ +113,3 @@
>  {
>    NS_NOTREACHED("Unimplemented on content process");
>    return NS_ERROR_NOT_IMPLEMENTED;

Seems like it would be handy to macro-ize these method bodies, but not essential.

Comment 20

[Richard Barnes [:rbarnes]]

Comment on attachment 8487575 [details] [diff] [review]
patch 2/2: nsNSSCertListFakeTransport

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

The only thing that was unclear to me was how an nsNSSCertListFakeTransport object gets created.  Keeler pointed out to me the "CONSTRUCTOR_BY_PROCESS" line in nsNSSModule.cpp, but I wonder if it might be helpful to have a comment somewhere to this effect.  (Or maybe it's obvious.)

::: security/manager/ssl/src/nsNSSCertificateFakeTransport.cpp
@@ +448,5 @@
> +
> +  for (size_t i = 0; i < certListLen; i++) {
> +    nsCOMPtr<nsIX509Cert> cert = mFakeCertList[i];
> +    nsCOMPtr<nsISerializable> serializableCert = do_QueryInterface(cert);
> +    rv = aStream->WriteCompoundObject(serializableCert, NS_GET_IID(nsIX509Cert),

Line break before NS_GET_IID?  This wraps in splinter at least.

@@ +482,5 @@
> +  return rv;
> +}
> +
> +NS_IMETHODIMP
> +nsNSSCertListFakeTransport::GetEnumerator(nsISimpleEnumerator**)

Might be a little more readable for these methods to be with the other nsIX509CertList methods above.

Comment 21

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

Attachment: patch 2/2: nsNSSCertListFakeTransport v2

Addressed comments and eventually got to pass try:
https://tbpl.mozilla.org/?tree=Try&rev=5e32b1f7b5d6
https://tbpl.mozilla.org/?tree=Try&rev=1b2391a430ae

Comment 22

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfdc5be13a2f
https://hg.mozilla.org/integration/mozilla-inbound/rev/89c373278e79

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/dfdc5be13a2f
https://hg.mozilla.org/mozilla-central/rev/89c373278e79

Comment 24

[Dietrich Ayala (:dietrich)]

Triage group reviewed, blocking+ because regression.

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora approval on this when you get a chance.

Comment 27

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

Comment on attachment 8490433 [details] [diff] [review]
patch 2/2: nsNSSCertListFakeTransport v2

Approval Request Comment
[Feature/regressing bug #]: bug 1029155
[User impact if declined]: certificate overrides are broken in b2g/e10s
[Describe test coverage new/current, TBPL]: there are tests for this in non-e10s, so we shouldn't break desktop cert overrides, but unfortunately we have no direct tests for this in b2g/e10s
[Risks and why]: low - I can't really think of a way this would make things worse
[String/UUID change made/needed]: none

Comment 28

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

Comment on attachment 8487573 [details] [diff] [review]
patch 1/2: some clean-up

Approval Request Comment
(see previous comment - this is just some limited cleanup and changes no functionality)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4b841e30bcd7
https://hg.mozilla.org/releases/mozilla-aurora/rev/ecad53dd6555

Comment 30

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame v2.1.MP4

This issue has been verified successfully on Flame 2.1.
See attachment: Verify_Video_Flame v2.1.MP4
Reproducing rate: 0/10

Flame v2.1 version:
Gaia-Rev        afdfa629be209dd53a1b7b6d6c95eab7077ffcd9
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/dc3018cbdbe6
Build-ID        20141123001201
Version         34.0

Comment 31

[Alissa（leave from Mozilla）]

Attachment: Verify_Video_Flame v2.2.3gp

Comment 32

[Alissa（leave from Mozilla）]

This issue has been verified as pass on latest build of Flame 2.2 and nexus5 2.2.

Repro Steps:
1) Update a Flame to 20140818040201
2) Navigate to https://tv.eurosport.com
3) Observe lack of SSL warning

Rate:0/5
Device: Flame2.2[pass]
Build ID               20150615162504
Gaia Revision          e7a0c6d5f4df04d45fb3f726efb9e8223600cb79
Gaia Date              2015-06-15 06:12:18
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8045028bf400
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150615.194936
Firmware Date          Mon Jun 15 19:49:47 EDT 2015
Bootloader             L1TC000118D0

Device: Nexus5 2.2[pass]
Build ID               20150615002503
Gaia Revision          e7a0c6d5f4df04d45fb3f726efb9e8223600cb79
Gaia Date              2015-06-15 06:12:18
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/f278c675d51f
Gecko Version          37.0
Device Name            hammerhead
Firmware(Release)      5.1
Firmware(Incremental)  eng.cltbld.20150615.040818
Firmware Date          Mon Jun 15 04:08:47 EDT 2015
Bootloader             HHZ12d