Skip to content

Commit

Permalink
Revert "[nrf fromtree] Do proper ACL checks on event reads/subscripti…
Browse files Browse the repository at this point in the history 
…ons. (project-chip#26761)"

This reverts commit 03fb17e.
  • Loading branch information
@kkasperczyk-no
kkasperczyk-no committed Nov 9, 2023
1 parent 565705b commit bc16468
Show file tree
Hide file tree
Showing 18 changed files with 125 additions and 547 deletions.
176 changes: 17 additions & 159 deletions src/app/InteractionModelEngine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,11 @@
#include "access/RequestPath.h"
#include "access/SubjectDescriptor.h"
#include <app/RequiredPrivilege.h>
#include <app/util/af-types.h>
#include <app/util/endpoint-config-api.h>
#include <lib/core/TLVUtilities.h>
#include <lib/support/CodeUtils.h>

extern bool emberAfContainsAttribute(chip::EndpointId endpoint, chip::ClusterId clusterId, chip::AttributeId attributeId);

namespace chip {
namespace app {

Expand Down Expand Up @@ -351,8 +351,10 @@ CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDesc
aHasValidAttributePath = false;
aRequestedAttributePathCount = 0;

while (CHIP_NO_ERROR == (err = pathReader.Next(TLV::AnonymousTag())))
while (CHIP_NO_ERROR == (err = pathReader.Next()))
{
VerifyOrReturnError(TLV::AnonymousTag() == pathReader.GetTag(), CHIP_ERROR_INVALID_TLV_TAG);

AttributePathIB::Parser path;
//
// We create an iterator to point to a single item object list that tracks the path we just parsed.
Expand Down Expand Up @@ -411,152 +413,6 @@ CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDesc
return err;
}

static bool CanAccess(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteClusterPath & aPath,
Access::Privilege aNeededPrivilege)
{
Access::RequestPath requestPath{ .cluster = aPath.mClusterId, .endpoint = aPath.mEndpointId };
CHIP_ERROR err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath, aNeededPrivilege);
return (err == CHIP_NO_ERROR);
}

static bool CanAccess(const Access::SubjectDescriptor & aSubjectDescriptor, const ConcreteEventPath & aPath)
{
return CanAccess(aSubjectDescriptor, aPath, RequiredPrivilege::ForReadEvent(aPath));
}

/**
* Helper to handle wildcard events in the event path.
*/
static bool HasValidEventPathForEndpointAndCluster(EndpointId aEndpoint, const EmberAfCluster * aCluster,
const EventPathParams & aEventPath,
const Access::SubjectDescriptor & aSubjectDescriptor)
{
if (aEventPath.HasWildcardEventId())
{
#if CHIP_CONFIG_ENABLE_EVENTLIST_ATTRIBUTE
for (decltype(aCluster->eventCount) idx = 0; idx > aCluster->eventCount; ++idx)
{
ConcreteEventPath path(aEndpoint, aCluster->clusterId, aCluster->eventList[idx]);
// If we get here, the path exists. We just have to do an ACL check for it.
bool isValid = CanAccess(aSubjectDescriptor, path);
if (isValid)
{
return true;
}
}

return false;
#else
// We have no way to expand wildcards. Just assume that we would need
// View permissions for whatever events are involved.
ConcreteClusterPath clusterPath(aEndpoint, aCluster->clusterId);
return CanAccess(aSubjectDescriptor, clusterPath, Access::Privilege::kView);
#endif
}

ConcreteEventPath path(aEndpoint, aCluster->clusterId, aEventPath.mEventId);
if (CheckEventSupportStatus(path) != Status::Success)
{
// Not an existing event path.
return false;
}
return CanAccess(aSubjectDescriptor, path);
}

/**
* Helper to handle wildcard clusters in the event path.
*/
static bool HasValidEventPathForEndpoint(EndpointId aEndpoint, const EventPathParams & aEventPath,
const Access::SubjectDescriptor & aSubjectDescriptor)
{
if (aEventPath.HasWildcardClusterId())
{
auto * endpointType = emberAfFindEndpointType(aEndpoint);
if (endpointType == nullptr)
{
// Not going to have any valid paths in here.
return false;
}

for (decltype(endpointType->clusterCount) idx = 0; idx < endpointType->clusterCount; ++idx)
{
bool hasValidPath =
HasValidEventPathForEndpointAndCluster(aEndpoint, &endpointType->cluster[idx], aEventPath, aSubjectDescriptor);
if (hasValidPath)
{
return true;
}
}

return false;
}

auto * cluster = emberAfFindServerCluster(aEndpoint, aEventPath.mClusterId);
if (cluster == nullptr)
{
// Nothing valid here.
return false;
}
return HasValidEventPathForEndpointAndCluster(aEndpoint, cluster, aEventPath, aSubjectDescriptor);
}

CHIP_ERROR InteractionModelEngine::ParseEventPaths(const Access::SubjectDescriptor & aSubjectDescriptor,
EventPathIBs::Parser & aEventPathListParser, bool & aHasValidEventPath,
size_t & aRequestedEventPathCount)
{
TLV::TLVReader pathReader;
aEventPathListParser.GetReader(&pathReader);
CHIP_ERROR err = CHIP_NO_ERROR;

aHasValidEventPath = false;
aRequestedEventPathCount = 0;

while (CHIP_NO_ERROR == (err = pathReader.Next(TLV::AnonymousTag())))
{
EventPathIB::Parser path;
ReturnErrorOnFailure(path.Init(pathReader));

EventPathParams eventPath;
ReturnErrorOnFailure(path.ParsePath(eventPath));

++aRequestedEventPathCount;

if (aHasValidEventPath)
{
// Can skip all the rest of the checking.
continue;
}

// The definition of "valid path" is "path exists and ACL allows
// access". We need to do some expansion of wildcards to handle that.
if (eventPath.HasWildcardEndpointId())
{
for (uint16_t endpointIndex = 0; !aHasValidEventPath && endpointIndex < emberAfEndpointCount(); ++endpointIndex)
{
if (!emberAfEndpointIndexIsEnabled(endpointIndex))
{
continue;
}
aHasValidEventPath =
HasValidEventPathForEndpoint(emberAfEndpointFromIndex(endpointIndex), eventPath, aSubjectDescriptor);
}
}
else
{
// No need to check whether the endpoint is enabled, because
// emberAfFindEndpointType returns null for disabled endpoints.
aHasValidEventPath = HasValidEventPathForEndpoint(eventPath.mEndpointId, eventPath, aSubjectDescriptor);
}
}

if (err == CHIP_ERROR_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}

return err;
}

Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeContext * apExchangeContext,
const PayloadHeader & aPayloadHeader,
System::PacketBufferHandle && aPayload,
Expand Down Expand Up @@ -616,7 +472,6 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
size_t requestedEventPathCount = 0;
AttributePathIBs::Parser attributePathListParser;
bool hasValidAttributePath = false;
bool hasValidEventPath = false;

CHIP_ERROR err = subscribeRequestParser.GetAttributeRequests(&attributePathListParser);
if (err == CHIP_NO_ERROR)
Expand All @@ -634,16 +489,14 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
return Status::InvalidAction;
}

EventPathIBs::Parser eventPathListParser;
err = subscribeRequestParser.GetEventRequests(&eventPathListParser);
EventPathIBs::Parser eventpathListParser;
err = subscribeRequestParser.GetEventRequests(&eventpathListParser);
if (err == CHIP_NO_ERROR)
{
auto subjectDescriptor = apExchangeContext->GetSessionHandle()->AsSecureSession()->GetSubjectDescriptor();
err = ParseEventPaths(subjectDescriptor, eventPathListParser, hasValidEventPath, requestedEventPathCount);
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}
TLV::TLVReader pathReader;
eventpathListParser.GetReader(&pathReader);
ReturnErrorCodeIf(TLV::Utilities::Count(pathReader, requestedEventPathCount, false) != CHIP_NO_ERROR,
Status::InvalidAction);
}
else if (err != CHIP_ERROR_END_OF_TLV)
{
Expand All @@ -659,7 +512,12 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
return Status::InvalidAction;
}

if (!hasValidAttributePath && !hasValidEventPath)
//
// TODO: We don't have an easy way to do a similar 'path expansion' for events to deduce
// access so for now, assume that the presence of any path in the event list means they
// might have some access to those events.
//
if (!hasValidAttributePath && requestedEventPathCount == 0)
{
ChipLogError(InteractionModel,
"Subscription from [%u:" ChipLogFormatX64 "] has no access at all. Rejecting request.",
Expand Down
21 changes: 1 addition & 20 deletions src/app/InteractionModelEngine.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@
#include <app/CommandSender.h>
#include <app/ConcreteAttributePath.h>
#include <app/ConcreteCommandPath.h>
#include <app/ConcreteEventPath.h>
#include <app/DataVersionFilter.h>
#include <app/EventPathParams.h>
#include <app/ObjectList.h>
Expand Down Expand Up @@ -399,19 +398,6 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
AttributePathIBs::Parser & aAttributePathListParser, bool & aHasValidAttributePath,
size_t & aRequestedAttributePathCount);

/**
* This parses the event path list to ensure it is well formed. If so, for each path in the list, it will expand to a list
* of concrete paths and walk each path to check if it has privileges to read that event.
*
* If there is AT LEAST one "existent path" (as the spec calls it) that has sufficient privilege, aHasValidEventPath
* will be set to true. Otherwise, it will be set to false.
*
* aRequestedEventPathCount will be updated to reflect the number of event paths in the request.
*/
static CHIP_ERROR ParseEventPaths(const Access::SubjectDescriptor & aSubjectDescriptor,
EventPathIBs::Parser & aEventPathListParser, bool & aHasValidEventPath,
size_t & aRequestedEventPathCount);

/**
* Called when Interaction Model receives a Read Request message. Errors processing
* the Read Request are handled entirely within this function. If the
Expand Down Expand Up @@ -691,12 +677,7 @@ bool IsDeviceTypeOnEndpoint(DeviceTypeId deviceType, EndpointId endpoint);
*
* @retval The metadata of the attribute, will return null if the given attribute does not exists.
*/
const EmberAfAttributeMetadata * GetAttributeMetadata(const ConcreteAttributePath & aPath);

/**
* Returns the event support status for the given event, as an interaction model status.
*/
Protocols::InteractionModel::Status CheckEventSupportStatus(const ConcreteEventPath & aPath);
const EmberAfAttributeMetadata * GetAttributeMetadata(const ConcreteAttributePath & aConcreteClusterPath);

} // namespace app
} // namespace chip
20 changes: 3 additions & 17 deletions src/app/reporting/Engine.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,6 @@ CHIP_ERROR Engine::BuildSingleReportDataAttributeReportIBs(ReportDataMessage::Bu

CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool & aHasEncodedData, ReadHandler * apReadHandler)
{
using Protocols::InteractionModel::Status;

CHIP_ERROR err = CHIP_NO_ERROR;
for (auto current = apReadHandler->mpEventPathList; current != nullptr;)
{
Expand All @@ -309,21 +307,8 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
continue;
}

ConcreteEventPath path(current->mValue.mEndpointId, current->mValue.mClusterId, current->mValue.mEventId);
Status status = CheckEventSupportStatus(path);
if (status != Status::Success)
{
TLV::TLVWriter checkpoint = aWriter;
err = EventReportIB::ConstructEventStatusIB(aWriter, path, StatusIB(status));
if (err != CHIP_NO_ERROR)
{
aWriter = checkpoint;
break;
}
aHasEncodedData = true;
}

Access::RequestPath requestPath{ .cluster = current->mValue.mClusterId, .endpoint = current->mValue.mEndpointId };
ConcreteEventPath path(current->mValue.mEndpointId, current->mValue.mClusterId, current->mValue.mEventId);
Access::Privilege requestPrivilege = RequiredPrivilege::ForReadEvent(path);

err = Access::GetAccessControl().Check(apReadHandler->GetSubjectDescriptor(), requestPath, requestPrivilege);
Expand All @@ -334,7 +319,8 @@ CHIP_ERROR Engine::CheckAccessDeniedEventPaths(TLV::TLVWriter & aWriter, bool &
else
{
TLV::TLVWriter checkpoint = aWriter;
err = EventReportIB::ConstructEventStatusIB(aWriter, path, StatusIB(Status::UnsupportedAccess));
err = EventReportIB::ConstructEventStatusIB(aWriter, path,
StatusIB(Protocols::InteractionModel::Status::UnsupportedAccess));
if (err != CHIP_NO_ERROR)
{
aWriter = checkpoint;
Expand Down
12 changes: 0 additions & 12 deletions src/app/tests/TestAclAttribute.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@
#include "lib/support/CHIPMem.h"
#include <access/examples/PermissiveAccessControlDelegate.h>
#include <app/AttributeAccessInterface.h>
#include <app/ConcreteAttributePath.h>
#include <app/ConcreteEventPath.h>
#include <app/InteractionModelEngine.h>
#include <app/MessageDef/AttributeReportIBs.h>
#include <app/MessageDef/EventDataIB.h>
Expand Down Expand Up @@ -142,16 +140,6 @@ bool ConcreteAttributePathExists(const ConcreteAttributePath & aPath)
return aPath.mClusterId != kTestDeniedClusterId1;
}

Protocols::InteractionModel::Status CheckEventSupportStatus(const ConcreteEventPath & aPath)
{
if (aPath.mClusterId == kTestDeniedClusterId1)
{
return Protocols::InteractionModel::Status::UnsupportedCluster;
}

return Protocols::InteractionModel::Status::Success;
}

class TestAclAttribute
{
public:
Expand Down
Loading

0 comments on commit bc16468

Please sign in to comment.