Improve frame validation logging.

PiperOrigin-RevId: 674365762
2026-09-16 15:36:12 -04:00 · 2024-09-13 11:15:58 -07:00
parent 153ea88c7a
commit 2f4eb8649a
3 changed files with 107 additions and 91 deletions
@@ -139,6 +139,7 @@ ExceptionOr<ByteArray> BaseEndpointChannel::Read(

  {
    MutexLock crypto_lock(&crypto_mutex_);
+    Exception message_exception{Exception::kInvalidProtocolBuffer};
    if (IsEncryptionEnabledLocked()) {
      // If encryption is enabled, decode the message.
      std::string input(std::move(result));
@@ -169,6 +170,7 @@ ExceptionOr<ByteArray> BaseEndpointChannel::Read(
                << parser::GetFrameType(parsed.result());
          }
        } else {
+          message_exception.value = parsed.exception();
          NEARBY_LOGS(WARNING)
              << __func__ << ": Unable to parse data as unencrypted message.";
        }
@@ -176,7 +178,7 @@ ExceptionOr<ByteArray> BaseEndpointChannel::Read(
      packet_meta_data.StopEncryption();
      if (result.Empty()) {
        NEARBY_LOGS(WARNING) << __func__ << ": Unable to parse read result.";
-        return ExceptionOr<ByteArray>(Exception::kInvalidProtocolBuffer);
+        return ExceptionOr<ByteArray>(message_exception);
      }
    }
  }
@@ -14,7 +14,7 @@

 #include "connections/implementation/offline_frames_validator.h"

-#include <algorithm>
+#include <cstddef>
 #include <regex>  //NOLINT
 #include <string>

@@ -22,7 +22,6 @@
 #include "connections/implementation/offline_frames.h"
 #include "connections/implementation/proto/offline_wire_formats.pb.h"
 #include "internal/platform/exception.h"
-#include "internal/platform/implementation/platform.h"
 #include "internal/platform/logging.h"

 namespace nearby {
@@ -91,18 +90,26 @@ Exception EnsureValidConnectionResponseFrame(

 Exception EnsureValidPayloadTransferDataFrame(const PayloadChunk& payload_chunk,
                                              std::int64_t totalSize) {
-  if (!payload_chunk.has_flags()) return {Exception::kInvalidProtocolBuffer};
+  if (!payload_chunk.has_flags()) {
+    LOG(ERROR) << "Missing payload chunk flags";
+    return {Exception::kInvalidProtocolBuffer};
+  }

  // Special case. The body can be null iff the chunk is flagged as the last
  // chunk.
  bool is_last_chunk = (payload_chunk.flags() &
                        PayloadTransferFrame::PayloadChunk::LAST_CHUNK) != 0;
-  if (!payload_chunk.has_body() && !is_last_chunk)
+  if (!payload_chunk.has_body() && !is_last_chunk) {
+    LOG(ERROR) << "Missing payload chunk body";
    return {Exception::kInvalidProtocolBuffer};
-  if (!payload_chunk.has_offset() || payload_chunk.offset() < 0)
+  }
+  if (!payload_chunk.has_offset() || payload_chunk.offset() < 0) {
+    LOG(ERROR) << "Invalid payload chunk offset";
    return {Exception::kInvalidProtocolBuffer};
+  }
  if (totalSize != InternalPayload::kIndeterminateSize &&
      totalSize < payload_chunk.offset()) {
+    LOG(ERROR) << "Payload chunk offset > totalSize";
    return {Exception::kInvalidProtocolBuffer};
  }

@@ -113,10 +120,13 @@ Exception EnsureValidPayloadTransferDataFrame(const PayloadChunk& payload_chunk,

 Exception EnsureValidPayloadTransferControlFrame(
    const ControlMessage& control_message, std::int64_t totalSize) {
-  if (!control_message.has_offset() || control_message.offset() < 0)
+  if (!control_message.has_offset() || control_message.offset() < 0) {
+    LOG(ERROR) << "Invalid control message offset";
    return {Exception::kInvalidProtocolBuffer};
+  }
  if (totalSize != InternalPayload::kIndeterminateSize &&
      totalSize < control_message.offset()) {
+    LOG(ERROR) << "Control message offset > totalSize";
    return {Exception::kInvalidProtocolBuffer};
  }

@@ -125,8 +135,37 @@ Exception EnsureValidPayloadTransferControlFrame(
  return {Exception::kSuccess};
 }

+bool CheckForIllegalCharacters(std::string toBeValidated,
+                               const absl::string_view illegalPatterns[],
+                               size_t illegalPatternsSize) {
+  if (toBeValidated.empty()) {
+    return false;
+  }
+
+  CHECK_GT(illegalPatternsSize, 0);
+
+  size_t found = 0;
+  for (int index = 0; index < illegalPatternsSize; index++) {
+    found = toBeValidated.find(std::string(illegalPatterns[index]));
+
+    if (found != std::string::npos) {
+      // TODO(jfcarroll): Find a way to issue a log statement here.
+      // Currently, this breaks the fuzzer, as a logging dep is not
+      // included for it in the BUILD file.
+      // NEARBY_LOGS(ERROR) << "In path " << toBeValidated
+      //                   << " found illegal character/pattern "
+      //                   << illegalPatterns[index];
+      return true;
+    }
+  }
+  return false;
+}
+
 Exception EnsureValidPayloadTransferFrame(const PayloadTransferFrame& frame) {
-  if (!frame.has_payload_header()) return {Exception::kInvalidProtocolBuffer};
+  if (!frame.has_payload_header()) {
+    LOG(ERROR) << "Missing payload header";
+    return {Exception::kInvalidProtocolBuffer};
+  }
  if (frame.packet_type() == PayloadTransferFrame::PAYLOAD_ACK) {
    // Phone side code doesn't set "total_size" for "payload_header", so skip
    // checking it.
@@ -135,16 +174,44 @@ Exception EnsureValidPayloadTransferFrame(const PayloadTransferFrame& frame) {
  if (!frame.payload_header().has_total_size() ||
      (frame.payload_header().total_size() < 0 &&
       frame.payload_header().total_size() !=
-           InternalPayload::kIndeterminateSize))
+           InternalPayload::kIndeterminateSize)) {
+    LOG(ERROR) << "Invalid payload header size";
    return {Exception::kInvalidProtocolBuffer};
-  if (!frame.has_packet_type()) return {Exception::kInvalidProtocolBuffer};
-
+  }
+  if (frame.payload_header().has_type() &&
+        frame.payload_header().type() ==
+            location::nearby::connections::PayloadTransferFrame::
+                PayloadHeader::FILE) {
+    if (frame.payload_header()
+            .has_file_name()) {
+      if (CheckForIllegalCharacters(frame.payload_header()
+                                        .file_name(),
+                                    kIllegalFileNamePatterns,
+                                    kIllegalFileNamePatternsSize)) {
+        return {Exception::kIllegalCharacters};
+      }
+    }
+    if (frame.payload_header()
+            .has_parent_folder()) {
+      if (CheckForIllegalCharacters(frame.payload_header()
+                                        .parent_folder(),
+                                    kIllegalParentFolderPatterns,
+                                    kIllegalParentFolderPatternsSize)) {
+        return {Exception::kIllegalCharacters};
+      }
+    }
+  }
+  if (!frame.has_packet_type()) {
+    LOG(ERROR) << "Missing packet type";
+    return {Exception::kInvalidProtocolBuffer};
+  }
  switch (frame.packet_type()) {
    case PayloadTransferFrame::DATA:
      if (frame.has_payload_chunk()) {
        return EnsureValidPayloadTransferDataFrame(
            frame.payload_chunk(), frame.payload_header().total_size());
      }
+      LOG(ERROR) << "Missing payload chunk";
      return {Exception::kInvalidProtocolBuffer};

    case PayloadTransferFrame::CONTROL:
@@ -152,6 +219,7 @@ Exception EnsureValidPayloadTransferFrame(const PayloadTransferFrame& frame) {
        return EnsureValidPayloadTransferControlFrame(
            frame.control_message(), frame.payload_header().total_size());
      }
+      LOG(ERROR) << "Missing control message";
      return {Exception::kInvalidProtocolBuffer};

    default:
@@ -347,32 +415,6 @@ Exception EnsureValidBandwidthUpgradeNegotiationFrame(
  return {Exception::kSuccess};
 }

-bool CheckForIllegalCharacters(std::string toBeValidated,
-                               const absl::string_view illegalPatterns[],
-                               size_t illegalPatternsSize) {
-  if (toBeValidated.empty()) {
-    return false;
-  }
-
-  CHECK_GT(illegalPatternsSize, 0);
-
-  size_t found = 0;
-  for (int index = 0; index < illegalPatternsSize; index++) {
-    found = toBeValidated.find(std::string(illegalPatterns[index]));
-
-    if (found != std::string::npos) {
-      // TODO(jfcarroll): Find a way to issue a log statement here.
-      // Currently, this breaks the fuzzer, as a logging dep is not
-      // included for it in the BUILD file.
-      // NEARBY_LOGS(ERROR) << "In path " << toBeValidated
-      //                   << " found illegal character/pattern "
-      //                   << illegalPatterns[index];
-      return true;
-    }
-  }
-  return false;
-}
-
 }  // namespace

 Exception EnsureValidOfflineFrame(
@@ -385,6 +427,7 @@ Exception EnsureValidOfflineFrame(
        return EnsureValidConnectionRequestFrame(
            offline_frame.v1().connection_request());
      }
+      LOG(ERROR) << "Missing connection request";
      return {Exception::kInvalidProtocolBuffer};

    case V1Frame::CONNECTION_RESPONSE:
@@ -393,46 +436,15 @@ Exception EnsureValidOfflineFrame(
        return EnsureValidConnectionResponseFrame(
            offline_frame.v1().connection_response());
      }
+      LOG(ERROR) << "Missing connection response";
      return {Exception::kInvalidProtocolBuffer};

    case V1Frame::PAYLOAD_TRANSFER:
-      if (offline_frame.has_v1() &&
-          (offline_frame.v1().payload_transfer().payload_header().has_type() &&
-           offline_frame.v1().payload_transfer().payload_header().type() ==
-               location::nearby::connections::
-                   PayloadTransferFrame_PayloadHeader_PayloadType::
-                       PayloadTransferFrame_PayloadHeader_PayloadType_FILE)) {
-        if (offline_frame.v1()
-                .payload_transfer()
-                .payload_header()
-                .has_file_name()) {
-          if (CheckForIllegalCharacters(offline_frame.v1()
-                                            .payload_transfer()
-                                            .payload_header()
-                                            .file_name(),
-                                        kIllegalFileNamePatterns,
-                                        kIllegalFileNamePatternsSize)) {
-            return {Exception::kIllegalCharacters};
-          }
-        }
-        if (offline_frame.v1()
-                .payload_transfer()
-                .payload_header()
-                .has_parent_folder()) {
-          if (CheckForIllegalCharacters(offline_frame.v1()
-                                            .payload_transfer()
-                                            .payload_header()
-                                            .parent_folder(),
-                                        kIllegalParentFolderPatterns,
-                                        kIllegalParentFolderPatternsSize)) {
-            return {Exception::kIllegalCharacters};
-          }
-        }
-      }
      if (offline_frame.has_v1() && offline_frame.v1().has_payload_transfer()) {
        return EnsureValidPayloadTransferFrame(
            offline_frame.v1().payload_transfer());
      }
+      LOG(ERROR) << "Missing payload transfer";
      return {Exception::kInvalidProtocolBuffer};

    case V1Frame::BANDWIDTH_UPGRADE_NEGOTIATION:
@@ -441,6 +453,7 @@ Exception EnsureValidOfflineFrame(
        return EnsureValidBandwidthUpgradeNegotiationFrame(
            offline_frame.v1().bandwidth_upgrade_negotiation());
      }
+      LOG(ERROR) << "Missing bandwidth upgrade negotiation";
      return {Exception::kInvalidProtocolBuffer};

    case V1Frame::KEEP_ALIVE:
@@ -309,7 +309,7 @@ TEST(OfflineFramesValidatorTest, ValidatesAsFailedTypeFileWithIllegalFilePath) {

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_TRUE(ret_value.value == Exception::kIllegalCharacters);
+  EXPECT_EQ(ret_value.value, Exception::kIllegalCharacters);
 }

 TEST(OfflineFramesValidatorTest, ValidatesAsOkTypeFileWithLegalParentFolder) {
@@ -334,7 +334,7 @@ TEST(OfflineFramesValidatorTest, ValidatesAsOkTypeFileWithLegalParentFolder) {

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_TRUE(ret_value.Ok());
+  EXPECT_TRUE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -360,8 +360,9 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_TRUE(ret_value.value == Exception::kIllegalCharacters);
+  EXPECT_EQ(ret_value.value, Exception::kIllegalCharacters);
 }
+
 TEST(OfflineFramesValidatorTest, ValidatesAsFailWithNullPayloadTransferFrame) {
  PayloadTransferFrame::PayloadHeader header;
  PayloadTransferFrame::PayloadChunk chunk;
@@ -379,7 +380,7 @@ TEST(OfflineFramesValidatorTest, ValidatesAsFailWithNullPayloadTransferFrame) {

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -404,7 +405,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -425,7 +426,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -450,7 +451,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -471,7 +472,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -492,7 +493,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -518,7 +519,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -543,7 +544,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -563,7 +564,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -583,7 +584,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -597,7 +598,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_TRUE(ret_value.Ok());
+  EXPECT_TRUE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -614,7 +615,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest, ValidatesAsOkBandwidthUpgradeWifiDirect) {
@@ -628,7 +629,7 @@ TEST(OfflineFramesValidatorTest, ValidatesAsOkBandwidthUpgradeWifiDirect) {

  auto ret_value = EnsureValidOfflineFrame(offline_frame);

-  ASSERT_TRUE(ret_value.Ok());
+  EXPECT_TRUE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -644,7 +645,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame_1);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());

  // But -1 itself is not invalid
  bytes = ForBwuWifiDirectPathAvailable(
@@ -654,7 +655,7 @@ TEST(OfflineFramesValidatorTest,

  ret_value = EnsureValidOfflineFrame(offline_frame_2);

-  ASSERT_TRUE(ret_value.Ok());
+  EXPECT_TRUE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -671,7 +672,7 @@ TEST(OfflineFramesValidatorTest,

  auto ret_value = EnsureValidOfflineFrame(offline_frame_1);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());

  std::string wifi_direct_ssid_wrong_length =
      std::string{kWifiDirectSsid} + "ABCDEFGHIJKLMNOPQRSTUVWXYZ123456789";
@@ -683,7 +684,7 @@ TEST(OfflineFramesValidatorTest,

  ret_value = EnsureValidOfflineFrame(offline_frame_2);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 TEST(OfflineFramesValidatorTest,
@@ -713,7 +714,7 @@ TEST(OfflineFramesValidatorTest,

  ret_value = EnsureValidOfflineFrame(offline_frame_2);

-  ASSERT_FALSE(ret_value.Ok());
+  EXPECT_FALSE(ret_value.Ok());
 }

 }  // namespace