1343609 - adapt the telemetry experiment from bug 1311479 to identify misissued certificates for mozilla properties

Status: RESOLVED DUPLICATE of bug 1346017

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

Description

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

In the process of disabling sha-1, we have been gathering telemetry data to answer the question of if the policy of "disable sha-1 except if the root certificate is not part of the web pki" would cause compatibility issues for users behind TLS intercepting proxies. Our assumption was that it would not, because if an intercepting proxy were issuing certificates that chained to a root in the web pki, such certificates would almost certainly be misissuances.

To gather the data, the telemetry experiment (and later the sha-1 disabling add-on) would attempt to contact telemetry.mozilla.org and see what certificate chain it was using. For each certificate in the chain, it would send back the sha256 hash and whether or not the certificate was a built-in root (i.e. part of the web pki), as well as whether or not the chain validated correctly for the host telemetry.mozilla.org.

Given this data, we have found a handful of cases where a user encountered a certificate chain purportedly sent by telemetry.mozilla.org that validated correctly from a root in the web pki yet differed from the real telemetry.mozilla.org. Given only the hashes of the certificates, we have been unable to identify the corresponding certificates in public data sets (e.g. CT/censys, etc.). Additionally, the issuing CA has indicated that they have no record of at least one of these certificates (as of this writing, we haven't asked about the others yet).

In order to determine if these certificates were misissued, we need to know what they are. Furthermore, we should attempt to identify other similar situations for other mozilla properties. To that end, we should extend the telemetry experiment from bug 1311479 to have it check a list of mozilla properties and send back the entire certificate chain if it identifies a possible misissuance.

Comment 1

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

Attachment: 1343609-certificate-checker.diff

Here's an initial approach - let me know what you think.

Comment 2

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8842638 [details] [diff] [review]
1343609-certificate-checker.diff

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

::: experiments/certificate-checker/code/bootstrap.js
@@ +56,5 @@
>                  .createInstance(Ci.nsIXMLHttpRequest);
>      req.open("GET", "https://" + hostname);
>      req.timeout = 30000;
>      req.addEventListener("error", (evt) => {
> +      let result = { hostname, error: "connection error" };

Might as well toss this object into the resolve() directly, like we do for timeout

@@ +101,5 @@
> +    // what we sould send if we encountered something we thought was wrong.
> +    "b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c",
> +    "154c433c491929c5ef686e838e323664a00e6a0d822ccc958fb4dab03e49a08f",
> +    "4348a0e9444c78cb265e058d5e8944b4d84f9662bd26db257f8934a443c70161"
> +  ],

Yup, this looks about right for me. We should pow-wow about what hosts to include. I'll start a list.

::: experiments/certificate-checker/code/install.rdf
@@ +11,5 @@
>      <em:targetApplication>
>        <Description>
>          <em:id>{5428e386-68cf-4283-8ee3-04c12d3f4f4e}</em:id>
>          <em:minVersion>50.0</em:minVersion>
>          <em:maxVersion>51.0</em:maxVersion>

We'll need to crank this up to 54.0 methinks

Comment 3

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

Attachment: patch

I set the versions to 51-55 since Nightly will almost certainly be 55 before we ship this.

Comment 4

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

Comment on attachment 8843354 [details] [diff] [review]
patch

data-review? to :bsmedberg.

Comment 5

[Benjamin Smedberg]

Comment on attachment 8843354 [details] [diff] [review]
patch

data-review=me. I did not review the code.

Comment 6

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8843354 [details] [diff] [review]
patch

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

Please check that error case I noted. Thanks!

::: experiments/certificate-checker/code/bootstrap.js
@@ +191,5 @@
> +  return Promise.all(promises);
> +}
> +
> +function analyzeAndReport(results) {
> +  let payload = { mismatches: [] };

Maybe we should stick a version marker in here? It would have been nice to know v1.2 vs 1.3 of the last addon.

@@ +195,5 @@
> +  let payload = { mismatches: [] };
> +  for (let result of results) {
> +    // Skip if the connection resulted in any kind of error.
> +    if ("error" in result) {
> +      console.log(`${result.hostname}: ${result.error} - skipping`);

This looks like it won't work; the objects constructed in the error cases don't define `hostname: `... on line 61/64/etc.

Comment 7

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

Attachment: patch v1.1

(In reply to J.C. Jones [:jcj] from comment #6)

Thanks!

> ::: experiments/certificate-checker/code/bootstrap.js
> @@ +191,5 @@
> > +  return Promise.all(promises);
> > +}
> > +
> > +function analyzeAndReport(results) {
> > +  let payload = { mismatches: [] };
> 
> Maybe we should stick a version marker in here? It would have been nice to
> know v1.2 vs 1.3 of the last addon.

Sure - I added a version field and set it to "1.0" (I'm carrying over data-review on this, since I'm assuming that adding a field that is the same across all responses doesn't have any privacy implications).

> @@ +195,5 @@
> > +  let payload = { mismatches: [] };
> > +  for (let result of results) {
> > +    // Skip if the connection resulted in any kind of error.
> > +    if ("error" in result) {
> > +      console.log(`${result.hostname}: ${result.error} - skipping`);
> 
> This looks like it won't work; the objects constructed in the error cases
> don't define `hostname: `... on line 61/64/etc.

Constructing an object like { hostname } is shorthand for { hostname: hostname }, so this should work (and I verified that it does in the error case).

Comment 8

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

Comment on attachment 8843414 [details] [diff] [review]
patch v1.1

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

Whoops I guess you didn't actually r+ that.

Comment 9

[J.C. Jones [:jcj] (he/him)]

Comment on attachment 8843414 [details] [diff] [review]
patch v1.1

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

Cool, LGTM. Also, that kind of JS initialization seems like cheating. :)

Comment 10

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

Attachment: experiment.xpi

Jason, if you could sign this experiment, that would be great - thanks!

Comment 11

[Jason Thomas [:jason]]

Attachment: experiment.xpi signed

Please see attached.

Comment 12

[J.C. Jones [:jcj] (he/him)]

Historical note, the SHA256-sums of the end-entity certificates we observed were:

d8cdca300ff9920c79335936c89f6947cfdb8cbeede604ed77d15fa3bf91a04b
7f8b25ddb928cfd0c65755ce1a60a73d7e0d81f2753f21e567d3f27acee8841b
b630beb62bbdf7cac7185a843140a92b8ba9959e40bb1ece4dbdfbc099a53c1f
7e0cc1c93c357ee17ff968ae5f6485c5a9c4981d0953ecd452775666d2f70983
c5e0aecdd348b9f34bd3b3bdecc21846ab2df9865c939a1fd0bf37f2a6bd887e
9ec14a5506b68c201bfb9afe077099457c6c5d3332640ed31a6ef64836d87d69
9735baed4c71b963eaaf3516b6c38b6c354d66150018af3a4d2ca3aa00a1a9dc
26e5d7a18caa7bab6c9ae7e69d03818231146d0e559c300a30851ba90e4d8f2b
65f924b4ca63aba96e433d916e2133bde502a855e41eaac9f0cb0746e941c486
71f15da582c2bc81b019cf5a2cc03a5cfefd61d58891c62940371b0b9b24e1b6

All of these chained up through this intermediate:
154c433c491929c5ef686e838e323664a00e6a0d822ccc958fb4dab03e49a08f

To this root:
4348a0e9444c78cb265e058d5e8944b4d84f9662bd26db257f8934a443c70161

None of the above end-entity certificates have been found yet; the issuer doesn't believe they exist, nor do they appear in Censys or CT.

Comment 13

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

This ended up being developed in bug 1346017. Analysis here: https://people-mozilla.org/~dkeeler/deployment-checker-analysis.html
Long story short, the most plausible explanation we came up with is that bad RAM caused flipped bits and resulted in mismatched hashes. There never were any misissued certificates.