Closed Bug 1233328 Opened 5 years ago Closed 5 years ago

Remove support for SHA-1 HPKP pins

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

Attachments

(2 files, 2 obsolete files)

https://chromium.googlesource.com/chromium/src/net/+/045698d824608d492f545455fc51ecc678a8c8ff removed SHA-1 pin support on the Chromium side.

This was the only source of SHA-1 pins we pulled in as part of the weekly HPKP periodic updates, so SHA-1 support can now be removed from the pinning implementation.
Comment on attachment 8709901 [details]
MozReview Request: Bug 1233328 - Part 1: Ignore SHA-1 pins in PublicKeyPinningService.cpp.

https://reviewboard.mozilla.org/r/31567/#review28365

This looks good. Just a couple of nits and a few more things I think we can get rid of.

::: security/manager/ssl/PublicKeyPinningService.h:22
(Diff revision 1)
> -   * false otherwise. If the host is pinned, return true if one of the keys in
> +   * checks, or to false otherwise. If the host is pinned, return true via

nit: probably should have been "returns" here originally

::: security/manager/ssl/PublicKeyPinningService.cpp:67
(Diff revision 1)
> -  nsresult rv = GetBase64HashSPKI(cert, hashType, base64Out);
> +  nsresult rv = GetBase64HashSPKI(cert, SEC_OID_SHA256, base64Out);

We can just hard-code SEC_OID_SHA256 in GetBase64HashSPKI now, right?

::: security/manager/ssl/PublicKeyPinningService.cpp:114
(Diff revision 1)
>    // This can happen if dynamicFingerprints is null and the static pinset

This shouldn't happen now, right? I think we should return NS_ERROR_INVALID_ARG in this case (or maybe even assert this isn't happening and return NS_ERROR_FAILURE).

::: security/manager/ssl/PublicKeyPinningService.cpp:147
(Diff revision 1)
>  EvalChainWithPinset(const CERTCertList* certList,

We probably don't even need this now, right?

::: security/manager/ssl/PublicKeyPinningService.cpp:284
(Diff revision 1)
> -    return EvalChainWithHashType(certList, SEC_OID_SHA256, nullptr,
> +    return EvalChain(certList, nullptr, &dynamicFingerprints, chainHasValidPins);

nit: long line?
Comment on attachment 8709902 [details]
MozReview Request: Bug 1233328 - Part 2: Use SHA-256 StaticFingerprints directly instead of StaticPinset since the SHA-1 StaticFingerprints entry will always be null.

https://reviewboard.mozilla.org/r/31569/#review28381

Great!
Attachment #8709902 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/31567/#review28365

Thanks for the review!

> This shouldn't happen now, right? I think we should return NS_ERROR_INVALID_ARG in this case (or maybe even assert this isn't happening and return NS_ERROR_FAILURE).

Yes, this shouldn't happen now. I added an assert.

> We probably don't even need this now, right?

Yup; removed.

> nit: long line?

Just over - 81 chars.
Attachment #8709901 - Attachment is obsolete: true
Attachment #8710739 - Flags: review+
Attachment #8709902 - Attachment is obsolete: true
Attachment #8710740 - Flags: review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=33af875d4083

- The M(oth) failures are caused by mochitest-a11y not having any tests to run, which is the expected case.
  - |mach try| for some reason decided to specify it anyways.
- The tc-M(a11y) failures seem to be caused by a Python import error.
  - In any case, the changes here are unlikely to impact a11y in any way.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/097afbd95b50
https://hg.mozilla.org/mozilla-central/rev/c70e41368252
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.