1229284 - Remove support for SHA-1 hashes in genHPKPStaticPins.js

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

See Bug 224968 comment 5 through Bug 224968 comment 8: genHPKPStaticPins.js no longer needs to support SHA-1 hashes, so the relevant code should be removed at some point to simplify the script.

Comment 1

[:Cykesiopka]

(In reply to Cykesiopka from comment #0)
> See Bug 224968 comment 5 through Bug 224968 comment 8: genHPKPStaticPins.js

Oops, Bug 1224968 comment 5 through Bug 1224968 comment 8.

Comment 2

[:Cykesiopka]

Attachment: MozReview Request: Bug 1229284 - Remove support for SHA-1 hashes in genHPKPStaticPins.js.

Bug 1229284 - Remove support for SHA-1 hashes in genHPKPStaticPins.js.

Comment 3

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

https://reviewboard.mozilla.org/r/27947/#review25133

This looks great. I just have a couple of suggestions. In a follow-up, we should probably simplify the actual pinning code to not even attempt sha-1 pins (since they're all null now anyway).

::: security/manager/tools/genHPKPStaticPins.js:255
(Diff revision 1)
>            certNameToSKD[chromeName] = hash;

Hmmm - I think certNameToSKD is undefined/unused in this function (this is a preexisting problem, but we should probably fix it).

::: security/manager/tools/genHPKPStaticPins.js:457
(Diff revision 1)
>    if (pinset.sha256_hashes && pinset.sha256_hashes.length > 0) {

pinset.sha256_hashes should always have a value now, right? We should make this a fatal error otherwise.

::: security/manager/tools/genHPKPStaticPins.js:565
(Diff revision 1)
>      if (pinset.sha256_hashes) {

Same idea here.

Comment 4

[:Cykesiopka]

https://reviewboard.mozilla.org/r/27947/#review25133

Thanks for the review!

> In a follow-up, we should probably simplify the actual pinning code to not even attempt sha-1 pins
Filed Bug 123332.

> Hmmm - I think certNameToSKD is undefined/unused in this function (this is a preexisting problem, but we should probably fix it).

Turns out that while certNameToSKD isn't in the param list of the function, it is defined at the global scope and used.

Comment 5

[:Cykesiopka]

Attachment: bug1229284_rm-sha1-support-genHPKPStaticPins.js_v2.patch

+ Address review comments.

v1 has r+ on the ReviewBoard side, but for some reason this wasn't propagated to the Bugzilla side.

Comment 6

[:Cykesiopka]

NPOTB, but: https://treeherder.mozilla.org/#/jobs?repo=try&revision=80746ee0f581

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/934aa92f8d54

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/934aa92f8d54