Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent Subscription to attributes with no access privilege and absence #22349

Merged
merged 3 commits into from
Sep 8, 2022
Merged

Prevent Subscription to attributes with no access privilege and absence #22349

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e88696b
Prevent Subscription to attributes with no access privilege
yunhanw-google Aug 29, 2022
8016e0f
Fix Darwin subscription test to follow the new failure semantics.
bzbarsky-apple Sep 8, 2022
0e467f9
Merge pull request #33 from bzbarsky-apple/testing
yunhanw-google Sep 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
142 changes: 115 additions & 27 deletions src/app/InteractionModelEngine.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@

#include <cinttypes>

#include "access/RequestPath.h"
#include "access/SubjectDescriptor.h"
#include <app/RequiredPrivilege.h>
#include <lib/core/CHIPTLVUtilities.hpp>
#include <lib/support/CodeUtils.h>

Expand Down Expand Up @@ -323,6 +326,79 @@ Status InteractionModelEngine::OnInvokeCommandRequest(Messaging::ExchangeContext
return Status::Success;
}

CHIP_ERROR InteractionModelEngine::ParseAttributePaths(const Access::SubjectDescriptor & aSubjectDescriptor,
AttributePathIBs::Parser & aAttributePathListParser,
bool & aHasValidAttributePath, size_t & aRequestedAttributePathCount)
{
TLV::TLVReader pathReader;
aAttributePathListParser.GetReader(&pathReader);
CHIP_ERROR err = CHIP_NO_ERROR;

aHasValidAttributePath = false;
aRequestedAttributePathCount = 0;

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.
// This avoids the 'parse all paths' approach that is employed in ReadHandler since we want to
// avoid allocating out of the path store during this minimal initial processing stage.
//
ObjectList<AttributePathParams> paramsList;

ReturnErrorOnFailure(path.Init(pathReader));
ReturnErrorOnFailure(path.ParsePath(paramsList.mValue));

if (paramsList.mValue.IsWildcardPath())
{
AttributePathExpandIterator pathIterator(&paramsList);
ConcreteAttributePath readPath;

// The definition of "valid path" is "path exists and ACL allows access". The "path exists" part is handled by
// AttributePathExpandIterator. So we just need to check the ACL bits.
for (; pathIterator.Get(readPath); pathIterator.Next())
{
Access::RequestPath requestPath{ .cluster = readPath.mClusterId, .endpoint = readPath.mEndpointId };
err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath,
RequiredPrivilege::ForReadAttribute(readPath));
if (err == CHIP_NO_ERROR)
{
aHasValidAttributePath = true;
break;
}
}
}
else
{
ConcreteAttributePath concretePath(paramsList.mValue.mEndpointId, paramsList.mValue.mClusterId,
paramsList.mValue.mAttributeId);
if (ConcreteAttributePathExists(concretePath))
{
Access::RequestPath requestPath{ .cluster = concretePath.mClusterId, .endpoint = concretePath.mEndpointId };

err = Access::GetAccessControl().Check(aSubjectDescriptor, requestPath,
RequiredPrivilege::ForReadAttribute(concretePath));
if (err == CHIP_NO_ERROR)
{
aHasValidAttributePath = true;
}
}
}

aRequestedAttributePathCount++;
}

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 All @@ -349,28 +425,25 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
reader.Init(aPayload.Retain());

SubscribeRequestMessage::Parser subscribeRequestParser;
CHIP_ERROR err = subscribeRequestParser.Init(reader);
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}

VerifyOrReturnError(subscribeRequestParser.Init(reader) == CHIP_NO_ERROR, Status::InvalidAction);
{
size_t requestedAttributePathCount = 0;
size_t requestedEventPathCount = 0;
AttributePathIBs::Parser attributePathListParser;
err = subscribeRequestParser.GetAttributeRequests(&attributePathListParser);
bool hasValidAttributePath = false;

CHIP_ERROR err = subscribeRequestParser.GetAttributeRequests(&attributePathListParser);
if (err == CHIP_NO_ERROR)
{
TLV::TLVReader pathReader;
attributePathListParser.GetReader(&pathReader);
err = TLV::Utilities::Count(pathReader, requestedAttributePathCount, false);
}
else if (err == CHIP_ERROR_END_OF_TLV)
{
err = CHIP_NO_ERROR;
auto subjectDescriptor = apExchangeContext->GetSessionHandle()->AsSecureSession()->GetSubjectDescriptor();
err = ParseAttributePaths(subjectDescriptor, attributePathListParser, hasValidAttributePath,
requestedAttributePathCount);
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}
}
if (err != CHIP_NO_ERROR)
else if (err != CHIP_ERROR_END_OF_TLV)
{
return Status::InvalidAction;
}
Expand All @@ -381,14 +454,34 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
{
TLV::TLVReader pathReader;
eventpathListParser.GetReader(&pathReader);
err = TLV::Utilities::Count(pathReader, requestedEventPathCount, false);
ReturnErrorCodeIf(TLV::Utilities::Count(pathReader, requestedEventPathCount, false) != CHIP_NO_ERROR,
Status::InvalidAction);
}
else if (err == CHIP_ERROR_END_OF_TLV)
else if (err != CHIP_ERROR_END_OF_TLV)
{
err = CHIP_NO_ERROR;
return Status::InvalidAction;
}
if (err != CHIP_NO_ERROR)

if (requestedAttributePathCount == 0 && requestedEventPathCount == 0)
{
ChipLogError(InteractionModel,
"Subscription from [%u:" ChipLogFormatX64 "] has no attribute or event paths. Rejecting request.",
apExchangeContext->GetSessionHandle()->GetFabricIndex(),
ChipLogValueX64(apExchangeContext->GetSessionHandle()->AsSecureSession()->GetPeerNodeId()));
return Status::InvalidAction;
}

//
// 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.",
apExchangeContext->GetSessionHandle()->GetFabricIndex(),
ChipLogValueX64(apExchangeContext->GetSessionHandle()->AsSecureSession()->GetPeerNodeId()));
return Status::InvalidAction;
}

Expand All @@ -400,12 +493,8 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
}
}

err = subscribeRequestParser.GetKeepSubscriptions(&keepExistingSubscriptions);
if (err != CHIP_NO_ERROR)
{
return Status::InvalidAction;
}

VerifyOrReturnError(subscribeRequestParser.GetKeepSubscriptions(&keepExistingSubscriptions) == CHIP_NO_ERROR,
Status::InvalidAction);
if (!keepExistingSubscriptions)
{
//
Expand All @@ -426,8 +515,7 @@ Protocols::InteractionModel::Status InteractionModelEngine::OnReadInitialRequest
});
}
}

if (aInteractionType == ReadHandler::InteractionType::Read)
else
{
System::PacketBufferTLVReader reader;
reader.Init(aPayload.Retain());
Expand Down
22 changes: 22 additions & 0 deletions src/app/InteractionModelEngine.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -378,6 +378,21 @@ class InteractionModelEngine : public Messaging::UnsolicitedMessageHandler,
System::PacketBufferHandle && aPayload) override;
void OnResponseTimeout(Messaging::ExchangeContext * ec) override;

/**
* This parses the attribute 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 attribute.
*
* If there is AT LEAST one "existent path" (as the spec calls it) that has sufficient privilege, aHasValidAttributePath
* will be set to true. Otherwise, it will be set to false.
*
* aRequestedAttributePathCount will be updated to reflect the number of attribute paths in the request.
*
*
*/
static CHIP_ERROR ParseAttributePaths(const Access::SubjectDescriptor & aSubjectDescriptor,
AttributePathIBs::Parser & aAttributePathListParser, bool & aHasValidAttributePath,
size_t & aRequestedAttributePathCount);

/**
* 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 @@ -617,6 +632,13 @@ CHIP_ERROR ReadSingleClusterData(const Access::SubjectDescriptor & aSubjectDescr
const ConcreteReadAttributePath & aPath, AttributeReportIBs::Builder & aAttributeReports,
AttributeValueEncoder::AttributeEncodeState * apEncoderState);

/**
* Check whether concrete attribute path is an "existent attribute path" in spec terms.
* @param[in] aPath The concrete path of the data being read.
* @retval boolean
*/
bool ConcreteAttributePathExists(const ConcreteAttributePath & aPath);

/**
* Get the registered attribute access override. nullptr when attribute access override is not found.
*
Expand Down
57 changes: 57 additions & 0 deletions src/app/MessageDef/AttributePathIB.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,63 @@ CHIP_ERROR AttributePathIB::Parser::GetListIndex(ConcreteDataAttributePath & aAt
return err;
}

CHIP_ERROR AttributePathIB::Parser::ParsePath(AttributePathParams & aAttribute) const
{
CHIP_ERROR err = CHIP_NO_ERROR;

err = GetEndpoint(&(aAttribute.mEndpointId));
if (err == CHIP_NO_ERROR)
{
VerifyOrReturnError(!aAttribute.HasWildcardEndpointId(), CHIP_IM_GLOBAL_STATUS(InvalidAction));
}
else if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));

err = GetCluster(&aAttribute.mClusterId);
if (err == CHIP_NO_ERROR)
{
VerifyOrReturnError(IsValidClusterId(aAttribute.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
}
else if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));

err = GetAttribute(&aAttribute.mAttributeId);
if (err == CHIP_NO_ERROR)
{
VerifyOrReturnError(IsValidAttributeId(aAttribute.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction));
}
else if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));

// A wildcard cluster requires that the attribute path either be
// wildcard or a global attribute.
VerifyOrReturnError(!aAttribute.HasWildcardClusterId() || aAttribute.HasWildcardAttributeId() ||
IsGlobalAttribute(aAttribute.mAttributeId),
CHIP_IM_GLOBAL_STATUS(InvalidAction));

err = GetListIndex(&aAttribute.mListIndex);
if (err == CHIP_NO_ERROR)
{
VerifyOrReturnError(!aAttribute.HasWildcardAttributeId() && !aAttribute.HasWildcardListIndex(),
CHIP_IM_GLOBAL_STATUS(InvalidAction));
}
else if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));
return CHIP_NO_ERROR;
}

AttributePathIB::Builder & AttributePathIB::Builder::EnableTagCompression(const bool aEnableTagCompression)
{
// skip if error has already been set
Expand Down
8 changes: 8 additions & 0 deletions src/app/MessageDef/AttributePathIB.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,14 @@ class Parser : public ListParser
CHIP_ERROR GetListIndex(ConcreteDataAttributePath & aAttributePath) const;

// TODO(#14934) Add a function to get ConcreteDataAttributePath from AttributePathIB::Parser directly.

/**
* @brief Parse the attribute path into an AttributePathParams object. As part of parsing,
* validity checks for each path item will be done as well.
*
* If any errors are encountered, an IM error of 'InvalidAction' will be returned.
*/
CHIP_ERROR ParsePath(AttributePathParams & attribute) const;
};

class Builder : public ListBuilder
Expand Down
46 changes: 46 additions & 0 deletions src/app/MessageDef/EventPathIB.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@

#include <app/AppConfig.h>

#include <protocols/interaction_model/Constants.h>

namespace chip {
namespace app {
#if CHIP_CONFIG_IM_ENABLE_SCHEMA_CHECK
Expand Down Expand Up @@ -171,6 +173,50 @@ CHIP_ERROR EventPathIB::Parser::GetIsUrgent(bool * const apIsUrgent) const
return GetSimpleValue(to_underlying(Tag::kIsUrgent), TLV::kTLVType_Boolean, apIsUrgent);
}

CHIP_ERROR EventPathIB::Parser::ParsePath(EventPathParams & aEvent) const
{
CHIP_ERROR err = GetEndpoint(&(aEvent.mEndpointId));
if (err == CHIP_NO_ERROR)
{
VerifyOrReturnError(!aEvent.HasWildcardEndpointId(), CHIP_IM_GLOBAL_STATUS(InvalidAction));
}
else if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));

err = GetCluster(&(aEvent.mClusterId));
if (err == CHIP_NO_ERROR)
{
VerifyOrReturnError(!aEvent.HasWildcardClusterId(), CHIP_IM_GLOBAL_STATUS(InvalidAction));
}
else if (err == CHIP_END_OF_TLV)
{
err = CHIP_NO_ERROR;
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));

err = GetEvent(&(aEvent.mEventId));
if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
else if (err == CHIP_NO_ERROR)
{
VerifyOrReturnError(!aEvent.HasWildcardEventId(), CHIP_IM_GLOBAL_STATUS(InvalidAction));
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));

err = GetIsUrgent(&(aEvent.mIsUrgentEvent));
if (CHIP_END_OF_TLV == err)
{
err = CHIP_NO_ERROR;
}
VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_IM_GLOBAL_STATUS(InvalidAction));
return CHIP_NO_ERROR;
}

EventPathIB::Builder & EventPathIB::Builder::Node(const NodeId aNode)
{
// skip if error has already been set
Expand Down
8 changes: 8 additions & 0 deletions src/app/MessageDef/EventPathIB.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,14 @@ class Parser : public ListParser
* #CHIP_ERROR_IM_MALFORMED_EVENT_PATH_IB if the path from the reader is not a valid concrere event path.
*/
CHIP_ERROR GetEventPath(ConcreteEventPath * const apPath) const;

/**
* @brief Parse the event path into an EventPathParams object. As part of parsing,
* validity checks for each path item will be done as well.
*
* If any errors are encountered, an IM error of 'InvalidAction' will be returned.
*/
CHIP_ERROR ParsePath(EventPathParams & aEvent) const;
};

class Builder : public ListBuilder
Expand Down
Loading