383390 - nsICrypto Hash truncates to 32 byte; renders sha384 and sha512 corrupt

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Nils Maier [:nmaier]]

Attachment: patch, v1 - using HASH_LENGTH_MAX instead of hard-coded 32

The implementation of nsICryptHash::Finish truncates all hashes to 32 bytes at most.
While this works for md5, sha1 and sha256 as they are 16, 20 or 256 bytes in length respectively it truncates sha384 and sha512 which are 48 or 64 bytes respectively.

Seems the code got updated at some point to use HASH_MAX_LENGTH but that hard-coded 32 as maxlen parameter to HASH_End was missed.

See patch.

Comment 1

[Ed Lee :Mardak]

The line in question..

HASH_End(mHashContext, pbuffer, &hashLen, 32);

first shows up in bug 292368 "patch v.3" (attachment 182551 [details] [diff] [review]) which replaced the following line of "patch v.2" (atachment 182506)..

nsresult rv = mHasher->HashEnd(mHashContext, &pbuffer, &hashLen, nsISignatureVerifier::MAX_HASH_LENGTH);

Comment 2

[Nils Maier [:nmaier]]

Comment on attachment 267354 [details] [diff] [review]
patch, v1 - using HASH_LENGTH_MAX instead of hard-coded 32

Gerv mentioned it would be better to ask Kai for review ;)

Comment 3

[Kai Engert [:KaiE:]]

Comment on attachment 267354 [details] [diff] [review]
patch, v1 - using HASH_LENGTH_MAX instead of hard-coded 32

Looks correct to me. r=kengert

We could also use sizeof(buffer) so the constant would only be used in a single place.

Comment 4

[Kai Engert [:KaiE:]]

Comment on attachment 267354 [details] [diff] [review]
patch, v1 - using HASH_LENGTH_MAX instead of hard-coded 32

Bob, should we get this fix onto the stable branches?

Comment 5

[Kai Engert [:KaiE:]]

Comment on attachment 267354 [details] [diff] [review]
patch, v1 - using HASH_LENGTH_MAX instead of hard-coded 32

Bob, should we get this fix onto the stable branches?

Comment 6

[Kai Engert [:KaiE:]]

fixed on trunk

Comment 7

[Jeff Walden [:Waldo]]

Nils, would you be able to write a quick testcase for this?  I bet it'd probably be 5-10 lines of code at most.

http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests

Comment 8

[Nils Maier [:nmaier]]

Attachment: unit test

Tests all currently available algorithms with a standard message and the empty message.
I don't know how to run the test-suite, so I didn't check.
However this works when directly feed to xpcshell...
Feel free to adopt.

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Patchified version of previous

I hoisted hexdigest outside of doHash and added use of Components.Constructor, but other than that this is the previous attachment and Makefile changes with diff packaging.  It's tested and works for me.

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 268027 [details] [diff] [review]
Patchified version of previous

Actually, sr isn't necessary for test-only changes.

Comment 11

[Kai Engert [:KaiE:]]

Note this is the first time I look at JS test scripts, and I'm not up to date on the latest JS features.

The md5+sha1sum hashes in your script match what I get using command line tools, so this looks good.

What about the function you use in the forEach statement. I read that it should take 3 arguments, but you only use 2. Is it allowed to omit the last parameter?

I find the hexdigest function difficult to read.

Can you please explain why you are need this unusual start index (-2) in the slice function?

And I've never seen a construct like
  [string for (i in array)].join()
before!

Could you please point me to the JS reference that explains this kind of statement? I'd like to learn more about it. Thanks.

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> What about the function you use in the forEach statement. I read that it
> should take 3 arguments, but you only use 2. Is it allowed to omit the last
> parameter?

You can call a JS function with more or fewer arguments than it accepts as written.  If you call with more, the extra arguments aren't available by name in the called function, and you must use |arguments[n]|, for n >= the number of arguments the function accepts by default, to access them.  If you call with fewer, the extra arguments are given the value |undefined|.

In this particular case the remaining arguments aren't needed, so they can be safely ignored (reducing brain-print in understanding what the function does, as well).


> I find the hexdigest function difficult to read.

> Can you please explain why you are need this unusual start index (-2) in the
> slice function?

[("0" + data.charCodeAt(i).toString(16)).slice(-2) for (i in data)].join("");

First, |data| is the final string value produced by the cryptohash.  |for (i in data)| uses the Mozilla JS extension that iterating over a string iterates over its character indexes, so that's 0..length-1 over the hash string.

Returning to the left, the |charCodeAt| gets the value of the character at that index in the string.

|slice(-2)| is equivalent to |slice(length of this string - 2)| as a convenient way of wrapping around to the other end of a string without doing the actual calculation.  When provided with only one argument, slice selects to the end of the string, so this chomps off the last two characters of the string.

The last-two-characters part clarifies the |"0" +| -- if the Unicode value is <10, we have a single-character hex string when we want one that's two characters, and unconditionally prepending a "0" solves the problem.

The array comprehension just creates an array whose elements are these two-character strings.

Here's a hopefully more understandable version (keeping in mind that map acts on an array and returns the array created by calling the given function on each value in the array, more or less), which I could swap into the test if you wanted:

function characterToTwoHexCharacters(c)
{
  var v = c.charCodeAt(0);
  if (v < 10)
    return "0" + v;
  else
    return v;
}
function hexdigest(data)
{
  var chars = data.split("");
  for (var i = 0; i < chars.length; i++)
    chars[i] = characterToTwoHexCharacters(chars[i]);
  return chars.join("");
}

(I personally don't go for this much "cuteness" in my JS, and I usually write something more like the second version, since array comprehensions are still a Mozilla extension at this point even if they'll be in the eventual ECMAScript edition 4.  Then again, I didn't write this -- thanks again to Nils for doing so!  :-) )


> And I've never seen a construct like
>   [string for (i in array)].join()
> before!
> 
> Could you please point me to the JS reference that explains this kind of
> statement? I'd like to learn more about it. Thanks.

This is an array comprehension, new to Mozilla's JS for around a year-ish now.  It's basically a more compact way of creating an array.  Basically, it creates an array whose elements are the value of |string|, which can be an arbitrary expression, at all the points enumerated in a loop |for (i in array)|, with |string| binding appropriately to the enumerated |i|.  The syntax also allows you to place further |for| loops after the first (say, to enumerate over deeper levels of an object) and to end with an if-statement to do filtering.

More details on array comprehensions are available at:

http://developer.mozilla.org/en/docs/New_in_JavaScript_1.7#Array_comprehensions


Any other questions?

Comment 13

[Jeff Walden [:Waldo]]

Ignore the map mentions in the last comment; they're from a rewritten version that I didn't end up posting, opting instead for the better clarity of an array if you're not already familiar with Mozilla's extra array functions.

Comment 14

[Jeff Walden [:Waldo]]

"better clarity of a for loop"

I suck at commenting today.

Comment 15

[Kai Engert [:KaiE:]]

Comment on attachment 268027 [details] [diff] [review]
Patchified version of previous

r=kengert

Thanks a lot for the patch and the very helpful explanation.

I will check in a version of this patch that has portions of Jeff's explanation added.

Comment 16

[Jeff Walden [:Waldo]]

Yay, this can't break again!  :-)

Comment 17

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 267354 [details] [diff] [review]
patch, v1 - using HASH_LENGTH_MAX instead of hard-coded 32

canceling undirected SR request for patch checked in and bug resolved.

Comment 18

[Ronny Perinke]

We need this patch for 2.0.0.8 because it causes also affects updates using sha512 (or sha384) as hash-function for the updateHash-entry in the update manifest.
I've recently updated one of my extensions and added updatehash + signature for Minefield compatibility. Minefield had no problems (because the patch was checked-in), but Firefox 2.0.0.7. It always gives me error 261 after downloading the the update. (Invalid file hash (possible download corruption))

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 267354 [details] [diff] [review]
patch, v1 - using HASH_LENGTH_MAX instead of hard-coded 32

approved for 1.8.1.8, a=dveditz for release-drivers

Comment 20

[Kai Engert [:KaiE:]]

checked in for 1.8.1.8

Comment 21

[Jay Patel [:jay]]

Jeff: Can you verify that this is fixed by running the unit tests with 2.0.0.8rc2 and update "fixed1.8.1.8" to "verified1.8.1.8" in the status whiteboard.  Thanks.

Comment 22

[Jeff Walden [:Waldo]]

Attachment: HTML test for branch

So xpcshell (or NSS?) on branch has this awesome bug that causes using its stuff to crash, so I had to convert this to a webpage test with enhanced privileges, which was of course a massive bit of fun.  In the end, however, this works on latest branch with the attached testcase.  The testcase failed when I intentionally broke the test, so it should be testing correctly.  You'll have to click through a dialog for it to work (and maybe download locally, not sure whether webpages have to do extra work to get the dialog to show) as it uses privileged methods and stuff.