298522 - Implement more power-up self tests, such as HMAC, RSA for FIPS 140-2

Status: RESOLVED FIXED

(NSS :: Libraries, enhancement, P1)

Description

[glen beasley]

Implement more power-up self tests, such as HMAC, RSA for FIPS 140-2

encryption and decryption need to be tested. 

If we test HMAC SHA1 we don't need to test SHA1 again. 

For each block cipher we only need to test one key size and one mode of operation.

Comment 1

[Wan-Teh Chang]

This bug is closely related to bug 259135.

Comment 2

[glen beasley]

Attachment: Added FIPS HMAC tests for SHA-1, SHA-256, SHA384, and SHA-512

Comment 3

[Wan-Teh Chang]

Comment on attachment 199823 [details] [diff] [review]
Added FIPS HMAC tests for SHA-1, SHA-256, SHA384, and SHA-512

This patch is correct.	It has two minor problems.
1. sftk_fips_HMAC should return SECStatus instead of
CK_RV.	2. In four comments, "bits" should be "bytes".

The patch has some formatting problems: one line has
a lot of white spaces, and the patch assumes tabstop=4,
but NSS uses a tabstop of 8.  I will attach a patch
that fixed these formatting problems.

Comment 4

[Wan-Teh Chang]

Attachment: Added FIPS HMAC tests for SHA-1, SHA-256, SHA384, and SHA-512, v2

Comment 5

[glen beasley]

Comment on attachment 200268 [details] [diff] [review]
Added FIPS HMAC tests for SHA-1, SHA-256, SHA384, and SHA-512, v2

looks good and thanks!

Comment 6

[Wan-Teh Chang]

Attachment: Added FIPS HMAC tests for SHA-1 and SHA-256 (checked in)

I checked in the self tests for HMAC SHA-1 and HMAC SHA-256 first.
We need to fix bug 313196 before we can check in the self tests for
HMAC SHA-384 and HMAC SHA-512.

Comment 7

[glen beasley]

Attachment: added FIPS Hmac tests for SHA-384 and SHA-512

calculated known answers for HMAC SHA-384 and HMAC SHA-512
using patch from 313196

Comment 8

[Wan-Teh Chang]

Comment on attachment 200764 [details] [diff] [review]
added FIPS Hmac tests for SHA-384 and SHA-512

r=wtc.  One minor error:

>+    /* known SHA256 hmac (48 bytes) */
>+    static const PRUint8 known_SHA384_hmac[] = {

The comment should say SHA384.

Would you like to review my patch for bug 313196?
It requires reading FIPS 198, but that algorithm
is quite short.

Comment 9

[Wan-Teh Chang]

Comment on attachment 200764 [details] [diff] [review]
added FIPS Hmac tests for SHA-384 and SHA-512

I've checked in this patch on the NSS trunk (NSS 3.11).

Glen, the RSA power-up self test uses a 512-bit modulus
(it's 520 bits with the 8 most significant bits equal to 0).
It is a good idea to increase that to 1024-bit, the minimum
RSA modulus size specified in FIPS 186-2 Change Notice 1.

Comment 10

[Julien Pierre]

Per our meeting today, retargetting to 3.11.1 .

Comment 11

[glen beasley]

Attachment: 1024-bit, the minimum RSA modulus size 1024

changed RSA modulus size to 1024 bits and 
added Known answer tests for RSA SHA1,
SHA256, SHA384, and SHA512 Signature tests.

Comment 12

[Wan-Teh Chang]

Attachment: 1024-bit, the minimum RSA modulus size 1024, v2

Glen, I reviewed your patch.  Instead of writing down my
suggested changes, I think it's easier for me to implement
and test my suggested changes (so that I know they work)
and ask you to review them.  You can use Bugzilla's patch
interdiff feature to see the changes I made to your patch.

1. I made some formatting changes to align the comments in
the same way as the original code.  I also made (or removed)
some whitespace changes.

2. I found out why you needed the extra leading 0x00 bytes:
the sizes of those buffers are one byte too big.  I believe
the leading 0x00 bytes may have been necessary before to
indicate that those sequences of bytes are *unsigned* big
integers.

I define those buffers to have the right sizes (powers of 2)
and removed the leading 0x00 bytes.

3. We can simply pass FIPS_RSA_SIGNATURE_LENGTH as the fifth
argument to RSA_HashSign.  It is better to use that constant
here because the RSA modulus size is fixed (whereas in
cmd/fipstest/fipstest.c we test several RSA modulus sizes).

4. You accidentally removed a "return rv" statement in an
if statement.  I added that back.

Comment 13

[Wan-Teh Chang]

Comment on attachment 219221 [details] [diff] [review]
1024-bit, the minimum RSA modulus size 1024, v2

I also removed two unused variables and renamed a few variables
(rsa_known_SHAxxx_signature ==> rsa_known_shaxxx_signature).

Comment 14

[Wan-Teh Chang]

Comment on attachment 219221 [details] [diff] [review]
1024-bit, the minimum RSA modulus size 1024, v2

I checked in this RSA power-up self test patch on
the NSS trunk (3.12) and NSS_3_11_BRANCH (3.11.1).

Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.14; previous revision: 1.13
done

Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.13.2.1; previous revision: 1.13
done

Glen, I just remembered that the testing lab told us
we don't need to test RSA signature with all the SHA
hash algorithms because we already test the SHA hash
algorithms individually.  Do you remember this?  In
any case, it's good to do all the tests required to
pass the FIPS RSA algorithm testing.  But for the
ECDSA power-up self test, let's just test it with
SHA-1 since the FIPS ECDSA algorithm testing only
tests ECDSA with SHA-1.

Comment 15

[glen beasley]

Attachment: ecdsa power up self test

Comment 16

[Wan-Teh Chang]

Attachment: ecdsa power up self test, v2

Glen, I made the following changes to your patch.

1. Do not include "keyhi.h" in lib/softoken because
it's layering violation.  This requires changing
SECKEYECParams to SECItem, which is the type used
by EC_DecodeParams and EC_CopyParams anyway.  (This
code was copied from cmd/fipstest cmd/bltest.  I
will fix those two later.)

2. It's not necessary to define the
FIPS_ECDSA_SIGNATURE_LENGTH macro.  We can take
sizeof on the known signature buffer to get this
length.

3. I restored a return statement that you accidentally
removed from the AES self test.

4. Instead of allocating the publicValue, privateValue,
and version SECItems in ecdsa_private_key, I just point
their data buffers to the static buffers.  This requires
adding the initial EC_POINT_FORM_UNCOMPRESSED byte to the
ecdsa_publicValue buffer.

5. I fixed the memory leak of ecparams.

6. I removed an unnecessary signature length check
(len%2 != 0).  It is sufficient to just compare the
signature length with the known signature length.
I also removed a redundant ecdsaStatus != SECSuccess
test there.

7. I use EC_CopyParams instead of structure copy to
copy ecdsa_private_key.ecParams to ecdsa_public_key.ecParams.
I believe it is fine to use structure copy, but since
we use EC_CopyParams to copy ecparams to ecdsa_private_key.ecParams
earlier in the function, it is better to be consistent.

Comment 17

[glen beasley]

Comment on attachment 220959 [details] [diff] [review]
ecdsa power up self test, v2

Patch looks good. Thanks Wan-teh.

Comment 18

[glen beasley]

Comment on attachment 220959 [details] [diff] [review]
ecdsa power up self test, v2

Patch looks good. Thanks Wan-teh.

Comment 19

[Wan-Teh Chang]

Comment on attachment 220959 [details] [diff] [review]
ecdsa power up self test, v2

I checked in the patch "ecdsa power up self test, v2"
on the NSS trunk (NSS 3.12) and NSS_3_11_BRANCH (NSS 3.11.2).

Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.15; previous revision: 1.14
done

Checking in fipstest.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstest.c,v  <--  fipstest.c
new revision: 1.13.2.2; previous revision: 1.13.2.1
done