From 106887050ce3e897350a8d9d720d75f9e7a48630 Mon Sep 17 00:00:00 2001 From: Andrei Litvin Date: Fri, 10 Nov 2023 11:51:17 -0500 Subject: [PATCH] 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 --- examples/chip-tool/args.gni | 4 ++++ .../static-supported-temperature-levels.cpp | 5 ++-- .../ota-requestor/DefaultOTARequestor.cpp | 2 +- src/app/clusters/scenes-server/SceneTable.h | 13 +++++------ .../clusters/scenes-server/SceneTableImpl.cpp | 1 + .../time-synchronization-server.cpp | 2 +- src/app/tests/TestTimeSyncDataProvider.cpp | 20 ++++++++-------- src/lib/core/BUILD.gn | 2 ++ src/lib/core/CHIPError.cpp | 6 +++++ src/lib/core/CHIPError.h | 19 +++++++++++++-- src/lib/core/TLVReader.cpp | 20 ++++++++++++++++ src/lib/core/TLVWriter.cpp | 23 ++++++++++++++++++- src/lib/core/TLVWriter.h | 2 +- src/lib/core/core.gni | 7 ++++++ src/lib/core/tests/TestCHIPErrorStr.cpp | 2 ++ 15 files changed, 103 insertions(+), 25 deletions(-) diff --git a/examples/chip-tool/args.gni b/examples/chip-tool/args.gni index a0b30a430f23f3..30f4000365e4ef 100644 --- 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 index 38a560e388f991..88ed689a6032a4 100644 --- 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 index 5f02c4df3f3dd5..213d9d64b8b5a4 100644 --- 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 index e7fb2328d9ed98..5c28ef8db9a0f3 100644 --- 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 index 7f5870f0ecf2b3..ed0a86ab282377 100644 --- 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 #include +#include #include #include @@ -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 index dd03742071afb4..60c27502bb0f3c 100644 --- 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 index 51db50cf14496a..fe7090007812dd 100644 --- 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 index 1b621a694fbc8c..ab0912c0ffb17e 100644 --- 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 index 7942b7f803bec9..9ef55edfe3a91f 100644 --- a/src/lib/core/TLVReader.cpp +++ b/src/lib/core/TLVReader.cpp @@ -32,6 +32,7 @@ #include #include #include +#include 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 index 5249e0b4789f7a..eb93e781524677 100644 --- a/src/lib/core/TLVWriter.cpp +++ b/src/lib/core/TLVWriter.cpp @@ -33,6 +33,7 @@ #include #include #include +#include #include #include @@ -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(buf), len); } -CHIP_ERROR TLVWriter::PutString(Tag tag, Span str) +CHIP_ERROR TLVWriter::PutString(Tag tag, CharSpan str) { if (!CanCastTo(str.size())) { diff --git a/src/lib/core/TLVWriter.h b/src/lib/core/TLVWriter.h index 9bbf8a6b4fbf32..59478f15e7123c 100644 --- 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 str); + CHIP_ERROR PutString(Tag tag, CharSpan str); /** * @brief diff --git a/src/lib/core/core.gni b/src/lib/core/core.gni index 839f9b6507b734..51ef44493b8461 100644 --- 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 index 3e9f41a30a4fdd..fec35d773d55bf 100644 --- 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,