Closed Bug 1233328 Opened 9 years ago Closed 9 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.
Attachment #8709901 - Flags: review?(dkeeler) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: