Make SHA256_update and SHA512_update if the passed in length is greater than UINT32_MAX

Make SHA256_update and SHA512_update if the passed in length is greater than UINT32_MAX

BUG=chrome-os-partner:2912
TEST=make && make runtests

Change-Id: If415a023d47b78ae2fc5af0b2fe5e410ef37086d

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=a01a62b

[Randall Spangler, 2011-03-26 02:02:46]

http://codereview.chromium.org/6750016/diff/1/firmware/lib/cryptolib/sha2.c
File firmware/lib/cryptolib/sha2.c (right):

http://codereview.chromium.org/6750016/diff/1/firmware/lib/cryptolib/sha2.c#n...
firmware/lib/cryptolib/sha2.c:609: SHA256_update(&ctx, data, len);
Is SHA256_update() used anywhere else?  If not, we could change its input arg to
uint32_t, and then have a while() loop here that passes in UINT32_MAX bytes at a
time.  

Otherwise, it seems like the early return in SHA256_update() will cause SHA256()
to return the same digest for all len > UINT32_MAX, regardless of data.

Alternately, SHA256() could return an error if len > UINT32_MAX.

(same comment for SHA512)

[gauravsh, 2011-03-26 23:21:41]

http://codereview.chromium.org/6750016/diff/1/firmware/lib/cryptolib/sha2.c
File firmware/lib/cryptolib/sha2.c (right):

http://codereview.chromium.org/6750016/diff/1/firmware/lib/cryptolib/sha2.c#n...
firmware/lib/cryptolib/sha2.c:609: SHA256_update(&ctx, data, len);
On 2011/03/26 02:02:46, Randall Spangler wrote:
> Is SHA256_update() used anywhere else?  If not, we could change its input arg
to
> uint32_t, and then have a while() loop here that passes in UINT32_MAX bytes at
a
> time.  
> 
> Otherwise, it seems like the early return in SHA256_update() will cause
SHA256()
> to return the same digest for all len > UINT32_MAX, regardless of data.
> 
> Alternately, SHA256() could return an error if len > UINT32_MAX.
> 
> (same comment for SHA512)

Well, I figured that we will pretty much never try to hash more than UINT32_MAX
bytes of data, so it doesn't matter how it fails.

In any case, I think hashing 32-byte chunks is reasonable. So I have done it
that way.

[Randall Spangler, 2011-03-28 16:15:04]

LGTM assuming the two extra (uint32_t) typecasts.

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/inclu...
File firmware/lib/cryptolib/include/sha.h (right):

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/inclu...
firmware/lib/cryptolib/include/sha.h:57: void SHA1_update(SHA1_CTX* ctx, const
uint8_t* data, uint64_t len);
Does SHA1 have the same problem?

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/sha2.c
File firmware/lib/cryptolib/sha2.c (right):

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/sha2....
firmware/lib/cryptolib/sha2.c:610: block_size = (remaining_len >= UINT32_MAX) ?
UINT32_MAX : remaining_len;
I suspect you need an explicit (uint32_t) here to keep MSVC from complaining.

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/sha2....
firmware/lib/cryptolib/sha2.c:638: block_size = (remaining_len >= UINT32_MAX) ?
UINT32_MAX : remaining_len;
(here too)

[gauravsh, 2011-03-28 18:49:41]

I will push the changes once the tree is open!

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/inclu...
File firmware/lib/cryptolib/include/sha.h (right):

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/inclu...
firmware/lib/cryptolib/include/sha.h:57: void SHA1_update(SHA1_CTX* ctx, const
uint8_t* data, uint64_t len);
On 2011/03/28 16:15:04, Randall Spangler wrote:
> Does SHA1 have the same problem?

No. SHA1_CTX use 64-bit ints for lengths and SHA1_update constructs, and so
don't have this issue.

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/sha2.c
File firmware/lib/cryptolib/sha2.c (right):

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/sha2....
firmware/lib/cryptolib/sha2.c:610: block_size = (remaining_len >= UINT32_MAX) ?
UINT32_MAX : remaining_len;
On 2011/03/28 16:15:04, Randall Spangler wrote:
> I suspect you need an explicit (uint32_t) here to keep MSVC from complaining.

Done.

http://codereview.chromium.org/6750016/diff/7001/firmware/lib/cryptolib/sha2....
firmware/lib/cryptolib/sha2.c:638: block_size = (remaining_len >= UINT32_MAX) ?
UINT32_MAX : remaining_len;
On 2011/03/28 16:15:04, Randall Spangler wrote:
> (here too)

Done.

[Randall Spangler, 2011-03-28 21:18:54]

http://codereview.chromium.org/6750016/diff/17001/firmware/lib/cryptolib/sha2.c
File firmware/lib/cryptolib/sha2.c (right):

http://codereview.chromium.org/6750016/diff/17001/firmware/lib/cryptolib/sha2...
firmware/lib/cryptolib/sha2.c:610: block_size = (remaining_len >= (uint32_t)
UINT32_MAX) ?
Actually, I meant

block_size = (uint32_t)((remaining_len >= UINT32_MAX) ? UINT32_MAX :
remaining_len);

Since it's the downcast from remaining_len that I suspect will cause warnings.

[gauravsh, 2011-03-29 17:44:45]

http://codereview.chromium.org/6750016/diff/17001/firmware/lib/cryptolib/sha2.c
File firmware/lib/cryptolib/sha2.c (right):

http://codereview.chromium.org/6750016/diff/17001/firmware/lib/cryptolib/sha2...
firmware/lib/cryptolib/sha2.c:610: block_size = (remaining_len >= (uint32_t)
UINT32_MAX) ?
On 2011/03/28 21:18:54, Randall Spangler wrote:
> Actually, I meant
> 
> block_size = (uint32_t)((remaining_len >= UINT32_MAX) ? UINT32_MAX :
> remaining_len);
> 
> Since it's the downcast from remaining_len that I suspect will cause warnings.

DOne.

[Randall Spangler, 2011-03-29 19:33:21]

LGTM, thanks