Crypto.util.bytesToWords assume that bytes are unsigned

[GoogleCodeExporter]

Crypto.util.bytesToWords assume that bytes are unsigned

[GoogleCodeExporter]

I'm undecided if this is a problem that needs fixing, since -1 is not a valid 
byte value, and none of CryptoJS's ToBytes methods will return values outside 
the range 0..255.

Can you describe the scenario that caused the issue?

Original comment by Jeff.Mott.OR on 23 Nov 2011 at 5:15

[GoogleCodeExporter]

-1 is a valid byte value if your bytes are signed. 

For instance, in Java bytes are signed and if you want to put 1's in all bits 
of the byte then you need to do
byte b = -1;
instead of 
byte b = 255; (which is a compiler error - you have to do casting to byte).


I can explain my scenario as follows. Let's say you are using Google Web 
Toolkit to develop a web application. Let's say the creation of bytes happen in 
Java. For instance, you a receive/produce 4 bytes like this:

First 2 bytes are all 0's, third one is all 1's, last one is all 0's.

If you want to see these as numbers in Java, Java would tell the values are:
[0, 0, -1, 0] (because that is how it interprets the bits - Java assumes bytes 
are signed and 2' complement is used for negative numbers)


The 32 bit representation of these "numbers" in hexadecimal are:
0x00000000
0x00000000
0xFFFFFFFF
0x00000000


Now let's say, in GWT using Javascript Native Interface(JSNI) you call the sha1 
of crypto-js and pass this array of numbers. Note that there is no way to pass 
bytes to JS, so you pass an array of numbers (which are 0,0,-1,0).

The first thing sha1 does is converting "bytes" (which are really numbers) to 
words. So it calls bytesToWords function. The goal of this one is to collapse 
all "bytes" into a 32 bit word.

The expected result of the collapsing is:
0000FF00 (first 8 bits is from the first number, second 8 bits from second 
numbers, and so on).

However, the current code does the following:

shift the first number to left 8 times, shift second number to left 16 times 
and so on:
(after shifting)
1st: 0x00000000 (24 times left)
2nd: 0x00000000 (16 times left)
3rd: 0xFFFFFF00 (8 times left)
4th: 0x00000000 (no shift)

when you "OR" these you get:
0xFFFFFF00 which is -256.



We can reproduce this problem for other signed "byte" values:

[1,2,3,-4]
0x00000001
0x00000002
0x00000003
0xFFFFFFF4

What we really want to produce is:
0x010203F4

However the code produces:
0xFFFFFFF4

because after shifting
0x01000000 (24 times left)
0x00020000 (16 times left)
0x00000300 (8 times left)
0xFFFFFFF4 (no shifting)

and the "OR" of these is 0xFFFFFFF4 which is incorrect.


If you still think negative values are not valid and there is no fixing needed, 
at least, it might be worth to add a check in the code to produce an error if 
the numbers are negative/out of range or add a comment to say the numbers the 
function accepts which correspond to byte values should be in a certain range 
(0-255).

Original comment by [email protected] on 23 Nov 2011 at 7:16

[GoogleCodeExporter]

Frankly I think this is a design error in Java. I think it should provide an 
unsigned byte data type. Nevertheless, I understand the situation you're stuck 
with, and I'll change CryptoJS per your suggestion.

Original comment by Jeff.Mott.OR on 23 Nov 2011 at 9:20

• Changed state: Accepted

[GoogleCodeExporter]

Original comment by Jeff.Mott.OR on 8 Jan 2012 at 12:40

• Changed state: Fixed