Move U2F encoding logic to U2fRequest interface

Move U2fApduCommand::Create{Version, Sign, Register}() to U2fRequest
interface. This is a preliminary work for migrating APDU to
src/components.

Bug: 807836
Change-Id: I2db9f503d9925c891f789fa9abb06f26a9fe98f8
Reviewed-on: https://chromium-review.googlesource.com/923417
Commit-Queue: Jun Choi <hongjunchoi@chromium.org>
Reviewed-by: Jan Wilken Dörrie <jdoerrie@chromium.org>
Cr-Commit-Position: refs/heads/master@{#542889}

device/fido/ctap_constants.cc

@@ -6,6 +6,16 @@
 
 namespace device {
 
+const std::array<uint8_t, 32> kBogusAppParam = {
+    0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
+    0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41,
+    0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41, 0x41};
+
+const std::array<uint8_t, 32> kBogusChallenge = {
+    0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42,
+    0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42,
+    0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42, 0x42};
+
 const char kResidentKeyMapKey[] = "rk";
 const char kUserVerificationMapKey[] = "uv";
 const char kUserPresenceMapKey[] = "up";
@@ -22,4 +32,15 @@ const size_t kHidContinuationPacketDataSize =
 const uint8_t kHidMaxLockSeconds = 10;
 const size_t kHidMaxMessageSize = 7609;
 
+const size_t kU2fMaxResponseSize = 65536;
+const uint8_t kP1TupRequired = 0x01;
+const uint8_t kP1TupConsumed = 0x02;
+const uint8_t kP1TupRequiredConsumed = kP1TupRequired | kP1TupConsumed;
+const uint8_t kP1CheckOnly = 0x07;
+const uint8_t kP1IndividualAttestation = 0x80;
+const size_t kMaxKeyHandleLength = 255;
+const size_t kU2fParameterLength = 32;
+
+const std::array<uint8_t, 2> kLegacyVersionSuffix = {0x00, 0x00};
+
 }  // namespace device

device/fido/ctap_constants.h

@@ -162,7 +162,23 @@ enum class CtapRequestCommand : uint8_t {
   kAuthenticatorReset = 0x07,
 };
 
-enum class kCoseAlgorithmIdentifier : int { kCoseEs256 = -7 };
+enum class CoseAlgorithmIdentifier : int { kCoseEs256 = -7 };
+
+// APDU instruction code for U2F request encoding.
+// https://fidoalliance.org/specs/fido-u2f-v1.0-ps-20141009/fido-u2f-u2f.h-v1.0-ps-20141009.pdf
+enum class U2fApduInstruction : uint8_t {
+  kRegister = 0x01,
+  kSign = 0x02,
+  kVersion = 0x03,
+  kVendorFirst = 0x40,
+  kVenderLast = 0xBF,
+};
+
+// Parameters for fake U2F registration used to check for user presence.
+COMPONENT_EXPORT(DEVICE_FIDO)
+extern const std::array<uint8_t, 32> kBogusAppParam;
+COMPONENT_EXPORT(DEVICE_FIDO)
+extern const std::array<uint8_t, 32> kBogusChallenge;
 
 // String key values for CTAP request optional parameters and
 // AuthenticatorGetInfo response.
@@ -179,10 +195,36 @@ COMPONENT_EXPORT(DEVICE_FIDO) extern const size_t kHidMaxPacketSize;
 COMPONENT_EXPORT(DEVICE_FIDO) extern const size_t kHidInitPacketDataSize;
 COMPONENT_EXPORT(DEVICE_FIDO)
 extern const size_t kHidContinuationPacketDataSize;
+
 COMPONENT_EXPORT(DEVICE_FIDO) extern const uint8_t kHidMaxLockSeconds;
+
 // Messages are limited to an initiation packet and 128 continuation packets.
 COMPONENT_EXPORT(DEVICE_FIDO) extern const size_t kHidMaxMessageSize;
 
+// U2F APDU encoding constants, as specified in
+// https://fidoalliance.org/specs/fido-u2f-v1.2-ps-20170411/fido-u2f-raw-message-formats-v1.2-ps-20170411.html#bib-U2FHeader
+COMPONENT_EXPORT(DEVICE_FIDO) extern const size_t kU2fMaxResponseSize;
+
+// P1 instructions.
+COMPONENT_EXPORT(DEVICE_FIDO) extern const uint8_t kP1TupRequired;
+COMPONENT_EXPORT(DEVICE_FIDO) extern const uint8_t kP1TupConsumed;
+COMPONENT_EXPORT(DEVICE_FIDO) extern const uint8_t kP1TupRequiredConsumed;
+
+// Control byte used for check-only setting. The check-only command is used to
+// determine if the provided key handle was originally created by this token
+// and whether it was created for the provided application parameter.
+COMPONENT_EXPORT(DEVICE_FIDO) extern const uint8_t kP1CheckOnly;
+
+// Indicates that an individual attestation certificate is acceptable to
+// return with this registration.
+COMPONENT_EXPORT(DEVICE_FIDO) extern const uint8_t kP1IndividualAttestation;
+COMPONENT_EXPORT(DEVICE_FIDO) extern const size_t kMaxKeyHandleLength;
+COMPONENT_EXPORT(DEVICE_FIDO) extern const size_t kU2fParameterLength;
+
+// Suffix added to APDU encoded command for legacy version request.
+COMPONENT_EXPORT(DEVICE_FIDO)
+extern const std::array<uint8_t, 2> kLegacyVersionSuffix;
+
 }  // namespace device
 
 #endif  // DEVICE_FIDO_CTAP_CONSTANTS_H_

device/fido/public_key_credential_params.h

@@ -24,8 +24,7 @@ class COMPONENT_EXPORT(DEVICE_FIDO) PublicKeyCredentialParams {
  public:
   struct CredentialInfo {
     std::string type;
-    int algorithm =
-        base::strict_cast<int>(kCoseAlgorithmIdentifier::kCoseEs256);
+    int algorithm = base::strict_cast<int>(CoseAlgorithmIdentifier::kCoseEs256);
   };
 
   explicit PublicKeyCredentialParams(

device/fido/u2f_apdu_command.cc

 // Copyright 2017 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 "device/fido/u2f_apdu_command.h"
 
-#include "base/memory/ptr_util.h"
+#include <utility>
 
-#include "device/fido/u2f_apdu_command.h"
+#include "base/memory/ptr_util.h"
 
 namespace device {
 
-std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateFromMessage(
+// static
+std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateFromMessageForTesting(
     const std::vector<uint8_t>& message) {
   uint16_t data_length = 0;
   size_t index = 0;
   size_t response_length = 0;
   std::vector<uint8_t> data;
-  std::vector<uint8_t> suffix;
 
   if (message.size() < kApduMinHeader || message.size() > kApduMaxLength)
     return nullptr;
@@ -63,10 +64,6 @@ std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateFromMessage(
         // Defined in ISO7816-4
         if (response_length == 0)
           response_length = kApduMaxResponseLength;
-        // Non-ISO7816-4 special legacy case where 2 suffix bytes are passed
-        // along with a version message
-        if (data_length == 0 && ins == kInsU2fVersion)
-          suffix = {0x0, 0x0};
       } else {
         return nullptr;
       }
@@ -74,9 +71,27 @@ std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateFromMessage(
   }
 
   return std::make_unique<U2fApduCommand>(cla, ins, p1, p2, response_length,
-                                          std::move(data), std::move(suffix));
+                                          std::move(data));
 }
 
+U2fApduCommand::U2fApduCommand()
+    : cla_(0), ins_(0), p1_(0), p2_(0), response_length_(0) {}
+
+U2fApduCommand::U2fApduCommand(uint8_t cla,
+                               uint8_t ins,
+                               uint8_t p1,
+                               uint8_t p2,
+                               size_t response_length,
+                               std::vector<uint8_t> data)
+    : cla_(cla),
+      ins_(ins),
+      p1_(p1),
+      p2_(p2),
+      response_length_(response_length),
+      data_(std::move(data)) {}
+
+U2fApduCommand::~U2fApduCommand() = default;
+
 std::vector<uint8_t> U2fApduCommand::GetEncodedCommand() const {
   std::vector<uint8_t> encoded = {cla_, ins_, p1_, p2_};
 
@@ -101,92 +116,7 @@ std::vector<uint8_t> U2fApduCommand::GetEncodedCommand() const {
     encoded.push_back((response_length_ >> 8) & 0xff);
     encoded.push_back(response_length_ & 0xff);
   }
-  // Add suffix, if required, for legacy compatibility
-  encoded.insert(encoded.end(), suffix_.begin(), suffix_.end());
   return encoded;
 }
 
-U2fApduCommand::U2fApduCommand()
-    : cla_(0), ins_(0), p1_(0), p2_(0), response_length_(0) {}
-
-U2fApduCommand::U2fApduCommand(uint8_t cla,
-                               uint8_t ins,
-                               uint8_t p1,
-                               uint8_t p2,
-                               size_t response_length,
-                               std::vector<uint8_t> data,
-                               std::vector<uint8_t> suffix)
-    : cla_(cla),
-      ins_(ins),
-      p1_(p1),
-      p2_(p2),
-      response_length_(response_length),
-      data_(std::move(data)),
-      suffix_(std::move(suffix)) {}
-
-U2fApduCommand::~U2fApduCommand() = default;
-
-// static
-std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateRegister(
-    const std::vector<uint8_t>& appid_digest,
-    const std::vector<uint8_t>& challenge_digest,
-    bool individual_attestation_ok) {
-  if (appid_digest.size() != kAppIdDigestLen ||
-      challenge_digest.size() != kChallengeDigestLen) {
-    return nullptr;
-  }
-
-  auto command = std::make_unique<U2fApduCommand>();
-  std::vector<uint8_t> data(challenge_digest.begin(), challenge_digest.end());
-  data.insert(data.end(), appid_digest.begin(), appid_digest.end());
-  command->set_ins(kInsU2fEnroll);
-  command->set_p1(kP1TupRequiredConsumed |
-                  (individual_attestation_ok ? kP1IndividualAttestation : 0));
-  command->set_data(data);
-  return command;
-}
-
-// static
-std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateVersion() {
-  auto command = std::make_unique<U2fApduCommand>();
-  command->set_ins(kInsU2fVersion);
-  command->set_response_length(kApduMaxResponseLength);
-  return command;
-}
-
-// static
-std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateLegacyVersion() {
-  auto command = std::make_unique<U2fApduCommand>();
-  command->set_ins(kInsU2fVersion);
-  command->set_response_length(kApduMaxResponseLength);
-  // Early U2F drafts defined the U2F version command a format
-  // incompatible with ISO 7816-4, so 2 additional 0x0 bytes are necessary.
-  // https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-raw-message-formats-v1.1-id-20160915.html#implementation-considerations
-  command->set_suffix(std::vector<uint8_t>(2, 0));
-  return command;
-}
-
-// static
-std::unique_ptr<U2fApduCommand> U2fApduCommand::CreateSign(
-    const std::vector<uint8_t>& appid_digest,
-    const std::vector<uint8_t>& challenge_digest,
-    const std::vector<uint8_t>& key_handle,
-    bool check_only) {
-  if (appid_digest.size() != kAppIdDigestLen ||
-      challenge_digest.size() != kChallengeDigestLen ||
-      key_handle.size() > kMaxKeyHandleLength) {
-    return nullptr;
-  }
-
-  auto command = std::make_unique<U2fApduCommand>();
-  std::vector<uint8_t> data(challenge_digest.begin(), challenge_digest.end());
-  data.insert(data.end(), appid_digest.begin(), appid_digest.end());
-  data.push_back(static_cast<uint8_t>(key_handle.size()));
-  data.insert(data.end(), key_handle.begin(), key_handle.end());
-  command->set_ins(kInsU2fSign);
-  command->set_p1(check_only ? kP1CheckOnly : kP1TupRequiredConsumed);
-  command->set_data(data);
-  return command;
-}
-
 }  // namespace device

device/fido/u2f_apdu_command.h

@@ -7,6 +7,7 @@
 
 #include <cinttypes>
 #include <memory>
+#include <utility>
 #include <vector>
 
 #include "base/component_export.h"
@@ -14,8 +15,6 @@
 
 namespace device {
 
-class VirtualU2fDevice;
-
 // APDU commands are defined as part of ISO 7816-4. Commands can be serialized
 // into either short length encodings, where the maximum data length is 256
 // bytes, or an extended length encoding, where the maximum data length is 65536
@@ -26,57 +25,42 @@ class VirtualU2fDevice;
 // a maximum expected response length (Le).
 class COMPONENT_EXPORT(DEVICE_FIDO) U2fApduCommand {
  public:
+  // Constructs an APDU command from the serialized message data.
+  static std::unique_ptr<U2fApduCommand> CreateFromMessageForTesting(
+      const std::vector<uint8_t>& data);
+
   U2fApduCommand();
   U2fApduCommand(uint8_t cla,
                  uint8_t ins,
                  uint8_t p1,
                  uint8_t p2,
                  size_t response_length,
-                 std::vector<uint8_t> data,
-                 std::vector<uint8_t> suffix);
+                 std::vector<uint8_t> data);
   ~U2fApduCommand();
 
-  // Constructs an APDU command from the serialized message data.
-  static std::unique_ptr<U2fApduCommand> CreateFromMessage(
-      const std::vector<uint8_t>& data);
   // Returns serialized message data.
   std::vector<uint8_t> GetEncodedCommand() const;
+
   void set_cla(uint8_t cla) { cla_ = cla; }
   void set_ins(uint8_t ins) { ins_ = ins; }
   void set_p1(uint8_t p1) { p1_ = p1; }
   void set_p2(uint8_t p2) { p2_ = p2; }
-  void set_data(const std::vector<uint8_t>& data) { data_ = data; }
+  void set_data(std::vector<uint8_t> data) { data_ = std::move(data); }
   void set_response_length(size_t response_length) {
     response_length_ = response_length;
   }
-  void set_suffix(const std::vector<uint8_t>& suffix) { suffix_ = suffix; }
-  static std::unique_ptr<U2fApduCommand> CreateRegister(
-      const std::vector<uint8_t>& appid_digest,
-      const std::vector<uint8_t>& challenge_digest,
-      bool individual_attestation_ok);
-  static std::unique_ptr<U2fApduCommand> CreateVersion();
-  // Early U2F drafts defined a non-ISO 7816-4 conforming layout.
-  static std::unique_ptr<U2fApduCommand> CreateLegacyVersion();
 
-  // Returns an APDU command for sign(). If optional parameter |check_only| is
-  // set to true, then control byte is set to 0X07.
-  static std::unique_ptr<U2fApduCommand> CreateSign(
-      const std::vector<uint8_t>& appid_digest,
-      const std::vector<uint8_t>& challenge_digest,
-      const std::vector<uint8_t>& key_handle,
-      bool check_only = false);
+  uint8_t cla() const { return cla_; }
+  uint8_t ins() const { return ins_; }
+  uint8_t p1() const { return p1_; }
+  uint8_t p2() const { return p2_; }
+  size_t response_length() const { return response_length_; }
+  const std::vector<uint8_t>& data() const { return data_; }
 
  private:
   FRIEND_TEST_ALL_PREFIXES(U2fApduTest, TestDeserializeBasic);
   FRIEND_TEST_ALL_PREFIXES(U2fApduTest, TestDeserializeComplex);
   FRIEND_TEST_ALL_PREFIXES(U2fApduTest, TestSerializeEdgeCases);
-  FRIEND_TEST_ALL_PREFIXES(U2fApduTest, TestCreateSign);
-  FRIEND_TEST_ALL_PREFIXES(U2fApduTest, TestCreateRegister);
-  FRIEND_TEST_ALL_PREFIXES(U2fApduTest, TestCreateVersion);
-  FRIEND_TEST_ALL_PREFIXES(U2fApduTest, TestCreateLegacyVersion);
-
-  // Built-in software key for testing.
-  friend class VirtualU2fDevice;
 
   static constexpr size_t kApduMinHeader = 4;
   static constexpr size_t kApduMaxHeader = 7;
@@ -89,34 +73,12 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fApduCommand {
   static constexpr size_t kApduMaxLength =
       kApduMaxDataLength + kApduMaxHeader + 2;
 
-  // APDU instructions.
-  static constexpr uint8_t kInsU2fEnroll = 0x01;
-  static constexpr uint8_t kInsU2fSign = 0x02;
-  static constexpr uint8_t kInsU2fVersion = 0x03;
-
-  // P1 instructions.
-  static constexpr uint8_t kP1TupRequired = 0x01;
-  static constexpr uint8_t kP1TupConsumed = 0x02;
-  static constexpr uint8_t kP1TupRequiredConsumed =
-      kP1TupRequired | kP1TupConsumed;
-  // Control byte used for check-only setting. The check-only command is used to
-  // determine if the provided key handle was originally created by this token
-  // and whether it was created for the provided application parameter.
-  static constexpr uint8_t kP1CheckOnly = 0x07;
-  // Indicates that an individual attestation certificate is acceptable to
-  // return with this registration.
-  static constexpr uint8_t kP1IndividualAttestation = 0x80;
-  static constexpr size_t kMaxKeyHandleLength = 255;
-  static constexpr size_t kChallengeDigestLen = 32;
-  static constexpr size_t kAppIdDigestLen = 32;
-
   uint8_t cla_;
   uint8_t ins_;
   uint8_t p1_;
   uint8_t p2_;
   size_t response_length_;
   std::vector<uint8_t> data_;
-  std::vector<uint8_t> suffix_;
 };
 
 }  // namespace device

device/fido/u2f_apdu_fuzzer.cc

@@ -12,7 +12,7 @@
 extern "C" int LLVMFuzzerTestOneInput(const uint8_t* data, size_t size) {
   std::vector<uint8_t> input(data, data + size);
   std::unique_ptr<device::U2fApduCommand> cmd =
-      device::U2fApduCommand::CreateFromMessage(input);
+      device::U2fApduCommand::CreateFromMessageForTesting(input);
   std::unique_ptr<device::U2fApduResponse> rsp =
       device::U2fApduResponse::CreateFromMessage(input);
   return 0;

device/fido/u2f_apdu_unittest.cc

@@ -19,7 +19,7 @@ TEST_F(U2fApduTest, TestDeserializeBasic) {
   uint8_t p2 = 0xAD;
   std::vector<uint8_t> message = {cla, ins, p1, p2};
   std::unique_ptr<U2fApduCommand> cmd =
-      U2fApduCommand::CreateFromMessage(message);
+      U2fApduCommand::CreateFromMessageForTesting(message);
 
   EXPECT_EQ(static_cast<size_t>(0), cmd->response_length_);
   EXPECT_THAT(cmd->data_, ::testing::ContainerEq(std::vector<uint8_t>()));
@@ -30,7 +30,7 @@ TEST_F(U2fApduTest, TestDeserializeBasic) {
 
   // Invalid length
   message = {cla, ins, p1};
-  EXPECT_EQ(nullptr, U2fApduCommand::CreateFromMessage(message));
+  EXPECT_EQ(nullptr, U2fApduCommand::CreateFromMessageForTesting(message));
   message.push_back(p2);
   message.push_back(0);
   message.push_back(0xFF);
@@ -39,9 +39,9 @@ TEST_F(U2fApduTest, TestDeserializeBasic) {
   message.insert(message.end(), oversized.begin(), oversized.end());
   message.push_back(0);
   message.push_back(0);
-  EXPECT_NE(nullptr, U2fApduCommand::CreateFromMessage(message));
+  EXPECT_NE(nullptr, U2fApduCommand::CreateFromMessageForTesting(message));
   message.push_back(0);
-  EXPECT_EQ(nullptr, U2fApduCommand::CreateFromMessage(message));
+  EXPECT_EQ(nullptr, U2fApduCommand::CreateFromMessageForTesting(message));
 }
 
 TEST_F(U2fApduTest, TestDeserializeComplex) {
@@ -59,7 +59,7 @@ TEST_F(U2fApduTest, TestDeserializeComplex) {
 
   // Create a message with no response expected
   std::unique_ptr<U2fApduCommand> cmd_no_response =
-      U2fApduCommand::CreateFromMessage(message);
+      U2fApduCommand::CreateFromMessageForTesting(message);
   EXPECT_EQ(static_cast<size_t>(0), cmd_no_response->response_length_);
   EXPECT_THAT(data, ::testing::ContainerEq(cmd_no_response->data_));
   EXPECT_EQ(cmd_no_response->cla_, cla);
@@ -71,7 +71,7 @@ TEST_F(U2fApduTest, TestDeserializeComplex) {
   message.push_back(0xF1);
   message.push_back(0xD0);
   std::unique_ptr<U2fApduCommand> cmd =
-      U2fApduCommand::CreateFromMessage(message);
+      U2fApduCommand::CreateFromMessageForTesting(message);
   EXPECT_THAT(data, ::testing::ContainerEq(cmd->data_));
   EXPECT_EQ(cmd->cla_, cla);
   EXPECT_EQ(cmd->ins_, ins);
@@ -122,19 +122,19 @@ TEST_F(U2fApduTest, TestSerializeCommand) {
   // No data, no response expected
   std::vector<uint8_t> expected({0xA, 0xB, 0xC, 0xD});
   ASSERT_THAT(expected, ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  EXPECT_THAT(
-      expected,
-      ::testing::ContainerEq(
-          U2fApduCommand::CreateFromMessage(expected)->GetEncodedCommand()));
+  EXPECT_THAT(expected,
+              ::testing::ContainerEq(
+                  U2fApduCommand::CreateFromMessageForTesting(expected)
+                      ->GetEncodedCommand()));
 
   // No data, response expected
   cmd->set_response_length(0xCAFE);
   expected = {0xA, 0xB, 0xC, 0xD, 0x0, 0xCA, 0xFE};
   EXPECT_THAT(expected, ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  EXPECT_THAT(
-      expected,
-      ::testing::ContainerEq(
-          U2fApduCommand::CreateFromMessage(expected)->GetEncodedCommand()));
+  EXPECT_THAT(expected,
+              ::testing::ContainerEq(
+                  U2fApduCommand::CreateFromMessageForTesting(expected)
+                      ->GetEncodedCommand()));
 
   // Data exists, response expected
   std::vector<uint8_t> data({0x1, 0x2, 0x3, 0x4});
@@ -142,19 +142,19 @@ TEST_F(U2fApduTest, TestSerializeCommand) {
   expected = {0xA, 0xB, 0xC, 0xD, 0x0,  0x0, 0x4,
               0x1, 0x2, 0x3, 0x4, 0xCA, 0xFE};
   EXPECT_THAT(expected, ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  EXPECT_THAT(
-      expected,
-      ::testing::ContainerEq(
-          U2fApduCommand::CreateFromMessage(expected)->GetEncodedCommand()));
+  EXPECT_THAT(expected,
+              ::testing::ContainerEq(
+                  U2fApduCommand::CreateFromMessageForTesting(expected)
+                      ->GetEncodedCommand()));
 
   // Data exists, no response expected
   cmd->set_response_length(0);
   expected = {0xA, 0xB, 0xC, 0xD, 0x0, 0x0, 0x4, 0x1, 0x2, 0x3, 0x4};
   EXPECT_THAT(expected, ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  EXPECT_THAT(
-      expected,
-      ::testing::ContainerEq(
-          U2fApduCommand::CreateFromMessage(expected)->GetEncodedCommand()));
+  EXPECT_THAT(expected,
+              ::testing::ContainerEq(
+                  U2fApduCommand::CreateFromMessageForTesting(expected)
+                      ->GetEncodedCommand()));
 }
 
 TEST_F(U2fApduTest, TestSerializeEdgeCases) {
@@ -169,105 +169,19 @@ TEST_F(U2fApduTest, TestSerializeEdgeCases) {
   cmd->set_response_length(U2fApduCommand::kApduMaxResponseLength);
   std::vector<uint8_t> expected = {0xA, 0xB, 0xC, 0xD, 0x0, 0x0, 0x0};
   EXPECT_THAT(expected, ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  EXPECT_THAT(
-      expected,
-      ::testing::ContainerEq(
-          U2fApduCommand::CreateFromMessage(expected)->GetEncodedCommand()));
+  EXPECT_THAT(expected,
+              ::testing::ContainerEq(
+                  U2fApduCommand::CreateFromMessageForTesting(expected)
+                      ->GetEncodedCommand()));
 
   // Maximum data size
   std::vector<uint8_t> oversized(U2fApduCommand::kApduMaxDataLength);
   cmd->set_data(oversized);
-  EXPECT_THAT(cmd->GetEncodedCommand(),
-              ::testing::ContainerEq(
-                  U2fApduCommand::CreateFromMessage(cmd->GetEncodedCommand())
-                      ->GetEncodedCommand()));
-}
-
-TEST_F(U2fApduTest, TestCreateSign) {
-  std::vector<uint8_t> appid(U2fApduCommand::kAppIdDigestLen, 0x01);
-  std::vector<uint8_t> challenge(U2fApduCommand::kChallengeDigestLen, 0xff);
-  std::vector<uint8_t> key_handle(U2fApduCommand::kMaxKeyHandleLength);
-
-  std::unique_ptr<U2fApduCommand> cmd =
-      U2fApduCommand::CreateSign(appid, challenge, key_handle);
-  ASSERT_NE(nullptr, cmd);
-  EXPECT_THAT(U2fApduCommand::CreateFromMessage(cmd->GetEncodedCommand())
-                  ->GetEncodedCommand(),
-              ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  // Expect null result with incorrectly sized key handle
-  key_handle.push_back(0x0f);
-  cmd = U2fApduCommand::CreateSign(appid, challenge, key_handle);
-  EXPECT_EQ(nullptr, cmd);
-  key_handle.pop_back();
-  // Expect null result with incorrectly sized appid
-  appid.pop_back();
-  cmd = U2fApduCommand::CreateSign(appid, challenge, key_handle);
-  EXPECT_EQ(nullptr, cmd);
-  appid.push_back(0xff);
-  // Expect null result with incorrectly sized challenge
-  challenge.push_back(0x0);
-  cmd = U2fApduCommand::CreateSign(appid, challenge, key_handle);
-  EXPECT_EQ(nullptr, cmd);
-}
-
-TEST_F(U2fApduTest, TestCreateRegister) {
-  constexpr bool kNoIndividualAttestation = false;
-  constexpr bool kIndividualAttestation = true;
-  std::vector<uint8_t> appid(U2fApduCommand::kAppIdDigestLen, 0x01);
-  std::vector<uint8_t> challenge(U2fApduCommand::kChallengeDigestLen, 0xff);
-  std::unique_ptr<U2fApduCommand> cmd = U2fApduCommand::CreateRegister(
-      appid, challenge, kNoIndividualAttestation);
-  ASSERT_NE(nullptr, cmd);
-  std::vector<uint8_t> encoded = cmd->GetEncodedCommand();
-  ASSERT_LE(2u, encoded.size());
-  // Individual attestation bit should be cleared.
-  EXPECT_EQ(0, encoded[2] & 0x80);
-  EXPECT_THAT(U2fApduCommand::CreateFromMessage(cmd->GetEncodedCommand())
-                  ->GetEncodedCommand(),
-              ::testing::ContainerEq(encoded));
-
-  cmd =
-      U2fApduCommand::CreateRegister(appid, challenge, kIndividualAttestation);
-  ASSERT_NE(nullptr, cmd);
-  encoded = cmd->GetEncodedCommand();
-  ASSERT_LE(2u, encoded.size());
-  // Individual attestation bit should be set.
-  EXPECT_EQ(0x80, encoded[2] & 0x80);
-
-  // Expect null result with incorrectly sized appid
-  appid.push_back(0xff);
-  cmd = U2fApduCommand::CreateRegister(appid, challenge,
-                                       kNoIndividualAttestation);
-  EXPECT_EQ(nullptr, cmd);
-  appid.pop_back();
-  // Expect null result with incorrectly sized challenge
-  challenge.push_back(0xff);
-  cmd = U2fApduCommand::CreateRegister(appid, challenge,
-                                       kNoIndividualAttestation);
-  EXPECT_EQ(nullptr, cmd);
-}
-
-TEST_F(U2fApduTest, TestCreateVersion) {
-  std::unique_ptr<U2fApduCommand> cmd = U2fApduCommand::CreateVersion();
-  std::vector<uint8_t> expected = {
-      0x0, U2fApduCommand::kInsU2fVersion, 0x0, 0x0, 0x0, 0x0, 0x0};
-
-  EXPECT_THAT(expected, ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  EXPECT_THAT(U2fApduCommand::CreateFromMessage(cmd->GetEncodedCommand())
-                  ->GetEncodedCommand(),
-              ::testing::ContainerEq(cmd->GetEncodedCommand()));
+  EXPECT_THAT(
+      cmd->GetEncodedCommand(),
+      ::testing::ContainerEq(
+          U2fApduCommand::CreateFromMessageForTesting(cmd->GetEncodedCommand())
+              ->GetEncodedCommand()));
 }
 
-TEST_F(U2fApduTest, TestCreateLegacyVersion) {
-  std::unique_ptr<U2fApduCommand> cmd = U2fApduCommand::CreateLegacyVersion();
-  // Legacy version command contains 2 extra null bytes compared to ISO 7816-4
-  // format
-  std::vector<uint8_t> expected = {
-      0x0, U2fApduCommand::kInsU2fVersion, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0};
-
-  EXPECT_THAT(expected, ::testing::ContainerEq(cmd->GetEncodedCommand()));
-  EXPECT_THAT(U2fApduCommand::CreateFromMessage(cmd->GetEncodedCommand())
-                  ->GetEncodedCommand(),
-              ::testing::ContainerEq(cmd->GetEncodedCommand()));
-}
 }  // namespace device

device/fido/u2f_device.cc

@@ -9,6 +9,7 @@
 #include "base/bind.h"
 #include "device/fido/u2f_apdu_command.h"
 #include "device/fido/u2f_apdu_response.h"
+#include "device/fido/u2f_request.h"
 
 namespace device {
 
@@ -18,46 +19,32 @@ U2fDevice::U2fDevice() = default;
 
 U2fDevice::~U2fDevice() = default;
 
-void U2fDevice::Register(const std::vector<uint8_t>& application_parameter,
-                         const std::vector<uint8_t>& challenge_param,
-                         bool individual_attestation_ok,
+void U2fDevice::Register(base::Optional<std::vector<uint8_t>> register_cmd,
                          MessageCallback callback) {
-  auto register_cmd = U2fApduCommand::CreateRegister(
-      application_parameter, challenge_param, individual_attestation_ok);
   if (!register_cmd) {
     std::move(callback).Run(U2fReturnCode::INVALID_PARAMS,
                             std::vector<uint8_t>());
     return;
   }
-  DeviceTransact(register_cmd->GetEncodedCommand(),
+  DeviceTransact(std::move(*register_cmd),
                  base::BindOnce(&U2fDevice::OnRegisterComplete, GetWeakPtr(),
                                 std::move(callback)));
 }
 
-void U2fDevice::Sign(const std::vector<uint8_t>& application_parameter,
-                     const std::vector<uint8_t>& challenge_param,
-                     const std::vector<uint8_t>& key_handle,
-                     MessageCallback callback,
-                     bool check_only) {
-  auto sign_cmd = U2fApduCommand::CreateSign(
-      application_parameter, challenge_param, key_handle, check_only);
+void U2fDevice::Sign(base::Optional<std::vector<uint8_t>> sign_cmd,
+                     MessageCallback callback) {
   if (!sign_cmd) {
     std::move(callback).Run(U2fReturnCode::INVALID_PARAMS,
                             std::vector<uint8_t>());
     return;
   }
-  DeviceTransact(sign_cmd->GetEncodedCommand(),
+  DeviceTransact(std::move(*sign_cmd),
                  base::BindOnce(&U2fDevice::OnSignComplete, GetWeakPtr(),
                                 std::move(callback)));
 }
 
 void U2fDevice::Version(VersionCallback callback) {
-  auto version_cmd = U2fApduCommand::CreateVersion();
-  if (!version_cmd) {
-    std::move(callback).Run(false, ProtocolVersion::UNKNOWN);
-    return;
-  }
-  DeviceTransact(version_cmd->GetEncodedCommand(),
+  DeviceTransact(U2fRequest::GetU2fVersionApduCommand(),
                  base::BindOnce(&U2fDevice::OnVersionComplete, GetWeakPtr(),
                                 std::move(callback), false /* legacy */));
 }
@@ -124,15 +111,10 @@ void U2fDevice::OnVersionComplete(
           std::vector<uint8_t>({'U', '2', 'F', '_', 'V', '2'})) {
     std::move(callback).Run(success, ProtocolVersion::U2F_V2);
   } else if (!legacy) {
-    // Standard GetVersion failed, attempt legacy GetVersion command
-    auto version_cmd = U2fApduCommand::CreateLegacyVersion();
-    if (!version_cmd) {
-      std::move(callback).Run(false, ProtocolVersion::UNKNOWN);
-    } else {
-      DeviceTransact(version_cmd->GetEncodedCommand(),
-                     base::BindOnce(&U2fDevice::OnVersionComplete, GetWeakPtr(),
-                                    std::move(callback), true /* legacy */));
-    }
+    // Standard GetVersion failed, attempt legacy GetVersion command.
+    DeviceTransact(U2fRequest::GetU2fVersionApduCommand(true /* legacy */),
+                   base::BindOnce(&U2fDevice::OnVersionComplete, GetWeakPtr(),
+                                  std::move(callback), true /* legacy */));
   } else {
     std::move(callback).Run(success, ProtocolVersion::UNKNOWN);
   }

device/fido/u2f_device.h

@@ -13,6 +13,7 @@
 #include "base/component_export.h"
 #include "base/macros.h"
 #include "base/memory/weak_ptr.h"
+#include "base/optional.h"
 #include "device/fido/u2f_apdu_response.h"
 #include "device/fido/u2f_return_code.h"
 
@@ -45,16 +46,11 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fDevice {
   // U2fDevice to U2fRequest.
   // Raw messages parameters are defined by the specification at
   // https://fidoalliance.org/specs/fido-u2f-v1.0-nfc-bt-amendment-20150514/fido-u2f-raw-message-formats.html
-  void Register(const std::vector<uint8_t>& appid_digest,
-                const std::vector<uint8_t>& challenge_digest,
-                bool individual_attestation_ok,
+  void Register(base::Optional<std::vector<uint8_t>> register_cmd,
                 MessageCallback callback);
+  void Sign(base::Optional<std::vector<uint8_t>> sign_cmd,
+            MessageCallback callback);
   void Version(VersionCallback callback);
-  void Sign(const std::vector<uint8_t>& appid_digest,
-            const std::vector<uint8_t>& challenge_digest,
-            const std::vector<uint8_t>& key_handle,
-            MessageCallback callback,
-            bool check_only = false);
 
   virtual void TryWink(WinkCallback callback) = 0;
   virtual std::string GetId() const = 0;

device/fido/u2f_hid_device_unittest.cc

@@ -17,6 +17,7 @@
 #include "device/fido/u2f_apdu_response.h"
 #include "device/fido/u2f_command_type.h"
 #include "device/fido/u2f_hid_device.h"
+#include "device/fido/u2f_request.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/interface_request.h"
 #include "services/device/public/cpp/hid/hid_device_filter.h"
@@ -171,7 +172,7 @@ TEST_F(U2fHidDeviceTest, TestMultipleRequests) {
   for (auto& device : receiver.TakeReturnedDevicesFiltered()) {
     TestVersionCallbackReceiver vc;
     TestVersionCallbackReceiver vc2;
-    // Call version twice to check message queueing
+    // Call version twice to check message queueing.
     device->Version(vc.callback());
     device->Version(vc2.callback());
     vc.WaitForCallback();
@@ -182,7 +183,7 @@ TEST_F(U2fHidDeviceTest, TestMultipleRequests) {
 }
 
 TEST_F(U2fHidDeviceTest, TestConnectionFailure) {
-  // Setup and enumerate mock device
+  // Setup and enumerate mock device.
   U2fDeviceEnumerateCallbackReceiver receiver(hid_manager_.get());
   auto hid_device = TestHidDevice();
   fake_hid_manager_->AddDevice(std::move(hid_device));
@@ -194,24 +195,21 @@ TEST_F(U2fHidDeviceTest, TestConnectionFailure) {
 
   ASSERT_EQ(static_cast<size_t>(1), u2f_devices.size());
   auto& device = u2f_devices.front();
-
-  // Put device in IDLE state
+  // Put device in IDLE state.
   device->state_ = U2fHidDevice::State::IDLE;
 
-  // Manually delete connection
+  // Manually delete connection.
   device->connection_ = nullptr;
 
   // Add pending transactions manually and ensure they are processed
   TestDeviceCallbackReceiver receiver_1;
-  device->pending_transactions_.emplace(
-      U2fApduCommand::CreateVersion()->GetEncodedCommand(),
-      receiver_1.callback());
+  device->pending_transactions_.emplace(U2fRequest::GetU2fVersionApduCommand(),
+                                        receiver_1.callback());
   TestDeviceCallbackReceiver receiver_2;
-  device->pending_transactions_.emplace(
-      U2fApduCommand::CreateVersion()->GetEncodedCommand(),
-      receiver_2.callback());
+  device->pending_transactions_.emplace(U2fRequest::GetU2fVersionApduCommand(),
+                                        receiver_2.callback());
   TestDeviceCallbackReceiver receiver_3;
-  device->DeviceTransact(U2fApduCommand::CreateVersion()->GetEncodedCommand(),
+  device->DeviceTransact(U2fRequest::GetU2fVersionApduCommand(),
                          receiver_3.callback());
   EXPECT_EQ(U2fHidDevice::State::DEVICE_ERROR, device->state_);
   EXPECT_EQ(nullptr, receiver_1.value());
@@ -220,8 +218,9 @@ TEST_F(U2fHidDeviceTest, TestConnectionFailure) {
 }
 
 TEST_F(U2fHidDeviceTest, TestDeviceError) {
-  // Setup and enumerate mock device
+  // Setup and enumerate mock device.
   U2fDeviceEnumerateCallbackReceiver receiver(hid_manager_.get());
+
   auto hid_device = TestHidDevice();
   fake_hid_manager_->AddDevice(std::move(hid_device));
   hid_manager_->GetDevices(receiver.callback());
@@ -233,28 +232,26 @@ TEST_F(U2fHidDeviceTest, TestDeviceError) {
   ASSERT_EQ(static_cast<size_t>(1), u2f_devices.size());
   auto& device = u2f_devices.front();
 
-  // Mock connection where writes always fail
+  // Mock connection where writes always fail.
   FakeHidConnection::mock_connection_error_ = true;
   device->state_ = U2fHidDevice::State::IDLE;
   TestDeviceCallbackReceiver receiver_0;
   std::unique_ptr<U2fApduResponse> response_0(
       U2fApduResponse::CreateFromMessage(std::vector<uint8_t>({0x0, 0x0})));
-  device->DeviceTransact(U2fApduCommand::CreateVersion()->GetEncodedCommand(),
+  device->DeviceTransact(U2fRequest::GetU2fVersionApduCommand(),
                          receiver_0.callback());
   EXPECT_EQ(nullptr, receiver_0.value());
   EXPECT_EQ(U2fHidDevice::State::DEVICE_ERROR, device->state_);
 
-  // Add pending transactions manually and ensure they are processed
+  // Add pending transactions manually and ensure they are processed.
   TestDeviceCallbackReceiver receiver_1;
-  device->pending_transactions_.emplace(
-      U2fApduCommand::CreateVersion()->GetEncodedCommand(),
-      receiver_1.callback());
+  device->pending_transactions_.emplace(U2fRequest::GetU2fVersionApduCommand(),
+                                        receiver_1.callback());
   TestDeviceCallbackReceiver receiver_2;
-  device->pending_transactions_.emplace(
-      U2fApduCommand::CreateVersion()->GetEncodedCommand(),
-      receiver_2.callback());
+  device->pending_transactions_.emplace(U2fRequest::GetU2fVersionApduCommand(),
+                                        receiver_2.callback());
   TestDeviceCallbackReceiver receiver_3;
-  device->DeviceTransact(U2fApduCommand::CreateVersion()->GetEncodedCommand(),
+  device->DeviceTransact(U2fRequest::GetU2fVersionApduCommand(),
                          receiver_3.callback());
   FakeHidConnection::mock_connection_error_ = false;
 

device/fido/u2f_register.cc

@@ -49,17 +49,17 @@ U2fRegister::~U2fRegister() = default;
 
 void U2fRegister::TryDevice() {
   DCHECK(current_device_);
-  if (registered_keys_.size() > 0 && !CheckedForDuplicateRegistration()) {
+  if (!registered_keys_.empty() && !CheckedForDuplicateRegistration()) {
     auto it = registered_keys_.cbegin();
-    current_device_->Sign(application_parameter_, challenge_digest_, *it,
+    current_device_->Sign(GetU2fSignApduCommand(application_parameter_, *it,
+                                                true /* check_only */),
                           base::BindOnce(&U2fRegister::OnTryCheckRegistration,
-                                         weak_factory_.GetWeakPtr(), it),
-                          true);
+                                         weak_factory_.GetWeakPtr(), it));
   } else {
     current_device_->Register(
-        application_parameter_, challenge_digest_, individual_attestation_ok_,
+        GetU2fRegisterApduCommand(individual_attestation_ok_),
         base::BindOnce(&U2fRegister::OnTryDevice, weak_factory_.GetWeakPtr(),
-                       false));
+                       false /* is_duplicate_registration */));
   }
 }
 
@@ -73,11 +73,9 @@ void U2fRegister::OnTryCheckRegistration(
       // Duplicate registration found. Call bogus registration to check for
       // user presence (touch) and terminate the registration process.
       current_device_->Register(
-          U2fRequest::GetBogusApplicationParameter(),
-          U2fRequest::GetBogusChallenge(),
-          false /* no individual attestation */,
+          U2fRequest::GetBogusRegisterCommand(),
           base::BindOnce(&U2fRegister::OnTryDevice, weak_factory_.GetWeakPtr(),
-                         true));
+                         true /* is_duplicate_registration */));
       break;
 
     case U2fReturnCode::INVALID_PARAMS:
@@ -85,10 +83,10 @@ void U2fRegister::OnTryCheckRegistration(
       // list and check for already registered keys.
       if (++it != registered_keys_.end()) {
         current_device_->Sign(
-            application_parameter_, challenge_digest_, *it,
+            GetU2fSignApduCommand(application_parameter_, *it,
+                                  true /* check_only */),
             base::BindOnce(&U2fRegister::OnTryCheckRegistration,
-                           weak_factory_.GetWeakPtr(), it),
-            true);
+                           weak_factory_.GetWeakPtr(), it));
       } else {
         checked_device_id_list_.insert(current_device_->GetId());
         if (devices_.empty()) {

device/fido/u2f_register_unittest.cc

@@ -13,6 +13,7 @@
 #include "device/fido/attestation_object.h"
 #include "device/fido/attested_credential_data.h"
 #include "device/fido/authenticator_data.h"
+#include "device/fido/ctap_constants.h"
 #include "device/fido/ec_public_key.h"
 #include "device/fido/fake_u2f_discovery.h"
 #include "device/fido/fido_attestation_statement.h"
@@ -324,7 +325,12 @@ class U2fRegisterTest : public ::testing::Test {
 
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
-
+  std::vector<uint8_t> application_parameter_{std::begin(kAppIdDigest),
+                                              std::end(kAppIdDigest)};
+  std::vector<uint8_t> challenge_parameter_{std::begin(kChallengeDigest),
+                                            std::end(kChallengeDigest)};
+  std::vector<std::vector<uint8_t>> key_handles_;
+  base::flat_set<U2fTransportProtocol> protocols_;
   test::ScopedFakeU2fDiscoveryFactory scoped_fake_discovery_factory_;
   test::FakeU2fDiscovery* discovery_;
   TestRegisterCallback register_callback_receiver_;
@@ -355,30 +361,69 @@ TEST_F(U2fRegisterTest, TestCreateU2fRegisterCommand) {
       0x04, 0x05, 0x06, 0x07, 0x08, 0x09, 0x00, 0x01,
   };
 
-  U2fRegister register_request(
-      nullptr /* connector */, {} /* transports */,
-      std::vector<std::vector<uint8_t>>() /* registered_keys */,
-      std::vector<uint8_t>(std::begin(kChallengeDigest),
-                           std::end(kChallengeDigest)),
-      std::vector<uint8_t>(std::begin(kAppIdDigest), std::end(kAppIdDigest)),
-      kNoIndividualAttestation, register_callback_receiver().callback());
+  U2fRegister register_request(nullptr /* connector */, protocols_,
+                               key_handles_, challenge_parameter_,
+                               application_parameter_, kNoIndividualAttestation,
+                               register_callback_receiver().callback());
 
   const auto register_command_without_individual_attestation =
       register_request.GetU2fRegisterApduCommand(kNoIndividualAttestation);
 
   ASSERT_TRUE(register_command_without_individual_attestation);
   EXPECT_THAT(
-      register_command_without_individual_attestation->GetEncodedCommand(),
+      *register_command_without_individual_attestation,
       ::testing::ElementsAreArray(kRegisterApduCommandWithoutAttestation));
 
   const auto register_command_with_individual_attestation =
       register_request.GetU2fRegisterApduCommand(kIndividualAttestation);
-
   ASSERT_TRUE(register_command_with_individual_attestation);
-  EXPECT_THAT(register_command_with_individual_attestation->GetEncodedCommand(),
+  EXPECT_THAT(*register_command_with_individual_attestation,
               ::testing::ElementsAreArray(kRegisterApduCommandWithAttestation));
 }
 
+TEST_F(U2fRegisterTest, TestCreateRegisterWithIncorrectParameters) {
+  std::vector<uint8_t> application_parameter(kU2fParameterLength, 0x01);
+  std::vector<uint8_t> challenge_parameter(kU2fParameterLength, 0xff);
+
+  U2fRegister register_request(nullptr, protocols_, key_handles_,
+                               challenge_parameter, application_parameter,
+                               kNoIndividualAttestation,
+                               register_callback_receiver().callback());
+  const auto register_without_individual_attestation =
+      register_request.GetU2fRegisterApduCommand(kNoIndividualAttestation);
+
+  ASSERT_TRUE(register_without_individual_attestation);
+  ASSERT_LE(3u, register_without_individual_attestation->size());
+  // Individual attestation bit should be cleared.
+  EXPECT_EQ(0, (*register_without_individual_attestation)[2] & 0x80);
+
+  const auto register_request_with_individual_attestation =
+      register_request.GetU2fRegisterApduCommand(kIndividualAttestation);
+  ASSERT_TRUE(register_request_with_individual_attestation);
+  ASSERT_LE(3u, register_request_with_individual_attestation->size());
+  // Individual attestation bit should be set.
+  EXPECT_EQ(0x80, (*register_request_with_individual_attestation)[2] & 0x80);
+
+  // Expect null result with incorrectly sized application_parameter.
+  application_parameter.push_back(0xff);
+  auto incorrect_register_cmd =
+      U2fRegister(nullptr, protocols_, key_handles_, challenge_parameter,
+                  application_parameter, kNoIndividualAttestation,
+                  register_callback_receiver().callback())
+          .GetU2fRegisterApduCommand(kNoIndividualAttestation);
+  EXPECT_FALSE(incorrect_register_cmd);
+  application_parameter.pop_back();
+
+  // Expect null result with incorrectly sized challenge.
+  challenge_parameter.push_back(0xff);
+  incorrect_register_cmd =
+      U2fRegister(nullptr, protocols_, key_handles_, challenge_parameter,
+                  application_parameter, kNoIndividualAttestation,
+                  register_callback_receiver().callback())
+          .GetU2fRegisterApduCommand(kNoIndividualAttestation);
+  EXPECT_FALSE(incorrect_register_cmd);
+}
+
 TEST_F(U2fRegisterTest, TestRegisterSuccess) {
   auto request = CreateRegisterRequest();
   request->Start();
@@ -763,9 +808,7 @@ TEST_F(U2fRegisterTest, TestIndividualAttestation) {
         nullptr /* connector */,
         base::flat_set<U2fTransportProtocol>(
             {U2fTransportProtocol::kUsbHumanInterfaceDevice}) /* transports */,
-        std::vector<std::vector<uint8_t>>() /* registration_keys */,
-        std::vector<uint8_t>(32) /* challenge_digest */,
-        std::vector<uint8_t>(32) /* application_parameter */,
+        key_handles_, challenge_parameter_, application_parameter_,
         individual_attestation, cb.callback());
     request->Start();
     discovery()->WaitForCallToStartAndSimulateSuccess();

device/fido/u2f_request.cc

@@ -48,35 +48,70 @@ void U2fRequest::Start() {
 }
 
 // static
-const std::vector<uint8_t>& U2fRequest::GetBogusApplicationParameter() {
-  static const std::vector<uint8_t> kBogusAppParam(32, 0x41);
-  return kBogusAppParam;
+std::vector<uint8_t> U2fRequest::GetBogusRegisterCommand() {
+  U2fApduCommand command;
+  std::vector<uint8_t> data(kBogusChallenge.cbegin(), kBogusChallenge.cend());
+  data.insert(data.end(), kBogusAppParam.cbegin(), kBogusAppParam.cend());
+  command.set_ins(base::strict_cast<uint8_t>(U2fApduInstruction::kRegister));
+  command.set_p1(kP1TupRequiredConsumed);
+  command.set_data(data);
+  return command.GetEncodedCommand();
 }
 
 // static
-const std::vector<uint8_t>& U2fRequest::GetBogusChallenge() {
-  static const std::vector<uint8_t> kBogusChallenge(32, 0x42);
-  return kBogusChallenge;
-}
-
-// static
-std::unique_ptr<U2fApduCommand> U2fRequest::GetU2fVersionApduCommand(
+std::vector<uint8_t> U2fRequest::GetU2fVersionApduCommand(
     bool is_legacy_version) {
-  return is_legacy_version ? U2fApduCommand::CreateLegacyVersion()
-                           : U2fApduCommand::CreateVersion();
+  U2fApduCommand command;
+  command.set_ins(base::strict_cast<uint8_t>(U2fApduInstruction::kVersion));
+  // Set maximum expected response length to maximum length possible.
+  command.set_response_length(kU2fMaxResponseSize);
+  // Early U2F drafts defined the U2F version command a format
+  // incompatible with ISO 7816-4, so 2 additional 0x0 bytes are necessary.
+  // https://fidoalliance.org/specs/fido-u2f-v1.1-id-20160915/fido-u2f-raw-message-formats-v1.1-id-20160915.html#implementation-considerations
+  auto version_cmd = command.GetEncodedCommand();
+  if (is_legacy_version)
+    version_cmd.insert(version_cmd.end(), kLegacyVersionSuffix.cbegin(),
+                       kLegacyVersionSuffix.cend());
+
+  return version_cmd;
 }
 
-std::unique_ptr<U2fApduCommand> U2fRequest::GetU2fSignApduCommand(
+base::Optional<std::vector<uint8_t>> U2fRequest::GetU2fSignApduCommand(
+    const std::vector<uint8_t>& application_parameter,
     const std::vector<uint8_t>& key_handle,
     bool is_check_only_sign) const {
-  return U2fApduCommand::CreateSign(application_parameter_, challenge_digest_,
-                                    key_handle, is_check_only_sign);
+  if (application_parameter.size() != kU2fParameterLength ||
+      challenge_digest_.size() != kU2fParameterLength ||
+      key_handle.size() > kMaxKeyHandleLength) {
+    return base::nullopt;
+  }
+  U2fApduCommand command;
+  std::vector<uint8_t> data(challenge_digest_.begin(), challenge_digest_.end());
+  data.insert(data.end(), application_parameter.begin(),
+              application_parameter.end());
+  data.push_back(static_cast<uint8_t>(key_handle.size()));
+  data.insert(data.end(), key_handle.begin(), key_handle.end());
+  command.set_ins(base::strict_cast<uint8_t>(U2fApduInstruction::kSign));
+  command.set_p1(is_check_only_sign ? kP1CheckOnly : kP1TupRequiredConsumed);
+  command.set_data(data);
+  return command.GetEncodedCommand();
 }
 
-std::unique_ptr<U2fApduCommand> U2fRequest::GetU2fRegisterApduCommand(
+base::Optional<std::vector<uint8_t>> U2fRequest::GetU2fRegisterApduCommand(
     bool is_individual_attestation) const {
-  return U2fApduCommand::CreateRegister(
-      application_parameter_, challenge_digest_, is_individual_attestation);
+  if (application_parameter_.size() != kU2fParameterLength ||
+      challenge_digest_.size() != kU2fParameterLength) {
+    return base::nullopt;
+  }
+  U2fApduCommand command;
+  std::vector<uint8_t> data(challenge_digest_.begin(), challenge_digest_.end());
+  data.insert(data.end(), application_parameter_.begin(),
+              application_parameter_.end());
+  command.set_ins(base::strict_cast<uint8_t>(U2fApduInstruction::kRegister));
+  command.set_p1(kP1TupRequiredConsumed |
+                 (is_individual_attestation ? kP1IndividualAttestation : 0));
+  command.set_data(data);
+  return command.GetEncodedCommand();
 }
 
 void U2fRequest::Transition() {

device/fido/u2f_request.h

@@ -15,6 +15,7 @@
 #include "base/containers/flat_set.h"
 #include "base/macros.h"
 #include "base/optional.h"
+#include "device/fido/ctap_constants.h"
 #include "device/fido/u2f_apdu_command.h"
 #include "device/fido/u2f_device.h"
 #include "device/fido/u2f_discovery.h"
@@ -41,20 +42,19 @@ class COMPONENT_EXPORT(DEVICE_FIDO) U2fRequest : public U2fDiscovery::Observer {
 
   void Start();
 
-  // Returns bogus application parameter and challenge to be used to verify user
-  // presence.
-  static const std::vector<uint8_t>& GetBogusApplicationParameter();
-  static const std::vector<uint8_t>& GetBogusChallenge();
+  // Returns bogus register command to be used to verify user presence.
+  static std::vector<uint8_t> GetBogusRegisterCommand();
   // Returns APDU formatted U2F version request command. If |is_legacy_version|
   // is set to true, suffix {0x00, 0x00} is added at the end.
-  static std::unique_ptr<U2fApduCommand> GetU2fVersionApduCommand(
-      bool is_legacy_version);
-  // Returns APDU U2F request commands. Nullptr is returned for
+  static std::vector<uint8_t> GetU2fVersionApduCommand(
+      bool is_legacy_version = false);
+  // Returns APDU U2F request commands. Null optional is returned for
   // incorrectly formatted parameter.
-  std::unique_ptr<U2fApduCommand> GetU2fSignApduCommand(
+  base::Optional<std::vector<uint8_t>> GetU2fSignApduCommand(
+      const std::vector<uint8_t>& application_parameter,
       const std::vector<uint8_t>& key_handle,
       bool is_check_only_sign = false) const;
-  std::unique_ptr<U2fApduCommand> GetU2fRegisterApduCommand(
+  base::Optional<std::vector<uint8_t>> GetU2fRegisterApduCommand(
       bool is_individual_attestation) const;
 
  protected:

device/fido/u2f_request_unittest.cc

@@ -67,18 +67,18 @@ TEST_F(U2fRequestTest, TestIterateDevice) {
   discovery->AddDevice(std::move(device0));
   discovery->AddDevice(std::move(device1));
 
-  // Move first device to current
+  // Move first device to current.
   request.IterateDevice();
   ASSERT_NE(nullptr, request.current_device_);
   EXPECT_EQ(static_cast<size_t>(1), request.devices_.size());
 
-  // Move second device to current, first to attempted
+  // Move second device to current, first to attempted.
   request.IterateDevice();
   ASSERT_NE(nullptr, request.current_device_);
   EXPECT_EQ(static_cast<size_t>(1), request.attempted_devices_.size());
 
   // Move second device from current to attempted, move attempted to devices as
-  // all devices have been attempted
+  // all devices have been attempted.
   request.IterateDevice();
 
   ASSERT_EQ(nullptr, request.current_device_);
@@ -325,14 +325,14 @@ TEST_F(U2fRequestTest, TestMultipleDiscoveriesWithFailures) {
 TEST_F(U2fRequestTest, TestEncodeVersionRequest) {
   constexpr uint8_t kEncodedU2fVersionRequest[] = {0x00, 0x03, 0x00, 0x00,
                                                    0x00, 0x00, 0x00};
-  EXPECT_THAT(U2fRequest::GetU2fVersionApduCommand(false)->GetEncodedCommand(),
+  EXPECT_THAT(U2fRequest::GetU2fVersionApduCommand(),
               ::testing::ElementsAreArray(kEncodedU2fVersionRequest));
 
   // Legacy version command contains 2 extra null bytes compared to ISO 7816-4
   // format.
   constexpr uint8_t kEncodedU2fLegacyVersionRequest[] = {
       0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
-  EXPECT_THAT(U2fRequest::GetU2fVersionApduCommand(true)->GetEncodedCommand(),
+  EXPECT_THAT(U2fRequest::GetU2fVersionApduCommand(true),
               ::testing::ElementsAreArray(kEncodedU2fLegacyVersionRequest));
 }
 

device/fido/u2f_sign.cc

@@ -64,21 +64,20 @@ void U2fSign::TryDevice() {
   // [2]
   // https://fidoalliance.org/specs/fido-v2.0-ps-20170927/fido-client-to-authenticator-protocol-v2.0-ps-20170927.html
   if (registered_keys_.size() == 0) {
-    // Send registration (Fake enroll) if no keys were provided
+    // Send registration (Fake enroll) if no keys were provided.
     current_device_->Register(
-        U2fRequest::GetBogusApplicationParameter(),
-        U2fRequest::GetBogusChallenge(), false /* no individual attestation */,
+        U2fRequest::GetBogusRegisterCommand(),
         base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(),
                        registered_keys_.cend(),
                        ApplicationParameterType::kPrimary));
     return;
   }
-  // Try signing current device with the first registered key
+  // Try signing current device with the first registered key.
   auto it = registered_keys_.cbegin();
   current_device_->Sign(
-      application_parameter_, challenge_digest_, *it,
-      base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(), it,
-                     ApplicationParameterType::kPrimary));
+      GetU2fSignApduCommand(application_parameter_, *it),
+      base::Bind(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(), it,
+                 ApplicationParameterType::kPrimary));
 }
 
 void U2fSign::OnTryDevice(std::vector<std::vector<uint8_t>>::const_iterator it,
@@ -122,22 +121,20 @@ void U2fSign::OnTryDevice(std::vector<std::vector<uint8_t>>::const_iterator it,
         // |application_parameter_| failed, but there is also
         // |alt_application_parameter_| to try.
         current_device_->Sign(
-            *alt_application_parameter_, challenge_digest_, *it,
-            base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(),
-                           it, ApplicationParameterType::kAlternative));
+            GetU2fSignApduCommand(*alt_application_parameter_, *it),
+            base::Bind(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(), it,
+                       ApplicationParameterType::kAlternative));
       } else if (++it != registered_keys_.end()) {
         // Key is not for this device. Try signing with the next key.
         current_device_->Sign(
-            application_parameter_, challenge_digest_, *it,
+            GetU2fSignApduCommand(application_parameter_, *it),
             base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(),
                            it, ApplicationParameterType::kPrimary));
       } else {
         // No provided key was accepted by this device. Send registration
         // (Fake enroll) request to device.
         current_device_->Register(
-            U2fRequest::GetBogusApplicationParameter(),
-            U2fRequest::GetBogusChallenge(),
-            false /* no individual attestation */,
+            GetBogusRegisterCommand(),
             base::BindOnce(&U2fSign::OnTryDevice, weak_factory_.GetWeakPtr(),
                            registered_keys_.cend(),
                            ApplicationParameterType::kPrimary));

device/fido/u2f_sign_unittest.cc

@@ -145,12 +145,21 @@ class U2fSignTest : public ::testing::Test {
 
  protected:
   base::test::ScopedTaskEnvironment scoped_task_environment_;
+  std::vector<uint8_t> application_parameter_{std::begin(kAppId),
+                                              std::end(kAppId)};
+  std::vector<uint8_t> challenge_parameter_{std::begin(kChallengeDigest),
+                                            std::end(kChallengeDigest)};
   test::ScopedFakeU2fDiscoveryFactory scoped_fake_discovery_factory_;
   test::FakeU2fDiscovery* discovery_;
   TestSignCallback sign_callback_receiver_;
+  std::vector<std::vector<uint8_t>> key_handles_;
+  base::flat_set<U2fTransportProtocol> protocols_;
 };
 
 TEST_F(U2fSignTest, TestCreateSignApduCommand) {
+  key_handles_.push_back(
+      std::vector<uint8_t>(std::begin(kKeyHandle), std::end(kKeyHandle)));
+
   constexpr uint8_t kSignApduEncoded[] = {
       // clang-format off
       0x00, 0x02, 0x03, 0x00,  // CLA, INS, P1, P2 APDU instruction parameters
@@ -173,6 +182,14 @@ TEST_F(U2fSignTest, TestCreateSignApduCommand) {
       // clang-format on
   };
 
+  U2fSign u2f_sign(nullptr, protocols_, key_handles_, challenge_parameter_,
+                   application_parameter_, base::nullopt,
+                   sign_callback_receiver_.callback());
+  const auto encoded_sign =
+      u2f_sign.GetU2fSignApduCommand(application_parameter_, key_handles_[0]);
+  ASSERT_TRUE(encoded_sign);
+  EXPECT_THAT(*encoded_sign, ::testing::ElementsAreArray(kSignApduEncoded));
+
   constexpr uint8_t kSignApduEncodedCheckOnly[] = {
       // clang-format off
       0x00, 0x02, 0x07, 0x00,  // CLA, INS, P1, P2 APDU instruction parameters
@@ -195,24 +212,10 @@ TEST_F(U2fSignTest, TestCreateSignApduCommand) {
       // clang-format on
   };
 
-  const std::vector<uint8_t> key_handle(std::begin(kKeyHandle),
-                                        std::end(kKeyHandle));
-  U2fSign u2f_sign(nullptr /* connector */, {} /* transports */,
-                   {key_handle} /* registered_keys */,
-                   std::vector<uint8_t>(std::begin(kChallengeDigest),
-                                        std::end(kChallengeDigest)),
-                   std::vector<uint8_t>(std::begin(kAppId), std::end(kAppId)),
-                   base::nullopt, sign_callback_receiver().callback());
-
-  const auto encoded_sign = u2f_sign.GetU2fSignApduCommand(key_handle);
-  ASSERT_TRUE(encoded_sign);
-  EXPECT_THAT(encoded_sign->GetEncodedCommand(),
-              ::testing::ElementsAreArray(kSignApduEncoded));
-
-  const auto encoded_sign_check_only =
-      u2f_sign.GetU2fSignApduCommand(key_handle, true);
+  const auto encoded_sign_check_only = u2f_sign.GetU2fSignApduCommand(
+      application_parameter_, key_handles_[0], true);
   ASSERT_TRUE(encoded_sign_check_only);
-  EXPECT_THAT(encoded_sign_check_only->GetEncodedCommand(),
+  EXPECT_THAT(*encoded_sign_check_only,
               ::testing::ElementsAreArray(kSignApduEncodedCheckOnly));
 }
 
@@ -295,11 +298,11 @@ TEST_F(U2fSignTest, TestDelayedSuccess) {
   sign_callback_receiver().WaitForCallback();
   EXPECT_EQ(U2fReturnCode::SUCCESS, sign_callback_receiver().status());
 
-  // Correct key was sent so a sign response is expected
+  // Correct key was sent so a sign response is expected.
   EXPECT_EQ(GetTestAssertionSignature(),
             sign_callback_receiver().value()->signature());
 
-  // Verify that we get the key handle used for signing
+  // Verify that we get the key handle used for signing.
   EXPECT_EQ(signing_key_handle, sign_callback_receiver().value()->raw_id());
 }
 
@@ -521,7 +524,6 @@ TEST_F(U2fSignTest, TestAlternativeApplicationParameter) {
       std::vector<std::vector<uint8_t>>({signing_key_handle}),
       std::vector<uint8_t>(32), primary_app_param, alt_app_param,
       sign_callback_receiver_.callback());
-
   request->Start();
   discovery()->WaitForCallToStartAndSimulateSuccess();
 

device/fido/virtual_u2f_device.cc

@@ -6,9 +6,11 @@
 
 #include <utility>
 
+#include "base/numerics/safe_conversions.h"
 #include "crypto/ec_private_key.h"
 #include "crypto/ec_signature_creator.h"
 #include "crypto/sha2.h"
+#include "device/fido/ctap_constants.h"
 #include "device/fido/u2f_apdu_command.h"
 
 namespace device {
@@ -128,17 +130,17 @@ void VirtualU2fDevice::AddRegistration(
 void VirtualU2fDevice::DeviceTransact(std::vector<uint8_t> command,
                                       DeviceCallback cb) {
   // Note, here we are using the code-under-test in this fake.
-  auto parsed_command = U2fApduCommand::CreateFromMessage(command);
-  switch (parsed_command->ins_) {
-    case U2fApduCommand::kInsU2fVersion:
+  auto parsed_command = U2fApduCommand::CreateFromMessageForTesting(command);
+  switch (parsed_command->ins()) {
+    case base::strict_cast<uint8_t>(U2fApduInstruction::kVersion):
       break;
-    case U2fApduCommand::kInsU2fEnroll:
-      DoRegister(parsed_command->ins_, parsed_command->p1_, parsed_command->p2_,
-                 parsed_command->data_, std::move(cb));
+    case base::strict_cast<uint8_t>(U2fApduInstruction::kRegister):
+      DoRegister(parsed_command->ins(), parsed_command->p1(),
+                 parsed_command->p2(), parsed_command->data(), std::move(cb));
       break;
-    case U2fApduCommand::kInsU2fSign:
-      DoSign(parsed_command->ins_, parsed_command->p1_, parsed_command->p2_,
-             parsed_command->data_, std::move(cb));
+    case base::strict_cast<uint8_t>(U2fApduInstruction::kSign):
+      DoSign(parsed_command->ins(), parsed_command->p1(), parsed_command->p2(),
+             parsed_command->data(), std::move(cb));
       break;
     default:
       std::move(cb).Run(true,
@@ -230,9 +232,8 @@ void VirtualU2fDevice::DoSign(uint8_t ins,
                               uint8_t p2,
                               base::span<const uint8_t> data,
                               DeviceCallback cb) {
-  if (!(p1 == U2fApduCommand::kP1CheckOnly ||
-        p1 == U2fApduCommand::kP1TupRequiredConsumed ||
-        p1 == U2fApduCommand::kP1IndividualAttestation) ||
+  if (!(p1 == kP1CheckOnly || p1 == kP1TupRequiredConsumed ||
+        p1 == kP1IndividualAttestation) ||
       p2 != 0) {
     std::move(cb).Run(true, std::make_unique<U2fApduResponse>(
                                 std::vector<uint8_t>(),