Skip to content

Commit cf45d5d

Browse files
Fix attr read overflows (project-chip#36003)
* Prevent Buffer Overflows with External Attribute Reads * Prevent buffer overflows by blocking creation of dynamic endpoints with attributes that would overflow the static attribute buffer * Added additional sanity checks to block reads of attributes that would overflow the attribute reads; return RESOURCE_EXHAUSTED instead * Fix style + minor compile issues * Fix include order * Address PR Comments
1 parent 2aaf2f8 commit cf45d5d

File tree

2 files changed

+41
-5
lines changed

2 files changed

+41
-5
lines changed

src/app/util/attribute-storage.cpp

+39-4
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424
#include <app/InteractionModelEngine.h>
2525
#include <app/reporting/reporting.h>
2626
#include <app/util/config.h>
27+
#include <app/util/ember-io-storage.h>
2728
#include <app/util/ember-strings.h>
2829
#include <app/util/endpoint-config-api.h>
2930
#include <app/util/generic-callbacks.h>
@@ -292,6 +293,30 @@ CHIP_ERROR emberAfSetDynamicEndpoint(uint16_t index, EndpointId id, const EmberA
292293
}
293294
}
294295

296+
const size_t bufferSize = Compatibility::Internal::gEmberAttributeIOBufferSpan.size();
297+
for (uint8_t i = 0; i < ep->clusterCount; i++)
298+
{
299+
const EmberAfCluster * cluster = &(ep->cluster[i]);
300+
if (!cluster->attributes)
301+
{
302+
continue;
303+
}
304+
305+
for (uint16_t j = 0; j < cluster->attributeCount; j++)
306+
{
307+
const EmberAfAttributeMetadata * attr = &(cluster->attributes[j]);
308+
uint16_t attrSize = emberAfAttributeSize(attr);
309+
if (attrSize > bufferSize)
310+
{
311+
ChipLogError(DataManagement,
312+
"Attribute size %u exceeds max size %lu, (attrId=" ChipLogFormatMEI ", clusterId=" ChipLogFormatMEI
313+
")",
314+
attrSize, static_cast<unsigned long>(bufferSize), ChipLogValueMEI(attr->attributeId),
315+
ChipLogValueMEI(cluster->clusterId));
316+
return CHIP_ERROR_NO_MEMORY;
317+
}
318+
}
319+
}
295320
emAfEndpoints[index].endpoint = id;
296321
emAfEndpoints[index].deviceTypeList = deviceTypeList;
297322
emAfEndpoints[index].endpointType = ep;
@@ -642,10 +667,20 @@ Status emAfReadOrWriteAttribute(const EmberAfAttributeSearchRecord * attRecord,
642667
// Is the attribute externally stored?
643668
if (am->mask & MATTER_ATTRIBUTE_FLAG_EXTERNAL_STORAGE)
644669
{
645-
return (write ? emberAfExternalAttributeWriteCallback(attRecord->endpoint, attRecord->clusterId,
646-
am, buffer)
647-
: emberAfExternalAttributeReadCallback(attRecord->endpoint, attRecord->clusterId,
648-
am, buffer, emberAfAttributeSize(am)));
670+
if (write)
671+
{
672+
return emberAfExternalAttributeWriteCallback(attRecord->endpoint, attRecord->clusterId, am,
673+
buffer);
674+
}
675+
676+
if (readLength < emberAfAttributeSize(am))
677+
{
678+
// Prevent a potential buffer overflow
679+
return Status::ResourceExhausted;
680+
}
681+
682+
return emberAfExternalAttributeReadCallback(attRecord->endpoint, attRecord->clusterId, am,
683+
buffer, emberAfAttributeSize(am));
649684
}
650685

651686
// Internal storage is only supported for fixed endpoints

src/controller/tests/TestServerCommandDispatch.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,8 @@ TEST_F(TestServerCommandDispatch, TestNoHandler)
215215
EXPECT_EQ(GetExchangeManager().GetNumActiveExchanges(), 0u);
216216
}
217217

218-
static const int kDescriptorAttributeArraySize = 254;
218+
// Use 8 so that we don't exceed the size of ATTRIBUTE_LARGEST defined by ZAP
219+
static const int kDescriptorAttributeArraySize = 8;
219220

220221
// Declare Descriptor cluster attributes
221222
DECLARE_DYNAMIC_ATTRIBUTE_LIST_BEGIN(descriptorAttrs)

0 commit comments

Comments
 (0)