Bug 1540541 - Don't unnecessarily strip leading 0's from key material during PKCS11 import. r=jcj,mt
authorKevin Jacobs <kjacobs@mozilla.com>
Fri, 14 Jun 2019 07:57:54 +0000
changeset 15173 6cfb54d262d030783137aa6478b45ecb3cbfc624
parent 15170 313dfef345bd93bc67982249bffa2cfdd5a9d1b5
child 15174 dabfe1160c682b4d1d19c5a7a13ab3828bb9d37f
push id3406
push userjjones@mozilla.com
push dateFri, 21 Jun 2019 18:14:09 +0000
reviewersjcj, mt
bugs1540541
Bug 1540541 - Don't unnecessarily strip leading 0's from key material during PKCS11 import. r=jcj,mt Differential Revision: https://phabricator.services.mozilla.com/D31671
lib/freebl/ecl/ecp_25519.c
lib/pk11wrap/pk11akey.c
lib/pk11wrap/pk11cert.c
lib/pk11wrap/pk11pk12.c
lib/softoken/legacydb/lgattr.c
lib/softoken/pkcs11c.c
--- a/lib/freebl/ecl/ecp_25519.c
+++ b/lib/freebl/ecl/ecp_25519.c
@@ -109,15 +109,18 @@ ec_Curve25519_pt_mul(SECItem *X, SECItem
         px = basePoint;
     } else {
         PORT_Assert(P->len == 32);
         if (P->len != 32) {
             return SECFailure;
         }
         px = P->data;
     }
+    if (k->len != 32) {
+        return SECFailure;
+    }
 
     SECStatus rv = ec_Curve25519_mul(X->data, k->data, px);
     if (NSS_SecureMemcmpZero(X->data, X->len) == 0) {
         return SECFailure;
     }
     return rv;
 }
--- a/lib/pk11wrap/pk11akey.c
+++ b/lib/pk11wrap/pk11akey.c
@@ -185,17 +185,16 @@ PK11_ImportPublicKey(PK11SlotInfo *slot,
                 attrs++;
                 break;
             case ecKey:
                 keyType = CKK_EC;
                 PK11_SETATTRS(attrs, CKA_VERIFY, &cktrue, sizeof(CK_BBOOL));
                 attrs++;
                 PK11_SETATTRS(attrs, CKA_DERIVE, &cktrue, sizeof(CK_BBOOL));
                 attrs++;
-                signedattr = attrs;
                 PK11_SETATTRS(attrs, CKA_EC_PARAMS,
                               pubKey->u.ec.DEREncodedParams.data,
                               pubKey->u.ec.DEREncodedParams.len);
                 attrs++;
                 if (PR_GetEnvSecure("NSS_USE_DECODED_CKA_EC_POINT")) {
                     PK11_SETATTRS(attrs, CKA_EC_POINT,
                                   pubKey->u.ec.publicValue.data,
                                   pubKey->u.ec.publicValue.len);
@@ -217,22 +216,24 @@ PK11_ImportPublicKey(PK11SlotInfo *slot,
                 break;
             default:
                 if (ckaId) {
                     SECITEM_FreeItem(ckaId, PR_TRUE);
                 }
                 PORT_SetError(SEC_ERROR_BAD_KEY);
                 return CK_INVALID_HANDLE;
         }
-
         templateCount = attrs - theTemplate;
-        signedcount = attrs - signedattr;
         PORT_Assert(templateCount <= (sizeof(theTemplate) / sizeof(CK_ATTRIBUTE)));
-        for (attrs = signedattr; signedcount; attrs++, signedcount--) {
-            pk11_SignedToUnsigned(attrs);
+        if (pubKey->keyType != ecKey) {
+            PORT_Assert(signedattr);
+            signedcount = attrs - signedattr;
+            for (attrs = signedattr; signedcount; attrs++, signedcount--) {
+                pk11_SignedToUnsigned(attrs);
+            }
         }
         rv = PK11_CreateNewObject(slot, CK_INVALID_SESSION, theTemplate,
                                   templateCount, isToken, &objectID);
         if (ckaId) {
             SECITEM_FreeItem(ckaId, PR_TRUE);
         }
         if (pubValue) {
             SECITEM_FreeItem(pubValue, PR_TRUE);
@@ -1069,19 +1070,23 @@ pk11_loadPrivKeyWithFlags(PK11SlotInfo *
         return NULL;
     }
 
     /* Set token, private, modifiable, sensitive, and extractable */
     count += pk11_AttrFlagsToAttributes(attrFlags, &privTemplate[count],
                                         &cktrue, &ckfalse);
 
     /* Not everyone can handle zero padded key values, give
-      * them the raw data as unsigned */
-    for (ap = attrs; extra_count; ap++, extra_count--) {
-        pk11_SignedToUnsigned(ap);
+     * them the raw data as unsigned. The exception is EC,
+     * where the values are encoded or zero-preserving
+     * per-RFC5915 */
+    if (privKey->keyType != ecKey) {
+        for (ap = attrs; extra_count; ap++, extra_count--) {
+            pk11_SignedToUnsigned(ap);
+        }
     }
 
     /* now Store the puppies */
     rv = PK11_CreateNewObject(slot, CK_INVALID_SESSION, privTemplate,
                               count, token, &objectID);
     PORT_FreeArena(arena, PR_TRUE);
     if (rv != SECSuccess) {
         return NULL;
--- a/lib/pk11wrap/pk11cert.c
+++ b/lib/pk11wrap/pk11cert.c
@@ -179,17 +179,19 @@ PK11_IsUserCert(PK11SlotInfo *slot, CERT
                 /* fall through and return false */
                 break;
         }
 
         if (theTemplate.ulValueLen == 0) {
             SECKEY_DestroyPublicKey(pubKey);
             return PR_FALSE;
         }
-        pk11_SignedToUnsigned(&theTemplate);
+        if (pubKey->keyType != ecKey) {
+            pk11_SignedToUnsigned(&theTemplate);
+        }
         if (pk11_FindObjectByTemplate(slot, &theTemplate, 1) != CK_INVALID_HANDLE) {
             SECKEY_DestroyPublicKey(pubKey);
             return PR_TRUE;
         }
         SECKEY_DestroyPublicKey(pubKey);
     }
     return PR_FALSE;
 }
--- a/lib/pk11wrap/pk11pk12.c
+++ b/lib/pk11wrap/pk11pk12.c
@@ -500,17 +500,17 @@ PK11_ImportAndReturnPrivateKey(PK11SlotI
                           sizeof(CK_BBOOL));
             attrs++;
             ck_id = PK11_MakeIDFromPubKey(&lpk->u.ec.publicValue);
             if (ck_id == NULL) {
                 goto loser;
             }
             PK11_SETATTRS(attrs, CKA_ID, ck_id->data, ck_id->len);
             attrs++;
-            signedattr = attrs;
+            /* No signed attrs for EC */
             /* curveOID always is a copy of AlgorithmID.parameters. */
             PK11_SETATTRS(attrs, CKA_EC_PARAMS, lpk->u.ec.curveOID.data,
                           lpk->u.ec.curveOID.len);
             attrs++;
             PK11_SETATTRS(attrs, CKA_VALUE, lpk->u.ec.privateValue.data,
                           lpk->u.ec.privateValue.len);
             attrs++;
             PK11_SETATTRS(attrs, CKA_EC_POINT, lpk->u.ec.publicValue.data,
@@ -518,21 +518,22 @@ PK11_ImportAndReturnPrivateKey(PK11SlotI
             attrs++;
             break;
         default:
             PORT_SetError(SEC_ERROR_BAD_KEY);
             goto loser;
     }
     templateCount = attrs - theTemplate;
     PORT_Assert(templateCount <= sizeof(theTemplate) / sizeof(CK_ATTRIBUTE));
-    PORT_Assert(signedattr != NULL);
-    signedcount = attrs - signedattr;
-
-    for (ap = signedattr; signedcount; ap++, signedcount--) {
-        pk11_SignedToUnsigned(ap);
+    if (lpk->keyType != ecKey) {
+        PORT_Assert(signedattr);
+        signedcount = attrs - signedattr;
+        for (ap = signedattr; signedcount; ap++, signedcount--) {
+            pk11_SignedToUnsigned(ap);
+        }
     }
 
     rv = PK11_CreateNewObject(slot, CK_INVALID_SESSION,
                               theTemplate, templateCount, isPerm, &objectID);
 
     /* create and return a SECKEYPrivateKey */
     if (rv == SECSuccess && privk != NULL) {
         *privk = PK11_MakePrivKey(slot, lpk->keyType, !isPerm, objectID, wincx);
--- a/lib/softoken/legacydb/lgattr.c
+++ b/lib/softoken/legacydb/lgattr.c
@@ -945,19 +945,19 @@ lg_FindECPrivateKeyAttribute(NSSLOWKEYPr
         case CKA_DERIVE:
         case CKA_SIGN:
             return LG_CLONE_ATTR(attribute, type, lg_StaticTrueAttr);
         case CKA_DECRYPT:
         case CKA_SIGN_RECOVER:
         case CKA_UNWRAP:
             return LG_CLONE_ATTR(attribute, type, lg_StaticFalseAttr);
         case CKA_VALUE:
-            return lg_CopyPrivAttrSigned(attribute, type,
-                                         key->u.ec.privateValue.data,
-                                         key->u.ec.privateValue.len, sdbpw);
+            return lg_CopyPrivAttribute(attribute, type,
+                                        key->u.ec.privateValue.data,
+                                        key->u.ec.privateValue.len, sdbpw);
         case CKA_EC_PARAMS:
             return lg_CopyAttributeSigned(attribute, type,
                                           key->u.ec.ecParams.DEREncoding.data,
                                           key->u.ec.ecParams.DEREncoding.len);
         case CKA_NETSCAPE_DB:
             return lg_CopyAttributeSigned(attribute, type,
                                           key->u.ec.publicValue.data,
                                           key->u.ec.publicValue.len);
--- a/lib/softoken/pkcs11c.c
+++ b/lib/softoken/pkcs11c.c
@@ -7756,17 +7756,17 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession
             }
 
             if (mechanism == CKM_ECDH1_COFACTOR_DERIVE) {
                 withCofactor = PR_TRUE;
             }
 
             rv = ECDH_Derive(&ecPoint, &privKey->u.ec.ecParams, &ecScalar,
                              withCofactor, &tmp);
-            PORT_Free(ecScalar.data);
+            PORT_ZFree(ecScalar.data, ecScalar.len);
             ecScalar.data = NULL;
             if (privKey != sourceKey->objectInfo) {
                 nsslowkey_DestroyPrivateKey(privKey);
                 privKey = NULL;
             }
             if (arena) {
                 PORT_FreeArena(arena, PR_FALSE);
                 arena = NULL;