From ce92c973c4312b010619191e283d67005d102756 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Fri, 15 Jul 2022 23:39:09 -0400 Subject: [PATCH] Implement updated spec rules related to CATs (#20776) * Implement updated spec rules related to CATs - CAT identifiers could previously collide, which made their use ambiguous for access conntrol - CAT tag class did not enforce strongly that all CATs are at the front and this is not required for correctness - CAT identifiers could be value 0 before Based on https://github.com/CHIP-Specifications/connectedhomeip-spec/pull/5437 we need to fix this. Fixes #20746 This PR: - Checks CATs are valid when adding/updating NOCs - Adds an `AreValid()` method to CATValues to ensure correctness Testing done: - Added unit tests for all new methods - Cert tests still pass * Apply review comment from @mstandstedt * Fix Darwin tests. * Restyle * Update access control to use CASEAuthTag methods * Rename variables to clarify logic * Fix another use of direct masks Co-authored-by: Boris Zbarsky --- src/access/AccessControl.cpp | 22 +++- .../operational-credentials-server.cpp | 4 + src/credentials/CHIPCert.cpp | 5 + .../Framework/CHIPTests/MTRCertificateTests.m | 66 +++++++++-- src/lib/core/CASEAuthTag.h | 109 +++++++++++++----- src/lib/core/tests/TestCATValues.cpp | 47 ++++++++ 6 files changed, 206 insertions(+), 47 deletions(-) diff --git a/src/access/AccessControl.cpp b/src/access/AccessControl.cpp index 22b8dab4a836c5..4a4691b81bd593 100644 --- a/src/access/AccessControl.cpp +++ b/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; diff --git a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp b/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp index 12f1a38c2d51ab..0894122eab8fb7 100644 --- a/src/app/clusters/operational-credentials-server/operational-credentials-server.cpp +++ b/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; diff --git a/src/credentials/CHIPCert.cpp b/src/credentials/CHIPCert.cpp index 3820920607bd25..a246558af8bf3a 100644 --- a/src/credentials/CHIPCert.cpp +++ b/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) @@ -1375,6 +1377,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; } diff --git a/src/darwin/Framework/CHIPTests/MTRCertificateTests.m b/src/darwin/Framework/CHIPTests/MTRCertificateTests.m index 106eaa7bcc716e..a62d7e4f40fc94 100644 --- a/src/darwin/Framework/CHIPTests/MTRCertificateTests.m +++ b/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); diff --git a/src/lib/core/CASEAuthTag.h b/src/lib/core/CASEAuthTag.h index b67e86709cb0b4..d83eaf5217ca1b 100644 --- a/src/lib/core/CASEAuthTag.h +++ b/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((aCAT & kTagIdentifierMask) >> kTagIdentifierShift); +} + +constexpr uint16_t GetCASEAuthTagVersion(CASEAuthTag aCAT) +{ + return static_cast(aCAT & kTagVersionMask); +} + struct CATValues { std::array 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((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((aCAT & kTagIdentifierMask) >> kTagIdentifierShift); -} - -constexpr uint16_t GetCASEAuthTagVersion(CASEAuthTag aCAT) -{ - return static_cast(aCAT & kTagVersionMask); -} - } // namespace chip diff --git a/src/lib/core/tests/TestCATValues.cpp b/src/lib/core/tests/TestCATValues.cpp index 95ab813f89e638..838d4cac10e97e 100644 --- a/src/lib/core/tests/TestCATValues.cpp +++ b/src/lib/core/tests/TestCATValues.cpp @@ -53,6 +53,22 @@ void TestEqualityOperator(nlTestSuite * inSuite, void * inContext) } } } + + { + auto a = CATValues{ { 0x1111'0001, kUndefinedCAT, 0x2222'0002 } }; + auto b = CATValues{ { 0x2222'0002, kUndefinedCAT, 0x1111'0001 } }; + auto c = CATValues{ { 0x1111'0001, 0x2222'0002 } }; + auto d = CATValues{ { 0x2222'0002, 0x1111'0001 } }; + CATValues candidates[] = { a, b, c, d }; + + for (auto & outer : candidates) + { + for (auto & inner : candidates) + { + NL_TEST_ASSERT(inSuite, inner == outer); + } + } + } } void TestInequalityOperator(nlTestSuite * inSuite, void * inContext) @@ -92,6 +108,33 @@ void TestInequalityOperator(nlTestSuite * inSuite, void * inContext) } } +void TestValidity(nlTestSuite * inSuite, void * inContext) +{ + { + auto a = CATValues{ { 0x1111'0001, 0x2222'0002, 0x3333'0003 } }; + auto b = CATValues{ { 0x1111'0001, 0x3333'0003, 0x2222'0002 } }; + auto c = CATValues{ { 0x2222'0002 } }; + auto d = CATValues{ { 0x2222'0002, 0x3333'0003 } }; + auto e = CATValues{ { 0x2222'0002, kUndefinedCAT, 0x3333'0003 } }; + CATValues validCandidates[] = { a, b, c, d, e }; + for (auto & candidate : validCandidates) + { + NL_TEST_ASSERT(inSuite, candidate.AreValid()); + } + } + + { + auto versionZero1 = CATValues{ { 0x1111'0000, 0x2222'0002, 0x3333'0003 } }; + auto versionZero2 = CATValues{ { 0x2222'0000 } }; + auto collidingId = CATValues{ { 0x1111'0001, 0x3333'0003, 0x1111'0002 } }; + CATValues invalidCandidates[] = { versionZero1, versionZero2, collidingId }; + for (auto & candidate : invalidCandidates) + { + NL_TEST_ASSERT(inSuite, !candidate.AreValid()); + } + } +} + void TestMembership(nlTestSuite * inSuite, void * inContext) { auto a = CATValues{ { 0x1111'0001 } }; @@ -104,12 +147,14 @@ void TestMembership(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, !a.Contains(0x2222'0002)); NL_TEST_ASSERT(inSuite, a.ContainsIdentifier(0x1111)); NL_TEST_ASSERT(inSuite, !a.ContainsIdentifier(0x2222)); + NL_TEST_ASSERT(inSuite, a.AreValid()); NL_TEST_ASSERT(inSuite, b.Contains(0x1111'0001)); NL_TEST_ASSERT(inSuite, b.Contains(0x2222'0002)); NL_TEST_ASSERT(inSuite, b.GetNumTagsPresent() == 2); NL_TEST_ASSERT(inSuite, b.ContainsIdentifier(0x1111)); NL_TEST_ASSERT(inSuite, b.ContainsIdentifier(0x2222)); + NL_TEST_ASSERT(inSuite, b.AreValid()); NL_TEST_ASSERT(inSuite, c.Contains(0x1111'0001)); NL_TEST_ASSERT(inSuite, c.Contains(0x2222'0002)); @@ -118,6 +163,7 @@ void TestMembership(nlTestSuite * inSuite, void * inContext) NL_TEST_ASSERT(inSuite, c.ContainsIdentifier(0x1111)); NL_TEST_ASSERT(inSuite, c.ContainsIdentifier(0x2222)); NL_TEST_ASSERT(inSuite, c.ContainsIdentifier(0x3333)); + NL_TEST_ASSERT(inSuite, c.AreValid()); } void TestSubjectMatching(nlTestSuite * inSuite, void * inContext) @@ -160,6 +206,7 @@ static const nlTest sTests[] = { NL_TEST_DEF("Equality operator", TestEqualityOperator), NL_TEST_DEF("Inequality operator", TestInequalityOperator), + NL_TEST_DEF("Validity checks", TestValidity), NL_TEST_DEF("Set operations", TestMembership), NL_TEST_DEF("Subject matching for ACL", TestSubjectMatching), NL_TEST_SENTINEL()