Opened 6 years ago

Closed 6 years ago

#11955 closed enhancement (fixed)

Backport Certificate Pinning to FF31ESR

Reported by: mikeperry Owned by: arthuredelstein
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Keywords: TorBrowserTeam201410D, tbb-firefox-patch, MikePerry201410R, tbb-4.5-alpha
Cc: gk Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Certificate pinning did not make it into Firefox 31ESR, however the patches should be an easy backport. We should take Mozilla's patches for this when we rebase to FF31.

Relevant Mozilla bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=744204
https://bugzilla.mozilla.org/show_bug.cgi?id=772756

Child Tickets

Attachments (1)

0001-Bug-11955-Backport-certificate-pinning.patch (260.1 KB) - added by arthuredelstein 6 years ago.

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by gk

Cc: gk added

comment:2 Changed 6 years ago by erinn

Keywords: tbb-firefox-patch added

comment:3 Changed 6 years ago by erinn

Component: Firefox Patch IssuesTor Browser
Owner: changed from mikeperry to tbb-team

comment:4 Changed 6 years ago by mikeperry

Keywords: TorBrowserTeam201409 added
Owner: changed from tbb-team to arthuredelstein
Status: newassigned

comment:5 Changed 6 years ago by arthuredelstein

I was able to backport the patches in Mozilla Bug 744204, but there seem to be many relevant patches that we'll need to sort through. See Mozilla Bug 1004350 ("Pin all the things").

comment:6 Changed 6 years ago by mikeperry

Sitting with Camilo right now. The "Pin all the things" bug is just about updating the json list with new sites. For now, we should just focus on getting this to work for our updater and addons.mozilla.org, and can add a couple sites later.

In terms of actual patches, we want:
https://bugzilla.mozilla.org/show_bug.cgi?id=744204
https://bugzilla.mozilla.org/show_bug.cgi?id=772756
https://bugzilla.mozilla.org/show_bug.cgi?id=1002696
https://bugzilla.mozilla.org/show_bug.cgi?id=1009635

There was a regression that should be fixed in the patch set for 772756 that broke the addons pane. We should verify our backport doesn't suffer from it either (note this ticket was "fixed" by backing out all pinning! we don't want to do that, but want the patch from 772756 instead):
https://bugzilla.mozilla.org/show_bug.cgi?id=1005364

From the "pin all the things" ticket, the following might be useful to test the waters if we are feeling good about addons and the updater:
https://bugzilla.mozilla.org/show_bug.cgi?id=1004353 (Tor)
https://bugzilla.mozilla.org/show_bug.cgi?id=1004351 (Twitter)
https://bugzilla.mozilla.org/show_bug.cgi?id=1004352 (Google)
https://bugzilla.mozilla.org/show_bug.cgi?id=1027133 (*.twitter.com)

After that, there is an updater script for keeping pins up to date. The instructions are at the top of this file:
https://mxr.mozilla.org/mozilla-central/source/security/manager/tools/genHPKPStaticPins.js

comment:7 Changed 6 years ago by arthuredelstein

That's super helpful -- thanks Camilo and Mike! I'll give this a try right away.

comment:8 in reply to:  6 Changed 6 years ago by arthuredelstein

Replying to mikeperry:

Here's what I have so far:
https://github.com/arthuredelstein/tor-browser/commits/tbb-esr31.1.0-certificate-pinning

As suggested, I applied the following patches (with some tweaks):

https://bugzilla.mozilla.org/show_bug.cgi?id=744204
https://bugzilla.mozilla.org/show_bug.cgi?id=772756
https://bugzilla.mozilla.org/show_bug.cgi?id=1002696
https://bugzilla.mozilla.org/show_bug.cgi?id=1009635

To get these patches to apply more or less cleanly, I also needed to include:

https://bugzilla.mozilla.org/show_bug.cgi?id=998057
https://bugzilla.mozilla.org/show_bug.cgi?id=951315
https://bugzilla.mozilla.org/show_bug.cgi?id=1004270

There was a regression that should be fixed in the patch set for 772756 that broke the addons pane. We should verify our backport doesn't suffer from it either (note this ticket was "fixed" by backing out all pinning! we don't want to do that, but want the patch from 772756 instead):
https://bugzilla.mozilla.org/show_bug.cgi?id=1005364

I've included 772756. I still need to test for the presence of the bug reported in 1005364.

From the "pin all the things" ticket, the following might be useful to test the waters if we are feeling good about addons and the updater:
https://bugzilla.mozilla.org/show_bug.cgi?id=1004353 (Tor)
https://bugzilla.mozilla.org/show_bug.cgi?id=1004351 (Twitter)
https://bugzilla.mozilla.org/show_bug.cgi?id=1004352 (Google)
https://bugzilla.mozilla.org/show_bug.cgi?id=1027133 (*.twitter.com)

I'll hold off on these until addons and updater are working OK.

I guess at this point I should run unit tests on the pinning code. Are there any manual tests for certificate pinning I should run, in addition to 1005364?

comment:9 Changed 6 years ago by mikeperry

Keywords: TorBrowserTeam201410 MikePerry201410R added; TorBrowserTeam201409 removed

We'll want to test update of a pinned TBB, as well as addon updates for HTTPS Everywhere and NoScript.

comment:10 Changed 6 years ago by mikeperry

Keywords: TorBrowserTeam201410D added; TorBrowserTeam201410 removed

comment:11 Changed 6 years ago by mikeperry

Keywords: ff31-esr removed

Changed 6 years ago by arthuredelstein

comment:12 Changed 6 years ago by arthuredelstein

Here is my backport of certificate pinning:

https://github.com/arthuredelstein/tor-browser/commits/tor-browser-31.1.1esr-4.x-1-pinning

I am also attaching a patch file in which these 13 commits have been squashed to a single commit.

comment:13 Changed 6 years ago by arthuredelstein

Status: assignedneeds_review

comment:14 Changed 6 years ago by mikeperry

Keywords: tbb-4.5-alpha added

comment:15 Changed 6 years ago by mikeperry

Resolution: fixed
Status: needs_reviewclosed

Ok, I have applied the all-in-one version of this backport for TBB 4.5. Thanks!

Note: See TracTickets for help on using tickets.