Skip to content

Commit

Permalink
Add missing validity checks to GroupKeyManagement cluster (#28379)
Browse files Browse the repository at this point in the history 
* Add missing validity checks to GroupKeyManagement cluster

Problem:
 - GroupKeyManagement cluster did not enforce length checks on
   EpochKey0/1/2
 - Some corner cases of checks were not covered.
 - Spec fixed in https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/7342
 - Fixes #28222

This PR:
 - Adds all the missing checks that spec updates imply
 - Adds tests for each of the individual checks
 - Adds improved logging for failures

Testing done:
 - New integration tests added
 - Integration tests pass

* Restyled by clang-format

* Restyled by prettier-yaml

* Add revision 2

* Fix revision 2 setting

* Address comments from @bzbarsky-apple

* Fix unit test

* Restyled by clang-format

* ZAP regen

---------

Co-authored-by: [email protected] <[email protected]>
Co-authored-by: Restyled.io <[email protected]>
  • Loading branch information
@tcarmelveilleux @tennessee-google
@restyled-commits @pull
3 people authored and pull[bot] committed Aug 15, 2023
1 parent c3f837d commit 3450283
Show file tree
Hide file tree
Showing 7 changed files with 2,092 additions and 435 deletions.
15 changes: 15 additions & 0 deletions src/app/CommandHandler.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,21 @@ CHIP_ERROR CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, c
return AddStatusInternal(aCommandPath, StatusIB(aStatus));
}

CHIP_ERROR CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Status aStatus,
const char * aMessage)
{
if (aStatus != Status::Success)
{
ChipLogError(DataManagement,
"Failed to handle on Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI
" with " ChipLogFormatIMStatus ": %s",
aCommandPath.mEndpointId, ChipLogValueMEI(aCommandPath.mClusterId), ChipLogValueMEI(aCommandPath.mCommandId),
ChipLogValueIMStatus(aStatus), aMessage);
}

return AddStatus(aCommandPath, aStatus);
}

CHIP_ERROR CommandHandler::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)
{
return AddStatusInternal(aCommandPath, StatusIB(Status::Success, aClusterStatus));
Expand Down
5 changes: 5 additions & 0 deletions src/app/CommandHandler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,11 @@ class CommandHandler : public Messaging::ExchangeDelegate
System::PacketBufferHandle && payload, bool isTimedInvoke);
CHIP_ERROR AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus);

// Same as AddStatus, but logs that the command represented by aCommandPath failed with the given
// error status and error message, if aStatus is an error.
CHIP_ERROR AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
const char * aMessage);

CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);

CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);
Expand Down
181 changes: 142 additions & 39 deletions src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,7 @@ class GroupKeyManagementAttributeAccess : public AttributeAccessInterface

// TODO: Once there is MCSP support, this may need to change.
static constexpr bool IsMCSPSupported() { return false; }
static constexpr uint16_t kImplementedClusterRevision = 2;

CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override
{
Expand All @@ -119,7 +120,7 @@ class GroupKeyManagementAttributeAccess : public AttributeAccessInterface
switch (aPath.mAttributeId)
{
case GroupKeyManagement::Attributes::ClusterRevision::Id:
return ReadClusterRevision(aPath.mEndpointId, aEncoder);
return aEncoder.Encode(kImplementedClusterRevision);
case Attributes::FeatureMap::Id: {
uint32_t features = 0;
if (IsMCSPSupported())
Expand Down Expand Up @@ -285,6 +286,121 @@ class GroupKeyManagementAttributeAccess : public AttributeAccessInterface
}
};

Status
ValidateKeySetWriteArguments(const chip::app::Clusters::GroupKeyManagement::Commands::KeySetWrite::DecodableType & commandData)
{
// SPEC: If the EpochKey0 field is null or its associated EpochStartTime0 field is null, then this command SHALL fail with an
// INVALID_COMMAND status code responded to the client.
if (commandData.groupKeySet.epochKey0.IsNull() || commandData.groupKeySet.epochStartTime0.IsNull())
{
return Status::InvalidCommand;
}

// SPEC: If the EpochStartTime0 is set to 0, then this command SHALL fail with an INVALID_COMMAND status code responded to the
// client.
if (0 == commandData.groupKeySet.epochStartTime0.Value())
{
return Status::InvalidCommand;
}

// By now we at least have epochKey0.
static_assert(GroupDataProvider::EpochKey::kLengthBytes == 16,
"Expect EpochKey internal data structure to have a length of 16 bytes.");

// SPEC: If the EpochKey0 field's length is not exactly 16 bytes, then this command SHALL fail with a CONSTRAINT_ERROR status
// code responded to the client.
if (commandData.groupKeySet.epochKey0.Value().size() != GroupDataProvider::EpochKey::kLengthBytes)
{
return Status::ConstraintError;
}

// Already known to be false by now
bool epoch_key0_is_null = false;
uint64_t epoch_start_time0 = commandData.groupKeySet.epochStartTime0.Value();

bool epoch_key1_is_null = commandData.groupKeySet.epochKey1.IsNull();
bool epoch_start_time1_is_null = commandData.groupKeySet.epochStartTime1.IsNull();

uint64_t epoch_start_time1 = 0; // Will be overridden when known to be present.

// SPEC: If exactly one of the EpochKey1 or EpochStartTime1 is null, rather than both being null, or neither being null, then
// this command SHALL fail with an INVALID_COMMAND status code responded to the client.
if (epoch_key1_is_null != epoch_start_time1_is_null)
{
return Status::InvalidCommand;
}

if (!epoch_key1_is_null)
{
// SPEC: If the EpochKey1 field is not null, then the EpochKey0 field SHALL NOT be null. Otherwise this command SHALL fail
// with an INVALID_COMMAND status code responded to the client.
if (epoch_key0_is_null)
{
return Status::InvalidCommand;
}

// SPEC: If the EpochKey1 field is not null, and the field's length is not exactly 16 bytes, then this command SHALL fail
// with a CONSTRAINT_ERROR status code responded to the client.
if (commandData.groupKeySet.epochKey1.Value().size() != GroupDataProvider::EpochKey::kLengthBytes)
{
return Status::ConstraintError;
}

// By now, if EpochKey1 was present, we know EpochStartTime1 was also present.
epoch_start_time1 = commandData.groupKeySet.epochStartTime1.Value();

// SPEC: If the EpochKey1 field is not null, its associated EpochStartTime1 field SHALL NOT be null and SHALL contain a
// later epoch start time than the epoch start time found in the EpochStartTime0 field. Otherwise this command SHALL fail
// with an INVALID_COMMAND status code responded to the client.
bool epoch1_later_than_epoch0 = epoch_start_time1 > epoch_start_time0;
if (!epoch1_later_than_epoch0)
{
return Status::InvalidCommand;
}
}

bool epoch_key2_is_null = commandData.groupKeySet.epochKey2.IsNull();
bool epoch_start_time2_is_null = commandData.groupKeySet.epochStartTime2.IsNull();

// SPEC: If exactly one of the EpochKey2 or EpochStartTime2 is null, rather than both being null, or neither being null, then
// this command SHALL fail with an INVALID_COMMAND status code responded to the client.
if (epoch_key2_is_null != epoch_start_time2_is_null)
{
return Status::InvalidCommand;
}

if (!epoch_key2_is_null)
{
// SPEC: If the EpochKey2 field is not null, then the EpochKey1 and EpochKey0 fields SHALL NOT be null. Otherwise this
// command SHALL fail with an INVALID_COMMAND status code responded to the client.
if (epoch_key0_is_null || epoch_key1_is_null)
{
return Status::InvalidCommand;
}

// SPEC: If the EpochKey2 field is not null, and the field's length is not exactly 16 bytes, then this command SHALL fail
// with a CONSTRAINT_ERROR status code responded to the client.
if (commandData.groupKeySet.epochKey2.Value().size() != GroupDataProvider::EpochKey::kLengthBytes)
{
return Status::ConstraintError;
}

// By now, if EpochKey2 was present, we know EpochStartTime2 was also present.
uint64_t epoch_start_time2 = commandData.groupKeySet.epochStartTime2.Value();

// SPEC: If the EpochKey2 field is not null, its associated EpochStartTime2 field SHALL NOT be null and SHALL contain a
// later epoch start time than the epoch start time found in the EpochStartTime1 field. Otherwise this command SHALL fail
// with an INVALID_COMMAND status code responded to the client.
bool epoch2_later_than_epoch1 = epoch_start_time2 > epoch_start_time1;
if (!epoch2_later_than_epoch1)
{
return Status::InvalidCommand;
}
}

return Status::Success;
}

constexpr uint16_t GroupKeyManagementAttributeAccess::kClusterRevision;

GroupKeyManagementAttributeAccess gAttribute;
Expand All @@ -304,12 +420,20 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
const chip::app::Clusters::GroupKeyManagement::Commands::KeySetWrite::DecodableType & commandData)
{
if (commandData.groupKeySet.epochKey0.IsNull() || commandData.groupKeySet.epochStartTime0.IsNull() ||
commandData.groupKeySet.epochKey0.Value().empty() || (0 == commandData.groupKeySet.epochStartTime0.Value()))
auto provider = GetGroupDataProvider();
auto fabric = Server::GetInstance().GetFabricTable().FindFabricWithIndex(commandObj->GetAccessingFabricIndex());

if (nullptr == provider || nullptr == fabric)
{
commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Internal consistency error on provider/fabric");
return true;
}

// Pre-validate all complex data dependency assumptions about the epoch keys
Status status = ValidateKeySetWriteArguments(commandData);
if (status != Status::Success)
{
// If the EpochKey0 field is null or its associated EpochStartTime0 field is null,
// then this command SHALL fail with an INVALID_COMMAND
commandObj->AddStatus(commandPath, Status::InvalidCommand);
commandObj->AddStatusAndLogIfFailure(commandPath, status, "Failure to validate KeySet data dependencies.");
return true;
}

Expand All @@ -319,7 +443,8 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
// supported by the server, because it is ... a new value unrecognized
// by a legacy server, then the server SHALL generate a general
// constraint error
commandObj->AddStatus(commandPath, Status::ConstraintError);
commandObj->AddStatusAndLogIfFailure(commandPath, Status::ConstraintError,
"Received unknown GroupKeySecurityPolicyEnum value");
return true;
}

Expand All @@ -330,62 +455,40 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
// any action attempting to set CacheAndSync in the
// GroupKeySecurityPolicy field SHALL fail with an INVALID_COMMAND
// error.
commandObj->AddStatus(commandPath, Status::InvalidCommand);
commandObj->AddStatusAndLogIfFailure(commandPath, Status::InvalidCommand,
"Received a CacheAndSync GroupKeySecurityPolicyEnum when MCSP not supported");
return true;
}

// All flight checks completed: by now we know that non-null keys are all valid and correct size.
bool epoch_key1_present = !commandData.groupKeySet.epochKey1.IsNull();
bool epoch_key2_present = !commandData.groupKeySet.epochKey2.IsNull();

GroupDataProvider::KeySet keyset(commandData.groupKeySet.groupKeySetID, commandData.groupKeySet.groupKeySecurityPolicy, 0);

// Epoch Key 0
// Epoch Key 0 always present
keyset.epoch_keys[0].start_time = commandData.groupKeySet.epochStartTime0.Value();
memcpy(keyset.epoch_keys[0].key, commandData.groupKeySet.epochKey0.Value().data(), GroupDataProvider::EpochKey::kLengthBytes);
keyset.num_keys_used++;

// Epoch Key 1
if (!commandData.groupKeySet.epochKey1.IsNull())
if (epoch_key1_present)
{
if (commandData.groupKeySet.epochStartTime1.IsNull() ||
commandData.groupKeySet.epochStartTime1.Value() <= commandData.groupKeySet.epochStartTime0.Value())
{
// If the EpochKey1 field is not null, its associated EpochStartTime1 field SHALL contain
// a later epoch start time than the epoch start time found in the EpochStartTime0 field.
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}
keyset.epoch_keys[1].start_time = commandData.groupKeySet.epochStartTime1.Value();
memcpy(keyset.epoch_keys[1].key, commandData.groupKeySet.epochKey1.Value().data(),
GroupDataProvider::EpochKey::kLengthBytes);
keyset.num_keys_used++;
}

// Epoch Key 2
if (!commandData.groupKeySet.epochKey2.IsNull())
if (epoch_key2_present)
{
if (commandData.groupKeySet.epochKey1.IsNull() || commandData.groupKeySet.epochStartTime2.IsNull() ||
commandData.groupKeySet.epochStartTime2.Value() <= commandData.groupKeySet.epochStartTime1.Value())
{
// If the EpochKey2 field is not null then:
// * The EpochKey1 field SHALL NOT be null
// * Its associated EpochStartTime1 field SHALL contain a later epoch start time
// than the epoch start time found in the EpochStartTime0 field.
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return true;
}
keyset.epoch_keys[2].start_time = commandData.groupKeySet.epochStartTime2.Value();
memcpy(keyset.epoch_keys[2].key, commandData.groupKeySet.epochKey2.Value().data(),
GroupDataProvider::EpochKey::kLengthBytes);
keyset.num_keys_used++;
}

auto provider = GetGroupDataProvider();
auto fabric = Server::GetInstance().GetFabricTable().FindFabricWithIndex(commandObj->GetAccessingFabricIndex());

if (nullptr == provider || nullptr == fabric)
{
commandObj->AddStatus(commandPath, Status::Failure);
return true;
}

uint8_t compressed_fabric_id_buffer[sizeof(uint64_t)];
MutableByteSpan compressed_fabric_id(compressed_fabric_id_buffer);
CHIP_ERROR err = fabric->GetCompressedFabricIdBytes(compressed_fabric_id);
Expand All @@ -403,7 +506,7 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
}
else
{
ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetWrite: %s", err.AsString());
ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetWrite: %" CHIP_ERROR_FORMAT, err.Format());
}

// Send response
Expand Down
Loading

0 comments on commit 3450283

Please sign in to comment.