Implement updated spec rules related to CATs

src/access/AccessControl.cpp

@@ -23,13 +23,16 @@
 
 #include "AccessControl.h"
 
-namespace {
+namespace chip {
+namespace Access {
 
 using chip::CATValues;
 using chip::FabricIndex;
 using chip::NodeId;
 using namespace chip::Access;
 
+namespace {
+
 AccessControl defaultAccessControl;
 AccessControl * globalAccessControl = &defaultAccessControl;
 
@@ -68,12 +71,22 @@ bool CheckRequestPrivilegeAgainstEntryPrivilege(Privilege requestPrivilege, Priv
 
 constexpr bool IsValidCaseNodeId(NodeId aNodeId)
 {
-    return chip::IsOperationalNodeId(aNodeId) || (chip::IsCASEAuthTag(aNodeId) && ((aNodeId & chip::kTagVersionMask) != 0));
+    if (IsOperationalNodeId(aNodeId))
+    {
+        return true;
+    }
+
+    if (IsCASEAuthTag(aNodeId) && (GetCASEAuthTagVersion(CASEAuthTagFromNodeId(aNodeId)) != 0))
+    {
+        return true;
+    }
+
+    return false;
 }
 
 constexpr bool IsValidGroupNodeId(NodeId aNodeId)
 {
-    return chip::IsGroupId(aNodeId) && chip::IsValidGroupId(chip::GroupIdFromNodeId(aNodeId));
+    return IsGroupId(aNodeId) && IsValidGroupId(GroupIdFromNodeId(aNodeId));
 }
 
 #if CHIP_PROGRESS_LOGGING && CHIP_CONFIG_ACCESS_CONTROL_POLICY_LOGGING_VERBOSITY > 1
@@ -161,9 +174,6 @@ char GetPrivilegeStringForLogging(Privilege privilege)
 
 } // namespace
 
-namespace chip {
-namespace Access {
-
 AccessControl::Entry::Delegate AccessControl::Entry::mDefaultDelegate;
 AccessControl::EntryIterator::Delegate AccessControl::EntryIterator::mDefaultDelegate;
 

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -564,6 +564,10 @@ OperationalCertStatus ConvertToNOCResponseStatus(CHIP_ERROR err)
     {
         return OperationalCertStatus::kInvalidNOC;
     }
+    if (err == CHIP_ERROR_WRONG_CERT_DN)
+    {
+        return OperationalCertStatus::kInvalidNOC;
+    }
     if (err == CHIP_ERROR_INCORRECT_STATE)
     {
         return OperationalCertStatus::kMissingCsr;

src/credentials/CHIPCert.cpp

@@ -655,6 +655,8 @@ CHIP_ERROR ChipDN::AddAttribute(chip::ASN1::OID oid, uint64_t val)
 
 CHIP_ERROR ChipDN::AddCATs(const chip::CATValues & cats)
 {
+    VerifyOrReturnError(cats.AreValid(), CHIP_ERROR_INVALID_ARGUMENT);
+
     for (auto & cat : cats.values)
     {
         if (cat != kUndefinedCAT)
@@ -1334,6 +1336,9 @@ CHIP_ERROR ExtractCATsFromOpCert(const ChipCertificateData & opcert, CATValues &
         cats.values[i] = kUndefinedCAT;
     }
 
+    // Make sure the set contained valid data, otherwise it's an invalid cert
+    VerifyOrReturnError(cats.AreValid(), CHIP_ERROR_WRONG_CERT_DN);
+
     return CHIP_NO_ERROR;
 }
 

src/darwin/Framework/CHIPTests/MTRCertificateTests.m

@@ -68,9 +68,10 @@ - (void)testGenerateOperationalCertNoIntermediate
     XCTAssertNotNil(operationalKeys);
 
     __auto_type * cats = [[NSMutableArray alloc] initWithCapacity:3];
-    [cats addObject:@1];
-    [cats addObject:@2];
-    [cats addObject:@3];
+    // High bits are identifier, low bits are version.
+    [cats addObject:@0x00010001];
+    [cats addObject:@0x00020001];
+    [cats addObject:@0x0003FFFF];
 
     __auto_type * operationalCert = [MTRCertificates generateOperationalCertificate:rootKeys
                                                                  signingCertificate:rootCert
@@ -125,11 +126,28 @@ - (void)testGenerateOperationalCertErrorCases
     __auto_type * operationalKeys = [[MTRTestKeys alloc] init];
     XCTAssertNotNil(operationalKeys);
 
-    __auto_type * cats = [[NSMutableArray alloc] initWithCapacity:4];
-    [cats addObject:@1];
-    [cats addObject:@2];
-    [cats addObject:@3];
-    [cats addObject:@4];
+    __auto_type * longCats = [[NSMutableArray alloc] initWithCapacity:4];
+    [longCats addObject:@0x00010001];
+    [longCats addObject:@0x00020001];
+    [longCats addObject:@0x00030001];
+    [longCats addObject:@0x00040001];
+
+    __auto_type * catsWithSameIdentifier = [[NSMutableArray alloc] initWithCapacity:3];
+    // High bits are identifier, low bits are version.
+    [catsWithSameIdentifier addObject:@0x00010001];
+    [catsWithSameIdentifier addObject:@0x00020001];
+    [catsWithSameIdentifier addObject:@0x00010002];
+
+    __auto_type * catsWithDuplicatedCAT = [[NSMutableArray alloc] initWithCapacity:3];
+    // High bits are identifier, low bits are version.
+    [catsWithDuplicatedCAT addObject:@0x00010001];
+    [catsWithDuplicatedCAT addObject:@0x00020001];
+    [catsWithDuplicatedCAT addObject:@0x00010001];
+
+    __auto_type * catsWithInvalidVersion = [[NSMutableArray alloc] initWithCapacity:2];
+    // High bits are identifier, low bits are version.
+    [catsWithInvalidVersion addObject:@0x00010001];
+    [catsWithInvalidVersion addObject:@0x00020000];
 
     // Check basic case works
     __auto_type * operationalCert = [MTRCertificates generateOperationalCertificate:rootKeys
@@ -147,7 +165,37 @@ - (void)testGenerateOperationalCertErrorCases
                                                  operationalPublicKey:operationalKeys.publicKey
                                                              fabricId:@1
                                                                nodeId:@1
-                                                caseAuthenticatedTags:cats
+                                                caseAuthenticatedTags:longCats
+                                                                error:nil];
+    XCTAssertNil(operationalCert);
+
+    // Multiple CATs with the same identifier but different versions
+    operationalCert = [MTRCertificates generateOperationalCertificate:rootKeys
+                                                   signingCertificate:rootCert
+                                                 operationalPublicKey:operationalKeys.publicKey
+                                                             fabricId:@1
+                                                               nodeId:@1
+                                                caseAuthenticatedTags:catsWithSameIdentifier
+                                                                error:nil];
+    XCTAssertNil(operationalCert);
+
+    // Multiple CATs with the same identifier and same version
+    operationalCert = [MTRCertificates generateOperationalCertificate:rootKeys
+                                                   signingCertificate:rootCert
+                                                 operationalPublicKey:operationalKeys.publicKey
+                                                             fabricId:@1
+                                                               nodeId:@1
+                                                caseAuthenticatedTags:catsWithDuplicatedCAT
+                                                                error:nil];
+    XCTAssertNil(operationalCert);
+
+    // CAT with invalid version
+    operationalCert = [MTRCertificates generateOperationalCertificate:rootKeys
+                                                   signingCertificate:rootCert
+                                                 operationalPublicKey:operationalKeys.publicKey
+                                                             fabricId:@1
+                                                               nodeId:@1
+                                                caseAuthenticatedTags:catsWithInvalidVersion
                                                                 error:nil];
     XCTAssertNil(operationalCert);
 

src/lib/core/CASEAuthTag.h

@@ -36,6 +36,31 @@ static constexpr NodeId kTagVersionMask       = 0x0000'0000'0000'FFFFULL;
 // Maximum number of CASE Authenticated Tags (CAT) in the CHIP certificate subject.
 static constexpr size_t kMaxSubjectCATAttributeCount = CHIP_CONFIG_CERT_MAX_RDN_ATTRIBUTES - 2;
 
+constexpr NodeId NodeIdFromCASEAuthTag(CASEAuthTag aCAT)
+{
+    return kMinCASEAuthTag | aCAT;
+}
+
+constexpr CASEAuthTag CASEAuthTagFromNodeId(NodeId aNodeId)
+{
+    return aNodeId & kMaskCASEAuthTag;
+}
+
+constexpr bool IsValidCASEAuthTag(CASEAuthTag aCAT)
+{
+    return (aCAT & kTagVersionMask) > 0;
+}
+
+constexpr uint16_t GetCASEAuthTagIdentifier(CASEAuthTag aCAT)
+{
+    return static_cast<uint16_t>((aCAT & kTagIdentifierMask) >> kTagIdentifierShift);
+}
+
+constexpr uint16_t GetCASEAuthTagVersion(CASEAuthTag aCAT)
+{
+    return static_cast<uint16_t>(aCAT & kTagVersionMask);
+}
+
 struct CATValues
 {
     std::array<CASEAuthTag, kMaxSubjectCATAttributeCount> values = { kUndefinedCAT };
@@ -73,6 +98,45 @@ struct CATValues
         return false;
     }
 
+    bool AreValid() const
+    {
+        for (size_t idx = 0; idx < size(); ++idx)
+        {
+            const auto & candidate = values[idx];
+            if (candidate == kUndefinedCAT)
+            {
+                continue;
+            }
+
+            // Every entry that is not empty must have version > 0
+            if (!IsValidCASEAuthTag(candidate))
+            {
+                return false;
+            }
+            // Identifiers cannot collide in set (there cannot be more than 1 version of an identifier)
+            for (size_t other_idx = 0; other_idx < size(); ++other_idx)
+            {
+                if (idx == other_idx)
+                {
+                    continue;
+                }
+                if (values[other_idx] == kUndefinedCAT)
+                {
+                    continue;
+                }
+
+                uint16_t other_identifier     = GetCASEAuthTagIdentifier(values[other_idx]);
+                uint16_t candidate_identifier = GetCASEAuthTagIdentifier(candidate);
+                if (other_identifier == candidate_identifier)
+                {
+                    return false;
+                }
+            }
+        }
+
+        return true;
+    }
+
     /**
      * @brief Returns true if this set contains any version of the `identifier`
      *
@@ -83,7 +147,7 @@ struct CATValues
     {
         for (auto candidate : values)
         {
-            uint16_t candidate_identifier = static_cast<uint16_t>((candidate & kTagIdentifierMask) >> kTagIdentifierShift);
+            uint16_t candidate_identifier = GetCASEAuthTagIdentifier(candidate);
             if ((candidate != kUndefinedCAT) && (identifier == candidate_identifier))
             {
                 return true;
@@ -98,13 +162,14 @@ struct CATValues
     bool CheckSubjectAgainstCATs(NodeId subject) const
     {
         VerifyOrReturnError(IsCASEAuthTag(subject), false);
+        CASEAuthTag catFromSubject = CASEAuthTagFromNodeId(subject);
 
-        for (auto cat : values)
+        for (auto catFromNoc : values)
         {
-            // All valid CAT values are always in the beginning of the array followed by kUndefinedCAT values.
-            ReturnErrorCodeIf(cat == kUndefinedCAT, false);
-            if (((cat & kTagIdentifierMask) == (subject & kTagIdentifierMask)) &&
-                ((cat & kTagVersionMask) >= (subject & kTagVersionMask)))
+            if ((catFromNoc != kUndefinedCAT) &&
+                (GetCASEAuthTagIdentifier(catFromNoc) == GetCASEAuthTagIdentifier(catFromSubject)) &&
+                (GetCASEAuthTagVersion(catFromSubject) > 0) &&
+                (GetCASEAuthTagVersion(catFromNoc) >= GetCASEAuthTagVersion(catFromSubject)))
             {
                 return true;
             }
@@ -114,12 +179,17 @@ struct CATValues
 
     bool operator==(const CATValues & other) const
     {
-        // Two sets of CATs confer equal permissions if the sets are exactly equal.
+        // Two sets of CATs confer equal permissions if the sets are exactly equal
+        // and the sets are valid.
         // Ignoring kUndefinedCAT values, evaluate this.
         if (this->GetNumTagsPresent() != other.GetNumTagsPresent())
         {
             return false;
         }
+        if (!this->AreValid() || !other.AreValid())
+        {
+            return false;
+        }
         for (auto cat : this->values)
         {
             if (cat == kUndefinedCAT)
@@ -162,29 +232,4 @@ struct CATValues
 
 static constexpr CATValues kUndefinedCATs = { { kUndefinedCAT } };
 
-constexpr NodeId NodeIdFromCASEAuthTag(CASEAuthTag aCAT)
-{
-    return kMinCASEAuthTag | aCAT;
-}
-
-constexpr CASEAuthTag CASEAuthTagFromNodeId(NodeId aNodeId)
-{
-    return aNodeId & kMaskCASEAuthTag;
-}
-
-constexpr bool IsValidCASEAuthTag(CASEAuthTag aCAT)
-{
-    return (aCAT & kTagVersionMask) > 0;
-}
-
-constexpr uint16_t GetCASEAuthTagIdentifier(CASEAuthTag aCAT)
-{
-    return static_cast<uint16_t>((aCAT & kTagIdentifierMask) >> kTagIdentifierShift);
-}
-
-constexpr uint16_t GetCASEAuthTagVersion(CASEAuthTag aCAT)
-{
-    return static_cast<uint16_t>(aCAT & kTagVersionMask);
-}
-
 } // namespace chip