443045 - Fix PK11_GenerateKeyPair for ECC keys on the 3.11 branch

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

Attachment: This is the full patch, including whitespace diffs, v1

The checkin for bug 376417 accomplished two separate purposes:
a) it added a new function to PK11wrap, by which the caller can specify the
PKCS#11 key usage attributes to be used with the newly generated key pair, 
and 
b) it changed the way that the PKCS11 usage attributes are computed/determined
for ECC key pairs, based on the output of C_GetMechanismInfo calls.  This fixed
a bug that prevented newly generated EC key pairs from being used for any 
purpose other than key derivation.

One of those changes (the new API) was only applicable to 3.12, but the other
is applicable to 3.11 also.  None of that patch was ever backported to 3.11.x.

This bug asks that fix for EC key pair attributes that was added on the trunk 
be also backported to the branch.  Patch to do that is attached.

Note: I will attach two versions of this patch, one of which ignores whitespace diffs.  I suggest that reviewers review that copy for functional correctness
and view the full copy for any indentation issues.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch without whitespace diffs, v1

Comment 2

[Robert Relyea]

Comment on attachment 327702 [details] [diff] [review]
This is the full patch, including whitespace diffs, v1

r+

This is the correct fragment to improve the ECC handling in NSS 3.11

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 327703 [details] [diff] [review]
patch without whitespace diffs, v1

Bob, your original patch made one more change that my new patch
did not copy.  Your patch also deleted the case CKM_ECDSA_SHA1
that is shown in the switch below.

> 	case CKM_ECDH1_DERIVE:
> 		mechanism_info.flags = CKF_DERIVE;
>+	    if (test_mech2.mechanism == CKM_ECDSA) {
>+		mechanism_info.flags |= CKF_SIGN | CKF_VERIFY;
>+	    }
> 		break;
> 	case CKM_ECDSA:
> 	case CKM_ECDSA_SHA1:
> 		mechanism_info.flags = CKF_SIGN | CKF_VERIFY;
> 		break;

I wonder why that was deleted, and if you think I should also 
delete it in the patch for 3.11.x.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Checking in pk11akey.c; new revision: 1.9.2.9; previous revision: 1.9.2.8

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

For posterity, I should explain what the bug was that was fixed by this 
checkin.  Comment 0 doesn't really explain it.

When you import/generate/derive a key in PKCS#11, you have to give it a set
of attributes.  One of those attributes is a boolean that says whether the 
private key can be used for signing.  The PKCS#11 module is free to use its 
own default values for any unspecified attributes, and it is free to ignore 
the values your application requests, and give its own values instead.  

At the time that it creates a new ECC private key object, NSS's own "softoken" 
module apparently ignores the application's requested value of the attribute 
that says whether or not the ECC private key can be used for signing, and 
always sets that boolean attribute to true, regardless of what the application 
requests.  

NSS's high layers were unwittingly dependent on that behavior.  They always 
requested that new ECC private keys have the signing attribute set to false.
That was OK with NSS's own softoken, which always forced that attribute to 
true (I believe).  But third party modules obeyed the request from the 
application, and set that attribute to false, because that's what NSS asked
them to do.  Then later, NSS attempted to use that newly created ECC private
key to do signing and it failed because the key was marked as not valid for
signing, just as NSS had requested when it requested the key's creation.

The fix was to request that ECC keys be valid for signing, by default, at the
time they are created.