Bug 480280 - The CKA_EC_POINT PKCS#11 attribute is encoded in the wrong way: missing encapsulating octet string

Add code to 1) accept either type of encoding on input (both NSS and softoken).
2) output the correct encoding unless the environment variable NSS_USE_DECODED_CKA_EC_POINT is set.
r= nelson

6 files changed, 549 insertions, 37 deletions

diff --git a/security/nss/lib/pk11wrap/pk11akey.c b/security/nss/lib/pk11wrap/pk11akey.c
index 5eec5130e..5095b2ae5 100644
--- a/security/nss/lib/pk11wrap/pk11akey.c
+++ b/security/nss/lib/pk11wrap/pk11akey.c
@@ -107,6 +107,7 @@ PK11_ImportPublicKey(PK11SlotInfo *slot, SECKEYPublicKey *pubKey,
     CK_ATTRIBUTE *signedattr = NULL;
     CK_ATTRIBUTE *attrs = theTemplate;
     SECItem *ckaId = NULL;
+    SECItem *pubValue = NULL;
     int signedcount = 0;
     int templateCount = 0;
     SECStatus rv;
@@ -203,8 +204,22 @@ PK11_ImportPublicKey(PK11SlotInfo *slot, SECKEYPublicKey *pubKey,
 	    PK11_SETATTRS(attrs, CKA_EC_PARAMS, 
 		          pubKey->u.ec.DEREncodedParams.data,
 		          pubKey->u.ec.DEREncodedParams.len); attrs++;
-	    PK11_SETATTRS(attrs, CKA_EC_POINT, pubKey->u.ec.publicValue.data,
+	    if (PR_GetEnv("NSS_USE_DECODED_CKA_EC_POINT")) {
+	    	PK11_SETATTRS(attrs, CKA_EC_POINT, 
+			  pubKey->u.ec.publicValue.data,
 			  pubKey->u.ec.publicValue.len); attrs++;
+	    } else {
+		pubValue = SEC_ASN1EncodeItem(NULL, NULL,
+			&pubKey->u.ec.publicValue, SEC_OctetStringTemplate);
+		if (pubValue == NULL) {
+		    if (ckaId) {
+			SECITEM_FreeItem(ckaId,PR_TRUE);
+		    }
+		    return CK_INVALID_HANDLE;
+		}
+	    	PK11_SETATTRS(attrs, CKA_EC_POINT, 
+			  pubValue->data, pubValue->len); attrs++;
+	    }
 	    break;
 	default:
 	    PORT_SetError( SEC_ERROR_BAD_KEY );
@@ -222,6 +237,9 @@ PK11_ImportPublicKey(PK11SlotInfo *slot, SECKEYPublicKey *pubKey,
 	if (ckaId) {
 	    SECITEM_FreeItem(ckaId,PR_TRUE);
 	}
+	if (pubValue) {
+	    SECITEM_FreeItem(pubValue,PR_TRUE);
+	}
 	if ( rv != SECSuccess) {
 	    return CK_INVALID_HANDLE;
 	}
@@ -237,7 +255,7 @@ PK11_ImportPublicKey(PK11SlotInfo *slot, SECKEYPublicKey *pubKey,
  * take an attribute and copy it into a secitem
  */
 static CK_RV
-pk11_Attr2SecItem(PRArenaPool *arena, CK_ATTRIBUTE *attr, SECItem *item) 
+pk11_Attr2SecItem(PRArenaPool *arena, const CK_ATTRIBUTE *attr, SECItem *item) 
 {
     item->data = NULL;
 
@@ -249,6 +267,309 @@ pk11_Attr2SecItem(PRArenaPool *arena, CK_ATTRIBUTE *attr, SECItem *item)
     return CKR_OK;
 }
 
+
+/*
+ * get a curve length from a set of ecParams.
+ * 
+ * We need this so we can reliably determine if a the ecPoint passed to us
+ * was encoded or not. With out this, for many curves, we would incorrectly
+ * identify an unencoded curve as an encoded curve 1 in 65536 times, and for
+ * a few we would make that same mistake 1 in 32768 times. These are bad 
+ * numbers since they are rare enough to pass tests, but common enough to
+ * be tripped over in the field. 
+ *
+ * This function will only work for curves we recognized as of March 2009.
+ * The assumption is curves in use after March of 2009 would be supplied by
+ * PKCS #11 modules that already pass the correct encoding to us.
+ *
+ * Point length = (Roundup(curveLenInBits/8)*2+1)
+ */
+static int
+pk11_get_EC_PointLenInBytes(PRArenaPool *arena, const SECItem *ecParams)
+{
+   SECItem oid;
+   SECOidTag tag;
+   SECStatus rv;
+
+   /* decode the OID tag */
+   rv = SEC_QuickDERDecodeItem(arena, &oid,
+		SEC_ObjectIDTemplate, ecParams);
+   if (rv != SECSuccess) {
+	/* could be explict curves, allow them to work if the 
+	 * PKCS #11 module support them. If we try to parse the
+	 * explicit curve value in the future, we may return -1 here
+	 * to indicate an invalid parameter if the explicit curve
+	 * decode fails. */
+	return 0;
+   }
+
+   tag = SECOID_FindOIDTag(&oid);
+   switch (tag) {
+    case SEC_OID_SECG_EC_SECP112R1:
+    case SEC_OID_SECG_EC_SECP112R2:
+	return 29; /* curve len in bytes = 14 bytes */
+    case SEC_OID_SECG_EC_SECT113R1:
+    case SEC_OID_SECG_EC_SECT113R2:
+	return 31; /* curve len in bytes = 15 bytes */
+    case SEC_OID_SECG_EC_SECP128R1:
+    case SEC_OID_SECG_EC_SECP128R2:
+	return 33; /* curve len in bytes = 16 bytes */
+    case SEC_OID_SECG_EC_SECT131R1:
+    case SEC_OID_SECG_EC_SECT131R2:
+	return 35; /* curve len in bytes = 17 bytes */
+    case SEC_OID_SECG_EC_SECP160K1:
+    case SEC_OID_SECG_EC_SECP160R1:
+    case SEC_OID_SECG_EC_SECP160R2:
+	return 41; /* curve len in bytes = 20 bytes */
+    case SEC_OID_SECG_EC_SECT163K1:
+    case SEC_OID_SECG_EC_SECT163R1:
+    case SEC_OID_SECG_EC_SECT163R2:
+    case SEC_OID_ANSIX962_EC_C2PNB163V1:
+    case SEC_OID_ANSIX962_EC_C2PNB163V2:
+    case SEC_OID_ANSIX962_EC_C2PNB163V3:
+	return 43; /* curve len in bytes = 21 bytes */
+    case SEC_OID_ANSIX962_EC_C2PNB176V1:
+	return 45; /* curve len in bytes = 22 bytes */
+    case SEC_OID_ANSIX962_EC_C2TNB191V1:
+    case SEC_OID_ANSIX962_EC_C2TNB191V2:
+    case SEC_OID_ANSIX962_EC_C2TNB191V3:
+    case SEC_OID_SECG_EC_SECP192K1:
+    case SEC_OID_ANSIX962_EC_PRIME192V1:
+    case SEC_OID_ANSIX962_EC_PRIME192V2:
+    case SEC_OID_ANSIX962_EC_PRIME192V3:
+	return 49; /*curve len in bytes = 24 bytes */
+    case SEC_OID_SECG_EC_SECT193R1:
+    case SEC_OID_SECG_EC_SECT193R2:
+	return 51; /*curve len in bytes = 25 bytes */
+    case SEC_OID_ANSIX962_EC_C2PNB208W1:
+	return 53; /*curve len in bytes = 26 bytes */
+    case SEC_OID_SECG_EC_SECP224K1:
+    case SEC_OID_SECG_EC_SECP224R1:
+	return 57; /*curve len in bytes = 28 bytes */
+    case SEC_OID_SECG_EC_SECT233K1:
+    case SEC_OID_SECG_EC_SECT233R1:
+    case SEC_OID_SECG_EC_SECT239K1:
+    case SEC_OID_ANSIX962_EC_PRIME239V1:
+    case SEC_OID_ANSIX962_EC_PRIME239V2:
+    case SEC_OID_ANSIX962_EC_PRIME239V3:
+    case SEC_OID_ANSIX962_EC_C2TNB239V1:
+    case SEC_OID_ANSIX962_EC_C2TNB239V2:
+    case SEC_OID_ANSIX962_EC_C2TNB239V3:
+	return 61; /*curve len in bytes = 30 bytes */
+    case SEC_OID_ANSIX962_EC_PRIME256V1:
+    case SEC_OID_SECG_EC_SECP256K1:
+	return 65; /*curve len in bytes = 32 bytes */
+    case SEC_OID_ANSIX962_EC_C2PNB272W1:
+	return 69; /*curve len in bytes = 34 bytes */
+    case SEC_OID_SECG_EC_SECT283K1:
+    case SEC_OID_SECG_EC_SECT283R1:
+	return 73; /*curve len in bytes = 36 bytes */
+    case SEC_OID_ANSIX962_EC_C2PNB304W1:
+	return 77; /*curve len in bytes = 38 bytes */
+    case SEC_OID_ANSIX962_EC_C2TNB359V1:
+	return 91; /*curve len in bytes = 45 bytes */
+    case SEC_OID_ANSIX962_EC_C2PNB368W1:
+	return 93; /*curve len in bytes = 46 bytes */
+    case SEC_OID_SECG_EC_SECP384R1:
+	return 97; /*curve len in bytes = 48 bytes */
+    case SEC_OID_SECG_EC_SECT409K1:
+    case SEC_OID_SECG_EC_SECT409R1:
+	return 105; /*curve len in bytes = 52 bytes */
+    case SEC_OID_ANSIX962_EC_C2TNB431R1:
+	return 109; /*curve len in bytes = 54 bytes */
+    case SEC_OID_SECG_EC_SECP521R1:
+	return 133; /*curve len in bytes = 66 bytes */
+    case SEC_OID_SECG_EC_SECT571K1:
+    case SEC_OID_SECG_EC_SECT571R1:
+	return 145; /*curve len in bytes = 72 bytes */
+    /* unknown or unrecognized OIDs. return unknown length */
+    default:
+	break;
+   }
+   return 0;
+}
+
+/*
+ * returns the decoded point. In some cases the point may already be decoded.
+ * this function tries to detect those cases and return the point in 
+ * publicKeyValue. In other cases it's DER encoded. In those cases the point
+ * is first decoded and returned. Space for the point is allocated out of 
+ * the passed in arena.
+ */
+static CK_RV
+pk11_get_Decoded_ECPoint(PRArenaPool *arena, const SECItem *ecParams, 
+	const CK_ATTRIBUTE *ecPoint, SECItem *publicKeyValue)
+{
+    SECItem encodedPublicValue;
+    SECStatus rv;
+    int keyLen;
+
+    if (ecPoint->ulValueLen == 0) {
+	return CKR_ATTRIBUTE_VALUE_INVALID;
+    }
+
+    /*
+     * The PKCS #11 spec requires ecPoints to be encoded as a DER OCTET String.
+     * NSS has mistakenly passed unencoded values, and some PKCS #11 vendors
+     * followed that mistake. Now we need to detect which encoding we were
+     * passed in. The task is made more complicated by the fact the the
+     * DER encoding byte (SEC_ASN_OCTET_STRING) is the same as the 
+     * EC_POINT_FORM_UNCOMPRESSED byte (0x04), so we can't use that to
+     * determine which curve we are using.
+     */
+
+    /* get the expected key length for the passed in curve.
+     * pk11_get_EC_PointLenInBytes only returns valid values for curves
+     * NSS has traditionally recognized. If the curve is not recognized,
+     * it will return '0', and we have to figure out if the key was
+     * encoded or not heuristically. If the ecParams are invalid, it
+     * will return -1 for the keyLen.
+     */
+    keyLen = pk11_get_EC_PointLenInBytes(arena, ecParams);
+    if (keyLen < 0) {
+	return CKR_ATTRIBUTE_VALUE_INVALID;
+    }
+
+
+    /* If the point is uncompressed and the lengths match, it
+     * must be an unencoded point */
+    if ((*((char *)ecPoint->pValue) == EC_POINT_FORM_UNCOMPRESSED) 
+	&& (ecPoint->ulValueLen == keyLen)) {
+	    return pk11_Attr2SecItem(arena, ecPoint, publicKeyValue);
+    }
+
+    /* now assume the key passed to us was encoded and decode it */
+    if (*((char *)ecPoint->pValue) == SEC_ASN1_OCTET_STRING) {
+	/* OK, now let's try to decode it and see if it's valid */
+	encodedPublicValue.data = ecPoint->pValue;
+	encodedPublicValue.len = ecPoint->ulValueLen;
+	rv = SEC_QuickDERDecodeItem(arena, publicKeyValue,
+		SEC_OctetStringTemplate, &encodedPublicValue);
+
+	/* it coded correctly & we know the key length (and they match)
+	 * then we are done, return the results. */
+        if (keyLen && rv == SECSuccess && publicKeyValue->len == keyLen) {
+	    return CKR_OK;
+	}
+
+	/* if we know the key length, one of the above tests should have
+	 * succeded. If it doesn't the module gave us bad data */
+	if (keyLen) {
+	    return CKR_ATTRIBUTE_VALUE_INVALID;
+	}
+		
+
+	/* We don't know the key length, so we don't know deterministically
+	 * which encoding was used. We now will try to pick the most likely 
+	 * form that's correct, with a preference for the encoded form if we
+	 * can't determine for sure. We do this by checking the key we got
+	 * back from SEC_QuickDERDecodeItem for defects. If no defects are
+	 * found, we assume the encoded paramter was was passed to us.
+	 * our defect tests include:
+	 *   1) it didn't decode.
+	 *   2) The decode key had an invalid length (must be odd).
+	 *   3) The decoded key wasn't an UNCOMPRESSED key.
+	 *   4) The decoded key didn't include the entire encoded block
+	 *   except the DER encoding values. (fixing DER length to one
+	 *   particular value).
+	 */
+	if ((rv != SECSuccess)
+	    || ((publicKeyValue->len & 1) != 1)
+	    || (publicKeyValue->data[0] != EC_POINT_FORM_UNCOMPRESSED)
+	    || (PORT_Memcmp(&encodedPublicValue.data[encodedPublicValue.len -
+		 	    publicKeyValue->len], publicKeyValue->data, 
+			    publicKeyValue->len) != 0)) {
+	    /* The decoded public key was flawed, the original key must have
+	     * already been in decoded form. Do a quick sanity check then 
+	     * return the original key value.
+	     */
+	    if ((encodedPublicValue.len & 1) == 0) {
+		return CKR_ATTRIBUTE_VALUE_INVALID;
+	    }
+	    return pk11_Attr2SecItem(arena, ecPoint, publicKeyValue);
+	}
+
+	/* as best we can figure, the passed in key was encoded, and we've
+	 * now decoded it. Note: there is a chance this could be wrong if the 
+	 * following conditions hold:
+	 *  1) The first byte or bytes of the X point looks like a valid length
+	 * of precisely the right size (2*curveSize -1). this means for curves
+	 * less than 512 bits (64 bytes), this will happen 1 in 256 times*.
+	 * for curves between 512 and 1024, this will happen 1 in 65,536 times*
+	 * for curves between 1024 and 256K this will happen 1 in 16 million*
+	 *  2) The length of the 'DER length field' is odd 
+	 * (making both the encoded and decode
+	 * values an odd length. this is true of all curves less than 512,
+	 * as well as curves between 1024 and 256K).
+	 *  3) The X[length of the 'DER length field'] == 0x04, 1 in 256.
+	 *
+	 *  (* assuming all values are equally likely in the first byte, 
+	 * This isn't true if the curve length is not a multiple of 8. In these
+	 * cases, if the DER length is possible, it's more likely, 
+	 * if it's not possible, then we have no false decodes).
+	 * 
+	 * For reference here are the odds for the various curves we currently
+	 * have support for (and the only curves SSL will negotiate at this
+	 * time). NOTE: None of the supported curves will show up here 
+	 * because we return a valid length for all of these curves. 
+	 * The only way to get here is to have some application (not SSL) 
+	 * which supports some unknown curve and have some vendor supplied 
+	 * PKCS #11 module support that curve. NOTE: in this case, one 
+	 * presumes that that pkcs #11 module is likely to be using the 
+	 * correct encodings.
+	 *
+	 * Prime Curves (GFp):
+	 *   Bit	False	    Odds of 
+	 *  Size	DER Len	 False Decode Positive
+	 *  112 	27	   1 in 65536 
+	 *  128 	31	   1 in 65536 
+	 *  160 	39	   1 in 65536 
+	 *  192 	47	   1 in 65536 
+	 *  224 	55	   1 in 65536 
+	 *  239 	59	   1 in 32768 (top byte can only be 0-127)
+	 *  256 	63	   1 in 65536 
+	 *  521 	129,131	     0        (decoded value would be even)
+	 *
+	 * Binary curves (GF2m).
+	 *   Bit	False	    Odds of 
+	 *  Size	DER Len	 False Decode Positive
+	 *  131 	33	     0        (top byte can only be 0-7)
+	 *  163 	41	     0        (top byte can only be 0-7)
+	 *  176 	43	   1 in 65536 
+	 *  191 	47	   1 in 32768 (top byte can only be 0-127)
+	 *  193 	49	     0        (top byte can only be 0-1)
+	 *  208 	51	   1 in 65536 
+	 *  233 	59	     0        (top byte can only be 0-1)
+	 *  239 	59	   1 in 32768 (top byte can only be 0-127)
+	 *  272 	67	   1 in 65536 
+	 *  283 	71	     0        (top byte can only be 0-7)
+	 *  304 	75	   1 in 65536 
+	 *  359 	89	   1 in 32768 (top byte can only be 0-127)
+	 *  368 	91	   1 in 65536 
+	 *  409 	103	     0        (top byte can only be 0-1)
+	 *  431 	107	   1 in 32768 (top byte can only be 0-127)
+	 *  571 	129,143	     0        (decoded value would be even)
+	 *
+	 */
+
+	return CKR_OK;
+    }
+
+    /* In theory, we should handle the case where the curve == 0 and
+     * the first byte is EC_POINT_FORM_UNCOMPRESSED, (which would be
+     * handled by doing a santity check on the key length and returning
+     * pk11_Attr2SecItem() to copy the ecPoint to the publicKeyValue).
+     *
+     * This test is unnecessary, however, due to the fact that 
+     * EC_POINT_FORM_UNCOMPRESSED == SEC_ASIN1_OCTET_STRING, that case is
+     * handled in the above if. That means if we get here, the initial
+     * byte of our ecPoint value was invalid, so we can safely return.
+     * invalid attribute.
+     */
+	
+    return CKR_ATTRIBUTE_VALUE_INVALID;
+}
+
 /*
  * extract a public key from a slot and id
  */
@@ -399,7 +720,7 @@ PK11_ExtractPublicKey(PK11SlotInfo *slot,KeyType keyType,CK_OBJECT_HANDLE id)
 	PK11_SETATTRS(attrs, CKA_EC_POINT, NULL, 0); attrs++; 
 	templateCount = attrs - template;
 	PR_ASSERT(templateCount <= sizeof(template)/sizeof(CK_ATTRIBUTE));
-	crv = PK11_GetAttributes(tmp_arena,slot,id,template,templateCount);
+	crv = PK11_GetAttributes(arena,slot,id,template,templateCount);
 	if (crv != CKR_OK) break;
 
 	if ((keyClass != CKO_PUBLIC_KEY) || (pk11KeyType != CKK_EC)) {
@@ -410,8 +731,9 @@ PK11_ExtractPublicKey(PK11SlotInfo *slot,KeyType keyType,CK_OBJECT_HANDLE id)
 	crv = pk11_Attr2SecItem(arena,ecparams,
 	                        &pubKey->u.ec.DEREncodedParams);
 	if (crv != CKR_OK) break;
-	crv = pk11_Attr2SecItem(arena,value,&pubKey->u.ec.publicValue);
-	if (crv != CKR_OK) break;
+	crv = pk11_get_Decoded_ECPoint(arena,
+		 &pubKey->u.ec.DEREncodedParams, value, 
+		 &pubKey->u.ec.publicValue);
 	break;
     case fortezzaKey:
     case nullKey:
diff --git a/security/nss/lib/pk11wrap/pk11skey.c b/security/nss/lib/pk11wrap/pk11skey.c
index 0527dcd0e..20e155e5b 100644
--- a/security/nss/lib/pk11wrap/pk11skey.c
+++ b/security/nss/lib/pk11wrap/pk11skey.c
@@ -1614,6 +1614,7 @@ PK11_PubDerive(SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey,
 	    mechanism.mechanism = derive;
 
 	    /* we can undefine these when we define diffie-helman keys */
+
 	    mechanism.pParameter = pubKey->u.dh.publicValue.data; 
 	    mechanism.ulParameterLen = pubKey->u.dh.publicValue.len;
 		
@@ -1635,6 +1636,7 @@ PK11_PubDerive(SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey,
 	    int templateCount;
 	    CK_ATTRIBUTE *attrs = keyTemplate;
 	    CK_ECDH1_DERIVE_PARAMS *mechParams = NULL;
+	    SECItem *pubValue = NULL;
 
 	    if (pubKey->keyType != ecKey) {
 		PORT_SetError(SEC_ERROR_BAD_KEY);
@@ -1660,8 +1662,20 @@ PK11_PubDerive(SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey,
 	    mechParams->kdf = CKD_SHA1_KDF;
 	    mechParams->ulSharedDataLen = 0;
 	    mechParams->pSharedData = NULL;
-	    mechParams->ulPublicDataLen =  pubKey->u.ec.publicValue.len;
-	    mechParams->pPublicData =  pubKey->u.ec.publicValue.data;
+
+	    if (PR_GetEnv("NSS_USE_DECODED_CKA_EC_POINT")) {
+		mechParams->ulPublicDataLen =  pubKey->u.ec.publicValue.len;
+		mechParams->pPublicData =  pubKey->u.ec.publicValue.data;
+	    } else {
+		pubValue = SEC_ASN1EncodeItem(NULL, NULL,
+			&pubKey->u.ec.publicValue, SEC_OctetStringTemplate);
+		if (pubValue == NULL) {
+	    	    PORT_ZFree(mechParams, sizeof(CK_ECDH1_DERIVE_PARAMS));
+		    break;
+		}
+		mechParams->ulPublicDataLen =  pubValue->len;
+		mechParams->pPublicData =  pubValue->data;
+	    }
 
 	    mechanism.mechanism = derive;
 	    mechanism.pParameter = mechParams;
@@ -1673,6 +1687,9 @@ PK11_PubDerive(SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey,
 		templateCount, &symKey->objectID);
 	    pk11_ExitKeyMonitor(symKey);
 
+	    if (pubValue) {
+		SECITEM_FreeItem(pubValue,PR_TRUE);
+	    }
 	    PORT_ZFree(mechParams, sizeof(CK_ECDH1_DERIVE_PARAMS));
 
 	    if (crv == CKR_OK) return symKey;
@@ -1704,6 +1721,7 @@ pk11_PubDeriveECKeyWithKDF(
     int                     templateCount;
     CK_ATTRIBUTE           *attrs           = keyTemplate;
     CK_ECDH1_DERIVE_PARAMS *mechParams      = NULL;
+    SECItem *pubValue = NULL;
 
     if (pubKey->keyType != ecKey) {
 	PORT_SetError(SEC_ERROR_BAD_KEY);
@@ -1748,8 +1766,20 @@ pk11_PubDeriveECKeyWithKDF(
 	mechParams->ulSharedDataLen = sharedData->len;
 	mechParams->pSharedData     = sharedData->data;
     }
-    mechParams->ulPublicDataLen = pubKey->u.ec.publicValue.len;
-    mechParams->pPublicData     = pubKey->u.ec.publicValue.data;
+    if (PR_GetEnv("NSS_USE_DECODED_CKA_EC_POINT")) {
+	mechParams->ulPublicDataLen =  pubKey->u.ec.publicValue.len;
+	mechParams->pPublicData =  pubKey->u.ec.publicValue.data;
+    } else {
+	pubValue = SEC_ASN1EncodeItem(NULL, NULL,
+		&pubKey->u.ec.publicValue, SEC_OctetStringTemplate);
+	if (pubValue == NULL) {
+    	    PORT_ZFree(mechParams, sizeof(CK_ECDH1_DERIVE_PARAMS));
+	    PK11_FreeSymKey(symKey);
+	    return NULL;
+	}
+	mechParams->ulPublicDataLen =  pubValue->len;
+	mechParams->pPublicData =  pubValue->data;
+    }
 
     mechanism.mechanism      = derive;
     mechanism.pParameter     = mechParams;
@@ -1761,6 +1791,9 @@ pk11_PubDeriveECKeyWithKDF(
     pk11_ExitKeyMonitor(symKey);
 
     PORT_ZFree(mechParams, sizeof(CK_ECDH1_DERIVE_PARAMS));
+    if (pubValue) {
+	SECITEM_FreeItem(pubValue,PR_TRUE);
+    }
 
     if (crv != CKR_OK) {
 	PK11_FreeSymKey(symKey);
diff --git a/security/nss/lib/softoken/legacydb/lgattr.c b/security/nss/lib/softoken/legacydb/lgattr.c
index 15cfc63ec..8c53ed6ab 100644
--- a/security/nss/lib/softoken/legacydb/lgattr.c
+++ b/security/nss/lib/softoken/legacydb/lgattr.c
@@ -45,6 +45,7 @@
 #include "pcert.h"
 #include "blapi.h"
 #include "secerr.h"
+#include "secasn1.h"
 
 /*
  * Cache the object we are working on during Set's and Get's
@@ -610,9 +611,23 @@ lg_FindECPublicKeyAttribute(NSSLOWKEYPublicKey *key, CK_ATTRIBUTE_TYPE type,
 					key->u.ec.ecParams.DEREncoding.data,
 					key->u.ec.ecParams.DEREncoding.len);
     case CKA_EC_POINT:
-	return lg_CopyAttributeSigned(attribute, type,
+	if (getenv("NSS_USE_DECODED_CKA_EC_POINT")) {
+	    return lg_CopyAttributeSigned(attribute, type,
 					key->u.ec.publicValue.data,
 					key->u.ec.publicValue.len);
+	} else {
+	    SECItem *pubValue = SEC_ASN1EncodeItem(NULL, NULL, 
+			&(key->u.ec.publicValue), SEC_OctetStringTemplate);
+	    CK_RV crv;
+	    if (!pubValue) {
+		return CKR_HOST_MEMORY;
+	    }
+	    crv = lg_CopyAttributeSigned(attribute, type,
+					pubValue->data,
+					pubValue->len);
+	    SECITEM_FreeItem(pubValue, PR_TRUE);
+	    return crv;
+	}
     default:
 	break;
     }
diff --git a/security/nss/lib/softoken/legacydb/lgcreate.c b/security/nss/lib/softoken/legacydb/lgcreate.c
index 1dd1b2709..d4fcca69e 100644
--- a/security/nss/lib/softoken/legacydb/lgcreate.c
+++ b/security/nss/lib/softoken/legacydb/lgcreate.c
@@ -41,6 +41,7 @@
 #include "lowkeyi.h"
 #include "blapi.h"
 #include "secder.h"
+#include "secasn1.h"
 
 #include "keydbi.h" 
 
@@ -429,10 +430,16 @@ lg_createPublicKeyObject(SDB *sdb, CK_KEY_TYPE key_type,
      CK_OBJECT_HANDLE *handle, const CK_ATTRIBUTE *templ, CK_ULONG count)
 {
     CK_ATTRIBUTE_TYPE pubKeyAttr = CKA_VALUE;
-    CK_RV crv;
+    CK_RV crv = CKR_OK;
     NSSLOWKEYPrivateKey *priv;
-    SECItem pubKey;
+    SECItem pubKeySpace = {siBuffer, NULL, 0};
+    SECItem *pubKey;
+#ifdef NSS_ENABLE_ECC
+    SECItem pubKey2Space = {siBuffer, NULL, 0};
+    PRArenaPool *arena = NULL;
+#endif /* NSS_ENABLE_ECC */
     NSSLOWKEYDBHandle *keyHandle = NULL;
+	
 
     switch (key_type) {
     case CKK_RSA:
@@ -451,34 +458,80 @@ lg_createPublicKeyObject(SDB *sdb, CK_KEY_TYPE key_type,
     }
 
 
-    crv = lg_Attribute2SSecItem(NULL,pubKeyAttr,templ,count,&pubKey);
+    pubKey = &pubKeySpace;
+    crv = lg_Attribute2SSecItem(NULL,pubKeyAttr,templ,count,pubKey);
     if (crv != CKR_OK) return crv;
 
-    PORT_Assert(pubKey.data);
+#ifdef NSS_ENABLE_ECC
+    if (key_type == CKK_EC) {
+	SECStatus rv;
+	/*
+	 * for ECC, use the decoded key first.
+	 */
+	arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
+	if (arena == NULL) {
+	    crv = CKR_HOST_MEMORY;
+	    goto done;
+	}
+	rv= SEC_QuickDERDecodeItem(arena, &pubKey2Space, 
+				SEC_OctetStringTemplate, pubKey);
+	if (rv != SECSuccess) {
+	    /* decode didn't work, just try the pubKey */
+	    PORT_FreeArena(arena, PR_FALSE);
+	    arena = NULL;
+	} else {
+	    /* try the decoded pub key first */
+	    pubKey = &pubKey2Space;
+	}
+    }
+#endif /* NSS_ENABLE_ECC */
+
+    PORT_Assert(pubKey->data);
+    if (pubKey->data == NULL) {
+	crv = CKR_ATTRIBUTE_VALUE_INVALID;
+	goto done;
+    }
     keyHandle = lg_getKeyDB(sdb);
     if (keyHandle == NULL) {
-	PORT_Free(pubKey.data);
-	return CKR_TOKEN_WRITE_PROTECTED;
+	crv = CKR_TOKEN_WRITE_PROTECTED;
+	goto done;
     }
     if (keyHandle->version != 3) {
 	unsigned char buf[SHA1_LENGTH];
-	SHA1_HashBuf(buf,pubKey.data,pubKey.len);
-	PORT_Memcpy(pubKey.data,buf,sizeof(buf));
-	pubKey.len = sizeof(buf);
+	SHA1_HashBuf(buf,pubKey->data,pubKey->len);
+	PORT_Memcpy(pubKey->data,buf,sizeof(buf));
+	pubKey->len = sizeof(buf);
     }
     /* make sure the associated private key already exists */
     /* only works if we are logged in */
-    priv = nsslowkey_FindKeyByPublicKey(keyHandle, &pubKey, sdb /*password*/);
+    priv = nsslowkey_FindKeyByPublicKey(keyHandle, pubKey, sdb /*password*/);
+#ifdef NSS_ENABLE_ECC
+    if (priv == NULL && pubKey == &pubKey2Space) {
+	/* no match on the decoded key, match the original pubkey */
+	pubKey = &pubKeySpace;
+    	priv = nsslowkey_FindKeyByPublicKey(keyHandle, pubKey, 
+					    sdb /*password*/);
+    }
+#endif
     if (priv == NULL) {
-	PORT_Free(pubKey.data);
-	return crv;
+	/* the legacy database can only 'store' public keys which already
+	 * have their corresponding private keys in the database */
+	crv = CKR_ATTRIBUTE_VALUE_INVALID;
+	goto done;
     }
     nsslowkey_DestroyPrivateKey(priv);
+    crv = CKR_OK;
 
-    *handle = lg_mkHandle(sdb, &pubKey, LG_TOKEN_TYPE_PUB);
-    PORT_Free(pubKey.data);
+    *handle = lg_mkHandle(sdb, pubKey, LG_TOKEN_TYPE_PUB);
 
-    return CKR_OK;
+done:
+    PORT_Free(pubKeySpace.data);
+#ifdef NSS_ENABLE_ECC
+    if (arena) 
+	PORT_FreeArena(arena, PR_FALSE);
+#endif
+
+    return crv;
 }
 
 /* make a private key from a verified object */
diff --git a/security/nss/lib/softoken/pkcs11.c b/security/nss/lib/softoken/pkcs11.c
index 19882fe89..71eaefb6e 100644
--- a/security/nss/lib/softoken/pkcs11.c
+++ b/security/nss/lib/softoken/pkcs11.c
@@ -67,6 +67,8 @@
 #include "secoid.h"
 #include "sftkdb.h"
 #include "sftkpars.h"
+#include "ec.h"
+#include "secasn1.h"
 
 PRBool parentForkedAfterC_Initialize;
 
@@ -851,7 +853,6 @@ sftk_handlePublicKeyObject(SFTKSession *session, SFTKObject *object,
     CK_BBOOL wrap = CK_TRUE;
     CK_BBOOL derive = CK_FALSE;
     CK_BBOOL verify = CK_TRUE;
-    CK_ATTRIBUTE_TYPE pubKeyAttr = CKA_VALUE;
     CK_RV crv;
 
     switch (key_type) {
@@ -865,7 +866,6 @@ sftk_handlePublicKeyObject(SFTKSession *session, SFTKObject *object,
 	if (crv != CKR_OK) {
 	    return crv;
 	}
-	pubKeyAttr = CKA_MODULUS;
 	break;
     case CKK_DSA:
 	crv = sftk_ConstrainAttribute(object, CKA_SUBPRIME, 
@@ -918,7 +918,6 @@ sftk_handlePublicKeyObject(SFTKSession *session, SFTKObject *object,
 	if ( !sftk_hasAttribute(object, CKA_EC_POINT)) {
 	    return CKR_TEMPLATE_INCOMPLETE;
 	}
-	pubKeyAttr = CKA_EC_POINT;
 	derive = CK_TRUE;    /* for ECDH */
 	verify = CK_TRUE;    /* for ECDSA */
 	encrypt = CK_FALSE;
@@ -1258,7 +1257,6 @@ sftk_handleKeyObject(SFTKSession *session, SFTKObject *object)
 {
     SFTKAttribute *attribute;
     CK_KEY_TYPE key_type;
-    CK_BBOOL cktrue = CK_TRUE;
     CK_BBOOL ckfalse = CK_FALSE;
     CK_RV crv;
 
@@ -1636,6 +1634,45 @@ NSSLOWKEYPublicKey *sftk_GetPubKey(SFTKObject *object,CK_KEY_TYPE key_type,
 	    
 	crv = sftk_Attribute2SSecItem(arena,&pubKey->u.ec.publicValue,
 	                              object,CKA_EC_POINT);
+	if (crv == CKR_OK) {
+	    int keyLen,curveLen;
+
+	    curveLen = (pubKey->u.ec.ecParams.fieldID.size +7)/8;
+	    keyLen = (2*curveLen)+1;
+
+	    /* special note: We can't just use the first byte to determine
+	     * between these 2 cases because both EC_POINT_FORM_UNCOMPRESSED 
+	     * and SEC_ASN1_OCTET_STRING are 0x04 */
+
+	    /* handle the non-DER encoded case (UNCOMPRESSED only) */	
+	    if (pubKey->u.ec.publicValue.data[0] == EC_POINT_FORM_UNCOMPRESSED
+		&& pubKey->u.ec.publicValue.len == keyLen) {
+		break; /* key was not DER encoded, no need to unwrap */
+	    }
+
+	    /* if we ever support compressed, handle it here */
+
+	    /* handle the encoded case */
+	    if ((pubKey->u.ec.publicValue.data[0] == SEC_ASN1_OCTET_STRING) 
+		&& pubKey->u.ec.publicValue.len > keyLen) {
+		SECItem publicValue;
+		SECStatus rv;
+
+		rv = SEC_QuickDERDecodeItem(arena, &publicValue, 
+			 SEC_OctetStringTemplate, &pubKey->u.ec.publicValue);
+		/* nope, didn't decode correctly */
+		if ((rv != SECSuccess)
+		    || (publicValue.data[0] != EC_POINT_FORM_UNCOMPRESSED)
+		    || (publicValue.len != keyLen)) {
+	   	    crv = CKR_ATTRIBUTE_VALUE_INVALID;
+		    break;
+		}
+		/* replace our previous with the decoded key */
+		pubKey->u.ec.publicValue = publicValue;
+		break;
+	    }
+	   crv = CKR_ATTRIBUTE_VALUE_INVALID;
+	}
 	break;
 #endif /* NSS_ENABLE_ECC */
     default:
diff --git a/security/nss/lib/softoken/pkcs11c.c b/security/nss/lib/softoken/pkcs11c.c
index 5ebbf8608..49f00109f 100644
--- a/security/nss/lib/softoken/pkcs11c.c
+++ b/security/nss/lib/softoken/pkcs11c.c
@@ -4017,8 +4017,20 @@ dhgn_done:
 	    break;
 	}
 
-	crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT, 
+	if (getenv("NSS_USE_DECODED_CKA_EC_POINT")) {
+	    crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT, 
 				sftk_item_expand(&ecPriv->publicValue));
+	} else {
+	    SECItem *pubValue = SEC_ASN1EncodeItem(NULL, NULL, 
+			&ecPriv->publicValue, SEC_OctetStringTemplate);
+	    if (!pubValue) {
+		crv = CKR_ARGUMENTS_BAD;
+		goto ecgn_done;
+	    }
+	    crv = sftk_AddAttributeType(publicKey, CKA_EC_POINT, 
+				sftk_item_expand(pubValue));
+	    SECITEM_FreeItem(pubValue, PR_TRUE);
+	}
 	if (crv != CKR_OK) goto ecgn_done;
 
 	crv = sftk_AddAttributeType(privateKey, CKA_VALUE, 
@@ -5843,9 +5855,10 @@ key_and_mac_derive_fail:
 	unsigned char secret_hash[20];
 	unsigned char *secret;
 	unsigned char *keyData = NULL;
-	int secretlen;
+	int secretlen, curveLen, pubKeyLen;
 	CK_ECDH1_DERIVE_PARAMS *mechParams;
 	NSSLOWKEYPrivateKey *privKey;
+	PLArenaPool *arena = NULL;
 
 	/* Check mechanism parameters */
 	mechParams = (CK_ECDH1_DERIVE_PARAMS *) pMechanism->pParameter;
@@ -5868,6 +5881,31 @@ key_and_mac_derive_fail:
 	ecPoint.data = mechParams->pPublicData;
 	ecPoint.len  = mechParams->ulPublicDataLen;
 
+	curveLen = (privKey->u.ec.ecParams.fieldID.size +7)/8;
+	pubKeyLen = (2*curveLen) + 1;
+
+	/* if the len is too small, can't be a valid point */
+	if (ecPoint.len < pubKeyLen) {
+	    goto ec_loser;
+	}
+	/* if the len is too large, must be an encoded point (length is
+	 * equal case just falls through */
+	if (ecPoint.len > pubKeyLen) {
+	    SECItem newPoint;
+
+	    arena = PORT_NewArena(DER_DEFAULT_CHUNKSIZE);
+	    if (arena == NULL) {
+		goto ec_loser;
+	    }
+
+	    rv = SEC_QuickDERDecodeItem(arena, &newPoint, 
+			SEC_OctetStringTemplate, &ecPoint);
+	    if (rv != SECSuccess) {
+		goto ec_loser;
+	    }
+	    ecPoint = newPoint;
+	}
+
 	if (pMechanism->mechanism == CKM_ECDH1_COFACTOR_DERIVE) {
 	    withCofactor = PR_TRUE;
 	} else {
@@ -5877,19 +5915,22 @@ key_and_mac_derive_fail:
 	     */
 	    if (EC_ValidatePublicKey(&privKey->u.ec.ecParams, &ecPoint) 
 		!= SECSuccess) {
-		crv = CKR_ARGUMENTS_BAD;
-		PORT_Free(ecScalar.data);
-		if (privKey != sourceKey->objectInfo)
-		    nsslowkey_DestroyPrivateKey(privKey);
-		break;
+		goto ec_loser;
 	    }
 	}
 
 	rv = ECDH_Derive(&ecPoint, &privKey->u.ec.ecParams, &ecScalar,
 	                 withCofactor, &tmp); 
 	PORT_Free(ecScalar.data);
-	if (privKey != sourceKey->objectInfo)
+	ecScalar.data = NULL;
+	if (privKey != sourceKey->objectInfo) {
 	   nsslowkey_DestroyPrivateKey(privKey);
+	   privKey=NULL;
+	}
+	if (arena) {
+	    PORT_FreeArena(arena,PR_FALSE);
+	    arena=NULL;
+	}
 
 	if (rv != SECSuccess) {
 	    crv = sftk_MapCryptError(PORT_GetError());
@@ -5951,6 +5992,17 @@ key_and_mac_derive_fail:
 	PORT_Memset(secret_hash, 0, 20);
 	    
 	break;
+
+ec_loser:
+	crv = CKR_ARGUMENTS_BAD;
+	PORT_Free(ecScalar.data);
+	if (privKey != sourceKey->objectInfo)
+	    nsslowkey_DestroyPrivateKey(privKey);
+	if (arena) {
+	    PORT_FreeArena(arena, PR_FALSE);
+	}
+	break;
+
       }
 #endif /* NSS_ENABLE_ECC */