device/fido: collect v1 elements of caBLE discovery

This is a prelude to adding v2 elements in a subsequent change.

Bug: 1002262

Change-Id: I4313a83d3dd9670ed0a92cd67794e47f0974bf88
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1785660Reviewed-by: Ken Buchanan <kenrb@chromium.org>
Commit-Queue: Adam Langley <agl@chromium.org>
Cr-Commit-Position: refs/heads/master@{#695172}

content/browser/webauth/authenticator_impl_unittest.cc

@@ -327,10 +327,11 @@ GetTestPublicKeyCredentialRequestOptions() {
 
 std::vector<device::CableDiscoveryData> GetTestCableExtension() {
   device::CableDiscoveryData cable;
-  cable.version = 1;
-  cable.client_eid.fill(0x01);
-  cable.authenticator_eid.fill(0x02);
-  cable.session_pre_key.fill(0x03);
+  cable.version = device::CableDiscoveryData::Version::V1;
+  cable.v1.emplace();
+  cable.v1->client_eid.fill(0x01);
+  cable.v1->authenticator_eid.fill(0x02);
+  cable.v1->session_pre_key.fill(0x03);
 
   std::vector<device::CableDiscoveryData> ret;
   ret.emplace_back(std::move(cable));

content/browser/webauth/authenticator_mojom_traits.cc

@@ -236,10 +236,14 @@ bool StructTraits<blink::mojom::CableAuthenticationDataView,
                   device::CableDiscoveryData>::
     Read(blink::mojom::CableAuthenticationDataView data,
          device::CableDiscoveryData* out) {
-  out->version = data.version();
-  if (!data.ReadClientEid(&out->client_eid) ||
-      !data.ReadAuthenticatorEid(&out->authenticator_eid) ||
-      !data.ReadSessionPreKey(&out->session_pre_key)) {
+  if (data.version() != 1) {
+    return false;
+  }
+  out->version = device::CableDiscoveryData::Version::V1;
+  out->v1.emplace();
+  if (!data.ReadClientEid(&out->v1->client_eid) ||
+      !data.ReadAuthenticatorEid(&out->v1->authenticator_eid) ||
+      !data.ReadSessionPreKey(&out->v1->session_pre_key)) {
     return false;
   }
   return true;

content/browser/webauth/authenticator_mojom_traits.h

@@ -186,22 +186,26 @@ struct BLINK_COMMON_EXPORT
     StructTraits<blink::mojom::CableAuthenticationDataView,
                  device::CableDiscoveryData> {
   static uint8_t version(const device::CableDiscoveryData& in) {
-    return in.version;
+    CHECK_EQ(device::CableDiscoveryData::Version::V1, in.version);
+    return 1;
   }
 
   static const device::CableEidArray& client_eid(
       const device::CableDiscoveryData& in) {
-    return in.client_eid;
+    CHECK_EQ(device::CableDiscoveryData::Version::V1, in.version);
+    return in.v1->client_eid;
   }
 
   static const device::CableEidArray& authenticator_eid(
       const device::CableDiscoveryData& in) {
-    return in.authenticator_eid;
+    CHECK_EQ(device::CableDiscoveryData::Version::V1, in.version);
+    return in.v1->authenticator_eid;
   }
 
   static const device::CableSessionPreKeyArray& session_pre_key(
       const device::CableDiscoveryData& in) {
-    return in.session_pre_key;
+    CHECK_EQ(device::CableDiscoveryData::Version::V1, in.version);
+    return in.v1->session_pre_key;
   }
 
   static bool Read(blink::mojom::CableAuthenticationDataView data,

content/browser/webauth/authenticator_mojom_traits_unittest.cc

@@ -159,7 +159,8 @@ TEST(AuthenticatorMojomTraitsTest, SerializePublicKeyCredentialUserEntity) {
 // Verify serialization and deserialization of CableDiscoveryData.
 TEST(AuthenticatorMojomTraitsTest, SerializeCableDiscoveryData) {
   std::vector<CableDiscoveryData> success_cases = {
-      CableDiscoveryData(0, kClientEid, kAuthenticatorEid, kSessionPreKey)};
+      CableDiscoveryData(CableDiscoveryData::Version::V1, kClientEid,
+                         kAuthenticatorEid, kSessionPreKey)};
 
   AssertSerializeAndDeserializeSucceeds<blink::mojom::CableAuthentication,
                                         CableDiscoveryData>(success_cases);

content/browser/webauth/webauth_browsertest.cc

@@ -1264,6 +1264,42 @@ IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
   }
 }
 
+IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest,
+                       BadCableExtensionVersions) {
+  // The caBLE extension should only contain v1 data. Test that nothing crashes
+  // if a site tries to set other versions.
+
+  InjectVirtualFidoDeviceFactory();
+  GetParameters parameters;
+  parameters.allow_credentials =
+      "allowCredentials: [{ type: 'public-key',"
+      "  id: new TextEncoder().encode('allowedCredential'),"
+      "  transports: ['cable']}],"
+      "extensions: {"
+      "  cableAuthentication: [{"
+      "    version: 1,"
+      "    clientEid: new Uint8Array(Array(16).fill(1)),"
+      "    authenticatorEid: new Uint8Array(Array(16).fill(2)),"
+      "    sessionPreKey: new Uint8Array(Array(32).fill(3)),"
+      "  },{"
+      "    version: 2,"
+      "    clientEid: new Uint8Array(Array(16).fill(1)),"
+      "    authenticatorEid: new Uint8Array(Array(16).fill(2)),"
+      "    sessionPreKey: new Uint8Array(Array(32).fill(3)),"
+      "  },{"
+      "    version: 3,"
+      "    clientEid: new Uint8Array(Array(16).fill(1)),"
+      "    authenticatorEid: new Uint8Array(Array(16).fill(2)),"
+      "    sessionPreKey: new Uint8Array(Array(32).fill(3)),"
+      "  }]"
+      "}";
+  std::string result;
+  ASSERT_TRUE(content::ExecuteScriptAndExtractString(
+      shell()->web_contents()->GetMainFrame(),
+      BuildGetCallWithParameters(parameters), &result));
+  ASSERT_EQ(kNotAllowedErrorMessage, result);
+}
+
 #if defined(OS_WIN)
 IN_PROC_BROWSER_TEST_F(WebAuthJavascriptClientBrowserTest, WinMakeCredential) {
   NavigateToURL(shell(), GetHttpsURL("www.acme.com", "/title1.html"));

device/fido/cable/cable_discovery_data.h

@@ -32,7 +32,13 @@ using QRGeneratorKey = std::array<uint8_t, 32>;
 // TODO(hongjunchoi): Add discovery data required for MakeCredential request.
 // See: https://crbug.com/837088
 struct COMPONENT_EXPORT(DEVICE_FIDO) CableDiscoveryData {
-  CableDiscoveryData(uint8_t version,
+  enum class Version {
+    INVALID,
+    V1,
+    V2,
+  };
+
+  CableDiscoveryData(Version version,
                      const CableEidArray& client_eid,
                      const CableEidArray& authenticator_eid,
                      const CableSessionPreKeyArray& session_pre_key);
@@ -61,10 +67,17 @@ struct COMPONENT_EXPORT(DEVICE_FIDO) CableDiscoveryData {
       base::span<const uint8_t, 32> qr_generator_key,
       const int64_t tick);
 
-  uint8_t version;
-  CableEidArray client_eid;
-  CableEidArray authenticator_eid;
-  CableSessionPreKeyArray session_pre_key;
+  // version indicates whether v1 or v2 data is contained in this object.
+  // |INVALID| is not a valid version but is set as the default to catch any
+  // cases where the version hasn't been set explicitly.
+  Version version = Version::INVALID;
+
+  struct V1Data {
+    CableEidArray client_eid;
+    CableEidArray authenticator_eid;
+    CableSessionPreKeyArray session_pre_key;
+  };
+  base::Optional<V1Data> v1;
 };
 
 }  // namespace device

device/fido/cable/fido_cable_discovery.cc

@@ -40,7 +40,6 @@ namespace {
 // instead, and on Mac our only option is to advertise an additional service
 // with the EID as its UUID.
 std::unique_ptr<BluetoothAdvertisement::Data> ConstructAdvertisementData(
-    uint8_t version_number,
     base::span<const uint8_t, kCableEphemeralIdSize> client_eid) {
   auto advertisement_data = std::make_unique<BluetoothAdvertisement::Data>(
       BluetoothAdvertisement::AdvertisementType::ADVERTISEMENT_TYPE_BROADCAST);
@@ -66,7 +65,7 @@ std::unique_ptr<BluetoothAdvertisement::Data> ConstructAdvertisementData(
       3u + kCableEphemeralIdSize;
   std::array<uint8_t, 4> kCableGoogleManufacturerDataHeader = {
       kCableGoogleManufacturerDataLength, kCableGoogleManufacturerDataType,
-      kCableFlags, version_number};
+      kCableFlags, /*version=*/1};
 
   auto manufacturer_data =
       std::make_unique<BluetoothAdvertisement::ManufacturerData>();
@@ -91,7 +90,7 @@ std::unique_ptr<BluetoothAdvertisement::Data> ConstructAdvertisementData(
   // Since the remainder of this service data field is a Cable EID, set the 5th
   // bit of the flag byte.
   service_data_value[0] = kCableFlags;
-  service_data_value[1] = version_number;
+  service_data_value[1] = 1 /* version */;
   std::copy(client_eid.begin(), client_eid.end(),
             service_data_value.begin() + 2);
   service_data->emplace(kCableAdvertisementUUID128,
@@ -109,14 +108,17 @@ std::unique_ptr<BluetoothAdvertisement::Data> ConstructAdvertisementData(
 CableDiscoveryData::CableDiscoveryData() = default;
 
 CableDiscoveryData::CableDiscoveryData(
-    uint8_t version,
+    CableDiscoveryData::Version version,
     const CableEidArray& client_eid,
     const CableEidArray& authenticator_eid,
     const CableSessionPreKeyArray& session_pre_key)
-    : version(version),
-      client_eid(client_eid),
-      authenticator_eid(authenticator_eid),
-      session_pre_key(session_pre_key) {}
+    : version(version) {
+  CHECK_EQ(Version::V1, version);
+  v1.emplace();
+  v1->client_eid = client_eid;
+  v1->authenticator_eid = authenticator_eid;
+  v1->session_pre_key = session_pre_key;
+}
 
 CableDiscoveryData::CableDiscoveryData(const CableDiscoveryData& data) =
     default;
@@ -127,9 +129,21 @@ CableDiscoveryData& CableDiscoveryData::operator=(
 CableDiscoveryData::~CableDiscoveryData() = default;
 
 bool CableDiscoveryData::operator==(const CableDiscoveryData& other) const {
-  return version == other.version && client_eid == other.client_eid &&
-         authenticator_eid == other.authenticator_eid &&
-         session_pre_key == other.session_pre_key;
+  if (version != other.version) {
+    return false;
+  }
+
+  switch (version) {
+    case CableDiscoveryData::Version::V1:
+      return v1->client_eid == other.v1->client_eid &&
+             v1->authenticator_eid == other.v1->authenticator_eid &&
+             v1->session_pre_key == other.v1->session_pre_key;
+
+    case CableDiscoveryData::Version::V2:
+    case CableDiscoveryData::Version::INVALID:
+      CHECK(false);
+      return false;
+  }
 }
 
 // static
@@ -207,29 +221,30 @@ FidoCableDiscovery::CreateHandshakeHandler(
     const CableDiscoveryData* discovery_data) {
   std::unique_ptr<FidoCableHandshakeHandler> handler;
   switch (discovery_data->version) {
-    case 1: {
+    case CableDiscoveryData::Version::V1: {
       // Nonce is embedded as first 8 bytes of client EID.
       std::array<uint8_t, 8> nonce;
       const bool ok = fido_parsing_utils::ExtractArray(
-          discovery_data->client_eid, 0, &nonce);
+          discovery_data->v1->client_eid, 0, &nonce);
       DCHECK(ok);
 
       handler.reset(new FidoCableV1HandshakeHandler(
-          device, nonce, discovery_data->session_pre_key));
+          device, nonce, discovery_data->v1->session_pre_key));
       break;
     }
 
-    case 2:
+    case CableDiscoveryData::Version::V2: {
+      // Nonce is embedded as first 8 bytes of client EID.
       if (!base::FeatureList::IsEnabled(device::kWebAuthPhoneSupport)) {
         return base::nullopt;
       }
       handler.reset(new FidoCableV2HandshakeHandler(
-          device, discovery_data->session_pre_key));
+          device, discovery_data->v1->session_pre_key));
       break;
+    }
 
-    default:
-      FIDO_LOG(DEBUG) << "Dropping caBLE handshake request for unknown version "
-                      << discovery_data->version;
+    case CableDiscoveryData::Version::INVALID:
+      CHECK(false);
       return base::nullopt;
   }
 
@@ -326,11 +341,14 @@ void FidoCableDiscovery::StartAdvertisement() {
 
   FIDO_LOG(DEBUG) << "Starting to advertise clientEID.";
   for (const auto& data : discovery_data_) {
+    if (data.version != CableDiscoveryData::Version::V1) {
+      continue;
+    }
     adapter()->RegisterAdvertisement(
-        ConstructAdvertisementData(data.version, data.client_eid),
+        ConstructAdvertisementData(data.v1->client_eid),
         base::AdaptCallbackForRepeating(
             base::BindOnce(&FidoCableDiscovery::OnAdvertisementRegistered,
-                           weak_factory_.GetWeakPtr(), data.client_eid)),
+                           weak_factory_.GetWeakPtr(), data.v1->client_eid)),
         base::AdaptCallbackForRepeating(
             base::BindOnce(&FidoCableDiscovery::OnAdvertisementRegisterError,
                            weak_factory_.GetWeakPtr())));
@@ -387,13 +405,14 @@ void FidoCableDiscovery::CableDeviceFound(BluetoothAdapter* adapter,
   const std::string device_address = device->GetAddress();
   if (!found_cable_device_data ||
       base::Contains(active_authenticator_eids_,
-                     found_cable_device_data->authenticator_eid) ||
+                     found_cable_device_data->v1->authenticator_eid) ||
       base::Contains(active_devices_, device_address)) {
     return;
   }
 
   FIDO_LOG(EVENT) << "Found new caBLE device.";
-  active_authenticator_eids_.insert(found_cable_device_data->authenticator_eid);
+  active_authenticator_eids_.insert(
+      found_cable_device_data->v1->authenticator_eid);
   active_devices_.insert(device_address);
 
   auto cable_device =
@@ -523,7 +542,7 @@ FidoCableDiscovery::GetCableDiscoveryDataFromAuthenticatorEid(
   auto discovery_data_iterator =
       std::find_if(discovery_data_.begin(), discovery_data_.end(),
                    [&authenticator_eid](const auto& data) {
-                     return authenticator_eid == data.authenticator_eid;
+                     return authenticator_eid == data.v1->authenticator_eid;
                    });
 
   if (discovery_data_iterator != discovery_data_.end()) {
@@ -547,8 +566,8 @@ FidoCableDiscovery::GetCableDiscoveryDataFromAuthenticatorEid(
           *qr_generator_key_, current_tick - i);
       if (expected_authenticator_eid == authenticator_eid) {
         CableEidArray zero_eid{};
-        return CableDiscoveryData(/*version=*/2, zero_eid, authenticator_eid,
-                                  session_pre_key);
+        return CableDiscoveryData(CableDiscoveryData::Version::V2, zero_eid,
+                                  authenticator_eid, session_pre_key);
       }
     }
   }

device/fido/cable/fido_cable_discovery_unittest.cc

@@ -32,7 +32,10 @@ namespace device {
 
 namespace {
 
-constexpr uint8_t kTestCableVersionNumber = 0x01;
+constexpr auto kTestCableVersion = CableDiscoveryData::Version::V1;
+#if defined(OS_WIN) || defined(OS_LINUX) || defined(OS_CHROMEOS)
+constexpr auto kTestCableVersionNumber = 1;
+#endif
 
 // Constants required for discovering and constructing a Cable device that
 // are given by the relying party via an extension.
@@ -300,11 +303,11 @@ class FakeFidoCableDiscovery : public FidoCableDiscovery {
                          const CableDiscoveryData* discovery_data) override {
     // Nonce is embedded as first 8 bytes of client EID.
     std::array<uint8_t, 8> nonce;
-    const bool ok =
-        fido_parsing_utils::ExtractArray(discovery_data->client_eid, 0, &nonce);
+    const bool ok = fido_parsing_utils::ExtractArray(
+        discovery_data->v1->client_eid, 0, &nonce);
     DCHECK(ok);
     return std::make_unique<FakeHandshakeHandler>(
-        device, nonce, discovery_data->session_pre_key);
+        device, nonce, discovery_data->v1->session_pre_key);
   }
 
   static std::array<uint8_t, 32> BogusQRGeneratorKey() {
@@ -320,7 +323,7 @@ class FidoCableDiscoveryTest : public ::testing::Test {
  public:
   std::unique_ptr<FidoCableDiscovery> CreateDiscovery() {
     std::vector<CableDiscoveryData> discovery_data;
-    discovery_data.emplace_back(kTestCableVersionNumber, kClientEid,
+    discovery_data.emplace_back(kTestCableVersion, kClientEid,
                                 kAuthenticatorEid, kTestSessionPreKey);
     return std::make_unique<FakeFidoCableDiscovery>(std::move(discovery_data));
   }
@@ -398,9 +401,9 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryFindsIncorrectDevice) {
 // BluetoothAdapter::RegisterAdvertisement().
 TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithMultipleEids) {
   std::vector<CableDiscoveryData> discovery_data;
-  discovery_data.emplace_back(kTestCableVersionNumber, kClientEid,
-                              kAuthenticatorEid, kTestSessionPreKey);
-  discovery_data.emplace_back(kTestCableVersionNumber, kSecondaryClientEid,
+  discovery_data.emplace_back(kTestCableVersion, kClientEid, kAuthenticatorEid,
+                              kTestSessionPreKey);
+  discovery_data.emplace_back(kTestCableVersion, kSecondaryClientEid,
                               kSecondaryAuthenticatorEid,
                               kSecondarySessionPreKey);
   auto cable_discovery =
@@ -432,9 +435,9 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithMultipleEids) {
 // scanning process should be invoked.
 TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithPartialAdvertisementSuccess) {
   std::vector<CableDiscoveryData> discovery_data;
-  discovery_data.emplace_back(kTestCableVersionNumber, kClientEid,
-                              kAuthenticatorEid, kTestSessionPreKey);
-  discovery_data.emplace_back(kTestCableVersionNumber, kSecondaryClientEid,
+  discovery_data.emplace_back(kTestCableVersion, kClientEid, kAuthenticatorEid,
+                              kTestSessionPreKey);
+  discovery_data.emplace_back(kTestCableVersion, kSecondaryClientEid,
                               kSecondaryAuthenticatorEid,
                               kSecondarySessionPreKey);
   auto cable_discovery =
@@ -463,9 +466,9 @@ TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithPartialAdvertisementSuccess) {
 // Test the scenario when all advertisement for client EID's fails.
 TEST_F(FidoCableDiscoveryTest, TestDiscoveryWithAdvertisementFailures) {
   std::vector<CableDiscoveryData> discovery_data;
-  discovery_data.emplace_back(kTestCableVersionNumber, kClientEid,
-                              kAuthenticatorEid, kTestSessionPreKey);
-  discovery_data.emplace_back(kTestCableVersionNumber, kSecondaryClientEid,
+  discovery_data.emplace_back(kTestCableVersion, kClientEid, kAuthenticatorEid,
+                              kTestSessionPreKey);
+  discovery_data.emplace_back(kTestCableVersion, kSecondaryClientEid,
                               kSecondaryAuthenticatorEid,
                               kSecondarySessionPreKey);
   auto cable_discovery =

third_party/blink/renderer/modules/credentialmanager/credential_manager_type_converters.cc

@@ -575,13 +575,18 @@ TypeConverter<PublicKeyCredentialRequestOptionsPtr,
     if (extensions->hasCableAuthentication()) {
       Vector<CableAuthenticationPtr> mojo_data;
       for (auto& data : extensions->cableAuthentication()) {
+        if (data->version() != 1) {
+          continue;
+        }
         CableAuthenticationPtr mojo_cable =
             CableAuthentication::From(data.Get());
         if (mojo_cable) {
           mojo_data.push_back(std::move(mojo_cable));
         }
       }
-      mojo_options->cable_authentication_data = std::move(mojo_data);
+      if (mojo_data.size() > 0) {
+        mojo_options->cable_authentication_data = std::move(mojo_data);
+      }
     }
 #if defined(OS_ANDROID)
     if (extensions->hasUvm()) {