Opened 4 years ago

Closed 3 years ago

#17442 closed defect (fixed)

adjust or remove updater cert pinning

Reported by: mcs Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: TorBrowserTeam201604
Cc: weasel, mikeperry, gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

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).

Child Tickets

Change History (16)

comment:1 Changed 4 years ago by gk

Cc: gk added

comment:2 Changed 4 years ago by mcs

Status: newneeds_information

comment:3 Changed 4 years ago by gk

Status: needs_informationassigned

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.

comment:4 Changed 4 years ago by 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*).

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.

comment:5 in reply to:  4 Changed 4 years ago by gk

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.)

comment:6 Changed 4 years ago by mcs

Keywords: TorBrowserTeam201511R added
Status: assignedneeds_review

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

comment:7 Changed 4 years ago by gk

Status: needs_reviewneeds_information

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?

comment:8 in reply to:  7 Changed 4 years ago by mcs

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.

comment:9 Changed 3 years ago by mikeperry

Keywords: TorBrowserTeam201512R added; TorBrowserTeam201511R removed

comment:10 Changed 3 years ago by mcs

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

comment:11 Changed 3 years ago by gk

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.

comment:12 Changed 3 years ago by gk

Keywords: TorBrowserTeam201601R added; TorBrowserTeam201512R removed

Carrying over reviews.

comment:13 Changed 3 years ago by gk

Keywords: TorBrowserTeam201602R added; TorBrowserTeam201601R removed

Carrying over review tickets.

comment:14 Changed 3 years ago by gk

Keywords: TorBrowserTeam201603R added; TorBrowserTeam201602R removed

comment:15 Changed 3 years ago by gk

Keywords: TorBrowserTeam201604R added; TorBrowserTeam201603R removed

No easy way to do reviews in March anymore.

comment:16 Changed 3 years ago by mcs

Keywords: TorBrowserTeam201604 added; TorBrowserTeam201604R removed
Resolution: fixed
Status: needs_informationclosed

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

Note: See TracTickets for help on using tickets.