Check for valid ids in command and write paths. (#29461)

Read/subscribe already had such checks (in AttributePathIB::Parser::ParsePath),
but commands and writes were missing them.

Fixes ACL test that was in fact using invalid cluster IDs as if they are valid.

Fixes TestCommandsById to use valid (but still unsupported) ids.

Fixes Darwin tests to use valid (but still unsupported) ids.

src/access/tests/TestAccessControl.cpp

@@ -259,15 +259,15 @@ constexpr ClusterId validClusters[] = {
     0x0001'FFFD,
     0x0001'FFFE, // end MS
 
-    0xFFFD'FC00, // start MS
-    0xFFFD'FC01,
-    0xFFFD'FFFD,
-    0xFFFD'FFFE, // end MS
-
-    0xFFFE'FC00, // start MS
-    0xFFFE'FC01,
-    0xFFFE'FFFD,
-    0xFFFE'FFFE, // end MS
+    0xFFF1'FC00, // start MS
+    0xFFF1'FC01,
+    0xFFF1'FFFD,
+    0xFFF1'FFFE, // end MS
+
+    0xFFF4'FC00, // start MS
+    0xFFF4'FC01,
+    0xFFF4'FFFD,
+    0xFFF4'FFFE, // end MS
 };
 // clang-format on
 
@@ -293,6 +293,30 @@ constexpr ClusterId invalidClusters[] = {
     0x0001'FBFF, // end unused
     0x0001'FFFF, // wildcard
 
+    0xFFF4'0000, // start std
+    0xFFF4'0001,
+    0xFFF4'7FFE,
+    0xFFF4'7FFF, // end std
+    0xFFF4'8000, // start unused
+    0xFFF4'8001,
+    0xFFF4'FBFE,
+    0xFFF4'FBFF, // end unused
+    0xFFF4'FFFF, // wildcard
+
+    0xFFFD'0000, // start std
+    0xFFFD'0001,
+    0xFFFD'7FFE,
+    0xFFFD'7FFF, // end std
+    0xFFFD'8000, // start unused
+    0xFFFD'8001,
+    0xFFFD'FBFE,
+    0xFFFD'FBFF, // end unused
+    0xFFFD'FC00, // start MS
+    0xFFFD'FC01,
+    0xFFFD'FFFD,
+    0xFFFD'FFFE, // end MS
+    0xFFFD'FFFF, // wildcard
+
     0xFFFE'0000, // start std
     0xFFFE'0001,
     0xFFFE'7FFE,
@@ -301,6 +325,10 @@ constexpr ClusterId invalidClusters[] = {
     0xFFFE'8001,
     0xFFFE'FBFE,
     0xFFFE'FBFF, // end unused
+    0xFFFE'FC00, // start MS
+    0xFFFE'FC01,
+    0xFFFE'FFFD,
+    0xFFFE'FFFE, // end MS
     0xFFFE'FFFF, // wildcard
 
     0xFFFF'0000, // start std

src/app/CommandHandler.cpp

@@ -256,13 +256,7 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem
     err = aCommandElement.GetPath(&commandPath);
     VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
 
-    err = commandPath.GetClusterId(&concretePath.mClusterId);
-    VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
-
-    err = commandPath.GetCommandId(&concretePath.mCommandId);
-    VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
-
-    err = commandPath.GetEndpointId(&concretePath.mEndpointId);
+    err = commandPath.GetConcreteCommandPath(concretePath);
     VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
 
     {
@@ -362,10 +356,7 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman
     err = aCommandElement.GetPath(&commandPath);
     VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
 
-    err = commandPath.GetClusterId(&clusterId);
-    VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
-
-    err = commandPath.GetCommandId(&commandId);
+    err = commandPath.GetGroupCommandPath(&clusterId, &commandId);
     VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction);
 
     groupId = mExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId();

src/app/MessageDef/AttributePathIB.cpp

@@ -174,7 +174,10 @@ CHIP_ERROR AttributePathIB::Parser::GetListIndex(DataModel::Nullable<ListIndex>
 CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const
 {
     ReturnErrorOnFailure(GetCluster(&aAttributePath.mClusterId));
+    VerifyOrReturnError(IsValidClusterId(aAttributePath.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
+
     ReturnErrorOnFailure(GetAttribute(&aAttributePath.mAttributeId));
+    VerifyOrReturnError(IsValidAttributeId(aAttributePath.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
 
     CHIP_ERROR err = CHIP_NO_ERROR;
     DataModel::Nullable<ListIndex> listIndex;

src/app/MessageDef/CommandPathIB.cpp

@@ -26,6 +26,8 @@
 
 #include <app/AppConfig.h>
 
+#include <protocols/interaction_model/Constants.h>
+
 using namespace chip;
 using namespace chip::TLV;
 
@@ -114,6 +116,24 @@ CHIP_ERROR CommandPathIB::Parser::GetCommandId(chip::CommandId * const apCommand
     return GetUnsignedInteger(to_underlying(Tag::kCommandId), apCommandId);
 }
 
+CHIP_ERROR CommandPathIB::Parser::GetConcreteCommandPath(ConcreteCommandPath & aCommandPath) const
+{
+    ReturnErrorOnFailure(GetGroupCommandPath(&aCommandPath.mClusterId, &aCommandPath.mCommandId));
+
+    return GetEndpointId(&aCommandPath.mEndpointId);
+}
+
+CHIP_ERROR CommandPathIB::Parser::GetGroupCommandPath(ClusterId * apClusterId, CommandId * apCommandId) const
+{
+    ReturnErrorOnFailure(GetClusterId(apClusterId));
+    VerifyOrReturnError(IsValidClusterId(*apClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
+
+    ReturnErrorOnFailure(GetCommandId(apCommandId));
+    VerifyOrReturnError(IsValidCommandId(*apCommandId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
+
+    return CHIP_NO_ERROR;
+}
+
 CommandPathIB::Builder & CommandPathIB::Builder::EndpointId(const chip::EndpointId aEndpointId)
 {
     // skip if error has already been set

src/app/MessageDef/CommandPathIB.h

@@ -79,6 +79,30 @@ class Parser : public ListParser
      *          #CHIP_END_OF_TLV if there is no such element
      */
     CHIP_ERROR GetCommandId(chip::CommandId * const apCommandId) const;
+
+    /**
+     * @brief Get the concrete command path, if this command path is a concrete
+     *        path.
+     *
+     * This will validate that the cluster id and command id are actually valid for a
+     * concrete path.
+     *
+     *  @param [in] aCommandPath    The command path object to write to.
+     */
+    CHIP_ERROR GetConcreteCommandPath(ConcreteCommandPath & aCommandPath) const;
+
+    /**
+     * @brief Get a group command path.
+     *
+     * This will validate that the cluster id and command id are actually valid for a
+     * group path.
+     *
+     *  @param [out] apClusterId    The cluster id in the path.
+     *  @param [out] apCommandId    The command id in the path.
+     *
+     *  @return #CHIP_NO_ERROR on success
+     */
+    CHIP_ERROR GetGroupCommandPath(ClusterId * apClusterId, CommandId * apCommandId) const;
 };
 
 class Builder : public ListBuilder

src/app/tests/suites/TestCommandsById.yaml

@@ -19,8 +19,8 @@ config:
     endpoint: 1
     cluster: "Unit Testing"
 
-    UnsupportedCluster: 0xFFF11FFF
-    UnsupportedCommand: 0xFFF11FFF
+    UnsupportedCluster: 0xFFF1FC00
+    UnsupportedCommand: 0xFFF100FF
     UnsupportedEndPoint: 254
 
     InvokeRequestMessage.Cluster: 0x00000006

src/darwin/Framework/CHIPTests/MTRDeviceTests.m

@@ -704,7 +704,7 @@ - (void)test008_InvokeCommandFailure
     [device
         invokeCommandWithEndpointID:@1
                           clusterID:@8
-                          commandID:@40000
+                          commandID:@(0xff)
                       commandFields:fields
                  timedInvokeTimeout:nil
                               queue:queue

src/darwin/Framework/CHIPTests/MTRXPCListenerSampleTests.m

@@ -875,7 +875,7 @@ - (void)test007_InvokeCommandFailure
     [device
         invokeCommandWithEndpointID:@1
                           clusterID:@8
-                          commandID:@40000
+                          commandID:@(0xff)
                       commandFields:fields
                  timedInvokeTimeout:nil
                               queue:queue

src/lib/core/DataModelTypes.h

@@ -73,6 +73,8 @@ static constexpr CommandId kInvalidCommandId     = 0xFFFF'FFFF;
 static constexpr EventId kInvalidEventId         = 0xFFFF'FFFF;
 static constexpr FieldId kInvalidFieldId         = 0xFFFF'FFFF;
 
+static constexpr uint16_t kMaxVendorId = VendorId::TestVendor4;
+
 static constexpr uint16_t ExtractIdFromMEI(uint32_t aMEI)
 {
     constexpr uint32_t kIdMask = 0x0000'FFFF;
@@ -86,6 +88,11 @@ static constexpr uint16_t ExtractVendorFromMEI(uint32_t aMEI)
     return static_cast<uint16_t>((aMEI & kVendorMask) >> kVendorShift);
 }
 
+static constexpr bool IsValidVendorId(uint16_t aVendorId)
+{
+    return aVendorId <= kMaxVendorId;
+}
+
 constexpr bool IsValidClusterId(ClusterId aClusterId)
 {
     const auto id     = ExtractIdFromMEI(aClusterId);
@@ -94,7 +101,7 @@ constexpr bool IsValidClusterId(ClusterId aClusterId)
     // cluster.
     //
     // Cluster id suffixes in the range 0xFC00 to 0xFFFE indicate an MS cluster.
-    return (vendor == 0x0000 && id <= 0x7FFF) || (vendor >= 0x0001 && vendor <= 0xFFFE && id >= 0xFC00 && id <= 0xFFFE);
+    return IsValidVendorId(vendor) && ((vendor == 0x0000 && id <= 0x7FFF) || (vendor >= 0x0001 && id >= 0xFC00 && id <= 0xFFFE));
 }
 
 constexpr bool IsGlobalAttribute(AttributeId aAttributeId)
@@ -113,7 +120,17 @@ constexpr bool IsValidAttributeId(AttributeId aAttributeId)
     // Attribute id suffixes in the range 0x0000 to 0x4FFF indicate a non-global
     // attribute (standard or MS).  The vendor id must not be wildcard in this
     // case.
-    return (id <= 0x4FFF && vendor != 0xFFFF) || IsGlobalAttribute(aAttributeId);
+    return (id <= 0x4FFF && IsValidVendorId(vendor)) || IsGlobalAttribute(aAttributeId);
+}
+
+constexpr bool IsValidCommandId(CommandId aCommandId)
+{
+    const auto id     = ExtractIdFromMEI(aCommandId);
+    const auto vendor = ExtractVendorFromMEI(aCommandId);
+
+    // Command id suffixes must be in the range 0x00 to 0xFF, and the prefix
+    // must be a valid prefix (standard or MC).
+    return id <= 0xFF && IsValidVendorId(vendor);
 }
 
 constexpr bool IsValidDeviceTypeId(DeviceTypeId aDeviceTypeId)