1465562 - sslStatus.succeededCertChain is null in CertUtils.jsm download xpi from github

Status: VERIFIED FIXED

(Core :: Security: PSM, defect, P1)

Description

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

STR: Visit https://perf-html.io/, click "Install Add-On", click "Allow"
Console says:

1527705044845	addons.xpi	WARN	Download of https://raw.githubusercontent.com/devtools-html/Gecko-Profiler-Addon/master/gecko_profiler.xpi failed: TypeError: sslStatus.succeededCertChain is null (resource://gre/modules/CertUtils.jsm:156:7) JS Stack trace: [e-mail]:156:7

This is probably directly a result of the changes in bug 1454504, but ultimately because the cert chain isn't getting set after certificate validation for some reason... (session resumption?)

Comment 1

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

Hmmm - I went to have lunch and now I can't reproduce this :(
Maybe it really was a network issue? If so, it seems we shouldn't access succeededCertChain without checking that it's non-null first.

Comment 3

[:Harald Kirschner :digitarald]

For testpilot, install fails from https://color.firefox.com/ and works from https://testpilot.firefox.com/

Comment 4

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

If you can still reproduce this, can you capture as much detail about the network connection as possible? (e.g. is it a tls session resumption? is it being loaded from the cache? http2/alt svc maybe?)

Comment 5

[:Harald Kirschner :digitarald]

It did work for me on a fresh profile.

On the profile where it reproduced before, I uninstalled the Color extension and tried again. Got a connection failure.

Request

Host: testpilot.firefox.com
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8
Accept-Language: en,en-GB;q=0.8,en-US;q=0.6,de;q=0.4,es;q=0.2
Accept-Encoding: gzip, deflate, br
Referer: https://color.firefox.com/
Cookie: _ga=GA1.2.1102208990.1497461978
DNT: 1
Connection: keep-alive
Upgrade-Insecure-Requests: 1

Response

HTTP/2.0 200 OK
content-type: application/x-xpinstall
content-length: 234425
accept-ranges: bytes
cache-control: max-age=30
date: Tue, 05 Jun 2018 04:52:09 GMT
etag: "80c3c342542f2380d864a2c649619a80"
last-modified: Mon, 04 Jun 2018 21:37:20 GMT
public-key-pins: max-age=5184000; pin-sha256="WoiWRyIOVNa9ihaBciRSC7XHjliYS9VwUGOIud4PB18="; pin-sha256="r/mIkG3eEpVdm+u/ko/cwxzOMo1bk4TyHIlByibiA5E="; pin-sha256="YLh1dUR9y6Kja30RrAn7JKnbQG/uEtLMkBgFF2Fuihg="; pin-sha256="sRHdihwgkaib1P1gxX8HFszlD+7/gTfNvuAybgLPNis="
strict-transport-security: max-age=31536000; includeSubDomains; preload
x-content-type-options: nosniff
x-cache: RefreshHit from cloudfront
via: 1.1 c7b4131244863241121573ea02fc44ad.cloudfront.net (CloudFront)
x-amz-cf-id: Rw65_DiaHh_BOTsclbF704K17MhO8EpvJyT5Vi4U-gzSyJf1BMmi3g==
X-Firefox-Spdy: h2

CURL

curl 'https://testpilot.firefox.com/files/[e-mail]/signed-addon.xpi' -H 'User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:62.0) Gecko/20100101 Firefox/62.0' -H 'Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8' -H 'Accept-Language: en,en-GB;q=0.8,en-US;q=0.6,de;q=0.4,es;q=0.2' --compressed -H 'Referer: https://color.firefox.com/' -H 'Cookie: _ga=GA1.2.1102208990.1497461978' -H 'DNT: 1' -H 'Connection: keep-alive' -H 'Upgrade-Insecure-Requests: 1'

Comment 6

[aries.nah]

Hello, 
I'm having the same issue on 61.0b11 with my own extension. I tested on 2 sites: 1st one uses InstallTrigger.install() method and works well in both r60, r61b11; 2nd one uses location.href= for installation of xpi file it works in r60 but doesn't in r61b11.
Is it better to create a separate bug or I can use this one for adding whatever you need for investigation?

Comment 7

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

Attachment: bug 1465562 - ensure succeededCertChain is set in TLS handshakes with session resumption

When doing TLS session resumption, Firefox currently does not have enough
information to trivially reconstitute the original connection's security
information. Consequently, we have to rebuild the certificate chain in the
handshake callback. Before this patch, we determined the EV and CT status of the
connection but did not set the succeeded cert chain unless the certificate was
EV. This was insufficient. In this patch, we set the succeeded cert chain
regardless of if the certificate is EV or not (provided we found a valid chain).

Review commit: https://reviewboard.mozilla.org/r/250036/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250036/

Comment 8

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

(In reply to aries.nah from comment #6)
> Hello, 
> I'm having the same issue on 61.0b11 with my own extension. I tested on 2
> sites: 1st one uses InstallTrigger.install() method and works well in both
> r60, r61b11; 2nd one uses location.href= for installation of xpi file it
> works in r60 but doesn't in r61b11.
> Is it better to create a separate bug or I can use this one for adding
> whatever you need for investigation?

Let's just use one bug for now - if fixing this bug doesn't address what you're seeing, feel free to open a new one.

Comment 9

[Franziskus Kiefer [:franziskus]]

Comment on attachment 8984248 [details]
bug 1465562 - ensure succeededCertChain is set in TLS handshakes with session resumption

https://reviewboard.mozilla.org/r/250036/#review256498

lgtm

Comment 10

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

Comment on attachment 8984248 [details]
bug 1465562 - ensure succeededCertChain is set in TLS handshakes with session resumption

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250036/diff/1-2/

Comment 11

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

Thanks!
The test wasn't quite right on windows for some reason: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b19c59e8ca6c5d893b66ca4b1734a04ebb2f2ad2
So I rearranged it slightly: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6b9610948af6219b58ce4ebc93283989b7b49d4

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b65a8f99622
ensure succeededCertChain is set in TLS handshakes with session resumption r=fkiefer

Comment 13

[Narcis Beleuzu [:NarcisB]]

Backed out for Android mochitest failures on test_cache_padding.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/3a3bbb2e68ef7429368c347c53b4ac819b5db6a0
Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=182582018&repo=autoland&lineNumber=2039

Comment 14

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

I believe that test may have already been faulty and we just happened to trigger a particular failure case for it. I'm investigating in bug 1468434.

Comment 15

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

I'm fairly confident that the test failure on android is due to a preexisting issue in the dom fetch/cache implementation (see bug 1468434 comment 6). I spoke with :RyanVM and we agreed that re-landing this with test_cache_padding.html disabled on android for now is a reasonable approach given that it would be best to be able to uplift this before the release candidate on Monday.

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

Comment 16

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

Comment on attachment 8984248 [details]
bug 1465562 - ensure succeededCertChain is set in TLS handshakes with session resumption

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250036/diff/2-3/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5d47226b6b8c
ensure succeededCertChain is set in TLS handshakes with session resumption r=fkiefer

Comment 18

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/5d47226b6b8c

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Beta approval when you get a chance.

Comment 20

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

Attachment: patch for beta

Approval Request Comment
[Feature/Bug causing the regression]: bug 1454504
[User impact if declined]: Users won't be able to install add-ons hosted on servers with non-EV certificates that use TLS session resumption.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very
[Why is the change risky/not risky?]: this is a small change that should make our security state tracking for TLS session resumption handshakes behave more like full handshakes (so this is almost certainly not a wrong thing to do)
[String changes made/needed]: none

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8985634 [details] [diff] [review]
patch for beta

Fix for broken extension installation from servers with certain security configurations. Includes new automated tests. Approved for 61.0rc1.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 23

[laszlo.bialis]

Tried to reproduce on an older version of Nightly v.62.0a1 Build ID: 20180530100110 and also on the latest Nightly 62.0a1 Build ID: 20180618220118 following the steps described in the description but couldn't reproduce on either of the versions.

Tried also on Beta 61.0b14 Build ID: 20180614135649 on Win 10/64 but couldn't install the add-on from https://perf-html.io/ after I clicked on "Allow" button I've received an error that stated that "the add-on could not be downloaded because of a connection failure." however there were no connection issues (other pages worked fine) and there was no error shown on the console page.

Comment 24

[laszlo.bialis]

Update:  I've retested the above issue on the same Beta version with a new profile and couldn't reproduce anymore. I think the problem was related with the profile I've used previously.

Comment 25

[laszlo.bialis]

Hi [:keeler]

Given the fact that I'm not able to reproduce the issue at all, on either of the browser versions, please take a look at this fix on the latest build to make sure that everything is ok.

Comment 26

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

I reproduced this on 61b10 and nightly 62 20180522100110 using https://color.firefox.com and couldn't reproduce it on the most recent 61 beta or 62 nightly so I think we're good.