[CrOS PhoneHub] Implement SecureChannel protocol v4

WireMessage protocol v3 used an unsigned 16-bit integer to represent
the message length. Unfortunately, when big messages are sent (e.g.,
images sent through Phone Hub), the size can exceed the maximum value of
a 16-bit int, so the protocol breaks down.

This CL introduces v4, which uses an unsigned 32-bit integer to
represent the message size. For backward compatibility, we still use v3
to send messages used for authentication, Smart Lock, and Instant
Tethering. If a message is sent for any other feature type, we use v4.

Bug: 1150565, 1106937
Change-Id: I2e074505f8efa8a6a7ba355229e8cd0adf7e7c19
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2552250
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: James Vecore <vecore@google.com>
Cr-Commit-Position: refs/heads/master@{#829896}

chromeos/services/secure_channel/wire_message.h

@@ -14,6 +14,33 @@ namespace chromeos {
 
 namespace secure_channel {
 
+// Message sent over the wire via SecureChannel. Each message sent using the
+// SecureChannel protocol has a feature name (a unique identifier for the client
+// sending/receiving a message) and a payload.
+//
+// A wire message in SecureChannel is serialized to a byte array (in the format
+// of a std::string). The first byte is always the protocol version expressed as
+// an unsigned 8-bit int. This class supports protocol versions 3 and 4.
+//
+// v3: One byte version (0x03), followed by 2 bytes of the message size as an
+//     unsigned 16-bit int, followed by a stringified JSON object containing a
+//     "feature" key whose value is the feature and a "payload" key whose value
+//     is the payload.
+//
+//     [ message version ] [ body length ] [ JSON body ]
+//           1 byte            2 bytes      body length
+//
+// v4: One byte version (0x04), followed by 4 bytes of the message size as an
+//     unsigned 32-bit int, followed by a stringified JSON object containing a
+//     "feature" key whose value is the feature and a "payload" key whose value
+//     is the payload.
+//
+//     [ message version ] [ body length ] [ JSON body ]
+//           1 byte            4 bytes      body length
+//
+// v3 is deprecated and all new features use v4, but we special-case features
+// which were released before v4 and send them via v3 for backward
+// compatibility.
 class WireMessage {
  public:
   // Creates a WireMessage containing |payload| for feature |feature| and

chromeos/services/secure_channel/wire_message_unittest.cc

@@ -30,7 +30,7 @@ TEST(SecureChannelWireMessageTest, Deserialize_EmptyMessage) {
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_IncompleteHeader) {
+TEST(SecureChannelWireMessageTest, Deserialize_IncompleteHeader_V3) {
   bool is_incomplete;
   std::unique_ptr<WireMessage> message =
       WireMessage::Deserialize("\3", &is_incomplete);
@@ -38,6 +38,14 @@ TEST(SecureChannelWireMessageTest, Deserialize_IncompleteHeader) {
   EXPECT_FALSE(message);
 }
 
+TEST(SecureChannelWireMessageTest, Deserialize_IncompleteHeader_V4) {
+  bool is_incomplete;
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize("\4", &is_incomplete);
+  EXPECT_TRUE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
 TEST(SecureChannelWireMessageTest, Deserialize_UnexpectedMessageFormatVersion) {
   bool is_incomplete;
   // Version 2 is below the minimum supported version.
@@ -47,7 +55,7 @@ TEST(SecureChannelWireMessageTest, Deserialize_UnexpectedMessageFormatVersion) {
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_BodyOfSizeZero) {
+TEST(SecureChannelWireMessageTest, Deserialize_BodyOfSizeZero_V3) {
   bool is_incomplete;
   std::unique_ptr<WireMessage> message =
       WireMessage::Deserialize(std::string("\3\0\0", 3), &is_incomplete);
@@ -55,7 +63,15 @@ TEST(SecureChannelWireMessageTest, Deserialize_BodyOfSizeZero) {
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_IncompleteBody) {
+TEST(SecureChannelWireMessageTest, Deserialize_BodyOfSizeZero_V4) {
+  bool is_incomplete;
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(std::string("\4\0\0\0\0", 5), &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
+TEST(SecureChannelWireMessageTest, Deserialize_IncompleteBody_V3) {
   bool is_incomplete;
   std::unique_ptr<WireMessage> message =
       WireMessage::Deserialize(std::string("\3\0\5", 3), &is_incomplete);
@@ -63,8 +79,16 @@ TEST(SecureChannelWireMessageTest, Deserialize_IncompleteBody) {
   EXPECT_FALSE(message);
 }
 
+TEST(SecureChannelWireMessageTest, Deserialize_IncompleteBody_V4) {
+  bool is_incomplete;
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(std::string("\4\0\0\0\5", 5), &is_incomplete);
+  EXPECT_TRUE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
 TEST(SecureChannelWireMessageTest,
-     Deserialize_BodyLongerThanSpecifiedInHeader) {
+     Deserialize_BodyLongerThanSpecifiedInHeader_V3) {
   bool is_incomplete;
   std::unique_ptr<WireMessage> message = WireMessage::Deserialize(
       std::string("\3\0\5", 3) + "123456", &is_incomplete);
@@ -72,7 +96,16 @@ TEST(SecureChannelWireMessageTest,
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotValidJSON) {
+TEST(SecureChannelWireMessageTest,
+     Deserialize_BodyLongerThanSpecifiedInHeader_V4) {
+  bool is_incomplete;
+  std::unique_ptr<WireMessage> message = WireMessage::Deserialize(
+      std::string("\4\0\0\0\5", 5) + "123456", &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
+TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotValidJSON_V3) {
   bool is_incomplete;
   std::unique_ptr<WireMessage> message = WireMessage::Deserialize(
       std::string("\3\0\5", 3) + "12345", &is_incomplete);
@@ -80,7 +113,15 @@ TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotValidJSON) {
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotADictionary) {
+TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotValidJSON_V4) {
+  bool is_incomplete;
+  std::unique_ptr<WireMessage> message = WireMessage::Deserialize(
+      std::string("\4\0\0\0\5", 5) + "12345", &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
+TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotADictionary_V3) {
   bool is_incomplete;
   std::string header("\3\0\x15", 3);
   std::string bytes = header + "[{\"payload\": \"YQ==\"}]";
@@ -90,7 +131,17 @@ TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotADictionary) {
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_NonEncryptedMessage) {
+TEST(SecureChannelWireMessageTest, Deserialize_BodyIsNotADictionary_V4) {
+  bool is_incomplete;
+  std::string header("\4\0\0\0\x15", 5);
+  std::string bytes = header + "[{\"payload\": \"YQ==\"}]";
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(bytes, &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
+TEST(SecureChannelWireMessageTest, Deserialize_NonEncryptedMessage_V3) {
   bool is_incomplete;
   std::string header("\3\0\x02", 3);
   std::string bytes = header + "{}";
@@ -101,7 +152,18 @@ TEST(SecureChannelWireMessageTest, Deserialize_NonEncryptedMessage) {
   EXPECT_EQ("{}", message->body());
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_BodyHasEmptyPayload) {
+TEST(SecureChannelWireMessageTest, Deserialize_NonEncryptedMessage_V4) {
+  bool is_incomplete;
+  std::string header("\4\0\0\0\x02", 5);
+  std::string bytes = header + "{}";
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(bytes, &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  ASSERT_TRUE(message);
+  EXPECT_EQ("{}", message->body());
+}
+
+TEST(SecureChannelWireMessageTest, Deserialize_BodyHasEmptyPayload_V3) {
   bool is_incomplete;
   std::string header("\3\0\x29", 3);
   std::string bytes =
@@ -112,7 +174,18 @@ TEST(SecureChannelWireMessageTest, Deserialize_BodyHasEmptyPayload) {
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_PayloadIsNotBase64Encoded) {
+TEST(SecureChannelWireMessageTest, Deserialize_BodyHasEmptyPayload_V4) {
+  bool is_incomplete;
+  std::string header("\4\0\0\0\x29", 5);
+  std::string bytes =
+      header + "{\"payload\": \"\", \"feature\": \"testFeature\"}";
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(bytes, &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
+TEST(SecureChannelWireMessageTest, Deserialize_PayloadIsNotBase64Encoded_V3) {
   bool is_incomplete;
   std::string header("\3\0\x30", 3);
   std::string bytes =
@@ -123,7 +196,18 @@ TEST(SecureChannelWireMessageTest, Deserialize_PayloadIsNotBase64Encoded) {
   EXPECT_FALSE(message);
 }
 
-TEST(SecureChannelWireMessageTest, Deserialize_ValidMessage) {
+TEST(SecureChannelWireMessageTest, Deserialize_PayloadIsNotBase64Encoded_V4) {
+  bool is_incomplete;
+  std::string header("\4\0\0\0\x30", 5);
+  std::string bytes =
+      header + "{\"payload\": \"garbage\", \"feature\": \"testFeature\"}";
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(bytes, &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  EXPECT_FALSE(message);
+}
+
+TEST(SecureChannelWireMessageTest, Deserialize_ValidMessage_V3) {
   bool is_incomplete;
   std::string header("\3\0\x2d", 3);
   std::string bytes =
@@ -136,8 +220,21 @@ TEST(SecureChannelWireMessageTest, Deserialize_ValidMessage) {
   EXPECT_EQ("testFeature", message->feature());
 }
 
+TEST(SecureChannelWireMessageTest, Deserialize_ValidMessage_V4) {
+  bool is_incomplete;
+  std::string header("\4\0\0\0\x2d", 5);
+  std::string bytes =
+      header + "{\"payload\": \"YQ==\", \"feature\": \"testFeature\"}";
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(bytes, &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  ASSERT_TRUE(message);
+  EXPECT_EQ("a", message->payload());
+  EXPECT_EQ("testFeature", message->feature());
+}
+
 TEST(SecureChannelWireMessageTest,
-     Deserialize_ValidMessageWithBase64UrlEncoding) {
+     Deserialize_ValidMessageWithBase64UrlEncoding_V3) {
   bool is_incomplete;
   std::string header("\3\0\x2d", 3);
   std::string bytes =
@@ -151,7 +248,21 @@ TEST(SecureChannelWireMessageTest,
 }
 
 TEST(SecureChannelWireMessageTest,
-     Deserialize_ValidMessageWithExtraUnknownFields) {
+     Deserialize_ValidMessageWithBase64UrlEncoding_V4) {
+  bool is_incomplete;
+  std::string header("\4\0\0\0\x2d", 5);
+  std::string bytes =
+      header + "{\"payload\": \"_-Y=\", \"feature\": \"testFeature\"}";
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(bytes, &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  ASSERT_TRUE(message);
+  EXPECT_EQ("\xFF\xE6", message->payload());
+  EXPECT_EQ("testFeature", message->feature());
+}
+
+TEST(SecureChannelWireMessageTest,
+     Deserialize_ValidMessageWithExtraUnknownFields_V3) {
   bool is_incomplete;
   std::string header("\3\0\x4c", 3);
   std::string bytes = header +
@@ -168,6 +279,24 @@ TEST(SecureChannelWireMessageTest,
   EXPECT_EQ("testFeature", message->feature());
 }
 
+TEST(SecureChannelWireMessageTest,
+     Deserialize_ValidMessageWithExtraUnknownFields_V4) {
+  bool is_incomplete;
+  std::string header("\4\0\0\0\x4c", 5);
+  std::string bytes = header +
+                      "{"
+                      "  \"payload\": \"YQ==\","
+                      "  \"feature\": \"testFeature\","
+                      "  \"unexpected\": \"surprise!\""
+                      "}";
+  std::unique_ptr<WireMessage> message =
+      WireMessage::Deserialize(bytes, &is_incomplete);
+  EXPECT_FALSE(is_incomplete);
+  ASSERT_TRUE(message);
+  EXPECT_EQ("a", message->payload());
+  EXPECT_EQ("testFeature", message->feature());
+}
+
 TEST(SecureChannelWireMessageTest, Deserialize_SizeEquals0x01FF) {
   // Create a message with a body of 0x01FF bytes to test the size contained in
   // the header is parsed correctly.