Bug 1340493 - EC_ValidatePublicKey() should reject Curve25519 keys wi…

…th length != 32 bytes r=franziskus

Differential Revision: https://nss-review.dev.mozaws.net/D220

--HG--
extra : amend_source : 9973a953e6f5d852143b515afde214cc8030f048

gtests/pk11_gtest/manifest.mn

@@ -9,6 +9,7 @@ MODULE = nss
 CPPSRCS = \
       pk11_aeskeywrap_unittest.cc \
       pk11_chacha20poly1305_unittest.cc \
+      pk11_curve25519_unittest.cc \
       pk11_ecdsa_unittest.cc \
       pk11_export_unittest.cc \
       pk11_pbkdf2_unittest.cc \

gtests/pk11_gtest/pk11_curve25519_unittest.cc

@@ -0,0 +1,117 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+#include <memory>
+#include "nss.h"
+#include "pk11pub.h"
+
+#include "gtest/gtest.h"
+#include "scoped_ptrs.h"
+
+namespace nss_test {
+
+// <https://tools.ietf.org/html/rfc7748#section-6.1>
+const uint8_t kPkcs8[] = {
+    0x30, 0x67, 0x02, 0x01, 0x00, 0x30, 0x14, 0x06, 0x07, 0x2a, 0x86, 0x48,
+    0xce, 0x3d, 0x02, 0x01, 0x06, 0x09, 0x2b, 0x06, 0x01, 0x04, 0x01, 0xda,
+    0x47, 0x0f, 0x01, 0x04, 0x4c, 0x30, 0x4a, 0x02, 0x01, 0x01, 0x04, 0x20,
+    0x77, 0x07, 0x6d, 0x0a, 0x73, 0x18, 0xa5, 0x7d, 0x3c, 0x16, 0xc1, 0x72,
+    0x51, 0xb2, 0x66, 0x45, 0xdf, 0x4c, 0x2f, 0x87, 0xeb, 0xc0, 0x99, 0x2a,
+    0xb1, 0x77, 0xfb, 0xa5, 0x1d, 0xb9, 0x2c, 0x2a, 0xa1, 0x23, 0x03, 0x21,
+    0x00, 0x85, 0x20, 0xf0, 0x09, 0x89, 0x30, 0xa7, 0x54, 0x74, 0x8b, 0x7d,
+    0xdc, 0xb4, 0x3e, 0xf7, 0x5a, 0x0d, 0xbf, 0x3a, 0x0d, 0x26, 0x38, 0x1a,
+    0xf4, 0xeb, 0xa4, 0xa9, 0x8e, 0xaa, 0x9b, 0x4e, 0x6a};
+const uint8_t kSpki[] = {
+    0x30, 0x39, 0x30, 0x14, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02,
+    0x01, 0x06, 0x09, 0x2b, 0x06, 0x01, 0x04, 0x01, 0xda, 0x47, 0x0f, 0x01,
+    0x03, 0x21, 0x00, 0xde, 0x9e, 0xdb, 0x7d, 0x7b, 0x7d, 0xc1, 0xb4, 0xd3,
+    0x5b, 0x61, 0xc2, 0xec, 0xe4, 0x35, 0x37, 0x3f, 0x83, 0x43, 0xc8, 0x5b,
+    0x78, 0x67, 0x4d, 0xad, 0xfc, 0x7e, 0x14, 0x6f, 0x88, 0x2b, 0x4f};
+const uint8_t kSecret[] = {0x4a, 0x5d, 0x9d, 0x5b, 0xa4, 0xce, 0x2d, 0xe1,
+                           0x72, 0x8e, 0x3b, 0xf4, 0x80, 0x35, 0x0f, 0x25,
+                           0xe0, 0x7e, 0x21, 0xc9, 0x47, 0xd1, 0x9e, 0x33,
+                           0x76, 0xf0, 0x9b, 0x3c, 0x1e, 0x16, 0x17, 0x42};
+
+// A public key that's too short (31 bytes).
+const uint8_t kSpkiShort[] = {
+    0x30, 0x38, 0x30, 0x14, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02,
+    0x01, 0x06, 0x09, 0x2b, 0x06, 0x01, 0x04, 0x01, 0xda, 0x47, 0x0f, 0x01,
+    0x03, 0x20, 0xde, 0x9e, 0xdb, 0x7d, 0x7b, 0x7d, 0xc1, 0xb4, 0xd3, 0x5b,
+    0x61, 0xc2, 0xec, 0xe4, 0x35, 0x37, 0x3f, 0x83, 0x43, 0xc8, 0x5b, 0x78,
+    0x67, 0x4d, 0xad, 0xfc, 0x7e, 0x14, 0x6f, 0x88, 0x2b, 0x4f};
+
+// A public key that's too long (33 bytes).
+const uint8_t kSpkiLong[] = {
+    0x30, 0x3a, 0x30, 0x14, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02,
+    0x01, 0x06, 0x09, 0x2b, 0x06, 0x01, 0x04, 0x01, 0xda, 0x47, 0x0f, 0x01,
+    0x03, 0x22, 0x00, 0xde, 0x9e, 0xdb, 0x7d, 0x7b, 0x7d, 0xc1, 0xb4, 0xd3,
+    0x5b, 0x61, 0xc2, 0xec, 0xe4, 0x35, 0x37, 0x3f, 0x83, 0x43, 0xc8, 0x5b,
+    0x78, 0x67, 0x4d, 0xad, 0xfc, 0x7e, 0x14, 0x6f, 0x88, 0x2b, 0x4f, 0x34};
+
+static unsigned char* toUcharPtr(const uint8_t* v) {
+  return const_cast<unsigned char*>(static_cast<const unsigned char*>(v));
+}
+
+class Pkcs11Curve25519Test : public ::testing::Test {
+ protected:
+  void Derive(const uint8_t* pkcs8, size_t pkcs8_len, const uint8_t* spki,
+              size_t spki_len, const uint8_t* secret, size_t secret_len,
+              bool expect_success) {
+    ScopedPK11SlotInfo slot(PK11_GetInternalSlot());
+    ASSERT_TRUE(slot);
+
+    SECItem pkcs8Item = {siBuffer, toUcharPtr(pkcs8),
+                         static_cast<unsigned int>(pkcs8_len)};
+
+    SECKEYPrivateKey* key = nullptr;
+    SECStatus rv = PK11_ImportDERPrivateKeyInfoAndReturnKey(
+        slot.get(), &pkcs8Item, nullptr, nullptr, false, false, KU_ALL, &key,
+        nullptr);
+    EXPECT_EQ(SECSuccess, rv);
+
+    ScopedSECKEYPrivateKey privKey(key);
+    ASSERT_TRUE(privKey);
+
+    SECItem spkiItem = {siBuffer, toUcharPtr(spki),
+                        static_cast<unsigned int>(spki_len)};
+
+    ScopedCERTSubjectPublicKeyInfo certSpki(
+        SECKEY_DecodeDERSubjectPublicKeyInfo(&spkiItem));
+    ASSERT_TRUE(certSpki);
+
+    ScopedSECKEYPublicKey pubKey(SECKEY_ExtractPublicKey(certSpki.get()));
+    ASSERT_TRUE(pubKey);
+
+    ScopedPK11SymKey symKey(PK11_PubDeriveWithKDF(
+        privKey.get(), pubKey.get(), false, nullptr, nullptr, CKM_ECDH1_DERIVE,
+        CKM_SHA512_HMAC, CKA_DERIVE, 0, CKD_NULL, nullptr, nullptr));
+    EXPECT_EQ(expect_success, !!symKey);
+
+    if (expect_success) {
+      rv = PK11_ExtractKeyValue(symKey.get());
+      EXPECT_EQ(SECSuccess, rv);
+
+      SECItem* keyData = PK11_GetKeyData(symKey.get());
+      EXPECT_EQ(secret_len, keyData->len);
+      EXPECT_EQ(memcmp(keyData->data, secret, secret_len), 0);
+    }
+  }
+};
+
+TEST_F(Pkcs11Curve25519Test, DeriveSharedSecret) {
+  Derive(kPkcs8, sizeof(kPkcs8), kSpki, sizeof(kSpki), kSecret, sizeof(kSecret),
+         true);
+}
+
+TEST_F(Pkcs11Curve25519Test, DeriveSharedSecretShort) {
+  Derive(kPkcs8, sizeof(kPkcs8), kSpkiShort, sizeof(kSpkiShort), nullptr, 0,
+         false);
+}
+
+TEST_F(Pkcs11Curve25519Test, DeriveSharedSecretLong) {
+  Derive(kPkcs8, sizeof(kPkcs8), kSpkiLong, sizeof(kSpkiLong), nullptr, 0,
+         false);
+}
+
+}  // namespace nss_test

gtests/pk11_gtest/pk11_gtest.gyp

@@ -13,6 +13,7 @@
       'sources': [
         'pk11_aeskeywrap_unittest.cc',
         'pk11_chacha20poly1305_unittest.cc',
+        'pk11_curve25519_unittest.cc',
         'pk11_ecdsa_unittest.cc',
         'pk11_pbkdf2_unittest.cc',
         'pk11_prf_unittest.cc',

lib/freebl/ecl/ecp_25519.c

@@ -79,8 +79,7 @@ ec_Curve25519_pt_validate(const SECItem *px)
           0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff },
     };
 
-    /* The point must not be longer than 32 (it can be smaller). */
-    if (px->len <= 32) {
+    if (px->len == 32) {
         p = px->data;
     } else {
         return SECFailure;

lib/softoken/pkcs11c.c

@@ -7242,12 +7242,7 @@ NSC_DeriveKey(CK_SESSION_HANDLE hSession,
 
             pubKeyLen = EC_GetPointSize(&privKey->u.ec.ecParams);
 
-            /* 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 the len is too large, might be an encoded point */
             if (ecPoint.len > pubKeyLen) {
                 SECItem newPoint;