device/fido: turn the caBLEv2 handshake around.

The current caBLEv2 handshake has the desktop speak first. We expect the
network-based design to have the phone speak first, thus this change
turns the handshake “around” to accommodate that. Also, the current v2
handshake includes pairing information in the response but, if the phone
speaks first, the response is coming from the wrong place to include
pairing information and the first message has insufficient protection to
include it. Thus that is removed. Instead, a getInfo message can be
included in the phone's first message because it'll be needed anyway and
it's generic and thus doesn't require forward security.

BUG=1002262

Change-Id: If2fb27b2a56ef21c0b21a454ff498f65ce62e81f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2340062
Commit-Queue: Adam Langley <agl@chromium.org>
Reviewed-by: Martin Kreichgauer <martinkr@google.com>
Cr-Commit-Position: refs/heads/master@{#795679}

chrome/android/features/cablev2_authenticator/native/cablev2_authenticator_android.cc

@@ -399,9 +399,7 @@ class BLEClient {
         }
 
         std::vector<uint8_t> plaintext;
-        if (!crypter_->Decrypt(
-                static_cast<device::FidoBleDeviceCommand>(message->first),
-                message->second, &plaintext) ||
+        if (!crypter_->Decrypt(message->second, &plaintext) ||
             plaintext.empty()) {
           FIDO_LOG(ERROR) << "Decryption failed";
           return false;
@@ -889,7 +887,8 @@ class CableInterface : public BLEClient::Delegate {
     std::array<uint8_t, AES_BLOCK_SIZE> eid;
     AES_encrypt(/*in=*/eid_plaintext, /*out=*/eid.data(), &key);
 
-    out_nonce_and_eid->first = nonce;
+    // TODO: nonces are now a different size.
+    // out_nonce_and_eid->first = nonce;
     out_nonce_and_eid->second = eid;
 
     Java_CableAuthenticator_sendBLEAdvert(env_, cable_authenticator_,

device/fido/BUILD.gn

@@ -326,6 +326,19 @@ fuzzer_test("fido_cable_handshake_handler_fuzzer") {
   libfuzzer_options = [ "max_len=2048" ]
 }
 
+fuzzer_test("v2_handshake_fuzzer") {
+  sources = [ "cable/v2_handshake_fuzzer.cc" ]
+  deps = [
+    ":fido",
+    "//base",
+    "//base/test:test_support",
+    "//device/bluetooth:mocks",
+    "//testing/gmock",
+    "//testing/gtest",
+  ]
+  libfuzzer_options = [ "max_len=2048" ]
+}
+
 is_linux_without_udev = (is_linux || is_chromeos) && !use_udev
 
 source_set("test_support") {

device/fido/cable/v2_handshake.h

@@ -13,6 +13,7 @@
 #include "base/component_export.h"
 #include "base/containers/span.h"
 #include "base/optional.h"
+#include "components/cbor/values.h"
 #include "device/fido/cable/cable_discovery_data.h"
 #include "device/fido/cable/noise.h"
 #include "device/fido/fido_constants.h"
@@ -21,15 +22,28 @@
 namespace device {
 namespace cablev2 {
 
+// EncodePaddedCBORMap encodes the given map and pads it to 256 bytes in such a
+// way that |DecodePaddedCBORMap| can decode it. The padding is done on the
+// assumption that the returned bytes will be encrypted and the encoded size of
+// the map should be hidden. The function can fail if the CBOR encoding fails
+// or, somehow, the size overflows.
+COMPONENT_EXPORT(DEVICE_FIDO)
+base::Optional<std::vector<uint8_t>> EncodePaddedCBORMap(
+    cbor::Value::MapValue map);
+
+// DecodePaddedCBORMap unpads and decodes a CBOR map as produced by
+// |EncodePaddedCBORMap|.
+COMPONENT_EXPORT(DEVICE_FIDO)
+base::Optional<cbor::Value> DecodePaddedCBORMap(
+    base::span<const uint8_t> input);
+
 // NonceAndEID contains both the random nonce chosen for an advert, as well as
 // the EID that was generated from it.
-typedef std::pair<std::array<uint8_t, device::kCableNonceSize>,
+constexpr size_t kNonceSize = 10;
+typedef std::pair<std::array<uint8_t, kNonceSize>,
                   std::array<uint8_t, device::kCableEphemeralIdSize>>
     NonceAndEID;
 
-// kP256PointSize is the number of bytes in an X9.62 encoding of a P-256 point.
-constexpr size_t kP256PointSize = 65;
-
 // Crypter handles the post-handshake encryption of CTAP2 messages.
 class COMPONENT_EXPORT(DEVICE_FIDO) Crypter {
  public:
@@ -47,8 +61,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) Crypter {
   //
   // (In practice, command must always be |kMsg|. But passing it here makes it
   // less likely that other code will forget to check that.)
-  bool Decrypt(FidoBleDeviceCommand command,
-               base::span<const uint8_t> ciphertext,
+  bool Decrypt(base::span<const uint8_t> ciphertext,
                std::vector<uint8_t>* out_plaintext);
 
   // IsCounterpartyOfForTesting returns true if |other| is the mirror-image of
@@ -62,7 +75,8 @@ class COMPONENT_EXPORT(DEVICE_FIDO) Crypter {
 };
 
 // HandshakeInitiator starts a caBLE v2 handshake and processes the single
-// response message from the other party.
+// response message from the other party. The handshake is always initiated from
+// the phone.
 class COMPONENT_EXPORT(DEVICE_FIDO) HandshakeInitiator {
  public:
   HandshakeInitiator(
@@ -71,58 +85,63 @@ class COMPONENT_EXPORT(DEVICE_FIDO) HandshakeInitiator {
       base::span<const uint8_t, 32> psk_gen_key,
       // nonce is randomly generated per advertisement and ensures that BLE
       // adverts are non-deterministic.
-      base::span<const uint8_t, 8> nonce,
-      // eid is the EID that was advertised for this handshake. This is checked
-      // as part of the handshake.
-      base::span<const uint8_t, kCableEphemeralIdSize> eid,
-      // peer_identity, if given, specifies that this is a paired handshake
-      // and then contains an X9.62, P-256 public key for the peer. Otherwise
-      // this is a QR-code handshake.
-      base::Optional<base::span<const uint8_t, kP256PointSize>> peer_identity,
-      // local_identity must be provided if |peer_identity| is not. It contains
-      // the seed for deriving the local identity key.
-      base::Optional<base::span<const uint8_t, kCableIdentityKeySeedSize>>
-          local_identity);
+      base::span<const uint8_t, kNonceSize> nonce,
+      // peer_identity, if not nullopt, specifies that this is a QR handshake
+      // and then contains a P-256 public key for the peer. Otherwise this is a
+      // paired handshake.
+      base::Optional<base::span<const uint8_t, kP256X962Length>> peer_identity,
+      // local_identity must be provided iff |peer_identity| is not. It contains
+      // the local identity key.
+      bssl::UniquePtr<EC_KEY> local_identity);
 
   ~HandshakeInitiator();
 
   // BuildInitialMessage returns the handshake message to send to the peer to
   // start a handshake.
-  std::vector<uint8_t> BuildInitialMessage();
+  std::vector<uint8_t> BuildInitialMessage(
+      // eid is the EID that was advertised for this handshake. This is checked
+      // as part of the handshake.
+      base::span<const uint8_t, kCableEphemeralIdSize> eid,
+      // getinfo contains the CBOR-serialised getInfo response for this
+      // authenticator. This is assumed not to contain highly-sensitive
+      // information and is included to avoid an extra round-trip. (It is
+      // encrypted but an attacker who could eavesdrop on the tunnel connection
+      // and observe the QR code could obtain it.)
+      base::span<const uint8_t> get_info_bytes);
 
   // ProcessResponse processes the handshake response from the peer. If
   // successful it returns a |Crypter| for protecting future messages on the
-  // connection. If the peer choose to send long-term pairing data, that is also
-  // returned.
-  base::Optional<std::pair<std::unique_ptr<Crypter>,
-                           base::Optional<std::unique_ptr<CableDiscoveryData>>>>
-  ProcessResponse(base::span<const uint8_t> response);
+  // connection.
+  base::Optional<std::unique_ptr<Crypter>> ProcessResponse(
+      base::span<const uint8_t> response);
 
  private:
   Noise noise_;
-  std::array<uint8_t, 16> eid_;
   std::array<uint8_t, 32> psk_;
 
-  base::Optional<std::array<uint8_t, kP256PointSize>> peer_identity_;
+  base::Optional<std::array<uint8_t, kP256X962Length>> peer_identity_;
   bssl::UniquePtr<EC_KEY> local_identity_;
   bssl::UniquePtr<EC_KEY> ephemeral_key_;
 };
 
-// RespondToHandshake responds to a caBLE v2 handshake started by a peer.
+// RespondToHandshake responds to a caBLE v2 handshake started by a peer. It
+// returns a Crypter for encrypting and decrypting future messages, as well as
+// the getInfo response from the phone.
 COMPONENT_EXPORT(DEVICE_FIDO)
-base::Optional<std::unique_ptr<Crypter>> RespondToHandshake(
-    // See |HandshakeInitiator| comments about these first three arguments.
+base::Optional<std::pair<std::unique_ptr<Crypter>, std::vector<uint8_t>>>
+RespondToHandshake(
+    // For the first two arguments see |HandshakeInitiator| comments about
+    // |psk_gen_key| and |nonce|, and the |BuildInitialMessage| comment about
+    // |eid|.
     base::span<const uint8_t, 32> psk_gen_key,
     const NonceAndEID& nonce_and_eid,
-    // identity, if not nullptr, specifies that this is a paired handshake and
-    // contains the long-term identity key for this authenticator.
-    const EC_KEY* identity,
-    // peer_identity, which must be non-nullptr iff |identity| is nullptr,
-    // contains the peer's public key as derived from the QR-code data.
-    const EC_POINT* peer_identity,
-    // pairing_data, if not nullptr, contains long-term pairing data that will
-    // be shared with the peer. This is mutually exclusive with |identity|.
-    const CableDiscoveryData* pairing_data,
+    // identity_seed, if not nullopt, specifies that this is a QR handshake and
+    // contains the seed for QR key for this client.
+    base::Optional<base::span<const uint8_t, kCableIdentityKeySeedSize>>
+        identity_seed,
+    // peer_identity, which must be non-nullopt iff |identity| is nullopt,
+    // contains the peer's public key as taken from the pairing data.
+    base::Optional<base::span<const uint8_t, kP256X962Length>> peer_identity,
     // in contains the initial handshake message from the peer.
     base::span<const uint8_t> in,
     // out_response is set to the response handshake message, if successful.

device/fido/cable/v2_handshake_fuzzer.cc

+// Copyright 2020 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <stddef.h>
+#include <stdint.h>
+
+#include <array>
+
+#include "base/containers/span.h"
+#include "device/fido/cable/v2_handshake.h"
+#include "third_party/boringssl/src/include/openssl/ec.h"
+#include "third_party/boringssl/src/include/openssl/ec_key.h"
+#include "third_party/boringssl/src/include/openssl/obj.h"
+
+namespace device {
+
+namespace {
+
+constexpr std::array<uint8_t, 32> kTestPSKGeneratorKey = {
+    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+    0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff,
+};
+constexpr std::array<uint8_t, cablev2::kNonceSize> kTestNonce = {
+    1, 2, 3, 4, 5, 6, 7, 8, 9, 10};
+constexpr std::array<uint8_t, 16> kTestEphemeralID = {
+    1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16};
+constexpr std::array<uint8_t, 65> kTestPeerIdentity = {
+    0x04, 0x67, 0x80, 0xc5, 0xfc, 0x70, 0x27, 0x5e, 0x2c, 0x70, 0x61,
+    0xa0, 0xe7, 0x87, 0x7b, 0xb1, 0x74, 0xde, 0xad, 0xeb, 0x98, 0x87,
+    0x02, 0x7f, 0x3f, 0xa8, 0x36, 0x54, 0x15, 0x8b, 0xa7, 0xf5, 0x0c,
+    0x3c, 0xba, 0x8c, 0x34, 0xbc, 0x35, 0xd2, 0x0e, 0x81, 0xf7, 0x30,
+    0xac, 0x1c, 0x7b, 0xd6, 0xd6, 0x61, 0xa9, 0x42, 0xf9, 0x0c, 0x6a,
+    0x9c, 0xa5, 0x5c, 0x51, 0x2f, 0x9e, 0x4a, 0x00, 0x12, 0x66,
+};
+constexpr std::array<uint8_t, 32> kTestLocalSeed = {
+    0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05,
+    0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05,
+    0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05, 0x05,
+};
+constexpr std::array<uint8_t, 5> kTestGetInfoBytes = {1, 2, 3, 4, 5};
+
+}  // namespace
+
+extern "C" int LLVMFuzzerTestOneInput(const uint8_t* raw_data, size_t size) {
+  auto input = base::make_span(raw_data, size);
+  if (input.empty()) {
+    return 0;
+  }
+  const bool initiate = input[0] & 1;
+  const bool have_local_key = input[0] & 2;
+  input = input.subspan(1);
+
+  base::Optional<base::span<const uint8_t, 65>> peer_identity;
+  base::Optional<base::span<const uint8_t, 32>> local_seed;
+  bssl::UniquePtr<EC_KEY> local_key;
+  if (have_local_key) {
+    local_seed = kTestLocalSeed;
+    bssl::UniquePtr<EC_GROUP> p256(
+        EC_GROUP_new_by_curve_name(NID_X9_62_prime256v1));
+    local_key.reset(EC_KEY_derive_from_secret(p256.get(), local_seed->data(),
+                                              local_seed->size()));
+  } else {
+    peer_identity = kTestPeerIdentity;
+  }
+
+  if (initiate) {
+    cablev2::HandshakeInitiator handshaker(kTestPSKGeneratorKey, kTestNonce,
+                                           peer_identity, std::move(local_key));
+    handshaker.BuildInitialMessage(kTestEphemeralID, kTestGetInfoBytes);
+    handshaker.ProcessResponse(input);
+  } else {
+    cablev2::NonceAndEID nonce_and_eid;
+    nonce_and_eid.first = kTestNonce;
+    nonce_and_eid.second = kTestEphemeralID;
+    std::vector<uint8_t> response;
+    cablev2::RespondToHandshake(kTestPSKGeneratorKey, nonce_and_eid, local_seed,
+                                peer_identity, input, &response);
+  }
+
+  return 0;
+}
+
+}  // namespace device