Enforce valid spec-conformant string encoding in TLV

examples/chip-tool/args.gni

@@ -27,3 +27,7 @@ matter_enable_tracing_support = true
 
 matter_log_json_payload_hex = true
 matter_log_json_payload_decode_full = true
+
+# make chip-tool very strict by default
+chip_tlv_validate_char_string_on_read = true
+chip_tlv_validate_char_string_on_write = true

src/app/clusters/scenes-server/SceneTable.h

@@ -186,6 +186,14 @@ class SceneTable
  }
  ~SceneData(){};
 
+ bool operator==(const SceneData &other) const {
+ return (
+ (CharSpan(mName, mNameLength) == CharSpan(other.mName, other.mNameLength)) &&
+ (mSceneTransitionTimeMs == other.mSceneTransitionTimeMs) &&
+ (mExtensionFieldSets == other.mExtensionFieldSets)
+ );
+ }
+
  void SetName(const CharSpan & sceneName)
  {
  if (nullptr == sceneName.data())
@@ -203,8 +211,7 @@ class SceneTable
 
  void Clear()
  {
- SetName(CharSpan());
-
+ memset(mName, 0, sizeof(mName));
  mSceneTransitionTimeMs = 0;
  mExtensionFieldSets.Clear();
  }

src/app/clusters/scenes-server/SceneTableImpl.cpp

@@ -209,7 +209,8 @@ struct SceneTableData : public SceneTableEntry, PersistentData<kPersistentSceneB
 
  CHIP_ERROR Serialize(TLV::TLVWriter & writer) const override
  {
- CharSpan nameSpan(mStorageData.mName, mStorageData.mNameLength);
+ // TODO: if we have mNameLength, should this bin ByteSpan instead?
+ CharSpan nameSpan(mStorageData.mName, strnlen(mStorageData.mName, mStorageData.mNameLength));
  TLV::TLVType container;
  ReturnErrorOnFailure(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, container));
 

src/app/clusters/time-synchronization-server/time-synchronization-server.cpp

@@ -551,7 +551,7 @@ void TimeSynchronizationServer::InitTimeZone()
  for (auto & tzStore : mTimeZoneObj.timeZoneList)
  {
  memset(tzStore.name, 0, sizeof(tzStore.name));
- tzStore.timeZone = { .offset = 0, .validAt = 0, .name = MakeOptional(CharSpan(tzStore.name, sizeof(tzStore.name))) };
+ tzStore.timeZone = { .offset = 0, .validAt = 0, .name = MakeOptional(CharSpan(tzStore.name, 0)) };
  }
 }
 

src/lib/core/BUILD.gn

@@ -66,6 +66,8 @@ buildconfig_header("chip_buildconfig") {
  "CHIP_CONFIG_MINMDNS_MAX_PARALLEL_RESOLVES=${chip_config_minmdns_max_parallel_resolves}",
  "CHIP_CONFIG_CANCELABLE_HAS_INFO_STRING_FIELD=${chip_config_cancelable_has_info_string_field}",
  "CHIP_CONFIG_BIG_ENDIAN_TARGET=${chip_target_is_big_endian}",
+ "CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_WRITE=${chip_tlv_validate_char_string_on_write}",
+ "CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ=${chip_tlv_validate_char_string_on_read}",
  ]
 }
 

src/lib/core/CHIPError.cpp

@@ -113,12 +113,18 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err)
  case CHIP_ERROR_WRONG_ENCRYPTION_TYPE.AsInteger():
  desc = "Wrong encryption type";
  break;
+ case CHIP_ERROR_INVALID_UTF8.AsInteger():
+ desc = "Character string is not a valid utf-8 encoding";
+ break;
  case CHIP_ERROR_INTEGRITY_CHECK_FAILED.AsInteger():
  desc = "Integrity check failed";
  break;
  case CHIP_ERROR_INVALID_SIGNATURE.AsInteger():
  desc = "Invalid signature";
  break;
+ case CHIP_ERROR_INVALID_TLV_CHAR_STRING.AsInteger():
+ desc = "Invalid TLV Char string encoding.";
+ break;
  case CHIP_ERROR_UNSUPPORTED_SIGNATURE_TYPE.AsInteger():
  desc = "Unsupported signature type";
  break;

src/lib/core/CHIPError.h

@@ -581,7 +581,14 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_WRONG_ENCRYPTION_TYPE CHIP_CORE_ERROR(0x11)
 
-// AVAILABLE: 0x12
+/**
+ * @def CHIP_ERROR_INVALID_UTF8
+ *
+ * @brief
+ * Invalid UTF8 string (contains some characters that are invalid)
+ *
+ */
+#define CHIP_ERROR_INVALID_UTF8 CHIP_CORE_ERROR(0x12)
 
 /**
  * @def CHIP_ERROR_INTEGRITY_CHECK_FAILED
@@ -602,7 +609,15 @@ using CHIP_ERROR = ::chip::ChipError;
  */
 #define CHIP_ERROR_INVALID_SIGNATURE CHIP_CORE_ERROR(0x14)
 
-// AVAILABLE: 0x15
+/**
+ * @def CHIP_ERROR_INVALID_TLV_CHAR_STRING
+ *
+ * @brief
+ * Invalid TLV character string (e.g. null terminator)
+ *
+ */
+#define CHIP_ERROR_INVALID_TLV_CHAR_STRING CHIP_CORE_ERROR(0x15)
+
 // AVAILABLE: 0x16
 
 /**

src/lib/core/TLVReader.cpp

@@ -32,6 +32,7 @@
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/SafeInt.h>
+#include <lib/support/utf8.h>
 
 namespace chip {
 namespace TLV {
@@ -330,6 +331,25 @@ CHIP_ERROR TLVReader::Get(CharSpan & v) const
  }
 
  v = CharSpan(Uint8::to_const_char(bytes), len);
+#if CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ
+ // Spec requirement: A.11.2. UTF-8 and Octet Strings
+ //
+ // For UTF-8 strings, the value octets SHALL encode a valid
+ // UTF-8 character (code points) sequence.
+ //
+ // Senders SHALL NOT include a terminating null character to
+ // mark the end of a string.
+
+ if (!Utf8::IsValid(v))
+ {
+ return CHIP_ERROR_INVALID_UTF8;
+ }
+
+ if ((len > 0) && (v[len - 1] == 0))
+ {
+ return CHIP_ERROR_INVALID_TLV_CHAR_STRING;
+ }
+#endif // CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ
  return CHIP_NO_ERROR;
 }
 

src/lib/core/TLVWriter.cpp

@@ -33,6 +33,7 @@
 #include <lib/support/CHIPMem.h>
 #include <lib/support/CodeUtils.h>
 #include <lib/support/SafeInt.h>
+#include <lib/support/utf8.h>
 
 #include <stdarg.h>
 #include <stdint.h>
@@ -252,10 +253,30 @@ CHIP_ERROR TLVWriter::PutString(Tag tag, const char * buf)
 
 CHIP_ERROR TLVWriter::PutString(Tag tag, const char * buf, uint32_t len)
 {
+#if CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_WRITE
+ // Spec requirement: A.11.2. UTF-8 and Octet Strings
+ //
+ // For UTF-8 strings, the value octets SHALL encode a valid
+ // UTF-8 character (code points) sequence.
+ //
+ // Senders SHALL NOT include a terminating null character to
+ // mark the end of a string.
+
+ if (!Utf8::IsValid(CharSpan(buf, len)))
+ {
+ return CHIP_ERROR_INVALID_UTF8;
+ }
+
+ if ((len > 0) && (buf[len - 1] == 0))
+ {
+ return CHIP_ERROR_INVALID_TLV_CHAR_STRING;
+ }
+#endif // CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ
+
  return WriteElementWithData(kTLVType_UTF8String, tag, reinterpret_cast<const uint8_t *>(buf), len);
 }
 
-CHIP_ERROR TLVWriter::PutString(Tag tag, Span<const char> str)
+CHIP_ERROR TLVWriter::PutString(Tag tag, CharSpan str)
 {
  if (!CanCastTo<uint32_t>(str.size()))
  {

src/lib/core/TLVWriter.h

@@ -573,7 +573,7 @@ class DLL_EXPORT TLVWriter
  * TLVBackingStore.
  *
  */
- CHIP_ERROR PutString(Tag tag, Span<const char> str);
+ CHIP_ERROR PutString(Tag tag, CharSpan str);
 
  /**
  * @brief

src/lib/core/core.gni

@@ -91,6 +91,13 @@ declare_args() {
 
  # Whether the target architecture is big-endian (true) or little-endian (false).
  chip_target_is_big_endian = false
+
+ # Validation on TLV encode/decode for string validity of character
+ # strings:
+ # - SHALL NOT end with binary 0 (spec still allows embedded 0)
+ # - SHALL be valid UTF8
+ chip_tlv_validate_char_string_on_write = true
+ chip_tlv_validate_char_string_on_read = false
 }
 
 if (chip_target_style == "") {

src/lib/core/tests/TestCHIPErrorStr.cpp

@@ -67,8 +67,10 @@ static const CHIP_ERROR kTestElements[] =
  CHIP_ERROR_UNKNOWN_KEY_TYPE,
  CHIP_ERROR_KEY_NOT_FOUND,
  CHIP_ERROR_WRONG_ENCRYPTION_TYPE,
+ CHIP_ERROR_INVALID_UTF8,
  CHIP_ERROR_INTEGRITY_CHECK_FAILED,
  CHIP_ERROR_INVALID_SIGNATURE,
+ CHIP_ERROR_INVALID_TLV_CHAR_STRING,
  CHIP_ERROR_UNSUPPORTED_SIGNATURE_TYPE,
  CHIP_ERROR_INVALID_MESSAGE_LENGTH,
  CHIP_ERROR_BUFFER_TOO_SMALL,

src/lib/support/tests/TestTlvJson.cpp

@@ -134,7 +134,7 @@ void TestConverter(nlTestSuite * inSuite, void * inContext)
  "}\n");
 
  const char charBuf[] = "hello";
- CharSpan charSpan(charBuf);
+ CharSpan charSpan = CharSpan::fromCharString(charBuf);
  EncodeAndValidate(charSpan,
  "{\n"
  " \"value\" : \"hello\"\n"