1027133 - broaden twitter's pinset to include *.twitter.com

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[[:mmc] Monica Chew (no longer reading bugmail)]

Neil asked us to pin all of *.twitter.com to the twitterCom + twitterCdn pinset. This is more than Chrome pins because of their tight integration with HSTS.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Enable test mode for *.twitter.com (

Comment 2

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8442177 [details] [diff] [review]
Enable test mode for *.twitter.com (

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

twitterCDN contains all of the certs listed in twitterCom. We could make our own pinset if you're worried about Google removing pins from twitterCDN, but then we don't get additions either. I think this is OK given that Neil is on board with the documented process and is supposed to tell us through bugzilla about cert changes anyway.

Also it's a good time to broaden their pinset since no-one is in test mode! :)

Comment 3

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8442177 [details] [diff] [review]
Enable test mode for *.twitter.com (

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

Unfortunately, I don't think this will work as-is. See comment. Let me know if you want to brainstorm some ideas for how to fix this, because I'm not coming up with anything particularly simple/easy.

::: security/manager/boot/src/StaticHPKPins.h
@@ +971,1 @@
>    { "twitter.com", false, false, false, -1, &kPinset_twitterCom },

I'm not sure this is going to work in all cases. Say we're looking up "subdomain.twitter.com". There's no exact match, so we look for "twitter.com". Binary search could find the non-cdn twitter.com entry, return it, and the pinning implementation would say, "oh, looks like this doesn't include subdomains, so there's no pinning information for subdomain.twitter.com."
I think we either need to make the new twitter.com entry mask the old one (i.e. only have one entry here) or have some way of saying "this entry only applies to exactly twitter.com, whereas this entry applies only to subdomains" or something, and have the pinning implementation be aware of that when searching.

::: security/manager/tools/PreloadedHPKPins.json
@@ +201,5 @@
>        "test_mode": false, "id": 0 },
>      { "name": "test-mode.pinning.example.com", "include_subdomains": true,
> +      "pins": "mozilla_test", "test_mode": true },
> +    // Expand twitter's pinset to include all of *.twitter.com. This rule will
> +    // only cover twitter domains not covered by more specific rules, since

By "more specific" do you mean something like "oauth.twitter.com"? If that's the case, then the reason the rule for oath.twitter.com will match before this rule is because we search by exact domain name first, and then repeatedly look for parent domains with include subdomains set.

Comment 4

[[:mmc] Monica Chew (no longer reading bugmail)]

Argh, I think you're right, I forgot the existing pinset includes twitter.com already. Here it is below:
      // Twitter
      "api.twitter.com",
      "business.twitter.com",
      "dev.twitter.com",
      "mobile.twitter.com",
      "oauth.twitter.com",
      "platform.twitter.com",
      "twimg.com",
      "twitter.com", // does not include subdomains.
      "www.twitter.com"

OK, how about this: re-add exclude_domains in the Chromium data blob, add twitter.com to exclude_domains, and then add our own entry for twitter.com?

Comment 5

[Dana Keeler (she/her) [:keeler]]

That seems reasonable. Unfortunately, I think it means twitter.com will go back to test mode until we're confident this works (but we probably don't want to go straight to production with this, and it's only nightly anyway, so that's probably fine).

Comment 6

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Enable test mode for *.twitter.com (

Comment 7

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8442228 [details] [diff] [review]
Enable test mode for *.twitter.com (

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

::: security/manager/boot/src/StaticHPKPins.h
@@ +966,5 @@
>    { "tor2web.org", true, true, false, -1, &kPinset_tor2web },
>    { "torproject.org", false, true, false, -1, &kPinset_tor },
>    { "translate.googleapis.com", true, false, false, -1, &kPinset_google_root_pems },
>    { "twimg.com", true, false, false, -1, &kPinset_twitterCDN },
> +  { "twitter.com", true, true, false, -1, &kPinset_twitterCDN },

We now have a regression where we were enforcing on ^twitter.com$ in nightly prior to this change and are now putting it back in test mode for the subdomains change. I think this may be acceptable if we resolve it in 2 weeks or less -- otherwise we have to write annoying code in PublicKeyPinningService.

Comment 8

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8442228 [details] [diff] [review]
Enable test mode for *.twitter.com (

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

Great!

::: security/manager/tools/PreloadedHPKPins.json
@@ +48,4 @@
>        "www.twitter.com"
> +    ],
> +    "exclude_domains" : [
> +      // Chrome's entry for twitter.com doesn't include subdomains, so replace

Well, and also we want to use a different pinset, right?

@@ +204,5 @@
>        "include_subdomains": false, "pins": "mozilla_test",
>        "test_mode": false, "id": 0 },
>      { "name": "test-mode.pinning.example.com", "include_subdomains": true,
> +      "pins": "mozilla_test", "test_mode": true },
> +    // Expand twitter's pinset to include all of *.twitter.com. More specific

"... and change it from twitterCom to twitterCDN"

Comment 9

[[:mmc] Monica Chew (no longer reading bugmail)]

Fixed and checked in: https://hg.mozilla.org/integration/mozilla-inbound/rev/aa1280217799

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/aa1280217799

Comment 11

[[:mmc] Monica Chew (no longer reading bugmail)]

Reopening to put them back in production mode.

Comment 12

[[:mmc] Monica Chew (no longer reading bugmail)]

Attachment: Set test_mode=false for *.twitter.com (

Comment 13

[[:mmc] Monica Chew (no longer reading bugmail)]

Comment on attachment 8449902 [details] [diff] [review]
Set test_mode=false for *.twitter.com (

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

Telemetry is still behind, but the numbers look ok so go ahead and prep this change for checkin.

Also I guess we need to reach out to dropbox :P

Comment 14

[[:mmc] Monica Chew (no longer reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fdc900ad296f

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fdc900ad296f