Skip to content

Commit

Permalink
Prevent Reads/Subscription to attributes with no access privilege (#2…
Browse files Browse the repository at this point in the history 
…2349)
  • Loading branch information
@yunhanw-google @pull
yunhanw-google authored and pull[bot] committed Sep 13, 2022
1 parent 4c070bf commit 1125455
Show file tree
Hide file tree
Showing 20 changed files with 706 additions and 163 deletions.
142 changes: 115 additions & 27 deletions src/app/InteractionModelEngine.cpp
View file
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
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
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
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
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
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

0 comments on commit 1125455

Please sign in to comment.