Closed
Bug 1233328
Opened 9 years ago
Closed 9 years ago
Remove support for SHA-1 HPKP pins
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
References
Details
Attachments
(2 files, 2 obsolete files)
11.67 KB,
patch
|
Cykesiopka
:
review+
|
Details | Diff | Splinter Review |
17.46 KB,
patch
|
Cykesiopka
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31567/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31567/
Attachment #8709901 -
Flags: review?(dkeeler)
![]() |
Assignee | |
Comment 2•9 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/31569/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31569/
Attachment #8709902 -
Flags: review?(dkeeler)
![]() |
Assignee | |
Comment 3•9 years ago
|
||
![]() |
||
Updated•9 years ago
|
Attachment #8709901 -
Flags: review?(dkeeler) → review+
![]() |
||
Comment 4•9 years ago
|
||
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 5•9 years ago
|
||
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+
![]() |
Assignee | |
Comment 6•9 years ago
|
||
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.
![]() |
Assignee | |
Comment 7•9 years ago
|
||
Attachment #8709901 -
Attachment is obsolete: true
Attachment #8710739 -
Flags: review+
![]() |
Assignee | |
Comment 8•9 years ago
|
||
Attachment #8709902 -
Attachment is obsolete: true
Attachment #8710740 -
Flags: review+
![]() |
Assignee | |
Comment 9•9 years ago
|
||
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
Comment 10•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/097afbd95b50
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70e41368252
Keywords: checkin-needed
Comment 11•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/097afbd95b50
https://hg.mozilla.org/mozilla-central/rev/c70e41368252
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•