Skip to content

Commit

Permalink
Implement updated spec rules related to CATs (#20776)
Browse files Browse the repository at this point in the history 
* 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 <[email protected]>
  • Loading branch information
@tcarmelveilleux @bzbarsky-apple @web-flow
2 people authored and web-flow committed Jul 16, 2022
1 parent f01a93f commit 4aca3f7
Show file tree
Hide file tree
Showing 6 changed files with 206 additions and 47 deletions.
22 changes: 16 additions & 6 deletions src/access/AccessControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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;

Expand Down
4 changes: 4 additions & 0 deletions src/app/clusters/operational-credentials-server/operational-credentials-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
5 changes: 5 additions & 0 deletions src/credentials/CHIPCert.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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;
}

Expand Down
66 changes: 57 additions & 9 deletions src/darwin/Framework/CHIPTests/MTRCertificateTests.m
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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);

Expand Down
109 changes: 77 additions & 32 deletions src/lib/core/CASEAuthTag.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 };
Expand Down Expand Up @@ -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`
*
Expand All @@ -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;
Expand All @@ -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;
}
Expand All @@ -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)
Expand Down Expand Up @@ -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
Loading

0 comments on commit 4aca3f7

Please sign in to comment.