313196 - HMAC SHA-384 and HMAC SHA-512 should use 128-byte block size

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Wan-Teh Chang]

Our current HMAC code has a hardcoded 64-byte block
size of the hash algorithm.  While this is correct
for MD2, MD5, SHA-1, and SHA-256, this block size
is wrong for SHA-384 and SHA-512, whose block size is
128 byte.  I believe this is why our HMAC SHA-384
and HMAC SHA-512 results are different from Sun JCE
provider's.

See FIPS 198, Section 2.3 for the definition of B:
block size (in bytes) of the input to the Approved
hash function.

See FIPS 180-2, Figure 1 for the block sizes of the
various SHA-X algorithms.

In alghmac.c, we have
#define HMAC_PAD_SIZE 64
which is the 'B' parameter in the HMAC standard.
This hardcoded constant (and equivalent values such
as sizeof(cx->ipad) and sizeof(cx->opad) will need
to be changed to a value that depends on the hash
algorithm.  Unfortunately, the SECRawHashObjects[]
array (or rather the SECHashObject structure) doesn't
contain this information (the input block size of
the hash algorithm).

Comment 1

[Wan-Teh Chang]

Attachment: Proposed patch: add an "input block size" field to the SECHashObject structure

This is the most elegant solution.  Unfortunately it
requires adding a field to an exported structure.  Let
me know what you think and if I should rename 'inblksize'
to 'inblocksize'.

If we don't want to extend the SECHashObject structure,
I'll need to change the prototype of HMAC_Create so that
we pass HASH_HashType instead of const SECHashObject *
as the first argument.	Perhaps this is a good change to
make anyway.  Do you know why we are passing const SECHashObject *
as the first argument?	Is it to avoid a lookup in the
SECRawHashObjects array (perhaps because the calling code
already has a hash object pointer)?

Comment 2

[glen beasley]

I tested wan-teh's patch and successfully compared the HMAC SHA-384 and HMAC SHA-512 results with the Sun JCE. see bug 263544

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch: add an "input block size" field to the SECHashObject structure, v2

As Nelson suggested, I added some space at the end of the
SECHashObject structure reserved for future expansion.
Is this the right way to declare the reserved space?
Is the reserved space large enough?

Comment 4

[Wan-Teh Chang]

Attachment: Alternate patch: pass HASH_HashType to HMAC_Create to avoid changing SECHashObject

To avoid changing the exported SECHashObject structure, we
need to pass HASH_HashType instead of const SECHashObject *
to HMAC_Create and HMAC_Init so that they can use the hash
type to look up the hash algorithm's input block size.

Note that in lib/softoken/lowpbe.c I deleted the dead PBKDF2
code, which used HMAC_Create.  I tried to modify the PBKDF2
code, but quickly concluded that there is no way that code was
ever compiled before, so it's not worth it to change it.

Comment 5

[Wan-Teh Chang]

Attachment: Third patch: add SECRawHashObject for lib/{freebl,softoken}

To beat this bug to death, here is a third solution:
split the SECHashObject type into two types: SECHashObject
for above the PKCS11 line, and SECRawHashObject for below
the PKCS11 line.  This solution is a variant of solution 1
(extending SECHashObject): SECRawHashObject is still exported,
but it is exported for use by NSS's SSL library, so it will be
less of an issue to extend SECRawHashObject in the future.

I found that what "high" and "low" share in hasht.h is
the HASH_HashType enumeration type definition.  So I moved
the HASH_HashType definition to blapit.h.  This eliminates
the need for lib/{freebl,softoken} to include hasht.h or
sechash.h, a layering violation.  I also deleted the duplicate
definitions of the <hash>_LENGTH macros in hasht.h; those macros
are already defined in blapit.h.

Comment 6

[Wan-Teh Chang]

Attachment: Proposed patch: add an "input block size" field to the SECHashObject structure, v3

At yesterday's NSS meeting, we decided to fix this bug
by extending the SECHashObject structure.  It occurred
to me that if we add the hash algorithm type to SECHashObject,
we will be able to use that as an index into supplmental
tables of information on the hash algorithms.  This will
avoid the need to change the SECHashObject structure in
the future.  I also reviewed FIPS 180-2 and found that
none of the other hash algorithm properties (maximum message
size, word size, and bits of security) needs to be stored
in SECHashObject.  Maximum message size is either 2^64
or 2^128 bits.  Word size is either 32 or 64 bits.  Security
is half of message digest  size (assuming birthday attack).
If you think any of these should be added to SECHashObject,
I can do that.

If you think we should implement some of the ideas from
the other two alternate patches, please let me know:
- pass HASH_HashType instead of const SECHashObject * to
  HMAC_Create.
- Split the shared SECHashObject type into the high-level
  SECHashObject type and the low-level SECRawHashObject type.
- Use a supplemental array to store hash algorithm properties
  that are not useful to all SECHashObject users.  (For example,
  hash input block size is only useful to HMAC.)

Thanks.
- Fix layering violation by moving the HASH_HashType enum type
  definition from lib/cryptohi/hasht.h to lib/freebl/blapit.h.

Comment 7

[Robert Relyea]

Comment on attachment 201801 [details] [diff] [review]
Proposed patch: add an "input block size" field to the SECHashObject structure, v3

sr=relyea.
Don't check in until you have r=nelson.

Also, I would really like to see a void*reserved pointer at the end of the structure (I know we can index off of the hashtype, but we still don't know which table we should use (raw or not raw).

the void * pointer should be initialized to NULL everywhere.

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 201801 [details] [diff] [review]
Proposed patch: add an "input block size" field to the SECHashObject structure, v3

r=nelsonb.
I have been concerned about breaking binary compatibility by changing struct SECHashObjectStr but Julien has persuaded me that the probability of any or our servers using it is very small.

Comment 9

[Wan-Teh Chang]

I checked in "Proposed patch v3" on the NSS trunk (NSS 3.11).

Checking in cryptohi/hasht.h;
/cvsroot/mozilla/security/nss/lib/cryptohi/hasht.h,v  <--  hasht.h
new revision: 1.7; previous revision: 1.6
done
Checking in cryptohi/sechash.c;
/cvsroot/mozilla/security/nss/lib/cryptohi/sechash.c,v  <--  sechash.c
new revision: 1.6; previous revision: 1.5
done
Checking in freebl/alghmac.c;
/cvsroot/mozilla/security/nss/lib/freebl/alghmac.c,v  <--  alghmac.c
new revision: 1.4; previous revision: 1.3
done
Checking in freebl/blapit.h;
/cvsroot/mozilla/security/nss/lib/freebl/blapit.h,v  <--  blapit.h
new revision: 1.18; previous revision: 1.17
done
Checking in freebl/rawhash.c;
/cvsroot/mozilla/security/nss/lib/freebl/rawhash.c,v  <--  rawhash.c
new revision: 1.4; previous revision: 1.3
done

Comment 10

[Wan-Teh Chang]

Attachment: Proposed patch v4 (as checked in)

This is what's actually checked in.  I had to fix
the header inclusion in lib/freebl/alghmac.c because
alghmac.c needs the new <hash-alg>_BLOCK_LENGTH macros
defined in blapit.h.

Checking in freebl/alghmac.c;
/cvsroot/mozilla/security/nss/lib/freebl/alghmac.c,v  <--  alghmac.c
new revision: 1.6; previous revision: 1.5
done