[WebAuthn] Roll back newly created credentials if an error occurs
authorjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 May 2020 02:33:15 +0000 (02:33 +0000)
committerjiewen_tan@apple.com <jiewen_tan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 8 May 2020 02:33:15 +0000 (02:33 +0000)
https://bugs.webkit.org/show_bug.cgi?id=183530
<rdar://problem/43357305>

Reviewed by Brent Fulgham.

Source/WebKit:

We should clean up any newly created credentials if an error occurs before the relying party
registers the identity. Otherwise we are left with a dangling credential.

Covered by API tests.

* UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:
* UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:
(WebKit::LocalAuthenticator::continueMakeCredentialAfterUserVerification):
(WebKit::LocalAuthenticator::continueGetAssertionAfterUserVerification):
(WebKit::LocalAuthenticator::receiveException const):

Tools:

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:
(TestWebKitAPI::TEST):
* TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la-no-attestation.html: Added.
* TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la.html:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h
Source/WebKit/UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm
Tools/ChangeLog
Tools/TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj
Tools/TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm
Tools/TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la-no-attestation.html [new file with mode: 0644]
Tools/TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la.html

index 27eaeb9..f98d508 100644 (file)
@@ -1,3 +1,22 @@
+2020-05-07  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthn] Roll back newly created credentials if an error occurs
+        https://bugs.webkit.org/show_bug.cgi?id=183530
+        <rdar://problem/43357305>
+
+        Reviewed by Brent Fulgham.
+
+        We should clean up any newly created credentials if an error occurs before the relying party
+        registers the identity. Otherwise we are left with a dangling credential.
+
+        Covered by API tests.
+
+        * UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.h:
+        * UIProcess/WebAuthentication/Cocoa/LocalAuthenticator.mm:
+        (WebKit::LocalAuthenticator::continueMakeCredentialAfterUserVerification):
+        (WebKit::LocalAuthenticator::continueGetAssertionAfterUserVerification):
+        (WebKit::LocalAuthenticator::receiveException const):
+
 2020-05-07  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, reverting r261252.
index 2dd0cb4..58479d4 100644 (file)
@@ -75,6 +75,7 @@ private:
     State m_state { State::Init };
     UniqueRef<LocalConnection> m_connection;
     Vector<Ref<WebCore::AuthenticatorAssertionResponse>> m_existingCredentials;
+    RetainPtr<NSData> m_provisionalCredentialId;
 };
 
 } // namespace WebKit
index 7ab65a8..ce419e9 100644 (file)
@@ -337,13 +337,14 @@ void LocalAuthenticator::continueMakeCredentialAfterUserVerification(SecAccessCo
         auto digest = PAL::CryptoDigest::create(PAL::CryptoDigest::Algorithm::SHA_1);
         digest->addBytes(nsPublicKeyData.bytes, nsPublicKeyData.length);
         credentialId = digest->computeHash();
+        m_provisionalCredentialId = toNSData(credentialId);
 
 #ifndef NDEBUG
         NSDictionary *credentialIdQuery = @{
             (id)kSecClass: (id)kSecClassKey,
             (id)kSecAttrKeyClass: (id)kSecAttrKeyClassPrivate,
             (id)kSecAttrLabel: secAttrLabel,
-            (id)kSecAttrApplicationLabel: toNSData(credentialId).get(),
+            (id)kSecAttrApplicationLabel: m_provisionalCredentialId.get(),
 #if HAVE(DATA_PROTECTION_KEYCHAIN)
             (id)kSecUseDataProtectionKeychain: @YES
 #else
@@ -559,11 +560,6 @@ void LocalAuthenticator::continueGetAssertionAfterUserVerification(Ref<WebCore::
         }
     }
 
-    // Step 13.
-    response->setAuthenticatorData(WTFMove(authData));
-    response->setSignature(toArrayBuffer((NSData *)signature.get()));
-    receiveRespond(WTFMove(response));
-
     // Extra step: update the Keychain item with the same value to update its modification date such that LRU can be used
     // for selectAssertionResponse
     NSDictionary *updateQuery = @{
@@ -582,13 +578,36 @@ void LocalAuthenticator::continueGetAssertionAfterUserVerification(Ref<WebCore::
     auto status = SecItemUpdate((__bridge CFDictionaryRef)updateQuery, (__bridge CFDictionaryRef)updateParams);
     if (status)
         LOG_ERROR("Couldn't update the Keychain item: %d", status);
+
+    // Step 13.
+    response->setAuthenticatorData(WTFMove(authData));
+    response->setSignature(toArrayBuffer((NSData *)signature.get()));
+    receiveRespond(WTFMove(response));
 }
 
 void LocalAuthenticator::receiveException(ExceptionData&& exception, WebAuthenticationStatus status) const
 {
     LOG_ERROR(exception.message.utf8().data());
+
+    // Roll back the just created credential.
+    if (m_provisionalCredentialId) {
+        NSDictionary* deleteQuery = @{
+            (id)kSecClass: (id)kSecClassKey,
+            (id)kSecAttrApplicationLabel: m_provisionalCredentialId.get(),
+#if HAVE(DATA_PROTECTION_KEYCHAIN)
+            (id)kSecUseDataProtectionKeychain: @YES
+#else
+            (id)kSecAttrNoLegacy: @YES
+#endif
+        };
+        OSStatus status = SecItemDelete((__bridge CFDictionaryRef)deleteQuery);
+        if (status)
+            LOG_ERROR(makeString("Couldn't delete provisional credential while handling error: "_s, status).utf8().data());
+    }
+
     if (auto* observer = this->observer())
         observer->authenticatorStatusUpdated(status);
+
     receiveRespond(WTFMove(exception));
     return;
 }
index b91d3eb..23ecb38 100644 (file)
@@ -1,3 +1,17 @@
+2020-05-07  Jiewen Tan  <jiewen_tan@apple.com>
+
+        [WebAuthn] Roll back newly created credentials if an error occurs
+        https://bugs.webkit.org/show_bug.cgi?id=183530
+        <rdar://problem/43357305>
+
+        Reviewed by Brent Fulgham.
+
+        * TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
+        * TestWebKitAPI/Tests/WebKitCocoa/_WKWebAuthenticationPanel.mm:
+        (TestWebKitAPI::TEST):
+        * TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la-no-attestation.html: Added.
+        * TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la.html:
+
 2020-05-07  Saam Barati  <sbarati@apple.com>
 
         run-javascriptcore-tests with remote should be verbose
index ae2c578..ee31317 100644 (file)
                55A817FF2181021A0004A39A /* 100x100-red.tga in Copy Resources */ = {isa = PBXBuildFile; fileRef = 55A817FE218101DF0004A39A /* 100x100-red.tga */; };
                55A81800218102210004A39A /* 400x400-green.png in Copy Resources */ = {isa = PBXBuildFile; fileRef = 55A817FD218101DF0004A39A /* 400x400-green.png */; };
                55F9D2E52205031800A9AB38 /* AdditionalSupportedImageTypes.mm in Sources */ = {isa = PBXBuildFile; fileRef = 55F9D2E42205031800A9AB38 /* AdditionalSupportedImageTypes.mm */; };
+               5703BB8D2463F88700475FB2 /* web-authentication-make-credential-la-no-attestation.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 5703BB8C2463F87200475FB2 /* web-authentication-make-credential-la-no-attestation.html */; };
                570D26F423C3CA6A00D5CF67 /* web-authentication-make-credential-hid-pin-get-key-agreement-error.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 570D26F323C3CA5500D5CF67 /* web-authentication-make-credential-hid-pin-get-key-agreement-error.html */; };
                570D26F623C3D33000D5CF67 /* web-authentication-make-credential-hid-pin.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 570D26F523C3D32700D5CF67 /* web-authentication-make-credential-hid-pin.html */; };
                570D26FA23C3F25100D5CF67 /* web-authentication-make-credential-hid-pin-get-pin-token-pin-blocked-error.html in Copy Resources */ = {isa = PBXBuildFile; fileRef = 570D26F923C3F24500D5CF67 /* web-authentication-make-credential-hid-pin-get-pin-token-pin-blocked-error.html */; };
                                5798337E236019A4008E5547 /* web-authentication-make-credential-hid.html in Copy Resources */,
                                5742178A2400AED8002B303D /* web-authentication-make-credential-la-duplicate-credential.html in Copy Resources */,
                                574217882400AC25002B303D /* web-authentication-make-credential-la-error.html in Copy Resources */,
+                               5703BB8D2463F88700475FB2 /* web-authentication-make-credential-la-no-attestation.html in Copy Resources */,
                                57EDFC5C245A1A3F00959521 /* web-authentication-make-credential-la-no-mock.html in Copy Resources */,
                                5742178E2400D2DF002B303D /* web-authentication-make-credential-la.html in Copy Resources */,
                                1C2B81861C89259D00A5529F /* webfont.html in Copy Resources */,
                55A817FD218101DF0004A39A /* 400x400-green.png */ = {isa = PBXFileReference; lastKnownFileType = image.png; path = "400x400-green.png"; sourceTree = "<group>"; };
                55A817FE218101DF0004A39A /* 100x100-red.tga */ = {isa = PBXFileReference; lastKnownFileType = file; path = "100x100-red.tga"; sourceTree = "<group>"; };
                55F9D2E42205031800A9AB38 /* AdditionalSupportedImageTypes.mm */ = {isa = PBXFileReference; explicitFileType = sourcecode.cpp.objcpp; path = AdditionalSupportedImageTypes.mm; sourceTree = "<group>"; };
+               5703BB8C2463F87200475FB2 /* web-authentication-make-credential-la-no-attestation.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "web-authentication-make-credential-la-no-attestation.html"; sourceTree = "<group>"; };
                570D26F323C3CA5500D5CF67 /* web-authentication-make-credential-hid-pin-get-key-agreement-error.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "web-authentication-make-credential-hid-pin-get-key-agreement-error.html"; sourceTree = "<group>"; };
                570D26F523C3D32700D5CF67 /* web-authentication-make-credential-hid-pin.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "web-authentication-make-credential-hid-pin.html"; sourceTree = "<group>"; };
                570D26F923C3F24500D5CF67 /* web-authentication-make-credential-hid-pin-get-pin-token-pin-blocked-error.html */ = {isa = PBXFileReference; lastKnownFileType = text.html; path = "web-authentication-make-credential-hid-pin-get-pin-token-pin-blocked-error.html"; sourceTree = "<group>"; };
                                5798337D2360196D008E5547 /* web-authentication-make-credential-hid.html */,
                                574217892400AED0002B303D /* web-authentication-make-credential-la-duplicate-credential.html */,
                                574217872400ABFD002B303D /* web-authentication-make-credential-la-error.html */,
+                               5703BB8C2463F87200475FB2 /* web-authentication-make-credential-la-no-attestation.html */,
                                57EDFC5B245A18F500959521 /* web-authentication-make-credential-la-no-mock.html */,
                                5742178D2400D26C002B303D /* web-authentication-make-credential-la.html */,
                                51714EB21CF8C761004723C4 /* WebProcessKillIDBCleanup-1.html */,
                        buildActionMask = 2147483647;
                        files = (
                                7C83DE991D0A590C00FEBCF3 /* AtomString.cpp in Sources */,
+                               FE2D9474245EB2F400E48135 /* Bitmap.cpp in Sources */,
                                1ADAD1501D77A9F600212586 /* BlockPtr.mm in Sources */,
                                7C83DE9C1D0A590C00FEBCF3 /* BloomFilter.cpp in Sources */,
                                04DB2396235E43EC00328F17 /* BumpPointerAllocator.cpp in Sources */,
                                E38D65CB23A45FAA0063D69A /* PackedRefPtr.cpp in Sources */,
                                7C83DF591D0A590C00FEBCF3 /* ParkingLot.cpp in Sources */,
                                53EC25411E96FD87000831B9 /* PriorityQueue.cpp in Sources */,
-                               FE2D9474245EB2F400E48135 /* Bitmap.cpp in Sources */,
                                7C83DF131D0A590C00FEBCF3 /* RedBlackTree.cpp in Sources */,
                                7C83DF141D0A590C00FEBCF3 /* Ref.cpp in Sources */,
                                7C83DF151D0A590C00FEBCF3 /* RefCounter.cpp in Sources */,
index f79bd45..259289b 100644 (file)
@@ -1331,6 +1331,36 @@ TEST(WebAuthenticationPanel, LAMakeCredentialNoMockNoUserGesture)
     checkPanel([delegate panel], @"", @[adoptNS([[NSNumber alloc] initWithInt:_WKWebAuthenticationTransportUSB]).get()], _WKWebAuthenticationTypeCreate);
 }
 
+TEST(WebAuthenticationPanel, LAMakeCredentialRollBackCredential)
+{
+    reset();
+    RetainPtr<NSURL> testURL = [[NSBundle mainBundle] URLForResource:@"web-authentication-make-credential-la-no-attestation" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
+
+    auto *configuration = [WKWebViewConfiguration _test_configurationWithTestPlugInClassName:@"WebProcessPlugInWithInternals" configureJSCForTesting:YES];
+    [[configuration preferences] _setEnabled:YES forExperimentalFeature:webAuthenticationExperimentalFeature()];
+
+    auto webView = adoptNS([[TestWKWebView alloc] initWithFrame:NSZeroRect configuration:configuration]);
+    auto delegate = adoptNS([[TestWebAuthenticationPanelUIDelegate alloc] init]);
+    [webView setUIDelegate:delegate.get()];
+
+    localAuthenticatorPolicy = _WKLocalAuthenticatorPolicyAllow;
+    [webView loadRequest:[NSURLRequest requestWithURL:testURL.get()]];
+    [webView waitForMessage:@"Couldn't attest: The operation couldn't complete."];
+
+    NSDictionary *query = @{
+        (id)kSecClass: (id)kSecClassKey,
+        (id)kSecAttrKeyClass: (id)kSecAttrKeyClassPrivate,
+        (id)kSecAttrLabel: @"",
+#if HAVE(DATA_PROTECTION_KEYCHAIN)
+        (id)kSecUseDataProtectionKeychain: @YES
+#else
+        (id)kSecAttrNoLegacy: @YES
+#endif
+    };
+    OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, nullptr);
+    EXPECT_EQ(status, errSecItemNotFound);
+}
+
 // Skip the test because of <rdar://problem/59635486>.
 #if PLATFORM(MAC)
 
diff --git a/Tools/TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la-no-attestation.html b/Tools/TestWebKitAPI/Tests/WebKitCocoa/web-authentication-make-credential-la-no-attestation.html
new file mode 100644 (file)
index 0000000..017aa41
--- /dev/null
@@ -0,0 +1,42 @@
+<input type="text" id="input">
+<script>
+    const testES256PrivateKeyBase64 =
+        "BDj/zxSkzKgaBuS3cdWDF558of8AaIpgFpsjF/Qm1749VBJPgqUIwfhWHJ91nb7U" +
+        "PH76c0+WFOzZKslPyyFse4goGIW2R7k9VHLPEZl5nfnBgEVFh5zev+/xpHQIvuq6" +
+        "RQ==";
+    if (window.internals) {
+        internals.setMockWebAuthenticationConfiguration({
+            local: {
+                userVerification: "yes",
+                acceptAttestation: false,
+                privateKeyBase64: testES256PrivateKeyBase64,
+           }
+        });
+        internals.withUserGesture(() => { input.focus(); });
+    }
+
+    const options = {
+        publicKey: {
+            rp: {
+                name: "localhost",
+            },
+            user: {
+                name: "John Appleseed",
+                id: new Uint8Array(16),
+                displayName: "Appleseed",
+            },
+            challenge: new Uint8Array(16),
+            pubKeyCredParams: [{ type: "public-key", alg: -7 }],
+            attestation: "direct",
+            timeout: 100,
+        }
+    };
+
+    navigator.credentials.create(options).then(credential => {
+        // console.log("Succeeded!");
+        window.webkit.messageHandlers.testHandler.postMessage("Succeeded!");
+    }, error => {
+        // console.log(error.message);
+        window.webkit.messageHandlers.testHandler.postMessage(error.message);
+    });
+</script>
index efacced..d75d7c8 100644 (file)
@@ -52,6 +52,7 @@
             },
             challenge: new Uint8Array(16),
             pubKeyCredParams: [{ type: "public-key", alg: -7 }],
+            attestation: "direct",
             timeout: 100,
         }
     };