Closed Bug 1092606 Opened 5 years ago Closed 5 years ago

Aurora is busted after today's HPKP update

Categories

(Core :: Security: PSM, defect, blocker)

33 Branch
defect
Not set
blocker

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox35 + fixed
firefox36 + fixed

People

(Reporter: RyanVM, Assigned: mmc)

Details

Attachments

(4 files, 1 obsolete file)

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
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.
This is because Chromium started pinning facebook, for which we were already injecting a pinset. Fix coming.
Assignee: nobody → mmc
Status: NEW → ASSIGNED
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.
Attachment #8516818 - Flags: review?(jjones)
Attachment #8516818 - Flags: review?(dkeeler)
Comment on attachment 8516819 [details] [diff] [review]
Not for checkin

This is sample output.
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.
Attachment #8516818 - Flags: review?(dkeeler) → review+
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.
Attachment #8516818 - Flags: review?(jjones) → review+
Attachment #8516818 - Attachment is obsolete: true
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.
Attachment #8516844 - Flags: review+
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
Attachment #8516844 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/1d0c12148cf6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Attachment #8516844 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: needinfo?(mmc)
Sigh -- this fix suppressed the duplicate pinsets but not their constituent parts. Sorry about that.
Flags: needinfo?(mmc)
Attachment #8516844 - Attachment is obsolete: true
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.
Attachment #8523991 - Flags: review?(dkeeler)
Attachment #8516844 - Attachment is obsolete: false
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.
Attachment #8523991 - Flags: review?(dkeeler) → review+
(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 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.
Attachment #8523991 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/6fcbd0ed5a28
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Attachment #8523991 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(coop)
It's running now. Will report back with status.
Flags: needinfo?(coop)
(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
Awesome, thanks Coop!
You need to log in before you can comment on or make changes to this bug.