[WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with CCECCryptorExportKey for...
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Aug 2017 21:30:45 +0000 (21:30 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 18 Aug 2017 21:30:45 +0000 (21:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=175657
<rdar://problem/33797150>

Reviewed by Brent Fulgham.

Source/WebCore:

CCECCryptorGetKeyComponents returns components in unpadded formats. In some minor cases, the ECC components
do need padding. Therefore, we occasionally see some corrupted outputs in JWKs. To overcome that, this patch
replaces CCECCryptorGetKeyComponents with CCECCryptorExportKey which does padding all the time.

In the meantime, this patch also makes export* methods return OperationError if any error occur in the
underlying operations though very unlikely.

Test: crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify.html

* crypto/algorithms/CryptoAlgorithmECDH.cpp:
(WebCore::CryptoAlgorithmECDH::exportKey):
* crypto/algorithms/CryptoAlgorithmECDSA.cpp:
(WebCore::CryptoAlgorithmECDSA::exportKey):
* crypto/gcrypt/CryptoKeyECGCrypt.cpp:
(WebCore::CryptoKeyEC::platformAddFieldElements const):
* crypto/keys/CryptoKeyEC.cpp:
(WebCore::CryptoKeyEC::exportRaw const):
(WebCore::CryptoKeyEC::exportJwk const):
(WebCore::CryptoKeyEC::exportSpki const):
(WebCore::CryptoKeyEC::exportPkcs8 const):
* crypto/keys/CryptoKeyEC.h:
* crypto/mac/CryptoKeyECMac.cpp:
(WebCore::CryptoKeyEC::platformExportRaw const):
(WebCore::CryptoKeyEC::platformAddFieldElements const):
(WebCore::CryptoKeyEC::platformExportSpki const):
(WebCore::CryptoKeyEC::platformExportPkcs8 const):

LayoutTests:

* crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify-expected.txt: Added.
* crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@220933 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify-expected.txt [new file with mode: 0644]
LayoutTests/crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/crypto/algorithms/CryptoAlgorithmECDH.cpp
Source/WebCore/crypto/algorithms/CryptoAlgorithmECDSA.cpp
Source/WebCore/crypto/gcrypt/CryptoKeyECGCrypt.cpp
Source/WebCore/crypto/keys/CryptoKeyEC.cpp
Source/WebCore/crypto/keys/CryptoKeyEC.h
Source/WebCore/crypto/mac/CryptoKeyECMac.cpp

index 2d7033eff8da7a14376e26c68a159b9e47511a27..b1a564782ac024dbb304b8ec0efe6579ce0882eb 100644 (file)
@@ -1,3 +1,14 @@
+2017-08-18  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with CCECCryptorExportKey for exporting ECC JWKs
+        https://bugs.webkit.org/show_bug.cgi?id=175657
+        <rdar://problem/33797150>
+
+        Reviewed by Brent Fulgham.
+
+        * crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify-expected.txt: Added.
+        * crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify.html: Added.
+
 2017-08-18  Matt Lewis  <jlewis3@apple.com>
 
         Marked imported/w3c/web-platform-tests/IndexedDB/open-request-queue.html as flaky timeout.
diff --git a/LayoutTests/crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify-expected.txt b/LayoutTests/crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify-expected.txt
new file mode 100644 (file)
index 0000000..5b80b3a
--- /dev/null
@@ -0,0 +1,23 @@
+This test tries to test whether the JWK format exporting/importing methods work as expected.To do so, it first generates an EC Key Pair, then exports them into JWKs, then imports them back, and finally uses them to do signature verification.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Generating a key pair...
+Exporting the private key in Jwk format...
+PASS jwkPrivateKey.x is defined.
+PASS jwkPrivateKey.y is defined.
+PASS jwkPrivateKey.d is defined.
+Importing the JWK private key...
+Signing the data...
+PASS signature.byteLength is 64
+Exporting the public key in Jwk format...
+PASS jwkPublicKey.x is defined.
+PASS jwkPublicKey.y is defined.
+Importing the JWK public key...
+Verifying the signature...
+PASS verified is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify.html b/LayoutTests/crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify.html
new file mode 100644 (file)
index 0000000..2e8aa8e
--- /dev/null
@@ -0,0 +1,70 @@
+<!DOCTYPE html>
+<html>
+<head>
+    <script src="../../resources/js-test.js"></script>
+    <script src="../resources/common.js"></script>
+</head>
+<body>
+<p id="description"></p>
+<div id="console"></div>
+
+<script>
+description("This test tries to test whether the JWK format exporting/importing methods work as expected." +
+    "To do so, it first generates an EC Key Pair, then exports them into JWKs, then imports them back, and finally uses them to do signature verification.");
+
+jsTestIsAsync = true;
+
+var extractable = true;
+var ecKeyParams = {
+    name: "ECDSA",
+    namedCurve: "P-256",
+}
+var ecdsaParams = {
+    name: "ECDSA",
+    hash: "SHA-256",
+}
+var data = asciiToUint8Array("Hello, World!");
+
+debug("Generating a key pair...");
+crypto.subtle.generateKey(ecKeyParams, extractable, ["verify", "sign"]).then(function(result) {
+    keyPair = result;
+
+    debug("Exporting the private key in Jwk format...");
+    return crypto.subtle.exportKey("jwk", keyPair.privateKey);
+}, failAndFinishJSTest).then(function(result) {
+    jwkPrivateKey = result;
+    shouldBeDefined("jwkPrivateKey.x");
+    shouldBeDefined("jwkPrivateKey.y");
+    shouldBeDefined("jwkPrivateKey.d");
+
+    debug("Importing the JWK private key...");
+    return crypto.subtle.importKey("jwk", jwkPrivateKey, ecKeyParams, extractable, ["sign"]);
+}, failAndFinishJSTest).then(function(privateKey) {
+    debug("Signing the data...");
+    return crypto.subtle.sign(ecdsaParams, privateKey, data);
+}, failAndFinishJSTest).then(function(result) {
+    signature = result;
+    shouldBe("signature.byteLength", "64");
+
+    debug("Exporting the public key in Jwk format...");
+    return crypto.subtle.exportKey("jwk", keyPair.publicKey);
+}, failAndFinishJSTest).then(function(result) {
+    jwkPublicKey = result;
+    shouldBeDefined("jwkPublicKey.x");
+    shouldBeDefined("jwkPublicKey.y");
+
+    debug("Importing the JWK public key...");
+    return crypto.subtle.importKey("jwk", jwkPublicKey, ecKeyParams, extractable, ["verify"]);
+}, failAndFinishJSTest).then(function(publicKey) {
+    debug("Verifying the signature...");
+    return crypto.subtle.verify(ecdsaParams, publicKey, signature, data);
+}, failAndFinishJSTest).then(function(result) {
+    verified = result;
+
+    shouldBeTrue("verified");
+
+    finishJSTest();
+}, failAndFinishJSTest);
+</script>
+</body>
+</html>
index 0a1de117f4fd3548d605a83ce2dbdb70b346484b..53eb94537c5eb0146280a468d393d67ab2fab8bc 100644 (file)
@@ -1,3 +1,38 @@
+2017-08-18  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebCrypto][Mac] Replace CCECCryptorGetKeyComponents with CCECCryptorExportKey for exporting ECC JWKs
+        https://bugs.webkit.org/show_bug.cgi?id=175657
+        <rdar://problem/33797150>
+
+        Reviewed by Brent Fulgham.
+
+        CCECCryptorGetKeyComponents returns components in unpadded formats. In some minor cases, the ECC components
+        do need padding. Therefore, we occasionally see some corrupted outputs in JWKs. To overcome that, this patch
+        replaces CCECCryptorGetKeyComponents with CCECCryptorExportKey which does padding all the time.
+
+        In the meantime, this patch also makes export* methods return OperationError if any error occur in the
+        underlying operations though very unlikely.
+
+        Test: crypto/subtle/ecdsa-generate-export-import-jwk-sign-verify.html
+
+        * crypto/algorithms/CryptoAlgorithmECDH.cpp:
+        (WebCore::CryptoAlgorithmECDH::exportKey):
+        * crypto/algorithms/CryptoAlgorithmECDSA.cpp:
+        (WebCore::CryptoAlgorithmECDSA::exportKey):
+        * crypto/gcrypt/CryptoKeyECGCrypt.cpp:
+        (WebCore::CryptoKeyEC::platformAddFieldElements const):
+        * crypto/keys/CryptoKeyEC.cpp:
+        (WebCore::CryptoKeyEC::exportRaw const):
+        (WebCore::CryptoKeyEC::exportJwk const):
+        (WebCore::CryptoKeyEC::exportSpki const):
+        (WebCore::CryptoKeyEC::exportPkcs8 const):
+        * crypto/keys/CryptoKeyEC.h:
+        * crypto/mac/CryptoKeyECMac.cpp:
+        (WebCore::CryptoKeyEC::platformExportRaw const):
+        (WebCore::CryptoKeyEC::platformAddFieldElements const):
+        (WebCore::CryptoKeyEC::platformExportSpki const):
+        (WebCore::CryptoKeyEC::platformExportPkcs8 const):
+
 2017-08-18  Per Arne Vollan  <pvollan@apple.com>
 
         [Win] accessibility/heading-crash-after-hidden.html is a flaky crash.
index cda52fa6219cdce48e1037395abc6360efe8d755..6310d447436fca9c4add226939eeeb20c7cca25a 100644 (file)
@@ -186,9 +186,15 @@ void CryptoAlgorithmECDH::exportKey(CryptoKeyFormat format, Ref<CryptoKey>&& key
 
     KeyData result;
     switch (format) {
-    case CryptoKeyFormat::Jwk:
-        result = ecKey.exportJwk();
+    case CryptoKeyFormat::Jwk: {
+        auto jwk = ecKey.exportJwk();
+        if (jwk.hasException()) {
+            exceptionCallback(jwk.releaseException().code());
+            return;
+        }
+        result = jwk.releaseReturnValue();
         break;
+    }
     case CryptoKeyFormat::Raw: {
         auto raw = ecKey.exportRaw();
         if (raw.hasException()) {
index 700e9e59d562577d711c03a9ad4012c3769e5a03..534813f084cc05ba9b67b248119c0d71fc2dd932 100644 (file)
@@ -160,9 +160,15 @@ void CryptoAlgorithmECDSA::exportKey(CryptoKeyFormat format, Ref<CryptoKey>&& ke
 
     KeyData result;
     switch (format) {
-    case CryptoKeyFormat::Jwk:
-        result = ecKey.exportJwk();
+    case CryptoKeyFormat::Jwk: {
+        auto jwk = ecKey.exportJwk();
+        if (jwk.hasException()) {
+            exceptionCallback(jwk.releaseException().code());
+            return;
+        }
+        result = jwk.releaseReturnValue();
         break;
+    }
     case CryptoKeyFormat::Raw: {
         auto raw = ecKey.exportRaw();
         if (raw.hasException()) {
index 40d800f8fff4cd3f971c3041f0fef4a009b18bde..5141a5415d7197eb66e3c43b66e3e537a376e043 100644 (file)
@@ -500,13 +500,13 @@ Vector<uint8_t> CryptoKeyEC::platformExportRaw() const
     return WTFMove(q.value());
 }
 
-void CryptoKeyEC::platformAddFieldElements(JsonWebKey& jwk) const
+bool CryptoKeyEC::platformAddFieldElements(JsonWebKey& jwk) const
 {
     PAL::GCrypt::Handle<gcry_ctx_t> context;
     gcry_error_t error = gcry_mpi_ec_new(&context, m_platformKey, nullptr);
     if (error != GPG_ERR_NO_ERROR) {
         PAL::GCrypt::logError(error);
-        return;
+        return false;
     }
 
     unsigned uncompressedFieldElementSize = curveUncompressedFieldElementSize(m_curve);
@@ -533,6 +533,8 @@ void CryptoKeyEC::platformAddFieldElements(JsonWebKey& jwk) const
                 jwk.d = base64URLEncode(*d);
         }
     }
+
+    return true;
 }
 
 Vector<uint8_t> CryptoKeyEC::platformExportSpki() const
index ac4caf47f30bf7de6d6d32597d52fbd9ffb0c758..8b1cd080ce8ad2a98a4f705354ac3b7753b135a6 100644 (file)
@@ -143,10 +143,13 @@ ExceptionOr<Vector<uint8_t>> CryptoKeyEC::exportRaw() const
     if (type() != CryptoKey::Type::Public)
         return Exception { InvalidAccessError };
 
-    return platformExportRaw();
+    auto&& result = platformExportRaw();
+    if (result.isEmpty())
+        return Exception { OperationError };
+    return WTFMove(result);
 }
 
-JsonWebKey CryptoKeyEC::exportJwk() const
+ExceptionOr<JsonWebKey> CryptoKeyEC::exportJwk() const
 {
     JsonWebKey result;
     result.kty = "EC";
@@ -160,8 +163,9 @@ JsonWebKey CryptoKeyEC::exportJwk() const
     }
     result.key_ops = usages();
     result.ext = extractable();
-    platformAddFieldElements(result);
-    return result;
+    if (!platformAddFieldElements(result))
+        return Exception { OperationError };
+    return WTFMove(result);
 }
 
 ExceptionOr<Vector<uint8_t>> CryptoKeyEC::exportSpki() const
@@ -169,7 +173,10 @@ ExceptionOr<Vector<uint8_t>> CryptoKeyEC::exportSpki() const
     if (type() != CryptoKey::Type::Public)
         return Exception { InvalidAccessError };
 
-    return platformExportSpki();
+    auto&& result = platformExportSpki();
+    if (result.isEmpty())
+        return Exception { OperationError };
+    return WTFMove(result);
 }
 
 ExceptionOr<Vector<uint8_t>> CryptoKeyEC::exportPkcs8() const
@@ -177,7 +184,10 @@ ExceptionOr<Vector<uint8_t>> CryptoKeyEC::exportPkcs8() const
     if (type() != CryptoKey::Type::Private)
         return Exception { InvalidAccessError };
 
-    return platformExportPkcs8();
+    auto&& result = platformExportPkcs8();
+    if (result.isEmpty())
+        return Exception { OperationError };
+    return WTFMove(result);
 }
 
 String CryptoKeyEC::namedCurveString() const
index c6398c0cb5bc937a4e132f5dfe42d3af570016e2..a0e488b4c1c995f6dc4bd430a9cee587e0d8cb2c 100644 (file)
@@ -86,7 +86,7 @@ public:
     static RefPtr<CryptoKeyEC> importPkcs8(CryptoAlgorithmIdentifier, const String& curve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap);
 
     ExceptionOr<Vector<uint8_t>> exportRaw() const;
-    JsonWebKey exportJwk() const;
+    ExceptionOr<JsonWebKey> exportJwk() const;
     ExceptionOr<Vector<uint8_t>> exportSpki() const;
     ExceptionOr<Vector<uint8_t>> exportPkcs8() const;
 
@@ -111,7 +111,7 @@ private:
     static RefPtr<CryptoKeyEC> platformImportSpki(CryptoAlgorithmIdentifier, NamedCurve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap);
     static RefPtr<CryptoKeyEC> platformImportPkcs8(CryptoAlgorithmIdentifier, NamedCurve, Vector<uint8_t>&& keyData, bool extractable, CryptoKeyUsageBitmap);
     Vector<uint8_t> platformExportRaw() const;
-    void platformAddFieldElements(JsonWebKey&) const;
+    bool platformAddFieldElements(JsonWebKey&) const;
     Vector<uint8_t> platformExportSpki() const;
     Vector<uint8_t> platformExportPkcs8() const;
 
index b7ad4b1d81df3041af6cf2bd56b55119b52a8b63..73004a88f81b10eea00aa367cf79c3ff3034f31e 100644 (file)
@@ -100,14 +100,6 @@ size_t CryptoKeyEC::keySizeInBits() const
     return result ? result : 0;
 }
 
-Vector<uint8_t> CryptoKeyEC::platformExportRaw() const
-{
-    Vector<uint8_t> result(keySizeInBits() / 4 + 1); // Per Section 2.3.4 of http://www.secg.org/sec1-v2.pdf
-    size_t size = result.size();
-    CCECCryptorExportKey(kCCImportKeyBinary, result.data(), &size, ccECKeyPublic, m_platformKey);
-    return result;
-}
-
 std::optional<CryptoKeyPair> CryptoKeyEC::platformGeneratePair(CryptoAlgorithmIdentifier identifier, NamedCurve curve, bool extractable, CryptoKeyUsageBitmap usages)
 {
     size_t size = getKeySizeFromNamedCurve(curve);
@@ -133,6 +125,16 @@ RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportRaw(CryptoAlgorithmIdentifier ide
     return create(identifier, curve, CryptoKeyType::Public, ccPublicKey, extractable, usages);
 }
 
+Vector<uint8_t> CryptoKeyEC::platformExportRaw() const
+{
+    size_t expectedSize = keySizeInBits() / 4 + 1; // Per Section 2.3.4 of http://www.secg.org/sec1-v2.pdf
+    Vector<uint8_t> result(expectedSize);
+    size_t size = result.size();
+    if (UNLIKELY(CCECCryptorExportKey(kCCImportKeyBinary, result.data(), &size, ccECKeyPublic, m_platformKey) || size != expectedSize))
+        return { };
+    return result;
+}
+
 RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportJWKPublic(CryptoAlgorithmIdentifier identifier, NamedCurve curve, Vector<uint8_t>&& x, Vector<uint8_t>&& y, bool extractable, CryptoKeyUsageBitmap usages)
 {
     if (!doesFieldElementMatchNamedCurve(curve, x.size()) || !doesFieldElementMatchNamedCurve(curve, y.size()))
@@ -166,22 +168,35 @@ RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportJWKPrivate(CryptoAlgorithmIdentif
     return create(identifier, curve, CryptoKeyType::Private, ccPrivateKey, extractable, usages);
 }
 
-void CryptoKeyEC::platformAddFieldElements(JsonWebKey& jwk) const
+bool CryptoKeyEC::platformAddFieldElements(JsonWebKey& jwk) const
 {
-    size_t size = getKeySizeFromNamedCurve(m_curve);
-    size_t sizeInBytes = size / 8;
-    Vector<uint8_t> x(sizeInBytes);
-    size_t xSize = x.size();
-    Vector<uint8_t> y(sizeInBytes);
-    size_t ySize = y.size();
-    Vector<uint8_t> d(sizeInBytes);
-    size_t dSize = d.size();
-
-    CCECCryptorGetKeyComponents(m_platformKey, &size, x.data(), &xSize, y.data(), &ySize, d.data(), &dSize);
-    jwk.x = base64URLEncode(x);
-    jwk.y = base64URLEncode(y);
-    if (type() == Type::Private)
-        jwk.d = base64URLEncode(d);
+    size_t keySizeInBytes = keySizeInBits() / 8;
+    size_t publicKeySize = keySizeInBytes * 2 + 1; // 04 + X + Y per Section 2.3.4 of http://www.secg.org/sec1-v2.pdf
+    size_t privateKeySize = keySizeInBytes * 3 + 1; // 04 + X + Y + D
+
+    Vector<uint8_t> result(privateKeySize);
+    size_t size = result.size();
+    switch (type()) {
+    case CryptoKeyType::Public:
+        if (UNLIKELY(CCECCryptorExportKey(kCCImportKeyBinary, result.data(), &size, ccECKeyPublic, m_platformKey)))
+            return false;
+        break;
+    case CryptoKeyType::Private:
+        if (UNLIKELY(CCECCryptorExportKey(kCCImportKeyBinary, result.data(), &size, ccECKeyPrivate, m_platformKey)))
+            return false;
+        break;
+    default:
+        ASSERT_NOT_REACHED();
+        return false;
+    }
+
+    if (UNLIKELY((size != publicKeySize) && (size != privateKeySize)))
+        return false;
+    jwk.x = WTF::base64URLEncode(result.data() + 1, keySizeInBytes);
+    jwk.y = WTF::base64URLEncode(result.data() + keySizeInBytes + 1, keySizeInBytes);
+    if (size > publicKeySize)
+        jwk.d = WTF::base64URLEncode(result.data() + publicKeySize, keySizeInBytes);
+    return true;
 }
 
 static size_t getOID(CryptoKeyEC::NamedCurve curve, const uint8_t*& oid)
@@ -246,9 +261,11 @@ RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportSpki(CryptoAlgorithmIdentifier id
 
 Vector<uint8_t> CryptoKeyEC::platformExportSpki() const
 {
-    Vector<uint8_t> keyBytes(keySizeInBits() / 4 + 1); // Per Section 2.3.4 of http://www.secg.org/sec1-v2.pdf
+    size_t expectedKeySize = keySizeInBits() / 4 + 1; // Per Section 2.3.4 of http://www.secg.org/sec1-v2.pdf
+    Vector<uint8_t> keyBytes(expectedKeySize);
     size_t keySize = keyBytes.size();
-    CCECCryptorExportKey(kCCImportKeyBinary, keyBytes.data(), &keySize, ccECKeyPublic, m_platformKey);
+    if (UNLIKELY(CCECCryptorExportKey(kCCImportKeyBinary, keyBytes.data(), &keySize, ccECKeyPublic, m_platformKey) || keySize != expectedKeySize))
+        return { };
 
     // The following addes SPKI header to a raw EC public key.
     // Once the underlying crypto library is updated to output SPKI EC Key, we should remove this hack.
@@ -342,9 +359,11 @@ RefPtr<CryptoKeyEC> CryptoKeyEC::platformImportPkcs8(CryptoAlgorithmIdentifier i
 Vector<uint8_t> CryptoKeyEC::platformExportPkcs8() const
 {
     size_t keySizeInBytes = keySizeInBits() / 8;
-    Vector<uint8_t> keyBytes(keySizeInBytes * 3 + 1); // 04 + X + Y + private key
+    size_t expectedKeySize = keySizeInBytes * 3 + 1; // 04 + X + Y + D
+    Vector<uint8_t> keyBytes(expectedKeySize);
     size_t keySize = keyBytes.size();
-    CCECCryptorExportKey(kCCImportKeyBinary, keyBytes.data(), &keySize, ccECKeyPrivate, m_platformKey);
+    if (UNLIKELY(CCECCryptorExportKey(kCCImportKeyBinary, keyBytes.data(), &keySize, ccECKeyPrivate, m_platformKey) || keySize != expectedKeySize))
+        return { };
 
     // The following addes PKCS8 header to a raw EC private key.
     // Once the underlying crypto library is updated to output PKCS8 EC Key, we should remove this hack.