298514 - Implement pairwise consistency for digitial signature key generation for FIPS 140-2

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[glen beasley]

Implement pairwise consistency for digitial signature key generation for FIPS
140-2. See VE.09.33.01.

Comment 1

[glen beasley]

we are already doing this in pk11wrap, we need to move it into the softoken
under the PKCS#11 boundary. 

Comment 2

[Wan-Teh Chang]

I'm going to address the closely related bug 298513 in
this bug's patches.

Comment 3

[Wan-Teh Chang]

Attachment: Proposed patch

I'm asking Bob to do a regular code review, and Nelson
to at least review the description of the patch below.

We currently perform the FIPS pairwise consistency check
in the pk11wrap layer on *all tokens* after C_GenerateKeyPair.
In this FIPS validation we need to perform the pairwise
consistency check inside the softokn3 shared library, our
new crypto module boundary.

This patch does the following.

1. It removes the pairwise consistency check from the
pk11wrap layer to avoid performing this check twice
for our own softoken. The consequence is that we won't
be performing this check for other vendors' tokens.
We can document in the NSS 3.11 release notes that
token vendors should implement the pairwise consistency
check in their tokens.

2. Reimplement the pairwise consistency check in the
new sftk_PairwiseConsistencyCheck function, which is
based on the original pk11_PairwiseConsistencyCheck
function.

3. NSC_GenerateKeyPair calls sftk_PairwiseConsistencyCheck
at the very end.  This means we perform pairwise consistency
check in both FIPS and non-FIPS modes, as we did before.

4. sftk_PairwiseConsistencyCheck calls the NSC_EncryptInit,
NSC_Encrypt, and other NSC_XXX functions (as opposed to the
lower-level freebl functions) to perform the pairwise
consistency check.  It performs those crypto operations on
the hSession that was passed to NSC_GenerateKeyPair.

5. The only important change I made to the pairwise consistency
check is that in the old pk11_PairwiseConsistencyCheck function,
the check fails if the actual encrypted data length is less than
the encrypted data buffer size (which is the length of the
RSA modulus), whereas in the new sftk_PairwiseConsistencyCheck
function I removed this test.  I just don't think this test
is useful for pairwise consistency.

6. NSC_GenerateKeyPair now returns the error code
CKR_GENERAL_ERROR if pairwise consistency check failed.
This is the best PKCS #11 error code I can find that
NSC_GenerateKeyPair wasn't returning before.  The
second best one is CKR_DEVICE_ERROR, but that error
code is sometimes misused in the softoken.  I need
a unique error code for pairwise consistency check
failure so that I can detect it in FIPS mode
(FC_GenerateKeyPair) and put the crypto module in
the error state (fatalError = PR_TRUE).

7. Bob, in NSC_GenerateKeyPair, please review the
error handling code for pairwise consistency check
failure.  I call both NSC_DestroyObject and
sftk_FreeObject on the public and private keys.
I hope that's the correct way to destroy the key
pair.

This is a long patch but is straightforward.  The
code review should focus on the design decisions
listed above and the new sftk_PairwiseConsistencyCheck
function.

Comment 4

[Robert Relyea]

Comment on attachment 190069 [details] [diff] [review]
Proposed patch

r+, my comments are only observations or some suggestions.

Comments:

Nothing critical here:

1) The use of NSC_XXXX functions requires the pairwise consistancy check to
appear where it does (almost the end of GenerateKeyPair). This necessitates
makeing the keys fully visible to the application. Once in the pairwise
consistancy check, it's possible that an application could find these keys with
C_FindObjects() (they become fully visible once sftk_handleObject is called).
Token versions of these objects will show up in the database (so if the system
crashes here, we will come up with the objects in the database). This is kind
of 'yucky', but the fact is we had this already in the previous check, so I
don't think it's a big issue. Calling freebl on 'half formed keys' would take a
lot more tweaking, and necessitate more complicated switches (pretty much on
the order of the switches in the various NSC_XXXXInit functions). In an idea
world, I think we would want to do this, but I believe there is true functional
issue with the code as is, and I don't think we'll have problems getting
validated.

2) The error handling is correct, particularly out of the pairwise checks
itself. In fact they are more correct than the error handling the the existing
version (which would leave token objects undeleted if the pairwise consistancy
checks failed!

3) Style issue. The 'else' is redunant in;
	if ((crv == CKR_SIGNATURE_LEN_RANGE) ||
		(crv == CKR_SIGNATURE_INVALID)) {
	    return CKR_GENERAL_ERROR;
	} else if (crv != CKR_OK) {
	    return crv;
	}

4) we could add a new error code in pkcs11n.h to handle FIPS failures.

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 190069 [details] [diff] [review]
Proposed patch

I think this patch does what it's trying to do OK, but I wonder 
if this is exactly what we want to do.	Two comments:

1. softoken/pkcs11c.c is getting HUGE.	5500 lines now, and this 
patch adds a few hundred more.	Maybe time to break it up?
Maybe this new function should go into a separate new file in softoken?
Maybe this file should be reserved for the NSC_ functions themselves
and not their helpers.

2. For FIPS 140-2, we need to do the pairwise check in the token.
And clearly we don't want to do TWO such checks for softoken.
But should we do pairwise consistency checks in PK11wrap for other 
tokens than our own?  
I'm thinking that rather than removing the old function entirely, 
we should just call it when the slot is not from owr own module.  

These are just suggestions.  If you think we should not do pairwise
checks on other modules, then sr+ = nelson.  Else sr-.	:)

Comment 6

[Wan-Teh Chang]

Attachment: Patch as checked in

I made the change that Bob suggested (removed the "else" after a "return"
statement) and four white space changes.  I checked in the patch on the
NSS trunk for NSS 3.11.

Checking in pk11wrap/pk11akey.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11akey.c,v  <--  pk11akey.c
new revision: 1.2; previous revision: 1.1
done
Checking in softoken/fipstokn.c;
/cvsroot/mozilla/security/nss/lib/softoken/fipstokn.c,v  <--  fipstokn.c
new revision: 1.10; previous revision: 1.9
done
Checking in softoken/pkcs11c.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11c.c,v	<--  pkcs11c.c
new revision: 1.62; previous revision: 1.61
done

Nelson, I feel it's a shame that we won't be performing the
pairwise consistency check on other vendors' tokens.  This issue
is similar to the doublecheck of RSA signature -- should we do
that for other vendors' tokens?  As long as we have a consistent
policy for such checks, I am okay either way.

There is a reason for not doing the pairwise consistency check
for other vendors' tokens: bug 298340.	Some tokens require
authentication (by entering the PIN) for every private key
operation, so the pairwise consistency check done in pk11wrap
generates two extra PIN prompts (for the test decrypt and test
sign), which is annoying and confusing.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Once upon a time, NSS had a requirement to check that the encryption 
implemented by third-party PKCS11 modules worked correctly and implemented
exactly what it said it did.  The idea was to prevent some token from 
claiming that it implemented some weak algorithm when it actually 
implemented some stronger one.  Maybe that's all ancient history now,
but it sticks in my mind as something we're supposed to do: verify that
algorithms are what they say, and that generated keys are of the type
and length they're supposed to be.  

If none of that applies any more, then I should try harder to forget it.
Anyone know for sure?

Comment 8

[Julien Pierre]

We could keep the high-level pk11wrap code and do the check only if the module
on which the operation happened is not the internal module, or if PK11_IsFIPS()
is PR_FALSE . Of course this will be redundant for other FIPS modules ...

Comment 9

[Wan-Teh Chang]

Attachment: If we want to restore pk11_PairwiseConsistencyCheck

If we want to restore pk11_PairwiseConsistencyCheck,
this is the patch to apply to the original pk11akey.c
(rev. 1.1), before I removed the function.

It fixes the following bugs in pk11_PairwiseConsistencyCheck.

1. Allocate the "ciphertext" buffer before calling C_EncryptInit,
to avoid the need to call C_EncryptFinal if memory allocation
fails after the session has been initialized for encryption.

2. Call PK11_DestroyObject to destroy token objects because
SECKEY_DestroyPublicKey and SECKEY_DestroyPrivateKey don't
destroy token objects.	We don't want to leave PKCS #11 public
and private key objects in the token if the pairwise consistency
check fails.

3. Only call pk11_PairwiseConsistencyCheck on other vendors'
(non-internal) tokens.

Comment 10

[Robert Relyea]

re comment 7:

That was an export rule. I don't know if we need it or not anymore. The export
rules are quite complexed. It might be that 'no we could get export permission
without it, but it would require a ton of export lawyers making a brand new
filing because the existing one is predicated on our checks'.

I'd like to forget that too;).

Comment 11

[Wan-Teh Chang]

Comment on attachment 190200 [details] [diff] [review]
If we want to restore pk11_PairwiseConsistencyCheck

At the NSS meeting yesterday we decided that we don't need
to perform FIPS 140-2 pairwise consistency tests on other
vendors' tokens.  So it's not necessary to review this
patch.	I will request reviews if we need to fix these bugs
in NSS 3.10.x or older.

Comment 12

[Wan-Teh Chang]

Attachment: Add a missing break statement to the CKK_EC case

This patch adds a missing break statement at the end of
the CKK_EC case.  It also removes an unused variable.

Comment 13

[Wan-Teh Chang]

Comment on attachment 191363 [details] [diff] [review]
Add a missing break statement to the CKK_EC case

Julien, this patch adds a missing break statement and removes
an unused variable.  Please review.  Thanks.