1344442 - Fix various issues with the nsICryptoHash and nsICryptoHMAC implementations

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement, P1)

Description

[:Cykesiopka]

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 1

[:Cykesiopka]

Attachment: Bug 1344442 - Part 1: Remove MD2 support from nsICryptoHMAC.

There are several reasons for doing this:
1. Nothing appears to be using MD2 with nsICryptoHMAC.
2. There don't seem to be any test vectors available.
3. Bug 160161 suggests the MD2 case doesn't work anyways.

Review commit: https://reviewboard.mozilla.org/r/117294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117294/

Comment 2

[:Cykesiopka]

Attachment: Bug 1344442 - Part 2: Improve test coverage of nsICryptoHash and nsICryptoHMAC implementations.

The equivalent base 64 digests for the existing test cases were obtained using:
> python2
> import binascii
> binascii.b2a_base64(binascii.unhexlify(<input hex>))

The large input hash digest was obtained like so:
> python2
> import hashlib
> hashlib.md5(" " * 4100).hexdigest()

The large input HMAC digest was obtained like so:
> python2
> import hashlib
> import hmac
> hmac.new("test", " " * 4100, hashlib.md5).hexdigest()

Review commit: https://reviewboard.mozilla.org/r/117296/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117296/

Comment 3

[:Cykesiopka]

Attachment: Bug 1344442 - Part 3: Use smart pointers.

Review commit: https://reviewboard.mozilla.org/r/117298/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117298/

Comment 4

[:Cykesiopka]

Attachment: Bug 1344442 - Part 4: Misc cleanups.

Review commit: https://reviewboard.mozilla.org/r/117300/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/117300/

Comment 5

[:Cykesiopka]

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

[Dana Keeler (she/her) [:keeler]]

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?

Comment 7

[Dana Keeler (she/her) [:keeler]]

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

[Dana Keeler (she/her) [:keeler]]

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.

Comment 9

[Dana Keeler (she/her) [:keeler]]

Comment on attachment 8843710 [details]
Bug 1344442 - Part 3: Use smart pointers.

https://reviewboard.mozilla.org/r/117298/#review119750

LGTM.

Comment 10

[Dana Keeler (she/her) [:keeler]]

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.

Comment 11

[:Cykesiopka]

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 12

[:Cykesiopka]

Comment on attachment 8843708 [details]
Bug 1344442 - Part 1: Remove MD2 support from nsICryptoHMAC.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117294/diff/1-2/

Comment 13

[:Cykesiopka]

Comment on attachment 8843709 [details]
Bug 1344442 - Part 2: Improve test coverage of nsICryptoHash and nsICryptoHMAC implementations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117296/diff/1-2/

Comment 14

[:Cykesiopka]

Comment on attachment 8843710 [details]
Bug 1344442 - Part 3: Use smart pointers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117298/diff/1-2/

Comment 15

[:Cykesiopka]

Comment on attachment 8843711 [details]
Bug 1344442 - Part 4: Misc cleanups.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/117300/diff/1-2/

Comment 16

[:Cykesiopka]

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.)

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Carsten Book [:Tomcat]]

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