LibCrypto+LibWeb: Check EC keys validity on SubtleCrypto import_key

Fix various TODO by checking the validity of ECDSA and ECDH keys when
they are imported. There are no checks in place for raw import because
the spec doesn't contemplate them yet.

Also add some internal tests since WPT doesn't seem to provide them.

Libraries/LibCrypto/Curves/SECPxxxr1.cpp

@@ -181,4 +181,32 @@ ErrorOr<SECPxxxr1Signature> SECPxxxr1::sign(ReadonlyBytes hash, UnsignedBigInteg
     };
 }
 
+ErrorOr<bool> SECPxxxr1::is_valid_point(SECPxxxr1Point pubkey, Optional<UnsignedBigInteger> private_key)
+{
+    auto* group = OPENSSL_TRY_PTR(EC_GROUP_new_by_curve_name(EC_curve_nist2nid(m_curve_name)));
+    ScopeGuard const free_group = [&] { EC_GROUP_free(group); };
+
+    auto x = TRY(unsigned_big_integer_to_openssl_bignum(pubkey.x));
+    auto y = TRY(unsigned_big_integer_to_openssl_bignum(pubkey.y));
+
+    auto* point = OPENSSL_TRY_PTR(EC_POINT_new(group));
+    ScopeGuard const free_point = [&] { EC_POINT_free(point); };
+
+    // Check public point is on the curve, EC_POINT_set_affine_coordinates calls EC_POINT_is_on_curve internally
+    if (EC_POINT_set_affine_coordinates(group, point, x.ptr(), y.ptr(), nullptr) != 1)
+        return false;
+
+    if (!private_key.has_value())
+        return true;
+
+    // Check that the public key is a valid point for the given private key
+    auto private_key_int = TRY(unsigned_big_integer_to_openssl_bignum(*private_key));
+
+    auto* expected_point = EC_POINT_new(group);
+    ScopeGuard const free_expected_point = [&] { EC_POINT_free(expected_point); };
+
+    OPENSSL_TRY(EC_POINT_mul(group, expected_point, private_key_int.ptr(), nullptr, nullptr, nullptr));
+    return EC_POINT_cmp(group, expected_point, point, nullptr) == 0;
+}
+
 }

Libraries/LibCrypto/Curves/SECPxxxr1.h

@@ -165,6 +165,7 @@ public:
     ErrorOr<SECPxxxr1Point> compute_coordinate(UnsignedBigInteger scalar, SECPxxxr1Point point);
     ErrorOr<bool> verify(ReadonlyBytes hash, SECPxxxr1Point pubkey, SECPxxxr1Signature signature);
     ErrorOr<SECPxxxr1Signature> sign(ReadonlyBytes hash, UnsignedBigInteger private_key);
+    ErrorOr<bool> is_valid_point(SECPxxxr1Point pubkey, Optional<UnsignedBigInteger> private_key = {});
 
 protected:
     SECPxxxr1(char const* curve_name, size_t scalar_size)

Libraries/LibWeb/Crypto/CryptoAlgorithms.cpp

@@ -4049,8 +4049,29 @@ WebIDL::ExceptionOr<GC::Ref<CryptoKey>> ECDSA::import_key(AlgorithmParams const&
         if (!named_curve.is_empty() && named_curve != normalized_algorithm.named_curve)
             return WebIDL::DataError::create(m_realm, "Invalid algorithm"_string);
 
-        // TODO: 12. If the public key value is not a valid point on the Elliptic Curve identified
+        // 12. If the public key value is not a valid point on the Elliptic Curve identified
         //     by the namedCurve member of normalizedAlgorithm throw a DataError.
+        auto curve = [&named_curve] -> Variant<::Crypto::Curves::SECP256r1, ::Crypto::Curves::SECP384r1, ::Crypto::Curves::SECP521r1> {
+            if (named_curve == "P-256")
+                return ::Crypto::Curves::SECP256r1 {};
+            if (named_curve == "P-384")
+                return ::Crypto::Curves::SECP384r1 {};
+            if (named_curve == "P-521")
+                return ::Crypto::Curves::SECP521r1 {};
+            VERIFY_NOT_REACHED();
+        }();
+        TRY(curve.visit([&](auto& instance) -> WebIDL::ExceptionOr<void> {
+            auto maybe_valid = key->handle().visit(
+                [](auto const&) -> ErrorOr<bool> { VERIFY_NOT_REACHED(); },
+                [&](::Crypto::PK::ECPublicKey<> const& public_key) {
+                    return instance.is_valid_point(public_key.to_secpxxxr1_point());
+                });
+            if (maybe_valid.is_error())
+                return WebIDL::DataError::create(m_realm, "Failed to verify key"_string);
+            if (!maybe_valid.value())
+                return WebIDL::DataError::create(m_realm, "Invalid key"_string);
+            return {};
+        }));
 
         // 13. Set the [[type]] internal slot of key to "public"
         key->set_type(Bindings::KeyType::Public);
@@ -4146,8 +4167,32 @@ WebIDL::ExceptionOr<GC::Ref<CryptoKey>> ECDSA::import_key(AlgorithmParams const&
         if (!named_curve.is_empty() && named_curve != normalized_algorithm.named_curve)
             return WebIDL::DataError::create(m_realm, "Invalid algorithm"_string);
 
-        // TODO: 12. If the key value is not a valid point on the Elliptic Curve identified
+        // 12. If the key value is not a valid point on the Elliptic Curve identified
         //     by the namedCurve member of normalizedAlgorithm throw a DataError.
+        auto curve = [&named_curve] -> Variant<::Crypto::Curves::SECP256r1, ::Crypto::Curves::SECP384r1, ::Crypto::Curves::SECP521r1> {
+            if (named_curve == "P-256")
+                return ::Crypto::Curves::SECP256r1 {};
+            if (named_curve == "P-384")
+                return ::Crypto::Curves::SECP384r1 {};
+            if (named_curve == "P-521")
+                return ::Crypto::Curves::SECP521r1 {};
+            VERIFY_NOT_REACHED();
+        }();
+        TRY(curve.visit([&](auto& instance) -> WebIDL::ExceptionOr<void> {
+            auto maybe_valid = key->handle().visit(
+                [](auto const&) -> ErrorOr<bool> { VERIFY_NOT_REACHED(); },
+                [&](::Crypto::PK::ECPrivateKey<> const& private_key) -> ErrorOr<bool> {
+                    if (!private_key.public_key().has_value())
+                        return true;
+
+                    return instance.is_valid_point(private_key.public_key()->to_secpxxxr1_point(), private_key.d());
+                });
+            if (maybe_valid.is_error())
+                return WebIDL::DataError::create(m_realm, "Failed to verify key"_string);
+            if (!maybe_valid.value())
+                return WebIDL::DataError::create(m_realm, "Invalid key"_string);
+            return {};
+        }));
 
         // 13. Set the [[type]] internal slot of key to "private".
         key->set_type(Bindings::KeyType::Private);
@@ -4322,8 +4367,32 @@ WebIDL::ExceptionOr<GC::Ref<CryptoKey>> ECDSA::import_key(AlgorithmParams const&
             return WebIDL::DataError::create(m_realm, "Invalid algorithm"_string);
         }
 
-        // TODO: 10. If the key value is not a valid point on the Elliptic Curve identified
+        // 10. If the key value is not a valid point on the Elliptic Curve identified
         //     by the namedCurve member of normalizedAlgorithm throw a DataError.
+        auto curve = [&named_curve] -> Variant<::Crypto::Curves::SECP256r1, ::Crypto::Curves::SECP384r1, ::Crypto::Curves::SECP521r1> {
+            if (named_curve == "P-256")
+                return ::Crypto::Curves::SECP256r1 {};
+            if (named_curve == "P-384")
+                return ::Crypto::Curves::SECP384r1 {};
+            if (named_curve == "P-521")
+                return ::Crypto::Curves::SECP521r1 {};
+            VERIFY_NOT_REACHED();
+        }();
+        TRY(curve.visit([&](auto& instance) -> WebIDL::ExceptionOr<void> {
+            auto maybe_valid = key->handle().visit(
+                [](auto const&) -> ErrorOr<bool> { VERIFY_NOT_REACHED(); },
+                [&](::Crypto::PK::ECPublicKey<> const& public_key) {
+                    return instance.is_valid_point(public_key.to_secpxxxr1_point());
+                },
+                [&](::Crypto::PK::ECPrivateKey<> const& private_key) {
+                    return instance.is_valid_point(private_key.public_key()->to_secpxxxr1_point(), private_key.d());
+                });
+            if (maybe_valid.is_error())
+                return WebIDL::DataError::create(m_realm, "Failed to verify key"_string);
+            if (!maybe_valid.value())
+                return WebIDL::DataError::create(m_realm, "Invalid key"_string);
+            return {};
+        }));
 
         // 11. Let algorithm be a new instance of an EcKeyAlgorithm object.
         auto algorithm = EcKeyAlgorithm::create(m_realm);
@@ -4983,8 +5052,29 @@ WebIDL::ExceptionOr<GC::Ref<CryptoKey>> ECDH::import_key(AlgorithmParams const&
         if (!named_curve.is_empty() && named_curve != normalized_algorithm.named_curve)
             return WebIDL::DataError::create(m_realm, "Invalid algorithm"_string);
 
-        // TODO: 12. If the key value is not a valid point on the Elliptic Curve identified
+        // 12. If the key value is not a valid point on the Elliptic Curve identified
         //     by the namedCurve member of normalizedAlgorithm throw a DataError.
+        auto curve = [&named_curve] -> Variant<::Crypto::Curves::SECP256r1, ::Crypto::Curves::SECP384r1, ::Crypto::Curves::SECP521r1> {
+            if (named_curve == "P-256")
+                return ::Crypto::Curves::SECP256r1 {};
+            if (named_curve == "P-384")
+                return ::Crypto::Curves::SECP384r1 {};
+            if (named_curve == "P-521")
+                return ::Crypto::Curves::SECP521r1 {};
+            VERIFY_NOT_REACHED();
+        }();
+        TRY(curve.visit([&](auto& instance) -> WebIDL::ExceptionOr<void> {
+            auto maybe_valid = key->handle().visit(
+                [](auto const&) -> ErrorOr<bool> { VERIFY_NOT_REACHED(); },
+                [&](::Crypto::PK::ECPublicKey<> const& public_key) {
+                    return instance.is_valid_point(public_key.to_secpxxxr1_point());
+                });
+            if (maybe_valid.is_error())
+                return WebIDL::DataError::create(m_realm, "Failed to verify key"_string);
+            if (!maybe_valid.value())
+                return WebIDL::DataError::create(m_realm, "Invalid key"_string);
+            return {};
+        }));
 
         // 13. Set the [[type]] internal slot of key to "public"
         key->set_type(Bindings::KeyType::Public);
@@ -5080,8 +5170,32 @@ WebIDL::ExceptionOr<GC::Ref<CryptoKey>> ECDH::import_key(AlgorithmParams const&
         if (!named_curve.is_empty() && named_curve != normalized_algorithm.named_curve)
             return WebIDL::DataError::create(m_realm, "Invalid algorithm"_string);
 
-        // TODO: 12. If the key value is not a valid point on the Elliptic Curve identified
+        // 12. If the key value is not a valid point on the Elliptic Curve identified
         //     by the namedCurve member of normalizedAlgorithm throw a DataError.
+        auto curve = [&named_curve] -> Variant<::Crypto::Curves::SECP256r1, ::Crypto::Curves::SECP384r1, ::Crypto::Curves::SECP521r1> {
+            if (named_curve == "P-256")
+                return ::Crypto::Curves::SECP256r1 {};
+            if (named_curve == "P-384")
+                return ::Crypto::Curves::SECP384r1 {};
+            if (named_curve == "P-521")
+                return ::Crypto::Curves::SECP521r1 {};
+            VERIFY_NOT_REACHED();
+        }();
+        TRY(curve.visit([&](auto& instance) -> WebIDL::ExceptionOr<void> {
+            auto maybe_valid = key->handle().visit(
+                [](auto const&) -> ErrorOr<bool> { VERIFY_NOT_REACHED(); },
+                [&](::Crypto::PK::ECPrivateKey<> const& private_key) -> ErrorOr<bool> {
+                    if (!private_key.public_key().has_value())
+                        return true;
+
+                    return instance.is_valid_point(private_key.public_key()->to_secpxxxr1_point(), private_key.d());
+                });
+            if (maybe_valid.is_error())
+                return WebIDL::DataError::create(m_realm, "Failed to verify key"_string);
+            if (!maybe_valid.value())
+                return WebIDL::DataError::create(m_realm, "Invalid key"_string);
+            return {};
+        }));
 
         // 13. Set the [[type]] internal slot of key to "private".
         key->set_type(Bindings::KeyType::Private);
@@ -5225,8 +5339,32 @@ WebIDL::ExceptionOr<GC::Ref<CryptoKey>> ECDH::import_key(AlgorithmParams const&
             return WebIDL::DataError::create(m_realm, "Invalid algorithm"_string);
         }
 
-        // TODO: 11. If the key value is not a valid point on the Elliptic Curve identified
+        // 11. If the key value is not a valid point on the Elliptic Curve identified
         //     by the namedCurve member of normalizedAlgorithm throw a DataError.
+        auto curve = [&named_curve] -> Variant<::Crypto::Curves::SECP256r1, ::Crypto::Curves::SECP384r1, ::Crypto::Curves::SECP521r1> {
+            if (named_curve == "P-256")
+                return ::Crypto::Curves::SECP256r1 {};
+            if (named_curve == "P-384")
+                return ::Crypto::Curves::SECP384r1 {};
+            if (named_curve == "P-521")
+                return ::Crypto::Curves::SECP521r1 {};
+            VERIFY_NOT_REACHED();
+        }();
+        TRY(curve.visit([&](auto& instance) -> WebIDL::ExceptionOr<void> {
+            auto maybe_valid = key->handle().visit(
+                [](auto const&) -> ErrorOr<bool> { VERIFY_NOT_REACHED(); },
+                [&](::Crypto::PK::ECPublicKey<> const& public_key) {
+                    return instance.is_valid_point(public_key.to_secpxxxr1_point());
+                },
+                [&](::Crypto::PK::ECPrivateKey<> const& private_key) {
+                    return instance.is_valid_point(private_key.public_key()->to_secpxxxr1_point(), private_key.d());
+                });
+            if (maybe_valid.is_error())
+                return WebIDL::DataError::create(m_realm, "Failed to verify key"_string);
+            if (!maybe_valid.value())
+                return WebIDL::DataError::create(m_realm, "Invalid key"_string);
+            return {};
+        }));
 
         // 12. Let algorithm be a new instance of an EcKeyAlgorithm object.
         auto algorithm = EcKeyAlgorithm::create(m_realm);

Tests/LibWeb/Text/expected/Crypto/SubtleCrypto-ec-bad-import-key.txt

@@ -0,0 +1,30 @@
+ECDSA P-256 PUB  - jwk   OK: DataError: Invalid key
+ECDSA P-256 PUB  - spki  OK: DataError: Invalid key
+ECDSA P-256 PRIV - jwk   OK: DataError: Invalid key
+ECDSA P-256 PRIV - pkcs8 OK: DataError: Invalid key
+ECDSA P-256 PRIV - pkcs8 OK: DataError: Invalid key
+ECDSA P-384 PUB  - jwk   OK: DataError: Invalid key
+ECDSA P-384 PUB  - spki  OK: DataError: Invalid key
+ECDSA P-384 PRIV - jwk   OK: DataError: Invalid key
+ECDSA P-384 PRIV - pkcs8 OK: DataError: Invalid key
+ECDSA P-384 PRIV - pkcs8 OK: DataError: Invalid key
+ECDSA P-521 PUB  - jwk   OK: DataError: Invalid key
+ECDSA P-521 PUB  - spki  OK: DataError: Invalid key
+ECDSA P-521 PRIV - jwk   OK: DataError: Invalid key
+ECDSA P-521 PRIV - pkcs8 OK: DataError: Invalid key
+ECDSA P-521 PRIV - pkcs8 OK: DataError: Invalid key
+ECDH  P-256 PUB  - jwk   OK: DataError: Invalid key
+ECDH  P-256 PUB  - spki  OK: DataError: Invalid key
+ECDH  P-256 PRIV - jwk   OK: DataError: Invalid key
+ECDH  P-256 PRIV - pkcs8 OK: DataError: Invalid key
+ECDH  P-256 PRIV - pkcs8 OK: DataError: Invalid key
+ECDH  P-384 PUB  - jwk   OK: DataError: Invalid key
+ECDH  P-384 PUB  - spki  OK: DataError: Invalid key
+ECDH  P-384 PRIV - jwk   OK: DataError: Invalid key
+ECDH  P-384 PRIV - pkcs8 OK: DataError: Invalid key
+ECDH  P-384 PRIV - pkcs8 OK: DataError: Invalid key
+ECDH  P-521 PUB  - jwk   OK: DataError: Invalid key
+ECDH  P-521 PUB  - spki  OK: DataError: Invalid key
+ECDH  P-521 PRIV - jwk   OK: DataError: Invalid key
+ECDH  P-521 PRIV - pkcs8 OK: DataError: Invalid key
+ECDH  P-521 PRIV - pkcs8 OK: DataError: Invalid key

Tests/LibWeb/Text/input/Crypto/SubtleCrypto-ec-bad-import-key.html

@@ -0,0 +1,112 @@
+<!DOCTYPE html>
+<script src="../include.js"></script>
+<script>
+  function b64ToBn(b64) {
+    const bin = atob(b64.replace(/-/g, '+').replace(/_/g, '/'));
+    const hex = bin.split('').map((ch) => ch.charCodeAt(0).toString(16).padStart(2, '0')).join('');
+    return [BigInt('0x' + hex), bin.length];
+  }
+
+  function bnToB64(bn, len) {
+    const hex = bn.toString(16);
+    const bin = hex.match(/.{1,2}/g).map((byte) => String.fromCharCode(parseInt(byte, 16))).join('');
+    return btoa(bin.padStart(len, '\x00')).replace(/\+/g, '-').replace(/\//g, '_').replace(/=+$/, '');
+  }
+
+  function corruptJwk(key) {
+    if (key.d) {
+      const [bn, len] = b64ToBn(key.d);
+
+      // Corrupt the private key
+      return {
+        ...key,
+        d: bnToB64(bn ^ 1n, len),
+      };
+    } else {
+      const [bn, len] = b64ToBn(key.x);
+
+      // Corrupt the public key
+      return {
+        ...key,
+        x: bnToB64(bn ^ 1n, len),
+      };
+    }
+  }
+
+  function corruptSpki(publicKey) {
+    // Corrupt the public key, which is encoded always at the end of the SPKI
+    const corrupted = new Uint8Array(publicKey);
+    corrupted[corrupted.length - 1] ^= 0x01;
+    return corrupted.buffer;
+  }
+
+  function corruptPkcs8Pub(privateKey) {
+    // Corrupt the public key, which is encoded always at the end of the PKCS8
+    const corrupted = new Uint8Array(privateKey);
+    corrupted[corrupted.length - 1] ^= 0x01;
+    return corrupted.buffer;
+  }
+
+  function corruptPkcs8Priv(privateKey) {
+    // Corrupt the private key, which is encoded between 34 and 68 for all curves for the PKCS8
+    const corrupted = new Uint8Array(privateKey);
+    corrupted[48] ^= 0x01;
+    return corrupted.buffer;
+  }
+
+  asyncTest(async done => {
+    for (const [name, genUsages, pubUsages, privUsages] of [
+      ["ECDSA", ["verify", "sign"], ["verify"], ["sign"]],
+      ["ECDH", ["deriveBits", "deriveKey"], [], ["deriveBits", "deriveKey"]],
+    ]) {
+      for (const curve of ["P-256", "P-384", "P-521"]) {
+        const keyPair = await window.crypto.subtle.generateKey({
+          name: name,
+          namedCurve: curve
+        }, true, genUsages);
+
+        for (const [format, corruptFn] of [["jwk", corruptJwk], ["spki", corruptSpki]]) {
+          const publicKey = await window.crypto.subtle.exportKey(format, keyPair.publicKey);
+
+          // Prove that the original key can be imported successfully
+          await window.crypto.subtle.importKey(format, publicKey, {name: name, namedCurve: curve}, false, pubUsages);
+
+          // Corrupt the private key and try to import it
+          const corruptedPublicKey = corruptFn(publicKey);
+
+          try {
+            await window.crypto.subtle.importKey(format, corruptedPublicKey, {
+              name: name,
+              namedCurve: curve
+            }, false, pubUsages);
+            println(`${name.padEnd(5, ' ')} ${curve} PUB  - ${format.padEnd(5, ' ')} FAILED`);
+          } catch (e) {
+            println(`${name.padEnd(5, ' ')} ${curve} PUB  - ${format.padEnd(5, ' ')} OK: ${e}`);
+          }
+        }
+
+        for (const [format, corruptFn] of [["jwk", corruptJwk], ["pkcs8", corruptPkcs8Priv], ["pkcs8", corruptPkcs8Pub]]) {
+          const privateKey = await window.crypto.subtle.exportKey(format, keyPair.privateKey);
+
+          // Prove that the original key can be imported successfully
+          await window.crypto.subtle.importKey(format, privateKey, {name: name, namedCurve: curve}, false, privUsages);
+
+          // Corrupt the private key and try to import it
+          const corruptedPrivateKey = corruptFn(privateKey);
+
+          try {
+            await window.crypto.subtle.importKey(format, corruptedPrivateKey, {
+              name: name,
+              namedCurve: curve
+            }, false, privUsages);
+            println(`${name.padEnd(5, ' ')} ${curve} PRIV - ${format.padEnd(5, ' ')} FAILED`);
+          } catch (e) {
+            println(`${name.padEnd(5, ' ')} ${curve} PRIV - ${format.padEnd(5, ' ')} OK: ${e}`);
+          }
+        }
+      }
+    }
+
+    done();
+  });
+</script>