Enforce valid spec-conformant string encoding in TLV

examples/all-clusters-app/nxp/mw320/main.cpp

@@ -172,7 +172,7 @@ void InitOTARequestor(void)
     // TODO: instatiate and initialize these values when QueryImageResponse tells us an image is available
     // TODO: add API for OTARequestor to pass QueryImageResponse info to the application to use for OTADownloader init
     // OTAImageProcessor ipParams;
-    // ipParams.imageFile = CharSpan("dnld_img.txt");
+    // ipParams.imageFile = CharSpan::fromCharString("dnld_img.txt");
     // gImageProcessor.SetOTAImageProcessorParams(ipParams);
     gImageProcessor.SetOTADownloader(&gDownloader);
 

examples/chef/common/stubs.cpp

@@ -186,8 +186,8 @@ class LockManager
         endpoints[0].id = 1;
         uint8_t pin[6]  = { 0x31, 0x32, 0x33, 0x34, 0x35, 0x36 };
         endpoints[0].credentials[0].set(DlCredentialStatus::kOccupied, CredentialTypeEnum::kPin, chip::ByteSpan(pin));
-        endpoints[0].users[0].set(chip::CharSpan("default"), 1, UserStatusEnum::kOccupiedEnabled, UserTypeEnum::kUnrestrictedUser,
-                                  CredentialRuleEnum::kSingle);
+        endpoints[0].users[0].set(chip::CharSpan::fromCharString("default"), 1, UserStatusEnum::kOccupiedEnabled,
+                                  UserTypeEnum::kUnrestrictedUser, CredentialRuleEnum::kSingle);
         endpoints[0].users[0].addCredential(CredentialTypeEnum::kPin, 1);
     }
 

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

examples/placeholder/linux/static-supported-temperature-levels.cpp

@@ -27,8 +27,9 @@ using namespace chip::app::Clusters::TemperatureControl;
 using chip::Protocols::InteractionModel::Status;
 
 // TODO: Configure your options for each endpoint
-CharSpan AppSupportedTemperatureLevelsDelegate::temperatureLevelOptions[] = { CharSpan("Hot", 3), CharSpan("Warm", 4),
-                                                                              CharSpan("Cold", 4) };
+CharSpan AppSupportedTemperatureLevelsDelegate::temperatureLevelOptions[] = { CharSpan::fromCharString("Hot"),
+                                                                              CharSpan::fromCharString("Warm"),
+                                                                              CharSpan::fromCharString("Cold") };
 
 const AppSupportedTemperatureLevelsDelegate::EndpointPair AppSupportedTemperatureLevelsDelegate::supportedOptionsByEndpoints
     [EMBER_AF_TEMPERATURE_CONTROL_CLUSTER_SERVER_ENDPOINT_COUNT] = {

src/app/clusters/door-lock-server/door-lock-server.cpp

@@ -1985,7 +1985,7 @@ Status DoorLockServer::clearUser(chip::EndpointId endpointId, chip::FabricIndex
     }
 
     // Remove the user entry
-    if (!emberAfPluginDoorLockSetUser(endpointId, userIndex, kUndefinedFabricIndex, kUndefinedFabricIndex, chip::CharSpan(""), 0,
+    if (!emberAfPluginDoorLockSetUser(endpointId, userIndex, kUndefinedFabricIndex, kUndefinedFabricIndex, chip::CharSpan(), 0,
                                       UserStatusEnum::kAvailable, UserTypeEnum::kUnrestrictedUser, CredentialRuleEnum::kSingle,
                                       nullptr, 0))
     {

src/app/clusters/ota-requestor/DefaultOTARequestor.cpp

@@ -761,7 +761,7 @@ CHIP_ERROR DefaultOTARequestor::SendQueryImageRequest(Messaging::ExchangeManager
     else
     {
         // Country code unavailable or invalid, use default
-        args.location.SetValue(CharSpan("XX", strlen("XX")));
+        args.location.SetValue(CharSpan::fromCharString("XX"));
     }
 
     args.metadataForProvider = mMetadataForProvider;

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

@@ -186,6 +186,12 @@ 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 +209,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/app/tests/TestSceneTable.cpp

@@ -102,21 +102,21 @@ static const SceneStorageId sceneId12(kScene6, kGroup4);
 CharSpan empty;
 
 // Scene data
-static const SceneData sceneData1(CharSpan("Scene #1"));
-static const SceneData sceneData2(CharSpan("Scene #2"), 2000);
-static const SceneData sceneData3(CharSpan("Scene #3"), 250);
-static const SceneData sceneData4(CharSpan("Scene num4"), 5000);
+static const SceneData sceneData1(CharSpan::fromCharString("Scene #1"));
+static const SceneData sceneData2(CharSpan::fromCharString("Scene #2"), 2000);
+static const SceneData sceneData3(CharSpan::fromCharString("Scene #3"), 250);
+static const SceneData sceneData4(CharSpan::fromCharString("Scene num4"), 5000);
 static const SceneData sceneData5(empty);
-static const SceneData sceneData6(CharSpan("Scene #6"), 3000);
-static const SceneData sceneData7(CharSpan("Scene #7"), 20000);
-static const SceneData sceneData8(CharSpan("NAME TOO LOOONNG!"), 15000);
-static const SceneData sceneData9(CharSpan("Scene #9"), 3000);
-static const SceneData sceneData10(CharSpan("Scene #10"), 1000);
-static const SceneData sceneData11(CharSpan("Scene #11"), 50);
-static const SceneData sceneData12(CharSpan("Scene #12"), 100);
-static const SceneData sceneData13(CharSpan("Scene #13"), 100);
-static const SceneData sceneData14(CharSpan("Scene #14"), 100);
-static const SceneData sceneData15(CharSpan("Scene #15"), 100);
+static const SceneData sceneData6(CharSpan::fromCharString("Scene #6"), 3000);
+static const SceneData sceneData7(CharSpan::fromCharString("Scene #7"), 20000);
+static const SceneData sceneData8(CharSpan::fromCharString("NAME TOO LOOONNG!"), 15000);
+static const SceneData sceneData9(CharSpan::fromCharString("Scene #9"), 3000);
+static const SceneData sceneData10(CharSpan::fromCharString("Scene #10"), 1000);
+static const SceneData sceneData11(CharSpan::fromCharString("Scene #11"), 50);
+static const SceneData sceneData12(CharSpan::fromCharString("Scene #12"), 100);
+static const SceneData sceneData13(CharSpan::fromCharString("Scene #13"), 100);
+static const SceneData sceneData14(CharSpan::fromCharString("Scene #14"), 100);
+static const SceneData sceneData15(CharSpan::fromCharString("Scene #15"), 100);
 
 // Scenes
 SceneTableEntry scene1(sceneId1, sceneData1);

src/app/tests/TestTimeSyncDataProvider.cpp

@@ -118,9 +118,9 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
     char tzShort[]                             = "LA";
     char tzLong[]                              = "MunichOnTheLongRiverOfIsarInNiceSummerWeatherWithAugustinerBeer";
     char tzBerlin[]                            = "Berlin";
-    TimeSyncDataProvider::TimeZoneStore tzS[3] = { makeTimeZone(1, 1, tzShort, sizeof(tzShort)),
-                                                   makeTimeZone(2, 2, tzLong, sizeof(tzLong)),
-                                                   makeTimeZone(3, 3, tzBerlin, sizeof(tzBerlin)) };
+    TimeSyncDataProvider::TimeZoneStore tzS[3] = { makeTimeZone(1, 1, tzShort, sizeof(tzShort)-1),
+                                                   makeTimeZone(2, 2, tzLong, sizeof(tzLong)-1),
+                                                   makeTimeZone(3, 3, tzBerlin, sizeof(tzBerlin)-1) };
     TimeZoneList tzL(tzS);
     NL_TEST_ASSERT(inSuite, tzL.size() == 3);
     NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == timeSyncDataProv.StoreTimeZone(tzL));
@@ -141,7 +141,7 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
         NL_TEST_ASSERT(inSuite, tz.offset == 1);
         NL_TEST_ASSERT(inSuite, tz.validAt == 1);
         NL_TEST_ASSERT(inSuite, tz.name.HasValue());
-        NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 3);
+        NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 2);
 
         tzL = tzL.SubSpan(1);
     }
@@ -152,7 +152,7 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
         NL_TEST_ASSERT(inSuite, tz.offset == 2);
         NL_TEST_ASSERT(inSuite, tz.validAt == 2);
         NL_TEST_ASSERT(inSuite, tz.name.HasValue());
-        NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 64);
+        NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 63);
 
         tzL = tzL.SubSpan(1);
     }
@@ -163,7 +163,7 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
         NL_TEST_ASSERT(inSuite, tz.offset == 3);
         NL_TEST_ASSERT(inSuite, tz.validAt == 3);
         NL_TEST_ASSERT(inSuite, tz.name.HasValue());
-        NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 7);
+        NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 6);
 
         tzL = tzL.SubSpan(1);
     }

src/credentials/tests/TestFabricTable.cpp

@@ -943,7 +943,7 @@ void TestBasicAddNocUpdateNocFlow(nlTestSuite * inSuite, void * inContext)
     // Sequence 4: Rename fabric index 2, applies immediately when nothing pending
     {
         NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 2);
-        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SetFabricLabel(2, CharSpan("roboto")));
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.SetFabricLabel(2, CharSpan::fromCharString("roboto")));
         NL_TEST_ASSERT_EQUALS(inSuite, fabricTable.FabricCount(), 2);
 
         NL_TEST_ASSERT_EQUALS(inSuite, storage.GetNumKeys(), numStorageAfterUpdate); // Number of keys unchanged

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"