Closed
Bug 1344442
Opened 7 years ago
Closed 7 years ago
Fix various issues with the nsICryptoHash and nsICryptoHMAC implementations
Categories
(Core :: Security: PSM, enhancement, P1)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: Cykesiopka, Assigned: Cykesiopka)
References
Details
(Whiteboard: [psm-assigned])
Attachments
(4 files)
There are a few things we can fix: - Test coverage is lacking in some areas - Manual memory management - Not using early returns - Other style issues
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8843709 [details] Bug 1344442 - Part 2: Improve test coverage of nsICryptoHash and nsICryptoHMAC implementations. https://reviewboard.mozilla.org/r/117296/#review118956 ::: security/manager/ssl/tests/unit/test_hash_algorithms.js:79 (Diff revision 1) > + hexHashes: [ > - "07e547d9586f6a73f73fbac0435ed76951218fb7d0c8d788a309d785436bbb642e93a252a954f23912547d1e8a3b5ed6e1bfd7097821233fa0538f3db854fee6", > + "07e547d9586f6a73f73fbac0435ed76951218fb7d0c8d788a309d785436bbb642e93a252a954f23912547d1e8a3b5ed6e1bfd7097821233fa0538f3db854fee6", > - "cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e" > - ] > -}; > - > + "cf83e1357eefb8bdf1542850d66d8007d620e4050b5715dc83f4a921d36ce9ce47d0d13c5d85f2b0ff8318d2877eec2f63b931bd47417a81a538327af927da3e", > + ], > + b64Hashes: [ > + // TODO(Bug 1338897): Stop inserting CRLFs every 64 characters. An addons DXR search suggests doing this won't have any compat impact. ::: security/manager/ssl/tests/unit/test_hmac.js:95 (Diff revision 1) > + expectedBase64: "r0XS43ZIQDFhf3jStYprG5x+9GT1oBtH5C7Dc2MiRF6OIkDKXmnix4syOez6shZJ", > + }, > + { > + algoID: Ci.nsICryptoHMAC.SHA512, algoName: "SHA-512", > + expectedDigest: "164b7a7bfcf819e2e395fbe73b56e0a387bd64222e831fd610270cd7ea2505549758bf75c05a994a6d034f65f8f0e6fdcaeab1a34d4a6b4b636e070a38bce737", > + // TODO(Bug 1338897): Stop inserting CRLFs every 64 characters. I'm less sure about this one. nsICryptoHash seems to be used a lot, and I don't know how to express "init with SHA-512, calls finish(true)" in a DXR search (if that's even possible). Not sure which is better: 1. Keep doing this for backwards compat 2. Stop inserting CRLFs and mark the change as addon-compat.
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8843708 [details] Bug 1344442 - Part 1: Remove MD2 support from nsICryptoHMAC. https://reviewboard.mozilla.org/r/117294/#review119696 Sounds good. ::: security/manager/ssl/nsICryptoHMAC.idl:21 (Diff revision 1) > { > /** > * Hashing Algorithms. These values are to be used by the > * |init| method to indicate which hashing function to > * use. These values map onto the values defined in > * mozilla/security/nss/lib/softoken/pkcs11t.h and are nit: trailing space while we're here?
Attachment #8843708 -
Flags: review?(dkeeler) → review+
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8843709 [details] Bug 1344442 - Part 2: Improve test coverage of nsICryptoHash and nsICryptoHMAC implementations. https://reviewboard.mozilla.org/r/117296/#review118956 > I'm less sure about this one. > > nsICryptoHash seems to be used a lot, and I don't know how to express "init with SHA-512, calls finish(true)" in a DXR search (if that's even possible). > > Not sure which is better: > 1. Keep doing this for backwards compat > 2. Stop inserting CRLFs and mark the change as addon-compat. I think we should just fix this.
Comment 8•7 years ago
|
||
mozreview-review |
Comment on attachment 8843709 [details] Bug 1344442 - Part 2: Improve test coverage of nsICryptoHash and nsICryptoHMAC implementations. https://reviewboard.mozilla.org/r/117296/#review119704 LGTM. Thanks for looking into the crlf issue. ::: security/manager/ssl/tests/unit/head_psm.js:867 (Diff revision 1) > + * @param {String} data > + * @returns {String} > + */ > +function hexify(data) { > + // |slice(-2)| chomps off the last two characters of a string. > + // nit: not sure this extra line is necessary ::: security/manager/ssl/tests/unit/head_psm.js:868 (Diff revision 1) > + * @returns {String} > + */ > +function hexify(data) { > + // |slice(-2)| chomps off the last two characters of a string. > + // > + // Therefore, if the Unicode value is < 10, we have a single-character hex 0x10, right? (Oh, I see - this was already like this) ::: security/manager/ssl/tests/unit/test_hash_algorithms.js:13 (Diff revision 1) > "" > ]; > -const hashes = { > - md2: [ > +const ALGORITHMS = [ > + { > + initString: "md2", > + initConstant: Ci.nsICryptoHash.MD2, Is anything actually using md2 with nsICryptoHash? I feel like we should remove it too.
Attachment #8843709 -
Flags: review?(dkeeler) → review+
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8843710 [details] Bug 1344442 - Part 3: Use smart pointers. https://reviewboard.mozilla.org/r/117298/#review119750 LGTM.
Attachment #8843710 -
Flags: review?(dkeeler) → review+
Comment 10•7 years ago
|
||
mozreview-review |
Comment on attachment 8843711 [details] Bug 1344442 - Part 4: Misc cleanups. https://reviewboard.mozilla.org/r/117300/#review119752 Cool - thanks! ::: security/manager/ssl/nsCryptoHash.cpp:72 (Diff revision 1) > return NS_ERROR_NOT_AVAILABLE; > } > > - HASH_HashType hashType = (HASH_HashType)algorithm; > + HASH_HashType hashType; > + switch (algorithm) { > + case nsCryptoHash::MD2: Let's stick with nsICryptoHash::MD2 etc.
Attachment #8843711 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8843709 [details] Bug 1344442 - Part 2: Improve test coverage of nsICryptoHash and nsICryptoHMAC implementations. https://reviewboard.mozilla.org/r/117296/#review119704 Thanks for the review! > nit: not sure this extra line is necessary Removed. > Is anything actually using md2 with nsICryptoHash? I feel like we should remove it too. I believe there are addons that use MD2, but I don't know if they're all doing so because "it exists, might as well use it". It'll take some time for me to find out, so I'll defer this to a follow up bug for now.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 16•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=fbc080c41431084b9a9bb7ac9938db69f753f301 (Just as a friendly reminder, the changes here need to land on top of the changes in Bug 1344441.)
Keywords: checkin-needed
Comment 17•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c971d946fb0d Part 1: Remove MD2 support from nsICryptoHMAC. r=keeler https://hg.mozilla.org/integration/autoland/rev/ae815898894f Part 2: Improve test coverage of nsICryptoHash and nsICryptoHMAC implementations. r=keeler https://hg.mozilla.org/integration/autoland/rev/55b551e3415b Part 3: Use smart pointers. r=keeler https://hg.mozilla.org/integration/autoland/rev/1b20c5c67b1f Part 4: Misc cleanups. r=keeler
Keywords: checkin-needed
Comment 18•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c971d946fb0d https://hg.mozilla.org/mozilla-central/rev/ae815898894f https://hg.mozilla.org/mozilla-central/rev/55b551e3415b https://hg.mozilla.org/mozilla-central/rev/1b20c5c67b1f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in
before you can comment on or make changes to this bug.
Description
•