Skip to content

Commit 8d8d8a2

Browse files
andy31415andreilitvin
andy31415
and
andreilitvin
authored
Enforce valid spec-conformant string encoding in TLV (project-chip#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 <andreilitvin@google.com>
1 parent bc5ef90 commit 8d8d8a2

15 files changed

+103
-25
lines changed

‎examples/chip-tool/args.gni

+4
Original file line numberDiff line numberDiff line change
@@ -27,3 +27,7 @@ matter_enable_tracing_support = true
2727

2828
matter_log_json_payload_hex = true
2929
matter_log_json_payload_decode_full = true
30+
31+
# make chip-tool very strict by default
32+
chip_tlv_validate_char_string_on_read = true
33+
chip_tlv_validate_char_string_on_write = true

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

+3-2
Original file line numberDiff line numberDiff line change
@@ -27,8 +27,9 @@ using namespace chip::app::Clusters::TemperatureControl;
2727
using chip::Protocols::InteractionModel::Status;
2828

2929
// TODO: Configure your options for each endpoint
30-
CharSpan AppSupportedTemperatureLevelsDelegate::temperatureLevelOptions[] = { CharSpan("Hot", 3), CharSpan("Warm", 4),
31-
CharSpan("Cold", 4) };
30+
CharSpan AppSupportedTemperatureLevelsDelegate::temperatureLevelOptions[] = { CharSpan::fromCharString("Hot"),
31+
CharSpan::fromCharString("Warm"),
32+
CharSpan::fromCharString("Cold") };
3233

3334
const AppSupportedTemperatureLevelsDelegate::EndpointPair AppSupportedTemperatureLevelsDelegate::supportedOptionsByEndpoints
3435
[EMBER_AF_TEMPERATURE_CONTROL_CLUSTER_SERVER_ENDPOINT_COUNT] = {

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -761,7 +761,7 @@ CHIP_ERROR DefaultOTARequestor::SendQueryImageRequest(Messaging::ExchangeManager
761761
else
762762
{
763763
// Country code unavailable or invalid, use default
764-
args.location.SetValue(CharSpan("XX", strlen("XX")));
764+
args.location.SetValue(CharSpan::fromCharString("XX"));
765765
}
766766

767767
args.metadataForProvider = mMetadataForProvider;

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

+6-7
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,12 @@ class SceneTable
186186
}
187187
~SceneData(){};
188188

189+
bool operator==(const SceneData & other) const
190+
{
191+
return ((CharSpan(mName, mNameLength).data_equal(CharSpan(other.mName, other.mNameLength))) &&
192+
(mSceneTransitionTimeMs == other.mSceneTransitionTimeMs) && (mExtensionFieldSets == other.mExtensionFieldSets));
193+
}
194+
189195
void SetName(const CharSpan & sceneName)
190196
{
191197
if (nullptr == sceneName.data())
@@ -204,17 +210,10 @@ class SceneTable
204210
void Clear()
205211
{
206212
SetName(CharSpan());
207-
208213
mSceneTransitionTimeMs = 0;
209214
mExtensionFieldSets.Clear();
210215
}
211216

212-
bool operator==(const SceneData & other)
213-
{
214-
return (mNameLength == other.mNameLength && !memcmp(mName, other.mName, mNameLength) &&
215-
(mSceneTransitionTimeMs == other.mSceneTransitionTimeMs) && (mExtensionFieldSets == other.mExtensionFieldSets));
216-
}
217-
218217
void operator=(const SceneData & other)
219218
{
220219
SetName(CharSpan(other.mName, other.mNameLength));

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

+1
Original file line numberDiff line numberDiff line change
@@ -209,6 +209,7 @@ struct SceneTableData : public SceneTableEntry, PersistentData<kPersistentSceneB
209209

210210
CHIP_ERROR Serialize(TLV::TLVWriter & writer) const override
211211
{
212+
// TODO: if we have mNameLength, should this bin ByteSpan instead?
212213
CharSpan nameSpan(mStorageData.mName, mStorageData.mNameLength);
213214
TLV::TLVType container;
214215
ReturnErrorOnFailure(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, container));

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -551,7 +551,7 @@ void TimeSynchronizationServer::InitTimeZone()
551551
for (auto & tzStore : mTimeZoneObj.timeZoneList)
552552
{
553553
memset(tzStore.name, 0, sizeof(tzStore.name));
554-
tzStore.timeZone = { .offset = 0, .validAt = 0, .name = MakeOptional(CharSpan(tzStore.name, sizeof(tzStore.name))) };
554+
tzStore.timeZone = { .offset = 0, .validAt = 0, .name = MakeOptional(CharSpan(tzStore.name, 0)) };
555555
}
556556
}
557557

‎src/app/tests/TestTimeSyncDataProvider.cpp

+10-10
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
#include <app/clusters/time-synchronization-server/TimeSyncDataProvider.h>
1919
#include <lib/core/CHIPPersistentStorageDelegate.h>
20+
#include <lib/support/CHIPMemString.h>
2021
#include <lib/support/TestPersistentStorageDelegate.h>
2122
#include <lib/support/UnitTestRegistration.h>
2223

@@ -104,23 +105,22 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
104105
TimeSyncDataProvider timeSyncDataProv;
105106
timeSyncDataProv.Init(persistentStorage);
106107

107-
const auto makeTimeZone = [](int32_t offset = 0, uint64_t validAt = 0, char * name = nullptr, uint8_t len = 0) {
108+
const auto makeTimeZone = [](int32_t offset = 0, uint64_t validAt = 0, char * name = nullptr) {
108109
TimeSyncDataProvider::TimeZoneStore tzS;
109110
tzS.timeZone.offset = offset;
110111
tzS.timeZone.validAt = validAt;
111-
if (name != nullptr && len)
112+
if (name != nullptr)
112113
{
113-
memcpy(tzS.name, name, len);
114-
tzS.timeZone.name.SetValue(chip::CharSpan(tzS.name, len));
114+
Platform::CopyString(tzS.name, name);
115+
tzS.timeZone.name.SetValue(chip::CharSpan::fromCharString(tzS.name));
115116
}
116117
return tzS;
117118
};
118119
char tzShort[] = "LA";
119120
char tzLong[] = "MunichOnTheLongRiverOfIsarInNiceSummerWeatherWithAugustinerBeer";
120121
char tzBerlin[] = "Berlin";
121-
TimeSyncDataProvider::TimeZoneStore tzS[3] = { makeTimeZone(1, 1, tzShort, sizeof(tzShort)),
122-
makeTimeZone(2, 2, tzLong, sizeof(tzLong)),
123-
makeTimeZone(3, 3, tzBerlin, sizeof(tzBerlin)) };
122+
TimeSyncDataProvider::TimeZoneStore tzS[3] = { makeTimeZone(1, 1, tzShort), makeTimeZone(2, 2, tzLong),
123+
makeTimeZone(3, 3, tzBerlin) };
124124
TimeZoneList tzL(tzS);
125125
NL_TEST_ASSERT(inSuite, tzL.size() == 3);
126126
NL_TEST_ASSERT(inSuite, CHIP_NO_ERROR == timeSyncDataProv.StoreTimeZone(tzL));
@@ -141,7 +141,7 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
141141
NL_TEST_ASSERT(inSuite, tz.offset == 1);
142142
NL_TEST_ASSERT(inSuite, tz.validAt == 1);
143143
NL_TEST_ASSERT(inSuite, tz.name.HasValue());
144-
NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 3);
144+
NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 2);
145145

146146
tzL = tzL.SubSpan(1);
147147
}
@@ -152,7 +152,7 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
152152
NL_TEST_ASSERT(inSuite, tz.offset == 2);
153153
NL_TEST_ASSERT(inSuite, tz.validAt == 2);
154154
NL_TEST_ASSERT(inSuite, tz.name.HasValue());
155-
NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 64);
155+
NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 63);
156156

157157
tzL = tzL.SubSpan(1);
158158
}
@@ -163,7 +163,7 @@ void TestTimeZoneStoreLoad(nlTestSuite * inSuite, void * inContext)
163163
NL_TEST_ASSERT(inSuite, tz.offset == 3);
164164
NL_TEST_ASSERT(inSuite, tz.validAt == 3);
165165
NL_TEST_ASSERT(inSuite, tz.name.HasValue());
166-
NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 7);
166+
NL_TEST_ASSERT(inSuite, tz.name.Value().size() == 6);
167167

168168
tzL = tzL.SubSpan(1);
169169
}

‎src/lib/core/BUILD.gn

+2
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,8 @@ buildconfig_header("chip_buildconfig") {
6666
"CHIP_CONFIG_MINMDNS_MAX_PARALLEL_RESOLVES=${chip_config_minmdns_max_parallel_resolves}",
6767
"CHIP_CONFIG_CANCELABLE_HAS_INFO_STRING_FIELD=${chip_config_cancelable_has_info_string_field}",
6868
"CHIP_CONFIG_BIG_ENDIAN_TARGET=${chip_target_is_big_endian}",
69+
"CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_WRITE=${chip_tlv_validate_char_string_on_write}",
70+
"CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ=${chip_tlv_validate_char_string_on_read}",
6971
]
7072
}
7173

‎src/lib/core/CHIPError.cpp

+6
Original file line numberDiff line numberDiff line change
@@ -113,12 +113,18 @@ bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err)
113113
case CHIP_ERROR_WRONG_ENCRYPTION_TYPE.AsInteger():
114114
desc = "Wrong encryption type";
115115
break;
116+
case CHIP_ERROR_INVALID_UTF8.AsInteger():
117+
desc = "Character string is not a valid utf-8 encoding";
118+
break;
116119
case CHIP_ERROR_INTEGRITY_CHECK_FAILED.AsInteger():
117120
desc = "Integrity check failed";
118121
break;
119122
case CHIP_ERROR_INVALID_SIGNATURE.AsInteger():
120123
desc = "Invalid signature";
121124
break;
125+
case CHIP_ERROR_INVALID_TLV_CHAR_STRING.AsInteger():
126+
desc = "Invalid TLV Char string encoding.";
127+
break;
122128
case CHIP_ERROR_UNSUPPORTED_SIGNATURE_TYPE.AsInteger():
123129
desc = "Unsupported signature type";
124130
break;

‎src/lib/core/CHIPError.h

+17-2
Original file line numberDiff line numberDiff line change
@@ -581,7 +581,14 @@ using CHIP_ERROR = ::chip::ChipError;
581581
*/
582582
#define CHIP_ERROR_WRONG_ENCRYPTION_TYPE CHIP_CORE_ERROR(0x11)
583583

584-
// AVAILABLE: 0x12
584+
/**
585+
* @def CHIP_ERROR_INVALID_UTF8
586+
*
587+
* @brief
588+
* Invalid UTF8 string (contains some characters that are invalid)
589+
*
590+
*/
591+
#define CHIP_ERROR_INVALID_UTF8 CHIP_CORE_ERROR(0x12)
585592

586593
/**
587594
* @def CHIP_ERROR_INTEGRITY_CHECK_FAILED
@@ -602,7 +609,15 @@ using CHIP_ERROR = ::chip::ChipError;
602609
*/
603610
#define CHIP_ERROR_INVALID_SIGNATURE CHIP_CORE_ERROR(0x14)
604611

605-
// AVAILABLE: 0x15
612+
/**
613+
* @def CHIP_ERROR_INVALID_TLV_CHAR_STRING
614+
*
615+
* @brief
616+
* Invalid TLV character string (e.g. null terminator)
617+
*
618+
*/
619+
#define CHIP_ERROR_INVALID_TLV_CHAR_STRING CHIP_CORE_ERROR(0x15)
620+
606621
// AVAILABLE: 0x16
607622

608623
/**

‎src/lib/core/TLVReader.cpp

+20
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@
3232
#include <lib/support/CHIPMem.h>
3333
#include <lib/support/CodeUtils.h>
3434
#include <lib/support/SafeInt.h>
35+
#include <lib/support/utf8.h>
3536

3637
namespace chip {
3738
namespace TLV {
@@ -330,6 +331,25 @@ CHIP_ERROR TLVReader::Get(CharSpan & v) const
330331
}
331332

332333
v = CharSpan(Uint8::to_const_char(bytes), len);
334+
#if CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ
335+
// Spec requirement: A.11.2. UTF-8 and Octet Strings
336+
//
337+
// For UTF-8 strings, the value octets SHALL encode a valid
338+
// UTF-8 character (code points) sequence.
339+
//
340+
// Senders SHALL NOT include a terminating null character to
341+
// mark the end of a string.
342+
343+
if (!Utf8::IsValid(v))
344+
{
345+
return CHIP_ERROR_INVALID_UTF8;
346+
}
347+
348+
if (!v.empty() && (v.back() == 0))
349+
{
350+
return CHIP_ERROR_INVALID_TLV_CHAR_STRING;
351+
}
352+
#endif // CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ
333353
return CHIP_NO_ERROR;
334354
}
335355

‎src/lib/core/TLVWriter.cpp

+22-1
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
#include <lib/support/CHIPMem.h>
3434
#include <lib/support/CodeUtils.h>
3535
#include <lib/support/SafeInt.h>
36+
#include <lib/support/utf8.h>
3637

3738
#include <stdarg.h>
3839
#include <stdint.h>
@@ -252,10 +253,30 @@ CHIP_ERROR TLVWriter::PutString(Tag tag, const char * buf)
252253

253254
CHIP_ERROR TLVWriter::PutString(Tag tag, const char * buf, uint32_t len)
254255
{
256+
#if CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_WRITE
257+
// Spec requirement: A.11.2. UTF-8 and Octet Strings
258+
//
259+
// For UTF-8 strings, the value octets SHALL encode a valid
260+
// UTF-8 character (code points) sequence.
261+
//
262+
// Senders SHALL NOT include a terminating null character to
263+
// mark the end of a string.
264+
265+
if (!Utf8::IsValid(CharSpan(buf, len)))
266+
{
267+
return CHIP_ERROR_INVALID_UTF8;
268+
}
269+
270+
if ((len > 0) && (buf[len - 1] == 0))
271+
{
272+
return CHIP_ERROR_INVALID_TLV_CHAR_STRING;
273+
}
274+
#endif // CHIP_CONFIG_TLV_VALIDATE_CHAR_STRING_ON_READ
275+
255276
return WriteElementWithData(kTLVType_UTF8String, tag, reinterpret_cast<const uint8_t *>(buf), len);
256277
}
257278

258-
CHIP_ERROR TLVWriter::PutString(Tag tag, Span<const char> str)
279+
CHIP_ERROR TLVWriter::PutString(Tag tag, CharSpan str)
259280
{
260281
if (!CanCastTo<uint32_t>(str.size()))
261282
{

‎src/lib/core/TLVWriter.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -573,7 +573,7 @@ class DLL_EXPORT TLVWriter
573573
* TLVBackingStore.
574574
*
575575
*/
576-
CHIP_ERROR PutString(Tag tag, Span<const char> str);
576+
CHIP_ERROR PutString(Tag tag, CharSpan str);
577577

578578
/**
579579
* @brief

‎src/lib/core/core.gni

+7
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,13 @@ declare_args() {
9191

9292
# Whether the target architecture is big-endian (true) or little-endian (false).
9393
chip_target_is_big_endian = false
94+
95+
# Validation on TLV encode/decode for string validity of character
96+
# strings:
97+
# - SHALL NOT end with binary 0 (spec still allows embedded 0)
98+
# - SHALL be valid UTF8
99+
chip_tlv_validate_char_string_on_write = true
100+
chip_tlv_validate_char_string_on_read = false
94101
}
95102

96103
if (chip_target_style == "") {

‎src/lib/core/tests/TestCHIPErrorStr.cpp

+2
Original file line numberDiff line numberDiff line change
@@ -67,8 +67,10 @@ static const CHIP_ERROR kTestElements[] =
6767
CHIP_ERROR_UNKNOWN_KEY_TYPE,
6868
CHIP_ERROR_KEY_NOT_FOUND,
6969
CHIP_ERROR_WRONG_ENCRYPTION_TYPE,
70+
CHIP_ERROR_INVALID_UTF8,
7071
CHIP_ERROR_INTEGRITY_CHECK_FAILED,
7172
CHIP_ERROR_INVALID_SIGNATURE,
73+
CHIP_ERROR_INVALID_TLV_CHAR_STRING,
7274
CHIP_ERROR_UNSUPPORTED_SIGNATURE_TYPE,
7375
CHIP_ERROR_INVALID_MESSAGE_LENGTH,
7476
CHIP_ERROR_BUFFER_TOO_SMALL,

0 commit comments

Comments
 (0)