[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 2d7033e..b1a5647 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 0a1de11..53eb945 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 cda52fa..6310d44 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 700e9e5..534813f 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 40d800f..5141a54 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 ac4caf4..8b1cd08 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 c6398c0..a0e488b 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 b7ad4b1..73004a8 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.