Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Type fabric_idx is not used uniformly #11286

Closed
tcarmelveilleux opened this issue Nov 1, 2021 · 3 comments · Fixed by #19321
Closed

Type fabric_idx is not used uniformly #11286

tcarmelveilleux opened this issue Nov 1, 2021 · 3 comments · Fixed by #19321
Assignees
@tehampson @mlepage-google
Labels
acl Access Control feature deferral candidate security V1.0

Comments

@tcarmelveilleux
Copy link
Contributor

Problem

Many fabric-scoped structs use INT8U type instead of fabric_idx.

Proposed Solution

We need to audit and update all such uses to type fabric_idx which is already supported.
@tcarmelveilleux tcarmelveilleux added the acl Access Control feature label Nov 1, 2021
hubTab pushed a commit to hubTab/connectedhomeip that referenced this issue Nov 2, 2021
Rebased accessControlClustersAnnotations to upstream/master.
d65c45f 
operational-credentials-cluster.xml and onoff-cluster.xml based on received feedback:
    normalized the <access> elements. Note that the writeable <attribute> attribute indicates if a field is writeable, or not. The <access> elements qualifies provileges on a per operation basis.

operational-credentials-cluster.xml:
    updated all <struct> elements to include <item> fieldId;
    updated NodeOperationalCertStatus to include missing item elements;
    updated NOCStruct to include a missing item element;
    project-chip#11286: normalized fabric_idx usage across;
hubTab pushed a commit to hubTab/connectedhomeip that referenced this issue Nov 8, 2021
https://github.com/project-chip/connectedhomeip/issues/10246
7b25c6f 
project-chip#11204
project-chip#11286

Annotated the operational-credentials-cluster.xml and onoff-cluster.xml based on the Matter Specs, section 11.18, and appclusters, respectively.
Normalized the <access> elements. Note that the writeable <attribute> attribute indicates if a field is writeable, or not. The <access> elements qualifies provileges on a per operation basis.
    updated all <struct> elements to include <item> fieldId;
    updated NodeOperationalCertStatus to include missing item elements;
    updated NOCStruct to include a missing item element;
Normalized fabric_idx usage across the two updated cluster xml files.
@hubTab hubTab mentioned this issue Nov 8, 2021
Closed
@bzbarsky-apple
Copy link
Contributor

Because we use the type to detect fabric-scoped things, this is a critical security issue.

@andy31415 andy31415 added v1_triage_split_7 V1.0 acl Access Control feature and removed V1.0 critical security issue acl Access Control feature labels Jan 30, 2022
@tehampson tehampson self-assigned this Jun 7, 2022
@tehampson
Copy link
Contributor

I was asked to check if we have any INT8U in XML items that are for FabricIndex.

tl;dr other than some xmls in src/app/zap-templates/zcl/data-model/ which I will look into further it seems like all FabricIndex in xml is using fabric_idx

I did: find -name *.xml -not -path "./third_party/*" | xargs grep -i fabric.*index | grep -v type=\"fabric_idx\" in chip repo and got :

./src/app/zap-templates/zcl/data-model/chip/test-cluster.xml:    <item name="fabricIndex" type="INT64U"/>
./src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml:    <item name="InvalidFabricIndex" value="0x0b"/>
./src/app/zap-templates/zcl/data-model/chip/operational-credentials-cluster.xml:      <description>This command is used by Administrative Nodes to remove a given fabric index and delete all associated fabric-scoped data.</description>
./src/app/zap-templates/zcl/data-model/chip/binding-cluster.xml:    <item fieldId="0xFE" name="FabricIndex" type="FABRIC_IDX"/>
./src/app/zap-templates/zcl/data-model/chip/chip-types.xml:    <type id="0xF9" description="Fabric Index"             name="fabric_idx"        size="1"  discrete="true" />

@bzbarsky-apple
Copy link
Contributor

@tehampson The test-cluster bit needs to be fixed to use the right type and the right field id (0xFE).

The others all look good.

@tehampson tehampson mentioned this issue Jun 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@tehampson tehampson

@mlepage-google mlepage-google

Labels
acl Access Control feature deferral candidate security V1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update TestListStructOctet to use member names that are generic tehampson/connectedhomeip
7 participants
@tcarmelveilleux @andy31415 @tehampson @woody-apple @bzbarsky-apple @kpschoedel @mlepage-google