Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2502103002:
Add FIDO U2F message and packet classes (Closed)
|
Created:
4 years, 1 month ago by Casey Piper Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd FIDO U2F message and packet classes
FIDO U2F defines initialization packets and continuation packets.
This adds class definitions for those packets, and for an
enclosing message class. The enclosing message class constructs
the initialization and continuation packets based on the payload
length, queueing them in a list format for consumption by a
device connection.
BUG=686306
Committed: https://crrev.com/de3a54f157b87492c1d0757e07c918ba1b2a0c17
Cr-Commit-Position: refs/heads/master@{#438925}
Patch Set 1 #Patch Set 2 : Add FIDO U2F message and packet classes #
Total comments: 28
Patch Set 3 : Add FIDO U2F message and packet classes #
Total comments: 3
Patch Set 4 : Add FIDO U2F message and packet classes #
Total comments: 46
Patch Set 5 : Add FIDO U2F message and packet classes #Patch Set 6 : Add FIDO U2F message and packet classes #Patch Set 7 : Adding constructors and fix issues when creating messages and packets from IOBuffers #
Total comments: 26
Patch Set 8 : Change channel id to uint32_t from uint8_t[4] and fix additional review comments #
Total comments: 14
Patch Set 9 : Fix review comments, change constants to constexpr #
Total comments: 7
Patch Set 10 : Remove const from primitives, fix minor comments #Patch Set 11 : Update logic for rejecting packets that come in with incorrect channel id #Patch Set 12 : Add basic fuzzer test, and updated constructor for U2fMessage to account for invalid packets #
Total comments: 18
Patch Set 13 : Update fuzzer test to add continuation packets and fix other review comments #
Total comments: 4
Patch Set 14 : Update fuzzer test to use entire provided buffer, check for null continuation packets before derefe… #Patch Set 15 : Add includes to u2f_packet #
Total comments: 4
Patch Set 16 : Remove excess includes and add forward declarations for IOBuffer #Patch Set 17 : Add include to fuzzer after forward declaration #Patch Set 18 : Add //net dependency to fuzzer in BUILD.gn #
Messages
Total messages: 71 (39 generated)
Description was changed from ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a USB or HID connection. BUG=NONE ========== to ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=NONE ==========
reillyg@chromium.org changed reviewers: + reillyg@chromium.org
https://codereview.chromium.org/2502103002/diff/20001/device/u2f/BUILD.gn File device/u2f/BUILD.gn (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/BUILD.gn#new... device/u2f/BUILD.gn:18: "//components/device_event_log", Unused. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/BUILD.gn#new... device/u2f/BUILD.gn:19: "//device/base", Unused. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/BUILD.gn#new... device/u2f/BUILD.gn:24: static_library("test_support") { Since you include u2f_message_unittest.cc in the device_unittests target above you don't need this here. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:13: size_t size = data.size(); Name this remaining_size. I was confused why we were setting this to 0 before building a packet. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:17: return; If you're doing validation in the constructor I would define a U2fMessage::Create() function that returns a new U2fMessage on success and nullptr on failure. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:34: static_cast<U2fPacket*>(new U2fInitPacket(channel_id, type, payload)))); A static cast is unnecessary here. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:46: payload.insert(std::end(payload), first, last); I found this reuse of payload confusing. You can just std::vector<uint8_t>(first, last) to the U2fPacket constructor? https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:62: return 0; s/0/nullptr/ https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.h:17: enum u2f_message_type : char { Use an enum class named Type. I'd rather we use uint8_t instead of char. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.h:44: const std::vector<char> data); Use std::vector<uint8_t> for byte strings. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.h:56: } } // namespace device https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:15: } nit: unnecessary braces https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:25: return buffer; one line: return make_scoped_refptr(new net::IOBufferWithSize(0)); https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:46: } nit (here and below): no braces for single line if/for/while. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:83: } // namespace device; no ;
https://codereview.chromium.org/2502103002/diff/20001/device/u2f/BUILD.gn File device/u2f/BUILD.gn (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/BUILD.gn#new... device/u2f/BUILD.gn:18: "//components/device_event_log", On 2016/11/23 22:21:50, Reilly Grant wrote: > Unused. Acknowledged. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/BUILD.gn#new... device/u2f/BUILD.gn:19: "//device/base", On 2016/11/23 22:21:50, Reilly Grant wrote: > Unused. Acknowledged. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:13: size_t size = data.size(); On 2016/11/23 22:21:50, Reilly Grant wrote: > Name this remaining_size. I was confused why we were setting this to 0 before > building a packet. Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:34: static_cast<U2fPacket*>(new U2fInitPacket(channel_id, type, payload)))); On 2016/11/23 22:21:50, Reilly Grant wrote: > A static cast is unnecessary here. Acknowledged. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:46: payload.insert(std::end(payload), first, last); On 2016/11/23 22:21:50, Reilly Grant wrote: > I found this reuse of payload confusing. You can just > std::vector<uint8_t>(first, last) to the U2fPacket constructor? Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:62: return 0; On 2016/11/23 22:21:50, Reilly Grant wrote: > s/0/nullptr/ Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.h:17: enum u2f_message_type : char { On 2016/11/23 22:21:50, Reilly Grant wrote: > Use an enum class named Type. > > I'd rather we use uint8_t instead of char. Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.h:44: const std::vector<char> data); On 2016/11/23 22:21:50, Reilly Grant wrote: > Use std::vector<uint8_t> for byte strings. Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_message.... device/u2f/u2f_message.h:56: } On 2016/11/23 22:21:51, Reilly Grant wrote: > } // namespace device Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:15: } On 2016/11/23 22:21:51, Reilly Grant wrote: > nit: unnecessary braces Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:25: return buffer; On 2016/11/23 22:21:51, Reilly Grant wrote: > one line: return make_scoped_refptr(new net::IOBufferWithSize(0)); Done. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:46: } On 2016/11/23 22:21:51, Reilly Grant wrote: > nit (here and below): no braces for single line if/for/while. Acknowledged. https://codereview.chromium.org/2502103002/diff/20001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:83: } // namespace device; On 2016/11/23 22:21:51, Reilly Grant wrote: > no ; Done.
https://codereview.chromium.org/2502103002/diff/40001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/40001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:12: const char channel_id[U2fPacket::kChannelIdSize], Should I change all char's to uint8_t?
https://codereview.chromium.org/2502103002/diff/40001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/40001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:12: const char channel_id[U2fPacket::kChannelIdSize], On 2016/11/29 at 22:17:56, piper wrote: > Should I change all char's to uint8_t? Yes.
https://codereview.chromium.org/2502103002/diff/40001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/40001/device/u2f/u2f_message.... device/u2f/u2f_message.cc:12: const char channel_id[U2fPacket::kChannelIdSize], On 2016/11/29 22:51:48, Reilly Grant wrote: > On 2016/11/29 at 22:17:56, piper wrote: > > Should I change all char's to uint8_t? > > Yes. Done.
juanlang@chromium.org changed reviewers: + juanlang@chromium.org
Looking good, Casey! I always have comments, so here are mine. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:17: enum class Type : uint8_t { It'd be helpful to reference the U2F specs somewhere in this file, e.g. http://fidoalliance.org/specs/u2f-specs-1.0-bt-nfc-id-amendment/fido-u2f-hid-.... https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:19: CMD_ATR = 0x82, This is not part of the U2F HID standard, please remove. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:20: CMD_APDU = 0x83, In the U2F HID standard, this is known as MSG rather than APDU. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:21: CMD_LOCK = 0x84, This is not part of the U2F HID standard, please remove. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:22: CMD_SYSINFO = 0x85, Ditto. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:24: CMD_PROMPT = 0x87, Ditto. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:26: CMD_BLE_UID = 0xb5, Ditto. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:27: CMD_USB_TEST = 0xb9, Ditto. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:28: CMD_DFU = 0xba, Ditto. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:29: CMD_SYNC = 0xbc, No action required, just an FYI: This is not part of the U2F HID standard. On the other hand, it's used by the Google-proprietary WinUSB protocol variant. Unless you already know you won't be implementing support for these, it seems safe to leave it in, though maybe with a TODO to remove once support for the proprietary protocol is removed. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:35: static const size_t kInitPacketDataSize = 57; Funny that such a small line will generate this volume of comments! Grab some popcorn, the show's about to begin. This is the result of subtracting kInitPacketHeader from an implicit limit: the maximum length of a HID packet (64 bytes.) I think it'd be clearer to make that result explicit rather than implicit, i.e. to define a new constant kMaxHidPacketSize and either define this limit in terms of it, or just use subtraction when you need to. However, see also my comment about the WinUSB protocol variant. If you intend to support it (do you?), it might be better to leave the maximum packet length as something the caller specifies, as the limit for an individual packet for the WinUSB protocol is rather larger, perhaps 2k.
https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:58: const std::vector<uint8_t>& data); Keep these definitions in order with blank lines between: friends, typedefs, constructors, methods, fields. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:61: virtual ~U2fMessage(); Protected before private. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:21: ASSERT_EQ(init_packet->GetBuffer()->size(), 65); cont_packet? https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:37: uint8_t cmd = 0x88; CMD_WINK? https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:47: ASSERT_EQ(init_packet->GetBuffer()->data()[index++], (char)cmd); This cast shouldn't be necessary if it wasn't for channel_id[0]. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:57: } Just use this loop for all the elements in the array. Add this to the end of the ASSERT_EQ to help debugging: ASSERT_EQ(...) << "mismatch at index " << index; https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:89: } General comment about these tests: We try to use EXPECT_FOO instead of ASSERT_FOO whenever possible because the former doesn't halt the test and can make it easier to see the full extent of the error. Only ASSERT on things that would cause the test to crash later. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:52: while ((int)index < serialized_->size()) static_cast<int> https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:78: : payload_size; payload_size = std::min(payload_size, kPacketSize - index); Though it seems safer to reject invalid packets. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:105: while ((int)index < serialized_->size()) static_cast<int> https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.h... device/u2f/u2f_packet.h:27: U2fPacket(); Same ordering issue here. Destructor and fields should come after the constructor.
https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:19: CMD_ATR = 0x82, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > This is not part of the U2F HID standard, please remove. Done. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:20: CMD_APDU = 0x83, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > In the U2F HID standard, this is known as MSG rather than APDU. Done. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:21: CMD_LOCK = 0x84, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > This is not part of the U2F HID standard, please remove. Done. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:22: CMD_SYSINFO = 0x85, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:24: CMD_PROMPT = 0x87, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:26: CMD_BLE_UID = 0xb5, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:27: CMD_USB_TEST = 0xb9, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:28: CMD_DFU = 0xba, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > Ditto. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:29: CMD_SYNC = 0xbc, On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > No action required, just an FYI: > > This is not part of the U2F HID standard. On the other hand, it's used by the > Google-proprietary WinUSB protocol variant. Unless you already know you won't be > implementing support for these, it seems safe to leave it in, though maybe with > a TODO to remove once support for the proprietary protocol is removed. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:35: static const size_t kInitPacketDataSize = 57; On 2016/11/30 19:41:32, juanlang (chromium.org) wrote: > Funny that such a small line will generate this volume of comments! Grab some > popcorn, the show's about to begin. > > This is the result of subtracting kInitPacketHeader from an implicit limit: the > maximum length of a HID packet (64 bytes.) I think it'd be clearer to make that > result explicit rather than implicit, i.e. to define a new constant > kMaxHidPacketSize and either define this limit in terms of it, or just use > subtraction when you need to. > > However, see also my comment about the WinUSB protocol variant. If you intend to > support it (do you?), it might be better to leave the maximum packet length as > something the caller specifies, as the limit for an individual packet for the > WinUSB protocol is rather larger, perhaps 2k. Done. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:58: const std::vector<uint8_t>& data); On 2016/11/30 20:53:46, Reilly Grant wrote: > Keep these definitions in order with blank lines between: friends, typedefs, > constructors, methods, fields. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message.... device/u2f/u2f_message.h:61: virtual ~U2fMessage(); On 2016/11/30 20:53:46, Reilly Grant wrote: > Protected before private. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:21: ASSERT_EQ(init_packet->GetBuffer()->size(), 65); On 2016/11/30 20:53:47, Reilly Grant wrote: > cont_packet? Done. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:37: uint8_t cmd = 0x88; On 2016/11/30 20:53:47, Reilly Grant wrote: > CMD_WINK? I'll add a static cast here to add clarity. My intention is to make packets agnostic of command types (since they are currently defined as U2fMessage constructs, rather than packet constructs). https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:47: ASSERT_EQ(init_packet->GetBuffer()->data()[index++], (char)cmd); On 2016/11/30 20:53:47, Reilly Grant wrote: > This cast shouldn't be necessary if it wasn't for channel_id[0]. Weirdly, the test will fail if I don't have the cast here: Value of: cmd Actual: '\x88' (136) Expected: init_packet->GetBuffer()->data()[index++] Which is: '\x88' (136) https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:57: } On 2016/11/30 20:53:47, Reilly Grant wrote: > Just use this loop for all the elements in the array. Add this to the end of the > ASSERT_EQ to help debugging: > > ASSERT_EQ(...) << "mismatch at index " << index; Can you clarify what you mean in the first sentence? https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:89: } On 2016/11/30 20:53:47, Reilly Grant wrote: > General comment about these tests: We try to use EXPECT_FOO instead of > ASSERT_FOO whenever possible because the former doesn't halt the test and can > make it easier to see the full extent of the error. Only ASSERT on things that > would cause the test to crash later. Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:52: while ((int)index < serialized_->size()) On 2016/11/30 20:53:47, Reilly Grant wrote: > static_cast<int> Done. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:78: : payload_size; On 2016/11/30 20:53:47, Reilly Grant wrote: > payload_size = std::min(payload_size, kPacketSize - index); > > Though it seems safer to reject invalid packets. Yeah, it was poorly written. To clarify, for payload size, it is the size of the total payload, which can be more than the total size of this particular packet (or less). It might be my reuse of the payload_size variable that is also confusing. I'm checking to see if there are going to be padded zeros in the buffer, which I don't need when reconstructing the packet from a buffer. I'll add an additional variable and add the std::min call. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.c... device/u2f/u2f_packet.cc:105: while ((int)index < serialized_->size()) On 2016/11/30 20:53:47, Reilly Grant wrote: > static_cast<int> Acknowledged. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_packet.h... device/u2f/u2f_packet.h:27: U2fPacket(); On 2016/11/30 20:53:47, Reilly Grant wrote: > Same ordering issue here. Destructor and fields should come after the > constructor. Done.
https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:47: ASSERT_EQ(init_packet->GetBuffer()->data()[index++], (char)cmd); On 2016/11/30 at 22:05:46, piper wrote: > On 2016/11/30 20:53:47, Reilly Grant wrote: > > This cast shouldn't be necessary if it wasn't for channel_id[0]. > > Weirdly, the test will fail if I don't have the cast here: > Value of: cmd > Actual: '\x88' (136) > Expected: init_packet->GetBuffer()->data()[index++] > Which is: '\x88' (136) Ah, right, they both get converted to an int for the comparison and -8 (chars are signed) != 136. This is why uint8_t is better than char. Unfortunately it's what IOBuffer is stuck with for the time being. https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:57: } On 2016/11/30 at 22:05:46, piper wrote: > On 2016/11/30 20:53:47, Reilly Grant wrote: > > Just use this loop for all the elements in the array. Add this to the end of the > > ASSERT_EQ to help debugging: > > > > ASSERT_EQ(...) << "mismatch at index " << index; > > Can you clarify what you mean in the first sentence? My mistake. I forgot that data.size() != init_packet->GetBuffer()->size(). https://codereview.chromium.org/2502103002/diff/60001/device/u2f/u2f_message_... device/u2f/u2f_message_unittest.cc:89: } On 2016/11/30 at 22:05:46, piper wrote: > On 2016/11/30 20:53:47, Reilly Grant wrote: > > General comment about these tests: We try to use EXPECT_FOO instead of > > ASSERT_FOO whenever possible because the former doesn't halt the test and can > > make it easier to see the full extent of the error. Only ASSERT on things that > > would cause the test to crash later. > > Acknowledged. Also, EXPECT_EQ wants the expected value first. Otherwise the errors it prints sound weird.
Hi Casey, some more comments to help my understanding. Thanks! https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:28: static const size_t kInitPacketHeader = 7; Nit: this, and the remaining constants, seem like a detail of the implementation, rather than part of the interface. Perhaps they shouldn't be public? https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:29: static const size_t kContPacketHeader = 5; Nit: prefer Continuation to Cont. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:40: const uint8_t channel_id[U2fPacket::kChannelIdSize], Passing arrays as parameters in C is a little dicey. While you're signalling to the caller that a 4-byte array is expected, there's no guarantee the caller will do so. I'd suggest you pass the channel_id as a uint32_t instead. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:43: static scoped_refptr<U2fMessage> Create( This method name makes it a little hard to differentiate from the previous one. In what circumstances should the caller call Create with a single parameter, vs. calling Create with 3 parameters? I'd suggest a name that signals a little more clearly what this method is for. Also, I believe it's the norm to add method documentation on public methods. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:14: U2fPacket(const std::vector<uint8_t> data, const uint8_t channel_id[]); Same comment about passing channel_id as a uint32_t rather than an array. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:16: scoped_refptr<net::IOBufferWithSize> GetBuffer(); What's the difference between GetBuffer() and GetData()? I mean, I can imagine they refer to the two different member variables, but I'm confused by what the distinction between the two could be. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:20: static const size_t kChannelIdSize = 4; If you use uint32_t for the channel_id, I don't think you need this any longer. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:22: static const size_t kPacketSize = 65; Seems like an implementation detail that could be not public.
https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.cc:129: it != end(); ++it) { Since this is internal I would just use for (const auto& packet : packets_) here for readability. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:78: reconstructed_packet->GetChannelId()[i]); nit: Braces around loop body when it is multiple lines. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:85: reconstructed_packet->GetBuffer()->data()[i]); nit: Braces around loop body when it is multiple lines. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:115: printf("%d:%d \n", (int)(*orig_it)->GetData().size(), Please avoid printfs in tests. You can use EXPECT_EQ(foo) << "foo" << foo type statements to have more context logged on failure. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:174: EXPECT_TRUE(data == filled_message->GetData()); You can use EXPECT_THAT(data, ContainerEq(filled_message->GetData()) from gmock to do comparisons between vectors.
https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.cc:129: it != end(); ++it) { On 2016/12/07 22:33:48, Reilly Grant wrote: > Since this is internal I would just use for (const auto& packet : packets_) here > for readability. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:28: static const size_t kInitPacketHeader = 7; On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > Nit: this, and the remaining constants, seem like a detail of the > implementation, rather than part of the interface. Perhaps they shouldn't be > public? Done, moved to private and test classes were added as friends. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:29: static const size_t kContPacketHeader = 5; On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > Nit: prefer Continuation to Cont. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:40: const uint8_t channel_id[U2fPacket::kChannelIdSize], On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > Passing arrays as parameters in C is a little dicey. While you're signalling to > the caller that a 4-byte array is expected, there's no guarantee the caller will > do so. I'd suggest you pass the channel_id as a uint32_t instead. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message.h:43: static scoped_refptr<U2fMessage> Create( On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > This method name makes it a little hard to differentiate from the previous one. > In what circumstances should the caller call Create with a single parameter, vs. > calling Create with 3 parameters? I'd suggest a name that signals a little more > clearly what this method is for. > > Also, I believe it's the norm to add method documentation on public methods. Create method names changed for clarity. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:78: reconstructed_packet->GetChannelId()[i]); On 2016/12/07 22:33:48, Reilly Grant wrote: > nit: Braces around loop body when it is multiple lines. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:85: reconstructed_packet->GetBuffer()->data()[i]); On 2016/12/07 22:33:48, Reilly Grant wrote: > nit: Braces around loop body when it is multiple lines. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:115: printf("%d:%d \n", (int)(*orig_it)->GetData().size(), On 2016/12/07 22:33:48, Reilly Grant wrote: > Please avoid printfs in tests. You can use EXPECT_EQ(foo) << "foo" << foo type > statements to have more context logged on failure. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:174: EXPECT_TRUE(data == filled_message->GetData()); On 2016/12/07 22:33:48, Reilly Grant wrote: > You can use EXPECT_THAT(data, ContainerEq(filled_message->GetData()) from gmock > to do comparisons between vectors. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:14: U2fPacket(const std::vector<uint8_t> data, const uint8_t channel_id[]); On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > Same comment about passing channel_id as a uint32_t rather than an array. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:16: scoped_refptr<net::IOBufferWithSize> GetBuffer(); On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > What's the difference between GetBuffer() and GetData()? I mean, I can imagine > they refer to the two different member variables, but I'm confused by what the > distinction between the two could be. I'll change it and hopefully they'll be more clear. GetData in this case returns the payload data (so the non-header data in a packet), while GetBuffer returns the entire packet in serialized IOBuffer form. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:20: static const size_t kChannelIdSize = 4; On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > If you use uint32_t for the channel_id, I don't think you need this any longer. Done. https://codereview.chromium.org/2502103002/diff/120001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:22: static const size_t kPacketSize = 65; On 2016/12/07 18:29:26, juanlang (chromium.org) wrote: > Seems like an implementation detail that could be not public. Moved to protected.
lgtm for FIDO stuff.
https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message.cc:119: return remaining_size_ == 0; This method is never called and it seems dubious to use an instance variable for remaining_size instead of a local in the constructor above. Why the change? https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message.cc:127: nit: this blank line isn't really necessary. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message.h:43: const std::vector<uint8_t> GetMessagePayload(); If the intent was to make this method const then the keyword goes at the end of the line. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:146: TEST_F(U2fMessageTest, TestMessagePartioning) { Partitioning? https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:49: serialized_->data()[index++] = 0; Add a comment about why the byte at offset 0 is always 0 (report ID). https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:79: size_t index = 1; Again, note about the report ID. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:96: uint8_t GetSequence(); uint8_t sequence() { return sequence_; } (For trivial accessors like this.)
https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message.cc:119: return remaining_size_ == 0; On 2016/12/09 01:19:43, Reilly Grant wrote: > This method is never called and it seems dubious to use an instance variable for > remaining_size instead of a local in the constructor above. Why the change? This method is intended to use when constructing a message from a list of incoming IOBuffers. Since the size of the total message is sent in the first packet, we need to know that size when creating the following packets. I can add a test to show how the function is intended to be used. In the constructor, I was intending to just reuse the instance variable rather than using a local variable; I can switch that to a local variable there. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message.cc:127: On 2016/12/09 01:19:43, Reilly Grant wrote: > nit: this blank line isn't really necessary. Acknowledged. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message.h:43: const std::vector<uint8_t> GetMessagePayload(); On 2016/12/09 01:19:43, Reilly Grant wrote: > If the intent was to make this method const then the keyword goes at the end of > the line. Done. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:146: TEST_F(U2fMessageTest, TestMessagePartioning) { On 2016/12/09 01:19:43, Reilly Grant wrote: > Partitioning? Done. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:49: serialized_->data()[index++] = 0; On 2016/12/09 01:19:43, Reilly Grant wrote: > Add a comment about why the byte at offset 0 is always 0 (report ID). Done. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:79: size_t index = 1; On 2016/12/09 01:19:43, Reilly Grant wrote: > Again, note about the report ID. Done. https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/140001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:96: uint8_t GetSequence(); On 2016/12/09 01:19:43, Reilly Grant wrote: > uint8_t sequence() { return sequence_; } > > (For trivial accessors like this.) Done.
I suggest writing a fuzzer for this packet parsing code similar to this one: https://cs.chromium.org/chromium/src/device/usb/usb_descriptors_fuzzer.cc More docs: https://chromium.googlesource.com/chromium/src/+/master/testing/libfuzzer/REA... https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message... device/u2f/u2f_message.cc:32: : packets_(), remaining_size_(0), channel_id_() { Initializing packets_ and channel_id_ is unnecessary. https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message... device/u2f/u2f_message.cc:102: U2fContinuationPacket::CreateFromSerializedData(buf, &remaining_size_); I think you meant to pass remaining_size here. https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message... device/u2f/u2f_message.h:31: const Type type, Don't pass primitive types by const value. Compilers are inconsistent about enforcing it.
https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message... device/u2f/u2f_message.cc:32: : packets_(), remaining_size_(0), channel_id_() { On 2016/12/09 20:55:20, Reilly Grant wrote: > Initializing packets_ and channel_id_ is unnecessary. Acknowledged. https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message... device/u2f/u2f_message.cc:102: U2fContinuationPacket::CreateFromSerializedData(buf, &remaining_size_); On 2016/12/09 20:55:20, Reilly Grant wrote: > I think you meant to pass remaining_size here. Done, good catch! https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message... device/u2f/u2f_message.h:31: const Type type, On 2016/12/09 20:55:20, Reilly Grant wrote: > Don't pass primitive types by const value. Compilers are inconsistent about > enforcing it. Done.
https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message.cc File device/u2f/u2f_message.cc (right): https://codereview.chromium.org/2502103002/diff/160001/device/u2f/u2f_message... device/u2f/u2f_message.cc:102: U2fContinuationPacket::CreateFromSerializedData(buf, &remaining_size_); On 2016/12/09 21:43:38, piper wrote: > On 2016/12/09 20:55:20, Reilly Grant wrote: > > I think you meant to pass remaining_size here. > > Done, good catch! Actually, I now realize I didn't, but I edited the logic slightly. What I'm doing is if there is a failure (we received something with a different channel ID), I revert back to the original remaining_size.
https://codereview.chromium.org/2502103002/diff/220001/device/u2f/BUILD.gn File device/u2f/BUILD.gn (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/BUILD.gn#ne... device/u2f/BUILD.gn:29: libfuzzer_options = [ "max_len=65" ] Set this higher so you get continuation packets as this is what I'm most interested in testing the parsing logic for. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... File device/u2f/u2f_message_fuzzer.cc (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_fuzzer.cc:11: make_scoped_refptr(new net::IOBufferWithSize(size)); scoped_refptr<net::IOBufferWithSize> buf( new net::IOBufferWithSize(size); (To avoid the need for make_scoped_refptr.) https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:13: U2fMessageTest() {} You actually don't have to define a constructor here. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:126: std::list<scoped_refptr<U2fPacket>>::const_iterator new_it = new_msg->begin(); nit: this is a good place to use auto. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:176: std::fill(std::begin(data), std::end(data), 0x7F); std::vector<uint8_t> data(U2fMessage::kMaxMessageSize, 0x7F); https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:12: U2fPacket::U2fPacket() : data_(), channel_id_() {} nit: These initializers are unnecessary. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:38: : U2fPacket(data, channel_id), command_(cmd), payload_length_() { nit: payload_length_() is unnecessary. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:71: : U2fPacket(), command_(), payload_length_() { nit: None of these initializers are necessary. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:143: : U2fPacket() { nit: Unnecessary initializer.
https://codereview.chromium.org/2502103002/diff/220001/device/u2f/BUILD.gn File device/u2f/BUILD.gn (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/BUILD.gn#ne... device/u2f/BUILD.gn:29: libfuzzer_options = [ "max_len=65" ] On 2016/12/10 00:02:44, Reilly Grant wrote: > Set this higher so you get continuation packets as this is what I'm most > interested in testing the parsing logic for. Since packets would be sent one at a time, I had to change the fuzzer test to manually split up the packets. The constructor wouldn't split things up by itself, but if there is a better way to do this, let me know. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... File device/u2f/u2f_message_fuzzer.cc (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_fuzzer.cc:11: make_scoped_refptr(new net::IOBufferWithSize(size)); On 2016/12/10 00:02:44, Reilly Grant wrote: > scoped_refptr<net::IOBufferWithSize> buf( > new net::IOBufferWithSize(size); > > (To avoid the need for make_scoped_refptr.) Done. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... File device/u2f/u2f_message_unittest.cc (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:13: U2fMessageTest() {} On 2016/12/10 00:02:44, Reilly Grant wrote: > You actually don't have to define a constructor here. Acknowledged. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:126: std::list<scoped_refptr<U2fPacket>>::const_iterator new_it = new_msg->begin(); On 2016/12/10 00:02:44, Reilly Grant wrote: > nit: this is a good place to use auto. Done. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_message... device/u2f/u2f_message_unittest.cc:176: std::fill(std::begin(data), std::end(data), 0x7F); On 2016/12/10 00:02:44, Reilly Grant wrote: > std::vector<uint8_t> data(U2fMessage::kMaxMessageSize, 0x7F); Done. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.cc File device/u2f/u2f_packet.cc (right): https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:12: U2fPacket::U2fPacket() : data_(), channel_id_() {} On 2016/12/10 00:02:44, Reilly Grant wrote: > nit: These initializers are unnecessary. Done. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:38: : U2fPacket(data, channel_id), command_(cmd), payload_length_() { On 2016/12/10 00:02:44, Reilly Grant wrote: > nit: payload_length_() is unnecessary. Done. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:71: : U2fPacket(), command_(), payload_length_() { On 2016/12/10 00:02:44, Reilly Grant wrote: > nit: None of these initializers are necessary. Done. https://codereview.chromium.org/2502103002/diff/220001/device/u2f/u2f_packet.... device/u2f/u2f_packet.cc:143: : U2fPacket() { On 2016/12/10 00:02:44, Reilly Grant wrote: > nit: Unnecessary initializer. Done.
https://codereview.chromium.org/2502103002/diff/240001/device/u2f/u2f_message... File device/u2f/u2f_message_fuzzer.cc (right): https://codereview.chromium.org/2502103002/diff/240001/device/u2f/u2f_message... device/u2f/u2f_message_fuzzer.cc:22: while (remaining_buffer > packet_size) { This means we never test a short packet. https://codereview.chromium.org/2502103002/diff/240001/device/u2f/u2f_message... device/u2f/u2f_message_fuzzer.cc:24: memcpy(buf->data(), start, packet_size); Allocate a new buffer each time so that ASAN can catch access to bytes beyond the end of packet (if the last one is shorter than 65 bytes).
https://codereview.chromium.org/2502103002/diff/240001/device/u2f/u2f_message... File device/u2f/u2f_message_fuzzer.cc (right): https://codereview.chromium.org/2502103002/diff/240001/device/u2f/u2f_message... device/u2f/u2f_message_fuzzer.cc:22: while (remaining_buffer > packet_size) { On 2016/12/10 01:52:02, Reilly Grant wrote: > This means we never test a short packet. Updated to use a small packet at the end of the buffer if given. https://codereview.chromium.org/2502103002/diff/240001/device/u2f/u2f_message... device/u2f/u2f_message_fuzzer.cc:24: memcpy(buf->data(), start, packet_size); On 2016/12/10 01:52:02, Reilly Grant wrote: > Allocate a new buffer each time so that ASAN can catch access to bytes beyond > the end of packet (if the last one is shorter than 65 bytes). Done.
lgtm
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by piperc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juanlang@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2502103002/#ps280001 (title: "Add includes to u2f_packet")
The CQ bit was unchecked by piperc@chromium.org
The CQ bit was checked by piperc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juanlang@chromium.org, reillyg@chromium.org Link to the patchset: https://codereview.chromium.org/2502103002/#ps280001 (title: "Add includes to u2f_packet")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
piperc@chromium.org changed reviewers: + xunjieli@chromium.org
piperc@chromium.org changed reviewers: + pauljensen@chromium.org
net/base DEPS lgtm https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_message... device/u2f/u2f_message.h:12: #include "device/u2f/u2f_packet.h" namespace net { class IOBufferWithSize; } // namespace net https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:12: #include "net/base/io_buffer.h" Can this be replaced with a forward declaration? this is the Chromium prefered style: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...
https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_message.h File device/u2f/u2f_message.h (right): https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_message... device/u2f/u2f_message.h:12: #include "device/u2f/u2f_packet.h" On 2016/12/15 01:59:22, pauljensen wrote: > namespace net { > class IOBufferWithSize; > } // namespace net Done. https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_packet.h File device/u2f/u2f_packet.h (right): https://codereview.chromium.org/2502103002/diff/280001/device/u2f/u2f_packet.... device/u2f/u2f_packet.h:12: #include "net/base/io_buffer.h" On 2016/12/15 01:59:22, pauljensen wrote: > Can this be replaced with a forward declaration? this is the Chromium prefered > style: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done.
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
net/base DEPS still lgtm
The CQ bit was checked by piperc@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by piperc@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from juanlang@chromium.org, reillyg@chromium.org, pauljensen@chromium.org Link to the patchset: https://codereview.chromium.org/2502103002/#ps340001 (title: "Add //net dependency to fuzzer in BUILD.gn")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1481836331210380,
"parent_rev": "8b56ca617fb863f27eb1832e64bbd936ebdf4054", "commit_rev":
"ad789cc7e483e86bb81defa1ec610f95542ec879"}
Message was sent while issue was closed.
Description was changed from ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=NONE ========== to ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=NONE Review-Url: https://codereview.chromium.org/2502103002 ==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001)
Message was sent while issue was closed.
Description was changed from ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=NONE Review-Url: https://codereview.chromium.org/2502103002 ========== to ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=NONE Committed: https://crrev.com/de3a54f157b87492c1d0757e07c918ba1b2a0c17 Cr-Commit-Position: refs/heads/master@{#438925} ==========
Message was sent while issue was closed.
Patchset 18 (id:??) landed as https://crrev.com/de3a54f157b87492c1d0757e07c918ba1b2a0c17 Cr-Commit-Position: refs/heads/master@{#438925}
Message was sent while issue was closed.
Patchset #19 (id:360001) has been deleted
Message was sent while issue was closed.
Description was changed from ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=NONE Committed: https://crrev.com/de3a54f157b87492c1d0757e07c918ba1b2a0c17 Cr-Commit-Position: refs/heads/master@{#438925} ========== to ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=687832 Committed: https://crrev.com/de3a54f157b87492c1d0757e07c918ba1b2a0c17 Cr-Commit-Position: refs/heads/master@{#438925} ==========
Message was sent while issue was closed.
Description was changed from ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=687832 Committed: https://crrev.com/de3a54f157b87492c1d0757e07c918ba1b2a0c17 Cr-Commit-Position: refs/heads/master@{#438925} ========== to ========== Add FIDO U2F message and packet classes FIDO U2F defines initialization packets and continuation packets. This adds class definitions for those packets, and for an enclosing message class. The enclosing message class constructs the initialization and continuation packets based on the payload length, queueing them in a list format for consumption by a device connection. BUG=686306 Committed: https://crrev.com/de3a54f157b87492c1d0757e07c918ba1b2a0c17 Cr-Commit-Position: refs/heads/master@{#438925} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
