fido: fix a leak in WinWebAuthnApi

Authenticator{MakeCredential,GetAssertion}Blocking meant to free the
unmanaged pointers returned by Windows by wrapping them in unique_ptrs.
But they assigned nullptr, and the actual pointers were never freed.

Also make FakeWinWebAuthnApi ensure callers free all returned pointers
before it is destructed. Also fix some doc strings.

This is a follow-up to comments in
https://chromium-review.googlesource.com/c/chromium/src/+/2197475.

Fixed: 1081450, 1084698
Change-Id: Ifc4d228a63f679f1f3cd2f2f75b11c49b4af8d3c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2207736
Commit-Queue: Martin Kreichgauer <martinkr@google.com>
Reviewed-by: Nina Satragno <nsatragno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#770360}

device/fido/virtual_fido_device.h

@@ -53,10 +53,10 @@ class COMPONENT_EXPORT(DEVICE_FIDO) VirtualFidoDevice : public FidoDevice {
     // FreshP256Key returns a randomly generated P-256 PrivateKey.
     static std::unique_ptr<PrivateKey> FreshP256Key();
 
-    // FreshP256Key returns a randomly generated RSA PrivateKey.
+    // FreshRSAKey returns a randomly generated RSA PrivateKey.
     static std::unique_ptr<PrivateKey> FreshRSAKey();
 
-    // FreshP256Key returns a randomly generated Ed25519 PrivateKey.
+    // FreshEd25519Key returns a randomly generated Ed25519 PrivateKey.
     static std::unique_ptr<PrivateKey> FreshEd25519Key();
 
     virtual ~PrivateKey();

device/fido/win/fake_webauthn_api.cc

@@ -29,7 +29,11 @@ struct FakeWinWebAuthnApi::WebAuthnAssertionEx {
 };
 
 FakeWinWebAuthnApi::FakeWinWebAuthnApi() = default;
-FakeWinWebAuthnApi::~FakeWinWebAuthnApi() = default;
+FakeWinWebAuthnApi::~FakeWinWebAuthnApi() {
+  // Ensure callers free unmanaged pointers returned by the real Windows API.
+  DCHECK(returned_attestations_.empty());
+  DCHECK(returned_assertions_.empty());
+}
 
 bool FakeWinWebAuthnApi::InjectNonDiscoverableCredential(
     base::span<const uint8_t> credential_id,
@@ -81,8 +85,13 @@ HRESULT FakeWinWebAuthnApi::AuthenticatorMakeCredential(
     PWEBAUTHN_CREDENTIAL_ATTESTATION* credential_attestation_ptr) {
   // TODO(martinkr): Implement to create a credential in |registrations_|.
   DCHECK(is_available_);
-  *credential_attestation_ptr = &fake_attestation_;
-  return result_override_;
+  if (result_override_ != S_OK) {
+    return result_override_;
+  }
+
+  returned_attestations_.push_back(FakeAttestation());
+  *credential_attestation_ptr = &returned_attestations_.back();
+  return S_OK;
 }
 
 HRESULT FakeWinWebAuthnApi::AuthenticatorGetAssertion(
@@ -223,12 +232,28 @@ PCWSTR FakeWinWebAuthnApi::GetErrorName(HRESULT hr) {
 }
 
 void FakeWinWebAuthnApi::FreeCredentialAttestation(
-    PWEBAUTHN_CREDENTIAL_ATTESTATION) {
-  // |returned_attestations_| gets freed upon destruction of |this|.
+    PWEBAUTHN_CREDENTIAL_ATTESTATION credential_attestation) {
+  for (auto it = returned_attestations_.begin();
+       it != returned_attestations_.end(); ++it) {
+    if (credential_attestation != &*it) {
+      continue;
+    }
+    returned_attestations_.erase(it);
+    return;
+  }
+  NOTREACHED();
 }
 
-void FakeWinWebAuthnApi::FreeAssertion(PWEBAUTHN_ASSERTION pWebAuthNAssertion) {
-  // |returned_assertions_| gets freed upon destruction of |this|.
+void FakeWinWebAuthnApi::FreeAssertion(PWEBAUTHN_ASSERTION assertion) {
+  for (auto it = returned_assertions_.begin(); it != returned_assertions_.end();
+       ++it) {
+    if (assertion != &it->assertion) {
+      continue;
+    }
+    returned_assertions_.erase(it);
+    return;
+  }
+  NOTREACHED();
 }
 
 int FakeWinWebAuthnApi::Version() {

device/fido/win/fake_webauthn_api.h

@@ -95,8 +95,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) FakeWinWebAuthnApi : public WinWebAuthnApi {
   int version_ = WEBAUTHN_API_VERSION_2;
   HRESULT result_override_ = S_OK;
 
-  // Owns the fake attestation returned by AuthenticatorMakeCredential().
-  WEBAUTHN_CREDENTIAL_ATTESTATION fake_attestation_ = FakeAttestation();
+  // Owns the attestations returned by AuthenticatorMakeCredential().
+  std::vector<WEBAUTHN_CREDENTIAL_ATTESTATION> returned_attestations_;
 
   // Owns assertions returned by AuthenticatorGetAssertion().
   std::vector<WebAuthnAssertionEx> returned_assertions_;

device/fido/win/webauthn_api.cc

@@ -295,13 +295,6 @@ AuthenticatorMakeCredentialBlocking(WinWebAuthnApi* webauthn_api,
   };
 
   WEBAUTHN_CREDENTIAL_ATTESTATION* credential_attestation = nullptr;
-  std::unique_ptr<WEBAUTHN_CREDENTIAL_ATTESTATION,
-                  std::function<void(PWEBAUTHN_CREDENTIAL_ATTESTATION)>>
-      credential_attestation_deleter(
-          credential_attestation,
-          [webauthn_api](PWEBAUTHN_CREDENTIAL_ATTESTATION ptr) {
-            webauthn_api->FreeCredentialAttestation(ptr);
-          });
 
   FIDO_LOG(DEBUG) << "WebAuthNAuthenticatorMakeCredential("
                   << "rp=" << rp_info << ", user=" << user_info
@@ -312,6 +305,14 @@ AuthenticatorMakeCredentialBlocking(WinWebAuthnApi* webauthn_api,
   HRESULT hresult = webauthn_api->AuthenticatorMakeCredential(
       h_wnd, &rp_info, &user_info, &cose_credential_parameters, &client_data,
       &options, &credential_attestation);
+  std::unique_ptr<WEBAUTHN_CREDENTIAL_ATTESTATION,
+                  std::function<void(PWEBAUTHN_CREDENTIAL_ATTESTATION)>>
+      credential_attestation_deleter(
+          credential_attestation,
+          [webauthn_api](PWEBAUTHN_CREDENTIAL_ATTESTATION ptr) {
+            webauthn_api->FreeCredentialAttestation(ptr);
+          });
+
   if (hresult != S_OK) {
     FIDO_LOG(DEBUG) << "WebAuthNAuthenticatorMakeCredential()="
                     << webauthn_api->GetErrorName(hresult);
@@ -403,16 +404,17 @@ AuthenticatorGetAssertionBlocking(WinWebAuthnApi* webauthn_api,
   };
 
   WEBAUTHN_ASSERTION* assertion = nullptr;
-  std::unique_ptr<WEBAUTHN_ASSERTION, std::function<void(PWEBAUTHN_ASSERTION)>>
-      assertion_deleter(assertion, [webauthn_api](PWEBAUTHN_ASSERTION ptr) {
-        webauthn_api->FreeAssertion(ptr);
-      });
 
   FIDO_LOG(DEBUG) << "WebAuthNAuthenticatorGetAssertion("
                   << "rp_id=\"" << rp_id16 << "\", client_data=" << client_data
                   << ", options=" << options << ")";
   HRESULT hresult = webauthn_api->AuthenticatorGetAssertion(
       h_wnd, base::as_wcstr(rp_id16), &client_data, &options, &assertion);
+  std::unique_ptr<WEBAUTHN_ASSERTION, std::function<void(PWEBAUTHN_ASSERTION)>>
+      assertion_deleter(assertion, [webauthn_api](PWEBAUTHN_ASSERTION ptr) {
+        webauthn_api->FreeAssertion(ptr);
+      });
+
   if (hresult != S_OK) {
     FIDO_LOG(DEBUG) << "WebAuthNAuthenticatorGetAssertion()="
                     << webauthn_api->GetErrorName(hresult);