Enforce valid spec-conformant string encoding in TLV (#30393)

* Add some validation configs and at least read-time implementation for this * Add TLVWriter implementation too * Add includes and implementation * make sure timezone is properly initialized to not send 64 bytes over the wire * make chip-tool always be strict * Fix some unit tests, scenes still broken * More unit test updates. Scenes pass now * Use fromCharString more * Fix more unit tests * Undo a setter that was not useful * Remove redundant operator == * More unit test cleanup and fixes * Restyle * mNameLength for scenes seems not a lie after all ... make code better * Use v on read for size instead of len --------- Co-authored-by: Andrei Litvin <[email protected]>
project-chip · Dec 7, 2023 · 1068870 · 1068870
1 parent d049755
commit 1068870
Show file tree

Hide file tree

Showing 15 changed files with 103 additions and 25 deletions.
diff --git a/examples/chip-tool/args.gni b/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
diff --git a/examples/placeholder/linux/static-supported-temperature-levels.cpp b/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] = {

diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp b/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;

diff --git a/src/app/clusters/scenes-server/SceneTable.h b/src/app/clusters/scenes-server/SceneTable.h
@@ -186,6 +186,12 @@ class SceneTable
         }
         ~SceneData(){};
 
+        bool operator==(const SceneData & other) const
+        {
+            return ((CharSpan(mName, mNameLength).data_equal(CharSpan(other.mName, other.mNameLength))) &&
+                    (mSceneTransitionTimeMs == other.mSceneTransitionTimeMs) && (mExtensionFieldSets == other.mExtensionFieldSets));
+        }
+
         void SetName(const CharSpan & sceneName)
         {
             if (nullptr == sceneName.data())
@@ -204,17 +210,10 @@ class SceneTable
         void Clear()
         {
             SetName(CharSpan());
-
             mSceneTransitionTimeMs = 0;
             mExtensionFieldSets.Clear();
         }
 
-        bool operator==(const SceneData & other)
-        {
-            return (mNameLength == other.mNameLength && !memcmp(mName, other.mName, mNameLength) &&
-                    (mSceneTransitionTimeMs == other.mSceneTransitionTimeMs) && (mExtensionFieldSets == other.mExtensionFieldSets));
-        }
-
         void operator=(const SceneData & other)
         {
             SetName(CharSpan(other.mName, other.mNameLength));

diff --git a/src/app/clusters/scenes-server/SceneTableImpl.cpp b/src/app/clusters/scenes-server/SceneTableImpl.cpp
@@ -209,6 +209,7 @@ struct SceneTableData : public SceneTableEntry, PersistentData<kPersistentSceneB
 
     CHIP_ERROR Serialize(TLV::TLVWriter & writer) const override
     {
+        // TODO: if we have mNameLength, should this bin ByteSpan instead?
         CharSpan nameSpan(mStorageData.mName, mStorageData.mNameLength);
         TLV::TLVType container;
         ReturnErrorOnFailure(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, container));

diff --git a/src/app/clusters/time-synchronization-server/time-synchronization-server.cpp b/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)) };
     }
 }
 

diff --git a/src/app/tests/TestTimeSyncDataProvider.cpp b/src/app/tests/TestTimeSyncDataProvider.cpp
@@ -17,6 +17,7 @@
 
 #include <app/clusters/time-synchronization-server/TimeSyncDataProvider.h>
 #include <lib/core/CHIPPersistentStorageDelegate.h>
+#include <lib/support/CHIPMemString.h>
 #include <lib/support/TestPersistentStorageDelegate.h>
 #include <lib/support/UnitTestRegistration.h>
 
@@ -104,23 +105,22 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
     TimeSyncDataProvider timeSyncDataProv;
     timeSyncDataProv.Init(persistentStorage);
 
-    const auto makeTimeZone = [](int32_t offset = 0, uint64_t validAt = 0, char * name = nullptr, uint8_t len = 0) {
+    const auto makeTimeZone = [](int32_t offset = 0, uint64_t validAt = 0, char * name = nullptr) {
         TimeSyncDataProvider::TimeZoneStore tzS;
         tzS.timeZone.offset  = offset;
         tzS.timeZone.validAt = validAt;
-        if (name != nullptr && len)
+        if (name != nullptr)
         {
-            memcpy(tzS.name, name, len);
-            tzS.timeZone.name.SetValue(chip::CharSpan(tzS.name, len));
+            Platform::CopyString(tzS.name, name);
+            tzS.timeZone.name.SetValue(chip::CharSpan::fromCharString(tzS.name));
         }
         return tzS;
     };
     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), makeTimeZone(2, 2, tzLong),
+                                                   makeTimeZone(3, 3, tzBerlin) };
     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);
     }

diff --git a/src/lib/core/BUILD.gn b/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}",
   ]
 }
 

diff --git a/src/lib/core/CHIPError.cpp b/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;

diff --git a/src/lib/core/CHIPError.h b/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
 
 /**

diff --git a/src/lib/core/TLVReader.cpp b/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 (!v.empty() && (v.back() == 0))
+    {
+        return CHIP_ERROR_INVALID_TLV_CHAR_STRING;
+    }
+#endif // CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ
     return CHIP_NO_ERROR;
 }
 

diff --git a/src/lib/core/TLVWriter.cpp b/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()))
     {

diff --git a/src/lib/core/TLVWriter.h b/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

diff --git a/src/lib/core/core.gni b/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 == "") {

diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/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,