Skip to content

Commit

Permalink
Disallow explicit PASE in access control (#14550)
Browse files Browse the repository at this point in the history 
* Disallow operational PASE in AccessControl::Check

We won't have explicit operational PASE ACL entries
for v1.0. We will enforce that PASE is only during
commissioning, therefore all PASE subjects will be
granted administer privilege.

Past v1.0, if/when we want operational PASE (requires
solving some tricky multi-fabric issues), we'll have
to check against PASE subjects in entries, and also
for implicite PASE administer privilege during
commissioning we'll have to verify that the incoming
PASE subject is commissioning (otherwise it should
not get that implicit privilege escalation).

Part of issue #10242

* Update unit tests

* Use unused variable in test code

To avoid warning about unused variable.
  • Loading branch information
@mlepage-google @pull
mlepage-google authored and pull[bot] committed Nov 14, 2023
1 parent e2d3f1a commit 2536f0b
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 56 deletions.
29 changes: 13 additions & 16 deletions src/access/AccessControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,9 @@ CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, con
// Don't check if using default delegate (e.g. test code that isn't testing access control)
ReturnErrorCodeIf(&mDelegate == &mDefaultDelegate, CHIP_NO_ERROR);

// Operational PASE not supported for v1.0, so PASE implies commissioning, which has highest privilege.
ReturnErrorCodeIf(subjectDescriptor.authMode == AuthMode::kPase, CHIP_NO_ERROR);

EntryIterator iterator;
ReturnErrorOnFailure(Entries(iterator, &subjectDescriptor.fabricIndex));

Expand All @@ -96,6 +99,8 @@ CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, con
{
AuthMode authMode = AuthMode::kNone;
ReturnErrorOnFailure(entry.GetAuthMode(authMode));
// Operational PASE not supported for v1.0.
VerifyOrReturnError(authMode == AuthMode::kCase || authMode == AuthMode::kGroup, CHIP_ERROR_INCORRECT_STATE);
if (authMode != subjectDescriptor.authMode)
{
continue;
Expand All @@ -119,43 +124,35 @@ CHIP_ERROR AccessControl::Check(const SubjectDescriptor & subjectDescriptor, con
ReturnErrorOnFailure(entry.GetSubject(i, subject));
if (IsOperationalNodeId(subject))
{
VerifyOrReturnError(authMode == AuthMode::kCase, CHIP_ERROR_INCORRECT_STATE);
if (subject == subjectDescriptor.subject)
{
subjectMatched = true;
break;
}
}
else if (IsGroupId(subject))
else if (IsCASEAuthTag(subject))
{
VerifyOrReturnError(authMode == AuthMode::kGroup, CHIP_ERROR_INVALID_ARGUMENT);
if (subject == subjectDescriptor.subject)
VerifyOrReturnError(authMode == AuthMode::kCase, CHIP_ERROR_INCORRECT_STATE);
if (subjectDescriptor.cats.CheckSubjectAgainstCATs(subject))
{
subjectMatched = true;
break;
}
}
// TODO: Add the implicit admit for PASE after the spec is updated.
else if (IsPAKEKeyId(subject))
else if (IsGroupId(subject))
{
VerifyOrReturnError(authMode == AuthMode::kPase, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(authMode == AuthMode::kGroup, CHIP_ERROR_INCORRECT_STATE);
if (subject == subjectDescriptor.subject)
{
subjectMatched = true;
break;
}
}
else if (IsCASEAuthTag(subject))
{
VerifyOrReturnError(authMode == AuthMode::kCase, CHIP_ERROR_INVALID_ARGUMENT);
if (subjectDescriptor.cats.CheckSubjectAgainstCATs(subject))
{
subjectMatched = true;
break;
}
}
else
{
return CHIP_ERROR_INVALID_ARGUMENT;
// Operational PASE not supported for v1.0.
return CHIP_ERROR_INCORRECT_STATE;
}
}
if (!subjectMatched)
Expand Down
71 changes: 31 additions & 40 deletions src/access/tests/TestAccessControl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ constexpr NodeId kOperationalNodeId1 = 0x1234567812345678;
constexpr NodeId kOperationalNodeId2 = 0x1122334455667788;
constexpr NodeId kOperationalNodeId3 = 0x1111111111111111;
constexpr NodeId kOperationalNodeId4 = 0x2222222222222222;
constexpr NodeId kOperationalNodeId5 = 0x3333333333333333;

constexpr CASEAuthTag kCASEAuthTag0 = 0x0001'0001;
constexpr CASEAuthTag kCASEAuthTag1 = 0x0002'0001;
Expand Down Expand Up @@ -322,8 +323,8 @@ constexpr EntryData entryData1[] = {
{
.fabricIndex = 2,
.privilege = Privilege::kManage,
.authMode = AuthMode::kPase,
.subjects = { kPaseVerifier1 },
.authMode = AuthMode::kCase,
.subjects = { kOperationalNodeId5 },
.targets = { { .flags = Target::kCluster | Target::kEndpoint, .cluster = kOnOffCluster, .endpoint = 2 } },
},
{
Expand All @@ -335,22 +336,19 @@ constexpr EntryData entryData1[] = {
{ .flags = Target::kCluster, .cluster = kOnOffCluster },
{ .flags = Target::kEndpoint, .endpoint = 2 } },
},
// entry 6
{
.fabricIndex = 1,
.privilege = Privilege::kAdminister,
.authMode = AuthMode::kCase,
.subjects = { kCASEAuthTagAsNodeId0 },
},
// entry 7
{
.fabricIndex = 2,
.privilege = Privilege::kManage,
.authMode = AuthMode::kCase,
.subjects = { kCASEAuthTagAsNodeId3, kCASEAuthTagAsNodeId1 },
.targets = { { .flags = Target::kCluster, .cluster = kOnOffCluster } },
},
// entry 8
{
.fabricIndex = 2,
.privilege = Privilege::kOperate,
Expand All @@ -371,6 +369,27 @@ struct CheckData
};

constexpr CheckData checkData1[] = {
// Checks for implicit PASE
{ .subjectDescriptor = { .fabricIndex = 0, .authMode = AuthMode::kPase, .subject = kPaseVerifier0 },
.requestPath = { .cluster = 1, .endpoint = 2 },
.privilege = Privilege::kAdminister,
.allow = true },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kPase, .subject = kPaseVerifier0 },
.requestPath = { .cluster = 3, .endpoint = 4 },
.privilege = Privilege::kAdminister,
.allow = true },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier0 },
.requestPath = { .cluster = 5, .endpoint = 6 },
.privilege = Privilege::kAdminister,
.allow = true },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier1 },
.requestPath = { .cluster = 5, .endpoint = 6 },
.privilege = Privilege::kAdminister,
.allow = true },
{ .subjectDescriptor = { .fabricIndex = 3, .authMode = AuthMode::kPase, .subject = kPaseVerifier3 },
.requestPath = { .cluster = 7, .endpoint = 8 },
.privilege = Privilege::kAdminister,
.allow = true },
// Checks for entry 0
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kCase, .subject = kOperationalNodeId3 },
.requestPath = { .cluster = kAccessControlCluster, .endpoint = 0 },
Expand All @@ -396,10 +415,6 @@ constexpr CheckData checkData1[] = {
.requestPath = { .cluster = 1, .endpoint = 2 },
.privilege = Privilege::kAdminister,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kPase, .subject = kOperationalNodeId3 },
.requestPath = { .cluster = 1, .endpoint = 2 },
.privilege = Privilege::kAdminister,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kGroup, .subject = kOperationalNodeId3 },
.requestPath = { .cluster = 1, .endpoint = 2 },
.privilege = Privilege::kAdminister,
Expand All @@ -421,10 +436,6 @@ constexpr CheckData checkData1[] = {
.requestPath = { .cluster = 11, .endpoint = 13 },
.privilege = Privilege::kView,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kPase, .subject = kOperationalNodeId1 },
.requestPath = { .cluster = 11, .endpoint = 13 },
.privilege = Privilege::kView,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kGroup, .subject = kOperationalNodeId1 },
.requestPath = { .cluster = 11, .endpoint = 13 },
.privilege = Privilege::kView,
Expand Down Expand Up @@ -454,10 +465,6 @@ constexpr CheckData checkData1[] = {
.requestPath = { .cluster = 1, .endpoint = 2 },
.privilege = Privilege::kAdminister,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kOperationalNodeId4 },
.requestPath = { .cluster = 1, .endpoint = 2 },
.privilege = Privilege::kAdminister,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kGroup, .subject = kOperationalNodeId4 },
.requestPath = { .cluster = 1, .endpoint = 2 },
.privilege = Privilege::kAdminister,
Expand All @@ -479,10 +486,6 @@ constexpr CheckData checkData1[] = {
.requestPath = { .cluster = kOnOffCluster, .endpoint = 11 },
.privilege = Privilege::kOperate,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kPase, .subject = kOperationalNodeId1 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 11 },
.privilege = Privilege::kOperate,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kCase, .subject = kOperationalNodeId1 },
.requestPath = { .cluster = 123, .endpoint = 11 },
.privilege = Privilege::kOperate,
Expand All @@ -492,39 +495,31 @@ constexpr CheckData checkData1[] = {
.privilege = Privilege::kManage,
.allow = false },
// Checks for entry 4
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier1 },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kCase, .subject = kOperationalNodeId5 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 2 },
.privilege = Privilege::kManage,
.allow = true },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kPase, .subject = kPaseVerifier1 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 2 },
.privilege = Privilege::kManage,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kCase, .subject = kPaseVerifier1 },
{ .subjectDescriptor = { .fabricIndex = 1, .authMode = AuthMode::kCase, .subject = kOperationalNodeId5 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 2 },
.privilege = Privilege::kManage,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kGroup, .subject = kPaseVerifier1 },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kGroup, .subject = kOperationalNodeId5 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 2 },
.privilege = Privilege::kManage,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier0 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 2 },
.privilege = Privilege::kManage,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier3 },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kCase, .subject = kOperationalNodeId3 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 2 },
.privilege = Privilege::kManage,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier1 },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kCase, .subject = kOperationalNodeId5 },
.requestPath = { .cluster = kLevelControlCluster, .endpoint = 2 },
.privilege = Privilege::kManage,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier1 },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kCase, .subject = kOperationalNodeId5 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 1 },
.privilege = Privilege::kManage,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kPaseVerifier1 },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kCase, .subject = kOperationalNodeId5 },
.requestPath = { .cluster = kOnOffCluster, .endpoint = 2 },
.privilege = Privilege::kAdminister,
.allow = false },
Expand All @@ -545,10 +540,6 @@ constexpr CheckData checkData1[] = {
.requestPath = { .cluster = kLevelControlCluster, .endpoint = 1 },
.privilege = Privilege::kProxyView,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kPase, .subject = kGroup2 },
.requestPath = { .cluster = kLevelControlCluster, .endpoint = 1 },
.privilege = Privilege::kProxyView,
.allow = false },
{ .subjectDescriptor = { .fabricIndex = 2, .authMode = AuthMode::kCase, .subject = kGroup2 },
.requestPath = { .cluster = kLevelControlCluster, .endpoint = 1 },
.privilege = Privilege::kProxyView,
Expand Down

0 comments on commit 2536f0b

Please sign in to comment.