Skip to content

Commit 1253178

Browse files
bzbarsky-applepull[bot]
bzbarsky-apple
authored and
pull[bot]
committed
Disallow writes to readonly attributes via AttributeAccessInterface (#11942)
* Disallow writes to readonly attributes even when the writes are intercepted. Need to move the IsReadOnly() check higher up the callstack. * Mark attributes in test cluster writable as needed.
1 parent 229e18e commit 1253178

File tree

18 files changed

+177
-146
lines changed

18 files changed

+177
-146
lines changed

‎examples/chip-tool/templates/commands.zapt

+8
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,9 @@ private:
459459
};
460460

461461
{{#if isWritableAttribute}}
462+
{{! No list support for writing yet. Need to figure out how to represent the
463+
values. }}
464+
{{#unless isList}}
462465
class Write{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}: public ModelCommand
463466
{
464467
public:
@@ -494,6 +497,7 @@ private:
494497
{{chipType}} mValue;
495498
};
496499

500+
{{/unless}}
497501
{{/if}}
498502
{{#if isReportableAttribute}}
499503
class Report{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}: public ModelCommand
@@ -563,7 +567,11 @@ void registerCluster{{asUpperCamelCase name}}(Commands & commands)
563567
{{#chip_server_cluster_attributes}}
564568
make_unique<Read{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //
565569
{{#if isWritableAttribute}}
570+
{{! No list support for writing yet. Need to figure out how to
571+
represent the values. }}
572+
{{#unless isList}}
566573
make_unique<Write{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //
574+
{{/unless}}
567575
{{/if}}
568576
{{#if isReportableAttribute}}
569577
make_unique<Report{{asUpperCamelCase parent.name}}{{asUpperCamelCase name}}>(), //

‎src/app/util/af-types.h

+5
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,11 @@ struct EmberAfAttributeMetadata
217217
* Check whether this attribute is nullable.
218218
*/
219219
bool IsNullable() const { return mask & ATTRIBUTE_MASK_NULLABLE; }
220+
221+
/**
222+
* Check whether this attribute is readonly.
223+
*/
224+
bool IsReadOnly() const { return !(mask & ATTRIBUTE_MASK_WRITABLE); }
220225
};
221226

222227
/**

‎src/app/util/af.h

-7
Original file line numberDiff line numberDiff line change
@@ -373,13 +373,6 @@ uint8_t emberAfGetDataSize(uint8_t dataType);
373373
*/
374374
#define emberAfClusterIsManufacturerSpecific(cluster) ((cluster)->clusterId >= 0xFC00)
375375

376-
/**
377-
* @brief macro that returns true if attribute is read only.
378-
*
379-
* @param metadata EmberAfAttributeMetadata* to consider.
380-
*/
381-
#define emberAfAttributeIsReadOnly(metadata) (((metadata)->mask & ATTRIBUTE_MASK_WRITABLE) == 0)
382-
383376
/**
384377
* @brief macro that returns true if client attribute, and false if server.
385378
*

‎src/app/util/attribute-table.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -296,7 +296,7 @@ void emberAfPrintAttributeTable(void)
296296
emberAfAttributesPrint("%2x", mfgCode);
297297
}
298298
emberAfAttributesPrint(" / %x (%x) / %p / %p / ", metaData->attributeType, emberAfAttributeSize(metaData),
299-
(emberAfAttributeIsReadOnly(metaData) ? "RO" : "RW"),
299+
(metaData->IsReadOnly() ? "RO" : "RW"),
300300
(emberAfAttributeIsTokenized(metaData)
301301
? " token "
302302
: (emberAfAttributeIsExternal(metaData) ? "extern " : " RAM ")));
@@ -524,7 +524,7 @@ EmberAfStatus emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, Attribu
524524
return EMBER_ZCL_STATUS_INVALID_DATA_TYPE;
525525
}
526526

527-
if (emberAfAttributeIsReadOnly(metadata))
527+
if (metadata->IsReadOnly())
528528
{
529529
emberAfAttributesPrintln("%pattr not writable", "WRITE ERR: ");
530530
emberAfAttributesFlush();

‎src/app/util/ember-compatibility-functions.cpp

+27-22
Original file line numberDiff line numberDiff line change
@@ -542,61 +542,66 @@ CHIP_ERROR prepareWriteData(const EmberAfAttributeMetadata * metadata, TLV::TLVR
542542
}
543543
} // namespace
544544

545-
static Protocols::InteractionModel::Status WriteSingleClusterDataInternal(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader,
546-
WriteHandler * apWriteHandler)
545+
static Protocols::InteractionModel::Status WriteSingleClusterDataInternal(const ConcreteAttributePath aPath,
546+
const EmberAfAttributeMetadata * aMetadata,
547+
TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
547548
{
548-
// Passing nullptr as buf to emberAfReadAttribute means we only need attribute type here, and ember will not do data read &
549-
// copy in this case.
550-
const EmberAfAttributeMetadata * attributeMetadata = emberAfLocateAttributeMetadata(
551-
aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId, CLUSTER_MASK_SERVER, 0);
552-
553-
if (attributeMetadata == nullptr)
554-
{
555-
return Protocols::InteractionModel::Status::UnsupportedAttribute;
556-
}
557-
558549
CHIP_ERROR preparationError = CHIP_NO_ERROR;
559550
uint16_t dataLen = 0;
560-
if ((preparationError = prepareWriteData(attributeMetadata, aReader, dataLen)) != CHIP_NO_ERROR)
551+
if ((preparationError = prepareWriteData(aMetadata, aReader, dataLen)) != CHIP_NO_ERROR)
561552
{
562553
ChipLogDetail(Zcl, "Failed to prepare data to write: %s", ErrorStr(preparationError));
563554
return Protocols::InteractionModel::Status::InvalidValue;
564555
}
565556

566-
if (dataLen > attributeMetadata->size)
557+
if (dataLen > aMetadata->size)
567558
{
568559
ChipLogDetail(Zcl, "Data to write exceedes the attribute size claimed.");
569560
return Protocols::InteractionModel::Status::InvalidValue;
570561
}
571562

572-
return ToInteractionModelStatus(emberAfWriteAttributeExternal(aClusterInfo.mEndpointId, aClusterInfo.mClusterId,
573-
aClusterInfo.mAttributeId, CLUSTER_MASK_SERVER, 0, attributeData,
574-
attributeMetadata->attributeType));
563+
return ToInteractionModelStatus(emberAfWriteAttributeExternal(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId,
564+
CLUSTER_MASK_SERVER, 0, attributeData, aMetadata->attributeType));
575565
}
576566

577567
CHIP_ERROR WriteSingleClusterData(ClusterInfo & aClusterInfo, TLV::TLVReader & aReader, WriteHandler * apWriteHandler)
578568
{
569+
// TODO: Refactor WriteSingleClusterData and all dependent functions to take ConcreteAttributePath instead of ClusterInfo
570+
// as the input argument.
579571
AttributePathParams attributePathParams;
580572
attributePathParams.mEndpointId = aClusterInfo.mEndpointId;
581573
attributePathParams.mClusterId = aClusterInfo.mClusterId;
582574
attributePathParams.mAttributeId = aClusterInfo.mAttributeId;
583575

584-
// TODO: Refactor WriteSingleClusterData and all dependent functions to take ConcreteAttributePath instead of ClusterInfo
585-
// as the input argument.
576+
// Named aPath for now to reduce the amount of code change that needs to
577+
// happen when the above TODO is resolved.
578+
ConcreteAttributePath aPath(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId);
579+
const EmberAfAttributeMetadata * attributeMetadata =
580+
emberAfLocateAttributeMetadata(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId, CLUSTER_MASK_SERVER, 0);
581+
582+
if (attributeMetadata == nullptr)
583+
{
584+
return apWriteHandler->AddStatus(attributePathParams, Protocols::InteractionModel::Status::UnsupportedAttribute);
585+
}
586+
587+
if (attributeMetadata->IsReadOnly())
588+
{
589+
return apWriteHandler->AddStatus(attributePathParams, Protocols::InteractionModel::Status::UnsupportedWrite);
590+
}
591+
586592
AttributeAccessInterface * attrOverride = findAttributeAccessOverride(aClusterInfo.mEndpointId, aClusterInfo.mClusterId);
587593
if (attrOverride != nullptr)
588594
{
589-
ConcreteAttributePath path(aClusterInfo.mEndpointId, aClusterInfo.mClusterId, aClusterInfo.mAttributeId);
590595
AttributeValueDecoder valueDecoder(aReader);
591-
ReturnErrorOnFailure(attrOverride->Write(path, valueDecoder));
596+
ReturnErrorOnFailure(attrOverride->Write(aPath, valueDecoder));
592597

593598
if (valueDecoder.TriedDecode())
594599
{
595600
return apWriteHandler->AddStatus(attributePathParams, Protocols::InteractionModel::Status::Success);
596601
}
597602
}
598603

599-
auto imCode = WriteSingleClusterDataInternal(aClusterInfo, aReader, apWriteHandler);
604+
auto imCode = WriteSingleClusterDataInternal(aPath, attributeMetadata, aReader, apWriteHandler);
600605
return apWriteHandler->AddStatus(attributePathParams, imCode);
601606
}
602607

‎src/app/zap-templates/templates/app/CHIPClusters-src.zapt

+2
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::ReadAttribute{{asUpperCamelC
8383
}
8484

8585
{{#if isWritableAttribute}}
86+
{{#unless isList}}
8687
CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::WriteAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, {{chipType}} value)
8788
{
8889
app::WriteClientHandle handle;
@@ -91,6 +92,7 @@ CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::WriteAttribute{{asUpperCamel
9192
return mDevice->SendWriteAttributeRequest(std::move(handle), onSuccessCallback, onFailureCallback);
9293
}
9394

95+
{{/unless}}
9496
{{/if}}
9597
{{#if isReportableAttribute}}
9698
CHIP_ERROR {{asUpperCamelCase parent.name}}Cluster::SubscribeAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, uint16_t minInterval, uint16_t maxInterval)

‎src/app/zap-templates/templates/app/CHIPClusters.zapt

+2
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,9 @@ public:
3434
{{/chip_server_cluster_attributes}}
3535
{{#chip_server_cluster_attributes}}
3636
{{#if isWritableAttribute}}
37+
{{#unless isList}}
3738
CHIP_ERROR WriteAttribute{{asUpperCamelCase name}}(Callback::Cancelable * onSuccessCallback, Callback::Cancelable * onFailureCallback, {{chipType}} value);
39+
{{/unless}}
3840
{{/if}}
3941
{{/chip_server_cluster_attributes}}
4042
{{#chip_server_cluster_attributes}}

‎src/app/zap-templates/zcl/data-model/chip/test-cluster.xml

+3-3
Original file line numberDiff line numberDiff line change
@@ -124,9 +124,9 @@ limitations under the License.
124124
<!--<attribute side="server" code="0x0017" define="FLOAT_SINGLE" type="FLOAT_SINGLE" writable="true" default="0" optional="false">float_single</attribute>-->
125125
<!--<attribute side="server" code="0x0018" define="FLOAT_DOUBLE" type="FLOAT_DOUBLE" writable="true" default="0" optional="false">float_double</attribute>-->
126126
<attribute side="server" code="0x0019" define="OCTET_STRING" type="OCTET_STRING" length="10" writable="true" optional="false">octet_string</attribute>
127-
<attribute side="server" code="0x001A" define="LIST" type="ARRAY" entryType="INT8U" length="10" writable="false" optional="false">list_int8u</attribute>
128-
<attribute side="server" code="0x001B" define="LIST_OCTET_STRING" type="ARRAY" entryType="OCTET_STRING" length="254" writable="false" optional="false">list_octet_string</attribute>
129-
<attribute side="server" code="0x001C" define="LIST_STRUCT_OCTET_STRING" type="ARRAY" entryType="TestListStructOctet" length="254" writable="false" optional="false">list_struct_octet_string</attribute>
127+
<attribute side="server" code="0x001A" define="LIST" type="ARRAY" entryType="INT8U" length="10" writable="true" optional="false">list_int8u</attribute>
128+
<attribute side="server" code="0x001B" define="LIST_OCTET_STRING" type="ARRAY" entryType="OCTET_STRING" length="254" writable="true" optional="false">list_octet_string</attribute>
129+
<attribute side="server" code="0x001C" define="LIST_STRUCT_OCTET_STRING" type="ARRAY" entryType="TestListStructOctet" length="254" writable="true" optional="false">list_struct_octet_string</attribute>
130130
<!--<attribute side="server" code="0x001B" define="STRUCT" type="ARRAY" writable="true" optional="false">struct</attribute>-->
131131
<attribute side="server" code="0x001D" define="LONG_OCTET_STRING" type="LONG_OCTET_STRING" length="1000" writable="true" optional="false">long_octet_string</attribute>
132132
<attribute side="server" code="0x001E" define="CHAR_STRING" type="CHAR_STRING" length="10" writable="true" optional="false">char_string</attribute>

‎src/controller/java/templates/CHIPClusters-JNI.zapt

+3
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,8 @@ JNI_METHOD(void, {{asUpperCamelCase ../name}}Cluster, {{asLowerCamelCase name}})
7878
{{/chip_cluster_commands}}
7979
{{#chip_server_cluster_attributes}}
8080
{{#if isWritableAttribute}}
81+
{{! TODO: Lists not supported in attribute writes yet. }}
82+
{{#unless isList}}
8183

8284
JNI_METHOD(void, {{asUpperCamelCase ../name}}Cluster, write{{asUpperCamelCase name}}Attribute)(JNIEnv * env, jobject self, jlong clusterPtr, jobject callback, {{asJniBasicType type false}} value)
8385
{
@@ -106,6 +108,7 @@ JNI_METHOD(void, {{asUpperCamelCase ../name}}Cluster, write{{asUpperCamelCase na
106108
onSuccess.release();
107109
onFailure.release();
108110
}
111+
{{/unless}}
109112
{{/if}}
110113
{{#if isReportableAttribute}}
111114

‎src/controller/java/templates/ChipClusters-java.zapt

+6
Original file line numberDiff line numberDiff line change
@@ -204,10 +204,13 @@ public class ChipClusters {
204204
read{{asUpperCamelCase name}}Attribute(chipClusterPtr, callback);
205205
}
206206
{{#if isWritableAttribute}}
207+
{{! TODO: Lists not supported in attribute writes yet. }}
208+
{{#unless isList}}
207209

208210
public void write{{asUpperCamelCase name}}Attribute(DefaultClusterCallback callback, {{asJavaBasicType type}} value) {
209211
write{{asUpperCamelCase name}}Attribute(chipClusterPtr, callback, value);
210212
}
213+
{{/unless}}
211214
{{/if}}
212215
{{#if isReportableAttribute}}
213216

@@ -230,8 +233,11 @@ public class ChipClusters {
230233
{{/if}}
231234
);
232235
{{#if isWritableAttribute}}
236+
{{! TODO: Lists not supported in attribute writes yet. }}
237+
{{#unless isList}}
233238

234239
private native void write{{asUpperCamelCase name}}Attribute(long chipClusterPtr, DefaultClusterCallback callback, {{asJavaBasicType type}} value);
240+
{{/unless}}
235241
{{/if}}
236242
{{#if isReportableAttribute}}
237243

‎src/controller/java/templates/ClusterInfo-write-interaction.zapt

+2
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ public class ClusterWriteMapping {
1717
Map<String, InteractionInfo> write{{asUpperCamelCase name}}InteractionInfo = new LinkedHashMap<>();
1818
{{#chip_server_cluster_attributes}}
1919
{{#if isWritableAttribute}}
20+
{{#unless isList}}
2021
Map<String, CommandParameterInfo> write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}CommandParams = new LinkedHashMap<String, CommandParameterInfo>();
2122
CommandParameterInfo {{asLowerCamelCase ../name}}{{asLowerCamelCase name}}CommandParameterInfo = new CommandParameterInfo("value", {{asJavaBasicType type}}.class);
2223
write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}CommandParams.put("value",{{asLowerCamelCase ../name}}{{asLowerCamelCase name}}CommandParameterInfo);
@@ -32,6 +33,7 @@ public class ClusterWriteMapping {
3233
write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}CommandParams
3334
);
3435
write{{asUpperCamelCase ../name}}InteractionInfo.put("write{{asUpperCamelCase name}}Attribute", write{{asUpperCamelCase ../name}}{{asUpperCamelCase name}}AttributeInteractionInfo);
36+
{{/unless}}
3537
{{/if}}
3638
{{/chip_server_cluster_attributes}}
3739
writeAttributeMap.put("{{asLowerCamelCase name}}", write{{asUpperCamelCase name}}InteractionInfo);

‎src/controller/python/chip/clusters/CHIPClusters.py

+3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎src/darwin/Framework/CHIP/zap-generated/CHIPClustersObjc.h

+3
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)