979474 - Telemetry experiments: Pin the experiment manifest fetch to a particular CA

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect, P2)

Description

[Benjamin Smedberg]

By default when fetching the telemetry experiment manifest, we should not only require valid SSL but also pin the manifest fetch to a particular CA the same way we do for AUS and AMO update checks.

Notes:
http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/CertUtils.jsm @see checkCert and http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/nsUpdateService.js#3719

It is not necessary to pin the XPI fetch because we're securing that using a hash (the channel might not even be HTTPS).

Comment 1

[Robert Kaiser]

Just as a comment, we probably should allow two or three root certs from different CAs as we do for AUS just so we can fall back if the primary one has some issue.

Comment 2

[Benjamin Smedberg]

I'm discussing that with webops. AUS is more important because breaking the pin means people can't update. Breaking this just means that we don't get new experiments until the next update, which is frequent enough for nightly/aurora/beta that I don't think it matters too much.

Comment 3

[Robert Kaiser]

Ah, yes, makes sense. Just wanted to ensure we think of this as we had pretty scary moments with AUS when the pinned certs ran thin a while ago - but of course, as long as AUS works we can update to a build with newer pinning (and as long as the add-on hotfix mechanism works, we can even deliver something to installed builds if really required).

Comment 4

[Benjamin Smedberg]

Attachment: Pin telemetry experiments, rev. 1

Comment 5

[Camilo Viecco (:cviecco)]

Comment on attachment 8390724 [details] [diff] [review]
Pin telemetry experiments, rev. 1

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

The logic is sane. Assuming you don’t care about protecting the outbound information and you are aware for the breakage risk.

::: browser/app/profile/firefox.js
@@ +1413,5 @@
>  pref("experiments.enabled", false);
>  pref("experiments.manifest.fetchIntervalSeconds", 86400);
> +pref("experiments.manifest.uri", "https://telemetry-experiment.cdn.mozilla.net/manifest/v1/firefox/%VERSION%/%CHANNEL%");
> +pref("experiments.manifest.certs.1.commonName", "*.cdn.mozilla.net");
> +pref("experiments.manifest.certs.1.issuerName", "CN=Cybertrust Public SureServer SV CA,O=Cybertrust Inc");

I am worried about this. Since a pinning failure would only make our experiments data dry up (and not affect any other part of the code) and there is a time requirement I am OK with landing this. If we really need to change this to something else, do you know how hard is to push an update to a pref to our release channel?

::: browser/experiments/Experiments.jsm
@@ +386,5 @@
>          deferred.reject(new Error("Experiments - XHR status for " + url + " is " + xhr.status));
>          return;
>        }
>  
> +      var certs = null;

let?

@@ +387,5 @@
>          return;
>        }
>  
> +      var certs = null;
> +      if (gPrefs.get("manifest.cert.checkAttributes", true)) {

use the const you defined above or remove the const

@@ +391,5 @@
> +      if (gPrefs.get("manifest.cert.checkAttributes", true)) {
> +        certs = CertUtils.readCertPrefs(PREF_BRANCH + "manifest.certs.");
> +      }
> +      try {
> +        var allowNonBuiltin = !gPrefs.get(PREF_BRANCH + "manifest.cert.requireBuiltin", true);

let

@@ +392,5 @@
> +        certs = CertUtils.readCertPrefs(PREF_BRANCH + "manifest.certs.");
> +      }
> +      try {
> +        var allowNonBuiltin = !gPrefs.get(PREF_BRANCH + "manifest.cert.requireBuiltin", true);
> +        CertUtils.checkCert(xhr.channel, allowNonBuiltin, certs);

Since you are checking for pinning here I think is it important to notice that the url/cookies/outbound headers/ post values/url parameters are not protected by this pinning mechanism.

Comment 6

[Camilo Viecco (:cviecco)]

How do you plan to test this?

Comment 7

[Benjamin Smedberg]

Automated tests for this really rely on bug 466524, so for now I'm doing manual testing and I'll file a followup for better automated testing.

Comment 8

[Georg Fritzsche [:gfritzsche]]

Attachment: Pin telemetry experiments, rev. 2

Rebased, minor fixup from above, rejection reason propagation restored.
bsmedberg, please take a look if you disagree with the propagation.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #8)
> rejection reason propagation restored.
> bsmedberg, please take a look if you disagree with the propagation.

Nevermind that part, wrong bug.

Comment 10

[Georg Fritzsche [:gfritzsche]]

Attachment: Pin telemetry experiments, rev. 3

Rebased.

Comment 11

[Georg Fritzsche [:gfritzsche]]

Attachment: Disable certificate checks for the tests.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8392035 [details] [diff] [review]
Disable certificate checks for the tests.

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

r+ with clearing the pref at the end of the tests

Comment 13

[Georg Fritzsche [:gfritzsche]]

Attachment: Disable certificate checks for the tests.

Pref cleanup, consolidation in header.js.

Comment 14

[Georg Fritzsche [:gfritzsche]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9d5b62a13de0
https://hg.mozilla.org/integration/mozilla-inbound/rev/887a2b5a7d42

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9d5b62a13de0
https://hg.mozilla.org/mozilla-central/rev/887a2b5a7d42

Comment 16

[Chris Turra [:cturra]]

Comment on attachment 8392034 [details] [diff] [review]
Pin telemetry experiments, rev. 3

clearing feedback.