Bug 1340103, Introduction of SECKEYECPublicKey.encoding in NSS 3.28 broke ABI, r=rrelyea/mt

7 files changed, 69 insertions, 64 deletions

diff --git a/lib/cryptohi/keyi.h b/lib/cryptohi/keyi.h
index 374a4ad9b..f8f5f7f7d 100644
--- a/lib/cryptohi/keyi.h
+++ b/lib/cryptohi/keyi.h
@@ -17,13 +17,6 @@ KeyType seckey_GetKeyType(SECOidTag pubKeyOid);
 SECStatus sec_DecodeSigAlg(const SECKEYPublicKey *key, SECOidTag sigAlg,
                            const SECItem *param, SECOidTag *encalg, SECOidTag *hashalg);
 
-/*
- * Set the point encoding of a SECKEYPublicKey from the OID.
- * This has to be called on any SECKEYPublicKey holding a SECKEYECPublicKey
- * before it can be used. The encoding is used to dermine the public key size.
- */
-SECStatus seckey_SetPointEncoding(PLArenaPool *arena, SECKEYPublicKey *pubKey);
-
 SEC_END_PROTOS
 
 #endif /* _KEYHI_H_ */
diff --git a/lib/cryptohi/keythi.h b/lib/cryptohi/keythi.h
index 1555ce2e2..36896540f 100644
--- a/lib/cryptohi/keythi.h
+++ b/lib/cryptohi/keythi.h
@@ -125,9 +125,9 @@ typedef SECItem SECKEYECParams;
 
 struct SECKEYECPublicKeyStr {
     SECKEYECParams DEREncodedParams;
-    int size;            /* size in bits */
-    SECItem publicValue; /* encoded point */
-    ECPointEncoding encoding;
+    int size;                 /* size in bits */
+    SECItem publicValue;      /* encoded point */
+    ECPointEncoding encoding; /* deprecated, ignored */
 };
 typedef struct SECKEYECPublicKeyStr SECKEYECPublicKey;
 
diff --git a/lib/cryptohi/seckey.c b/lib/cryptohi/seckey.c
index 1f053e583..359de8e46 100644
--- a/lib/cryptohi/seckey.c
+++ b/lib/cryptohi/seckey.c
@@ -547,6 +547,23 @@ CERT_GetCertKeyType(const CERTSubjectPublicKeyInfo *spki)
     return seckey_GetKeyType(SECOID_GetAlgorithmTag(&spki->algorithm));
 }
 
+/* Ensure pubKey contains an OID */
+static SECStatus
+seckey_HasCurveOID(const SECKEYPublicKey *pubKey)
+{
+    SECItem oid;
+    SECStatus rv;
+    PORTCheapArenaPool tmpArena;
+
+    PORT_InitCheapArena(&tmpArena, DER_DEFAULT_CHUNKSIZE);
+    /* If we can decode it, an OID is available. */
+    rv = SEC_QuickDERDecodeItem(&tmpArena.arena, &oid,
+                                SEC_ASN1_GET(SEC_ObjectIDTemplate),
+                                &pubKey->u.ec.DEREncodedParams);
+    PORT_DestroyCheapArena(&tmpArena);
+    return rv;
+}
+
 static SECKEYPublicKey *
 seckey_ExtractPublicKey(const CERTSubjectPublicKeyInfo *spki)
 {
@@ -639,7 +656,8 @@ seckey_ExtractPublicKey(const CERTSubjectPublicKeyInfo *spki)
                 if (rv != SECSuccess) {
                     break;
                 }
-                rv = seckey_SetPointEncoding(arena, pubk);
+                pubk->u.ec.encoding = ECPoint_Undefined;
+                rv = seckey_HasCurveOID(pubk);
                 if (rv == SECSuccess) {
                     return pubk;
                 }
@@ -1162,16 +1180,16 @@ SECKEY_CopyPublicKey(const SECKEYPublicKey *pubk)
             break;
         case ecKey:
             copyk->u.ec.size = pubk->u.ec.size;
-            rv = SECITEM_CopyItem(arena, &copyk->u.ec.DEREncodedParams,
-                                  &pubk->u.ec.DEREncodedParams);
+            rv = seckey_HasCurveOID(pubk);
             if (rv != SECSuccess) {
                 break;
             }
-            rv = seckey_SetPointEncoding(arena, copyk);
+            rv = SECITEM_CopyItem(arena, &copyk->u.ec.DEREncodedParams,
+                                  &pubk->u.ec.DEREncodedParams);
             if (rv != SECSuccess) {
                 break;
             }
-            PORT_Assert(copyk->u.ec.encoding == pubk->u.ec.encoding);
+            copyk->u.ec.encoding = ECPoint_Undefined;
             rv = SECITEM_CopyItem(arena, &copyk->u.ec.publicValue,
                                   &pubk->u.ec.publicValue);
             break;
@@ -1943,39 +1961,3 @@ SECKEY_GetECCOid(const SECKEYECParams *params)
 
     return oidData->offset;
 }
-
-/* Set curve encoding in SECKEYECPublicKey in pubKey from OID.
- * If the encoding is not set, determining the key size of EC public keys will
- * fail.
- */
-SECStatus
-seckey_SetPointEncoding(PLArenaPool *arena, SECKEYPublicKey *pubKey)
-{
-    SECItem oid;
-    SECOidTag tag;
-    SECStatus rv;
-
-    /* decode the OID tag */
-    rv = SEC_QuickDERDecodeItem(arena, &oid, SEC_ASN1_GET(SEC_ObjectIDTemplate),
-                                &pubKey->u.ec.DEREncodedParams);
-    if (rv != SECSuccess) {
-        return SECFailure;
-    }
-
-    tag = SECOID_FindOIDTag(&oid);
-    switch (tag) {
-        case SEC_OID_CURVE25519:
-            pubKey->u.ec.encoding = ECPoint_XOnly;
-            break;
-        case SEC_OID_SECG_EC_SECP256R1:
-        /* fall through */
-        case SEC_OID_SECG_EC_SECP384R1:
-        /* fall through */
-        case SEC_OID_SECG_EC_SECP521R1:
-        /* fall through */
-        default:
-            /* unknown curve, default to uncompressed */
-            pubKey->u.ec.encoding = ECPoint_Uncompressed;
-    }
-    return SECSuccess;
-}
diff --git a/lib/pk11wrap/pk11akey.c b/lib/pk11wrap/pk11akey.c
index d086ed4e1..01d1d7fd9 100644
--- a/lib/pk11wrap/pk11akey.c
+++ b/lib/pk11wrap/pk11akey.c
@@ -765,12 +765,10 @@ PK11_ExtractPublicKey(PK11SlotInfo *slot, KeyType keyType, CK_OBJECT_HANDLE id)
                                     &pubKey->u.ec.DEREncodedParams);
             if (crv != CKR_OK)
                 break;
+            pubKey->u.ec.encoding = ECPoint_Undefined;
             crv = pk11_get_Decoded_ECPoint(arena,
                                            &pubKey->u.ec.DEREncodedParams, value,
                                            &pubKey->u.ec.publicValue);
-            if (seckey_SetPointEncoding(arena, pubKey) != SECSuccess) {
-                crv |= CKR_GENERAL_ERROR;
-            }
             break;
         case fortezzaKey:
         case nullKey:
diff --git a/lib/pk11wrap/pk11skey.c b/lib/pk11wrap/pk11skey.c
index 850ec026b..e6301388e 100644
--- a/lib/pk11wrap/pk11skey.c
+++ b/lib/pk11wrap/pk11skey.c
@@ -2037,6 +2037,40 @@ PK11_PubDerive(SECKEYPrivateKey *privKey, SECKEYPublicKey *pubKey,
     return NULL;
 }
 
+/* Test for curves that are known to use a special encoding.
+ * Extend this function when additional curves are added. */
+static ECPointEncoding
+pk11_ECGetPubkeyEncoding(const SECKEYPublicKey *pubKey)
+{
+    SECItem oid;
+    SECStatus rv;
+    PORTCheapArenaPool tmpArena;
+    ECPointEncoding encoding = ECPoint_Undefined;
+
+    PORT_InitCheapArena(&tmpArena, DER_DEFAULT_CHUNKSIZE);
+
+    /* decode the OID tag */
+    rv = SEC_QuickDERDecodeItem(&tmpArena.arena, &oid,
+                                SEC_ASN1_GET(SEC_ObjectIDTemplate),
+                                &pubKey->u.ec.DEREncodedParams);
+    if (rv == SECSuccess) {
+        SECOidTag tag = SECOID_FindOIDTag(&oid);
+        switch (tag) {
+            case SEC_OID_CURVE25519:
+                encoding = ECPoint_XOnly;
+                break;
+            case SEC_OID_SECG_EC_SECP256R1:
+            case SEC_OID_SECG_EC_SECP384R1:
+            case SEC_OID_SECG_EC_SECP521R1:
+            default:
+                /* unknown curve, default to uncompressed */
+                encoding = ECPoint_Uncompressed;
+        }
+    }
+    PORT_DestroyCheapArena(&tmpArena);
+    return encoding;
+}
+
 /* Returns the size of the public key, or 0 if there
  * is an error. */
 static CK_ULONG
@@ -2044,10 +2078,11 @@ pk11_ECPubKeySize(SECKEYPublicKey *pubKey)
 {
     SECItem *publicValue = &pubKey->u.ec.publicValue;
 
-    if (pubKey->u.ec.encoding == ECPoint_XOnly) {
+    ECPointEncoding encoding = pk11_ECGetPubkeyEncoding(pubKey);
+    if (encoding == ECPoint_XOnly) {
         return publicValue->len;
     }
-    if (publicValue->data[0] == 0x04) {
+    if (encoding == ECPoint_Uncompressed) {
         /* key encoded in uncompressed form */
         return ((publicValue->len - 1) / 2);
     }
diff --git a/lib/ssl/ssl3ecc.c b/lib/ssl/ssl3ecc.c
index 72c4ba56e..9f2f4d621 100644
--- a/lib/ssl/ssl3ecc.c
+++ b/lib/ssl/ssl3ecc.c
@@ -303,7 +303,7 @@ ssl3_HandleECDHClientKeyExchange(sslSocket *ss, SSL3Opaque *b,
         serverKeyPair->pubKey->u.ec.DEREncodedParams.len;
     clntPubKey.u.ec.DEREncodedParams.data =
         serverKeyPair->pubKey->u.ec.DEREncodedParams.data;
-    clntPubKey.u.ec.encoding = serverKeyPair->pubKey->u.ec.encoding;
+    clntPubKey.u.ec.encoding = ECPoint_Undefined;
 
     rv = ssl3_ConsumeHandshakeVariable(ss, &clntPubKey.u.ec.publicValue,
                                        1, &b, &length);
@@ -387,11 +387,7 @@ ssl_ImportECDHKeyShare(sslSocket *ss, SECKEYPublicKey *peerKey,
         ssl_MapLowLevelError(SSL_ERROR_RX_MALFORMED_ECDHE_KEY_SHARE);
         return SECFailure;
     }
-    if (ecGroup->name == ssl_grp_ec_curve25519) {
-        peerKey->u.ec.encoding = ECPoint_XOnly;
-    } else {
-        peerKey->u.ec.encoding = ECPoint_Uncompressed;
-    }
+    peerKey->u.ec.encoding = ECPoint_Undefined;
 
     /* copy publicValue in peerKey */
     ecPoint.data = b;
diff --git a/lib/util/eccutil.h b/lib/util/eccutil.h
index 0d4caad28..8c627e18b 100644
--- a/lib/util/eccutil.h
+++ b/lib/util/eccutil.h
@@ -5,10 +5,11 @@
 #ifndef _FREEBL_H_
 #define _FREEBL_H_
 
-/* point encoding type */
+/* deprecated */
 typedef enum {
     ECPoint_Uncompressed,
-    ECPoint_XOnly
+    ECPoint_XOnly,
+    ECPoint_Undefined
 } ECPointEncoding;
 
 #endif /* _FREEBL_H_ */