Skip to content

Commit

Permalink
Add missing validity checks to GroupKeyManagement cluster
Browse files Browse the repository at this point in the history
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 project-chip#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
  • Loading branch information
tennessee-google committed Jul 28, 2023
1 parent 5dd4b06 commit 0535763
Show file tree
Hide file tree
Showing 5 changed files with 430 additions and 41 deletions.
13 changes: 13 additions & 0 deletions src/app/CommandHandler.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -470,6 +470,19 @@ 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* message)
{
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), message);

}

return AddStatus(aCommandPath, aStatus);
}

CHIP_ERROR CommandHandler::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)
{
return AddStatusInternal(aCommandPath, StatusIB(Status::Success, aClusterStatus));
Expand Down
3 changes: 3 additions & 0 deletions src/app/CommandHandler.h
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,9 @@ 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 `aCommandName` failed with the given error status, on error.
CHIP_ERROR AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus, const char* aCommandName);

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

CHIP_ERROR AddClusterSpecificFailure(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);
Expand Down
161 changes: 123 additions & 38 deletions src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,106 @@ 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 +404,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 +427,7 @@ 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 +438,39 @@ 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 +488,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 0535763

Please sign in to comment.