Closed Bug 1004781 Opened 6 years ago Closed 5 years ago

enable pinning for facebook

Categories

(Core :: Security: PSM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: mmc, Assigned: mmc)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 9 obsolete files)

1.11 KB, patch
keeler
: review+
Details | Diff | Splinter Review
10.83 KB, patch
keeler
: feedback+
Details | Diff | Splinter Review
Facebook is not in Chrome's pinset. However, from looking through the CT database, they use 44 certs. That's pretty good. We should keep an eye on them over time to see if the cert set changes, as well as make contact to see if they're willing to coordinate.
Why would we ever want to pin something without explicit consent from a site? That seems like a massive problem just waiting to happen.
(In reply to Reed Loden [:reed] from comment #1)
> Why would we ever want to pin something without explicit consent from a
> site? That seems like a massive problem just waiting to happen.

We don't, due to coordination issues. They're an obvious candidate due to high volume, though, and responded positively to our request.

From Scott: *.facebook.com should be pinned to

DigiCert EV Root (Subject Key Identifier
B1:3E:C3:69:03:F8:BF:47:01:D4:98:26:1A:08:02:EF:63:64:2B:C3) and the

Verisign Class 3 G3 root (no SKI, but serial number
9b:7e:06:49:a3:3e:62:b9:d5:ee:90:48:71:29:ef:57).
Assignee: nobody → mmc
Status: NEW → ASSIGNED
Comment on attachment 8480144 [details] [diff] [review]
Enable pinning in test mode for facebook (

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

David's out today, let's try to get this merged into m-c before the script generation on Saturday to avoid conflicts.
Attachment #8480144 - Flags: review?(rlb)
Attachment #8480144 - Flags: review?(cviecco)
Comment on attachment 8480144 [details] [diff] [review]
Enable pinning in test mode for facebook (

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

Looks good to me. I assume you have tried browsing basic fb pages?
Attachment #8480144 - Flags: review?(cviecco) → review+
(In reply to Camilo Viecco (:cviecco) from comment #5)
> Comment on attachment 8480144 [details] [diff] [review]
> Enable pinning in test mode for facebook (
> 
> Review of attachment 8480144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me. I assume you have tried browsing basic fb pages?

That wouldn't do anything, since we're in test mode.
Attachment #8480144 - Flags: review?(rlb)
This doesn't seem like too much of a hack. We could even add Google's missing certs, if we wanted.
Attachment #8485223 - Flags: review?(mmc)
Comment on attachment 8485223 [details] [diff] [review]
follow-up to add DigiCert ECC Secure Server CA

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

Thanks!
Attachment #8485223 - Flags: review?(mmc) → review+
Btw, buildbot touches these 3am Saturday so you either need to make sure it merges to m-c before then or wait until after that to checkin.
Comment on attachment 8480144 [details] [diff] [review]
Enable pinning in test mode for facebook (

This is a similar approval request to https://bugzilla.mozilla.org/show_bug.cgi?id=1030135#c21, except I'd like to target FF 34.

Approval Request Comment
[Feature/regressing bug #]: None.
[User impact if declined]: Facebook won't be pinned until Firefox 35.
[Describe test coverage new/current, TBPL]: Telemetry dashboard at people.mozilla.org/~mchew/pinning_dashboard.
[Risks and why]: This is a telemetry change at this point.
[String/UUID change made/needed]: None.
Attachment #8480144 - Flags: approval-mozilla-aurora?
Comment on attachment 8485223 [details] [diff] [review]
follow-up to add DigiCert ECC Secure Server CA

Approval Request Comment

See comment 14.
Attachment #8485223 - Flags: approval-mozilla-aurora?
Attachment #8485223 - Flags: approval-mozilla-aurora?
Comment on attachment 8487414 [details] [diff] [review]
Aurora-specific patch to enable pinning in test mode for facebook (

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

See comment 14.
Attachment #8487414 - Flags: approval-mozilla-aurora?
Attachment #8480144 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8487414 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8480144 - Flags: approval-mozilla-aurora+
I misattributed this patch to a=lmandel instead of bhavana. Sorry!

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f5c35805e16
Attachment #8480144 - Attachment is obsolete: true
Comment on attachment 8513899 [details] [diff] [review]
Remove our pinset for facebook since it's in chromium now (

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

https://chromium.googlesource.com/chromium/src/+/master/net/http/transport_security_state_static.json + ctrl-f "facebook"
Attachment #8513899 - Flags: review?(dkeeler)
Comment on attachment 8513899 [details] [diff] [review]
Remove our pinset for facebook since it's in chromium now (

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

LGTM. Have you confirmed that the pinning information generation script successfully finds all the certificates it needs to with this change?
Attachment #8513899 - Flags: review?(dkeeler) → review+
Attached patch fixup (obsolete) — Splinter Review
Yep, this is the output of manually re-running the generator script.
Attachment #8485223 - Attachment is obsolete: true
Attachment #8487414 - Attachment is obsolete: true
Attachment #8514668 - Attachment is obsolete: true
Attachment #8514655 - Attachment is obsolete: true
Comment on attachment 8514670 [details] [diff] [review]
Actually remove the pinset (

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

::: security/manager/tools/PreloadedHPKPins.json
@@ +230,4 @@
>    ],
>  
>    "extra_certificates": [
>       // DigiCert ECC Secure Server CA (for Facebook)

Should this be removed as well, or is it still needed, even though that cert could be pulled from the Chromium list now?
Comment on attachment 8516821 [details] [diff] [review]
Remove unnecessary cert for facebook (

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

Thanks, Reed. We don't need that entry since we're also pulling certs from chromium. The only difference is that since it's not a root cert, it will be called kGOOGLE_PIN_DigiCertECCSecureServerCAFingerprint instead of kDigiCert_ECC_Secure_Server_CAFingerprint in the output file.
Attachment #8516821 - Flags: review?(dkeeler)
Attached patch facebookSplinter Review
Metrics look good.
Attachment #8513899 - Attachment is obsolete: true
Attachment #8514669 - Attachment is obsolete: true
Attachment #8514670 - Attachment is obsolete: true
Attachment #8516821 - Attachment is obsolete: true
Attachment #8535082 - Flags: review?(dkeeler)
Attached patch outputSplinter Review
Output, not for checkin.
Attachment #8535083 - Flags: review?(dkeeler)
Comment on attachment 8535082 [details] [diff] [review]
facebook

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

Great!
Attachment #8535082 - Flags: review?(dkeeler) → review+
Comment on attachment 8535083 [details] [diff] [review]
output

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

This looks right. I'll just give feedback+ since this isn't for check-in.
Attachment #8535083 - Flags: review?(dkeeler) → feedback+
I should have marked this bug fixed when we switched to production mode. We're enforcing Chrome's pins for facebook as of FF 37.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Keywords: leave-open
Target Milestone: --- → mozilla37
You need to log in before you can comment on or make changes to this bug.