1092606 - Aurora is busted after today's HPKP update

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

Aurora is closed.

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=363459&repo=mozilla-aurora

In file included from /builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/PublicKeyPinningService.cpp:23:0:
/builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/StaticHPKPins.h:762:49: error: redefinition of 'const char* kPinset_facebook_sha256_Data []'
/builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/StaticHPKPins.h:378:20: error: 'const char* kPinset_facebook_sha256_Data [3]' previously defined here
/builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/StaticHPKPins.h:767:33: error: redefinition of 'const StaticFingerprints kPinset_facebook_sha256'
/builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/StaticHPKPins.h:383:33: error: 'const StaticFingerprints kPinset_facebook_sha256' previously defined here
/builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/StaticHPKPins.h:772:27: error: redefinition of 'const StaticPinset kPinset_facebook'
/builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/StaticHPKPins.h:388:27: error: 'const StaticPinset kPinset_facebook' previously defined here
make[6]: *** [PublicKeyPinningService.i_o] Error 1
make[6]: Leaving directory `/builds/slave/m-aurora-lx-000000000000000000/build/obj-firefox/security/manager/boot/src'
make[5]: *** [security/manager/boot/src/target] Error 2

Comment 1

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/releases/mozilla-aurora/rev/f4385d27bff1

These things increasingly seem too prone to bustage for Saturday morning automation, which is a bad thing since they have never ever in even the slightest seemed to not be too prone to bustage for it.

Comment 2

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

This is because Chromium started pinning facebook, for which we were already injecting a pinset. Fix coming.

Comment 3

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

Attachment: Don't import Chromium pinsets for domains that are already in our list (

Comment 4

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

Attachment: Not for checkin

Comment 5

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

Comment on attachment 8516818 [details] [diff] [review]
Don't import Chromium pinsets for domains that are already in our list (

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

This fixes all the JS warnings as well.

Comment 6

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

Comment on attachment 8516819 [details] [diff] [review]
Not for checkin

This is sample output.

Comment 7

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

Comment on attachment 8516818 [details] [diff] [review]
Don't import Chromium pinsets for domains that are already in our list (

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

Looks good to me. If you feel like it, I think there are about 7 instances of 'var' we could change to 'let' while we're making these changes. Thanks for taking care of this.

::: security/manager/tools/genHPKPStaticPins.js
@@ +195,5 @@
>    let pemCert = "";
>    let hash = "";
>    let chromeNameToHash = {};
>    let chromeNameToMozName = {}
> +  let mozName;

I think we can declare mozName closer to where it's used.

Comment 8

[J.C. Jones [:jcj] (he/they)]

Comment on attachment 8516818 [details] [diff] [review]
Don't import Chromium pinsets for domains that are already in our list (

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

Keeler got here ahead of me. I don't see any issues with this patch, either.

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/1d0c12148cf6

Comment 10

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

Attachment: Don't import Chromium pinsets for domains that are already in our list (

Comment 11

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

Comment on attachment 8516844 [details] [diff] [review]
Don't import Chromium pinsets for domains that are already in our list (

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

Fixed comments and checked in.

Comment 12

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

Comment on attachment 8516844 [details] [diff] [review]
Don't import Chromium pinsets for domains that are already in our list (

Approval Request Comment
[Feature/regressing bug #]: Chromium started pinning facebook (comment 2)
[User impact if declined]: No pin updates on Aurora, no pinning enforcement on release if buildbot updates don't happen in Aurora (pin lifetimes are 14 weeks since last update, which is typically 8 weeks after release)
[Describe test coverage new/current, TBPL]: ran manually on mozilla-aurora
[Risks and why]: pretty low
[String/UUID change made/needed]: none

Comment 13

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

https://hg.mozilla.org/mozilla-central/rev/1d0c12148cf6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f46d7b20b032

Comment 15

[Phil Ringnalda (:philor)]

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=399715&repo=mozilla-aurora

/builds/slave/m-aurora-lx-000000000000000000/build/security/manager/boot/src/StaticHPKPins.h:762:49: error: redefinition of 'const char* kPinset_facebook_sha256_Data []' 

Backed out this week's update in https://hg.mozilla.org/releases/mozilla-aurora/rev/2f9a72986443

Comment 16

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

Sigh -- this fix suppressed the duplicate pinsets but not their constituent parts. Sorry about that.

Comment 17

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

Attachment: Filter out duplicate pinsets as well as domains (

Comment 18

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

Comment on attachment 8523991 [details] [diff] [review]
Filter out duplicate pinsets as well as domains (

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

Tested by running and building on Aurora.

Comment 19

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

Attachment: Sample output (not for checkin)

Comment 20

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

Comment on attachment 8523991 [details] [diff] [review]
Filter out duplicate pinsets as well as domains (

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

LGTM.
However, looking at our definition of Facebook's pinset on aurora and chrome's definition of Facebook's pinset, we have a discrepancy:

"Verisign Class 3 Public Primary Certification Authority - G3",
"DigiCert High Assurance EV Root CA",
"DigiCert ECC Secure Server CA"

vs

"SymantecClass3EVG3",
"DigiCertECCSecureServerCA",
"DigiCertEVRoot",
"FacebookBackup"

We should probably uplift the patch in bug 1004781 that fixed this in mozilla-central.

Comment 21

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/6fcbd0ed5a28

Comment 22

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

(In reply to David Keeler (:keeler) [use needinfo?] from comment #20)
> Comment on attachment 8523991 [details] [diff] [review]
> Filter out duplicate pinsets as well as domains (
> 
> Review of attachment 8523991 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM.
> However, looking at our definition of Facebook's pinset on aurora and
> chrome's definition of Facebook's pinset, we have a discrepancy:
> 
> "Verisign Class 3 Public Primary Certification Authority - G3",
> "DigiCert High Assurance EV Root CA",
> "DigiCert ECC Secure Server CA"
> 
> vs
> 
> "SymantecClass3EVG3",
> "DigiCertECCSecureServerCA",
> "DigiCertEVRoot",
> "FacebookBackup"
> 
> We should probably uplift the patch in bug 1004781 that fixed this in
> mozilla-central.

Well, I'd like to make sure that this patch resolves the duplication issue in case we ever have conflicting pinsets again. I think it's OK that facebook's test pinset will be different from Nightly's the last 2 weeks of Aurora.

Comment 23

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

Comment on attachment 8523991 [details] [diff] [review]
Filter out duplicate pinsets as well as domains (

Approval Request Comment
[Feature/regressing bug #]: See comment 2
[User impact if declined]: No updates to Aurora in 3 weeks, decreased pinset lifetime on release.
[Describe test coverage new/current, TBPL]: This time I ran the pin generation manually and did *not* forget to build the result.
[Risks and why]: Should be pretty low, this just fixes existing bustage.
[String/UUID change made/needed]: None.

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6fcbd0ed5a28

Comment 25

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

Pushed: https://hg.mozilla.org/releases/mozilla-aurora/rev/776650aa7757

Coop, would you mind running the build script manually on aurora? If it didn't work, I'd like to know before Saturday.

Comment 26

[Chris Cooper [:coop] (he/him)]

It's running now. Will report back with status.

Comment 27

[Chris Cooper [:coop] (he/him)]

(In reply to Chris Cooper [:coop] from comment #26)
> It's running now. Will report back with status.

Success:

https://hg.mozilla.org/releases/mozilla-aurora/rev/cffcb649cd0d

Comment 28

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

Awesome, thanks Coop!