Closed Bug 383390 Opened 17 years ago Closed 17 years ago

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

Categories

(Core :: Security: PSM, defect)

x86
Windows XP
defect
Not set
major

Tracking

()

RESOLVED FIXED

People

(Reporter: nmaier, Assigned: KaiE)

Details

(Keywords: verified1.8.1.8)

Attachments

(4 files)

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.
Attachment #267354 - Flags: review?(wtc)
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 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 ;)
Attachment #267354 - Flags: review?(wtc) → review?(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.
Attachment #267354 - Flags: review?(kaie) → review+
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?
Attachment #267354 - Flags: superreview?
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?
fixed on trunk
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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
Flags: in-testsuite?
Attached file 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.
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.
Attachment #268027 - Flags: superreview?(kengert)
Attachment #268027 - Flags: review?(kengert)
Comment on attachment 268027 [details] [diff] [review]
Patchified version of previous

Actually, sr isn't necessary for test-only changes.
Attachment #268027 - Flags: superreview?(kengert)
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.
(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?
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.
"better clarity of a for loop"

I suck at commenting today.
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.
Attachment #268027 - Flags: review?(kengert) → review+
Yay, this can't break again!  :-)
Flags: in-testsuite? → in-testsuite+
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.
Attachment #267354 - Flags: superreview?
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))
Flags: blocking1.8.1.8?
Flags: blocking1.8.1.8? → blocking1.8.1.8+
Attachment #267354 - Flags: approval1.8.1.8?
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
Attachment #267354 - Flags: approval1.8.1.8? → approval1.8.1.8+
checked in for 1.8.1.8
Keywords: fixed1.8.1.8
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.
Attached file 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.
You need to log in before you can comment on or make changes to this bug.