adjust or remove updater cert pinning

The updater uses a couple of hidden prefs. to do its own form of cert pinning. But changes are afoot on the server side; see https://bugzilla.mozilla.org/show_bug.cgi?id=1219185

Here are the hidden prefs we currently use inside Tor Browser:

pref("app.update.certs.1.issuerName", "CN=DigiCert SHA2 High Assurance Server CA,OU=www.digicert.com,O=DigiCert Inc,C=US");
pref("app.update.certs.1.commonName", "*.torproject.org");

These prefs are consulted when the update code connects to https://www.torproject.org

I am not an expert in this area, but it seems like it might be better to just disable the updater-specific checks that use the above prefs. and instead rely on the more general pinning that is defined inside security/manager/boot/src/StaticHPKPins.h (when we added these updater prefs, we did not yet have the more general form of pinning in place).

added TorBrowserTeam201604 component::applications/tor browser owner::tbb-team priority::medium resolution::fixed severity::normal status::closed type::defect labels

Trac:
Cc: weasel, mikeperry to weasel, mikeperry, gk

Trac:
Status: new to needs_information

Yes, we should get rid of that part. FWIW: Mozilla already did the same https://bugzilla.mozilla.org/show_bug.cgi?id=1151485 and plans to remove the custom checks code in general, now that they have signed MAR files on all platforms: https://bugzilla.mozilla.org/show_bug.cgi?id=1182352. It is worth noting, too, that there are voices that think pinning (esp. the strict mode we enforce) is not the ideal thing for the updater if one has already signed MAR files, see e.g.: https://bugzilla.mozilla.org/show_bug.cgi?id=1063111#c3.

Trac:
Status: needs_information to assigned

It does sound like this update-specific pin is redundant to (and weaker than) the HPKP pins. However, I very much disagree with https://bugzilla.mozilla.org/show_bug.cgi?id=1063111#c3. We should keep an eye on that and make sure that the HPKP pins always apply to the updater, as we do not have the problem of needing to support corporate or OEM-installed MITMs (cough Lenovo Superfish cough).

For us, continuing to enforce HPKP for the updater ensures that the adversary must compromise both the current MAR signing key and the webserver cert in order to give users bad updates. This is a much better position for us to be in than having there be a single point of security failure for compromising users during update.

Replying to mikeperry:

It does sound like this update-specific pin is redundant to (and weaker than) the HPKP pins. However, I very much disagree with https://bugzilla.mozilla.org/show_bug.cgi?id=1063111#c3. We should keep an eye on that and make sure that the HPKP pins always apply to the updater, as we do not have the problem of needing to support corporate or OEM-installed MITMs (cough Lenovo Superfish cough).

Yes, I think we get this already with our security.cert_pinning.enforcement_level set to 2 but looking closer might be good (Especially as Mozilla seems to pin the certificate for the Firefox updater, too.)

It turns out that we can take care of this by backporting a couple of Mozilla patches. Please review the most recent two commits on the bug17442-01 branch within the brade/tor-browser repo, here: https://gitweb.torproject.org/user/brade/tor-browser.git/log/?h=bug17442-01

Trac:
Status: assigned to needs_review
Keywords: N/A deleted, TorBrowserTeam201511R added

The backported patches look good to me (you even made sure all the typos stayed in place ;) ). I think this is fine for the alpha and I applied them to tor-browser-38.4.0esr-5.5-1 (commits c429e391927b9f6462274c5a7b51cf66cd253ddf and f90a87efb57f9e2fd7f3b23e812af721f092a733).

Would you look into whether we are fine with pinning the certs for the updater as well given that Mozilla is pinning them, too, but is still claiming they don't want the update breaking if MITM proxies are tampering with TLS?

Trac:
Status: needs_review to needs_information

Replying to gk:

Would you look into whether we are fine with pinning the certs for the updater as well given that Mozilla is pinning them, too, but is still claiming they don't want the update breaking if MITM proxies are tampering with TLS?

Kathy and I looked at this a little bit. The aus4.mozilla.org pin configuration has the mTestMode flag set to true (this is also the case for aus5.m.o on mozilla-central; they seem to have switched their update URLs to aus5 now). The mTestMode == true means that unless security.cert_pinning.enforcement_level is set to 3, would-be failures are ignored and just reported via Mozilla's telemetry service. So I think they are just gathering data on potential failures.

Trac:
Keywords: TorBrowserTeam201511R deleted, TorBrowserTeam201512R added

Can we resolve this ticket as fixed or are there still concerns that we need to address?

I think this is important enough for warranting s test which makes sure we really get the pinning we want. This can probably happen in a follow-up ticket or here if you think. Did you specifically look at what security.cert_pinning.enforcement_level set to 2 does and that this does not have holes which can bypass pinning? That was the other things that held me back from closing this ticket.

Carrying over reviews.

Trac:
Keywords: TorBrowserTeam201512R deleted, TorBrowserTeam201601R added

Carrying over review tickets.

Trac:
Keywords: TorBrowserTeam201601R deleted, TorBrowserTeam201602R added

Trac:
Keywords: TorBrowserTeam201602R deleted, TorBrowserTeam201603R added

No easy way to do reviews in March anymore.

Trac:
Keywords: TorBrowserTeam201603R deleted, TorBrowserTeam201604R added

I filed #18912 (moved) to track the remaining work (add tests).

Trac:
Reviewer: N/A to N/A
Keywords: TorBrowserTeam201604R deleted, TorBrowserTeam201604 added
Resolution: N/A to fixed
Status: needs_information to closed

closed

mentioned in issue #17662 (moved)

mentioned in issue #18912 (moved)

mentioned in issue #19481 (moved)

mentioned in issue tpo/applications/tor-browser-bundle-testsuite#17662

moved to tpo/applications/tor-browser#17442 (closed)

mentioned in issue tpo/applications/tor-browser#18912 (closed)