From 05e4c10358a18f9c9295c2dcd7cdd880502d539a Mon Sep 17 00:00:00 2001 From: Karsten Sperling <113487422+ksperling-apple@users.noreply.github.com> Date: Mon, 15 Jul 2024 03:55:45 +1200 Subject: [PATCH] Support raw ByteSpan values correctly in SafeAttributePersistenceProvider (#34306) * Support raw ByteSpan values correctly in SafeattributePersistenceProvider The DefaultAttributePersistenceProvider implementation of SafeReadValue() had the undocumented behavior of enforcing that the value exactly filled the provided buffer. This does not make sense for an API that purports to store general ByteSpan values. Instead move this validation into ReadScalarValue() in SafeAttributePersistenceProvider, and separate InternalReadValue() into one method that does the only the actual read, and a second one that performs the additional validation for reading typed ember attributes (strings with length prefixes etc). Use the former one from SafeReadValue(). Also fix possible out of bounds access when reading ember strings by ensuring we have read the length byte(s) before attempting to interpret them. * Take integral argument T by value * Add test --- .../DefaultAttributePersistenceProvider.cpp | 27 ++++++++----- src/app/DefaultAttributePersistenceProvider.h | 4 +- src/app/SafeAttributePersistenceProvider.h | 40 +++++++++---------- .../TestAttributePersistenceProvider.cpp | 9 ++++- 4 files changed, 45 insertions(+), 35 deletions(-) diff --git a/src/app/DefaultAttributePersistenceProvider.cpp b/src/app/DefaultAttributePersistenceProvider.cpp index 5bef4c11fb8c1a..6e7120999fe519 100644 --- a/src/app/DefaultAttributePersistenceProvider.cpp +++ b/src/app/DefaultAttributePersistenceProvider.cpp @@ -37,34 +37,40 @@ CHIP_ERROR DefaultAttributePersistenceProvider::InternalWriteValue(const Storage return mStorage->SyncSetKeyValue(aKey.KeyName(), aValue.data(), static_cast(aValue.size())); } -CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, - size_t aSize, MutableByteSpan & aValue) +CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue) { VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INCORRECT_STATE); uint16_t size = static_cast(min(aValue.size(), static_cast(UINT16_MAX))); ReturnErrorOnFailure(mStorage->SyncGetKeyValue(aKey.KeyName(), aValue.data(), size)); - EmberAfAttributeType type = aType; - if (emberAfIsStringAttributeType(type)) + aValue.reduce_size(size); + return CHIP_NO_ERROR; +} + +CHIP_ERROR DefaultAttributePersistenceProvider::InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, + size_t aExpectedSize, MutableByteSpan & aValue) +{ + ReturnErrorOnFailure(InternalReadValue(aKey, aValue)); + size_t size = aValue.size(); + if (emberAfIsStringAttributeType(aType)) { // Ensure that we've read enough bytes that we are not ending up with // un-initialized memory. Should have read length + 1 (for the length // byte). - VerifyOrReturnError(size >= emberAfStringLength(aValue.data()) + 1, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(size >= 1 && size - 1 >= emberAfStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE); } - else if (emberAfIsLongStringAttributeType(type)) + else if (emberAfIsLongStringAttributeType(aType)) { // Ensure that we've read enough bytes that we are not ending up with // un-initialized memory. Should have read length + 2 (for the length // bytes). - VerifyOrReturnError(size >= emberAfLongStringLength(aValue.data()) + 2, CHIP_ERROR_INCORRECT_STATE); + VerifyOrReturnError(size >= 2 && size - 2 >= emberAfLongStringLength(aValue.data()), CHIP_ERROR_INCORRECT_STATE); } else { // Ensure we got the expected number of bytes for all other types. - VerifyOrReturnError(size == aSize, CHIP_ERROR_INVALID_ARGUMENT); + VerifyOrReturnError(size == aExpectedSize, CHIP_ERROR_INVALID_ARGUMENT); } - aValue.reduce_size(size); return CHIP_NO_ERROR; } @@ -90,8 +96,7 @@ CHIP_ERROR DefaultAttributePersistenceProvider::SafeWriteValue(const ConcreteAtt CHIP_ERROR DefaultAttributePersistenceProvider::SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) { return InternalReadValue( - DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), 0x20, - aValue.size(), aValue); + DefaultStorageKeyAllocator::SafeAttributeValue(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId), aValue); } namespace { diff --git a/src/app/DefaultAttributePersistenceProvider.h b/src/app/DefaultAttributePersistenceProvider.h index 4db77e8919222c..3e18808f366d37 100644 --- a/src/app/DefaultAttributePersistenceProvider.h +++ b/src/app/DefaultAttributePersistenceProvider.h @@ -63,7 +63,9 @@ class DefaultAttributePersistenceProvider : public AttributePersistenceProvider, private: CHIP_ERROR InternalWriteValue(const StorageKeyName & aKey, const ByteSpan & aValue); - CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aSize, MutableByteSpan & aValue); + CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, MutableByteSpan & aValue); + CHIP_ERROR InternalReadValue(const StorageKeyName & aKey, EmberAfAttributeType aType, size_t aExpectedSize, + MutableByteSpan & aValue); }; } // namespace app diff --git a/src/app/SafeAttributePersistenceProvider.h b/src/app/SafeAttributePersistenceProvider.h index ed02d6039043ad..8d11d95d47b02d 100644 --- a/src/app/SafeAttributePersistenceProvider.h +++ b/src/app/SafeAttributePersistenceProvider.h @@ -40,8 +40,8 @@ class SafeAttributePersistenceProvider // The following API provides helper functions to simplify the access of commonly used types. // The API may not be complete. - // Currently implemented write and read types are: uint8_t, uint16_t, uint32_t, unit64_t and - // their nullable varieties, and bool. + // Currently implemented write and read types are: bool, uint8_t, uint16_t, uint32_t, unit64_t and + // their nullable varieties, as well as ByteSpans. /** * Write an attribute value of type intX, uintX or bool to non-volatile memory. @@ -50,7 +50,7 @@ class SafeAttributePersistenceProvider * @param [in] aValue the data to write. */ template ::value, bool> = true> - CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T & aValue) + CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, T aValue) { uint8_t value[sizeof(T)]; auto w = Encoding::LittleEndian::BufferWriter(value, sizeof(T)); @@ -71,18 +71,16 @@ class SafeAttributePersistenceProvider * * @param [in] aPath the attribute path for the data being persisted. * @param [in,out] aValue where to place the data. + * + * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute */ template ::value, bool> = true> CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, T & aValue) { uint8_t attrData[sizeof(T)]; MutableByteSpan tempVal(attrData); - auto err = SafeReadValue(aPath, tempVal); - if (err != CHIP_NO_ERROR) - { - return err; - } - + ReturnErrorOnFailure(SafeReadValue(aPath, tempVal)); + VerifyOrReturnError(tempVal.size() == sizeof(T), CHIP_ERROR_INCORRECT_STATE); Encoding::LittleEndian::Reader r(tempVal.data(), tempVal.size()); r.RawReadLowLevelBeCareful(&aValue); return r.StatusCode(); @@ -95,7 +93,7 @@ class SafeAttributePersistenceProvider * @param [in] aValue the data to write. */ template ::value, bool> = true> - CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) + CHIP_ERROR WriteScalarValue(const ConcreteAttributePath & aPath, const DataModel::Nullable & aValue) { typename NumericAttributeTraits::StorageType storageValue; if (aValue.IsNull()) @@ -114,16 +112,14 @@ class SafeAttributePersistenceProvider * * @param [in] aPath the attribute path for the data being persisted. * @param [in,out] aValue where to place the data. + * + * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute */ template ::value, bool> = true> CHIP_ERROR ReadScalarValue(const ConcreteAttributePath & aPath, DataModel::Nullable & aValue) { typename NumericAttributeTraits::StorageType storageValue; - CHIP_ERROR err = ReadScalarValue(aPath, storageValue); - if (err != CHIP_NO_ERROR) - { - return err; - } + ReturnErrorOnFailure(ReadScalarValue(aPath, storageValue)); if (NumericAttributeTraits::IsNullValue(storageValue)) { @@ -137,25 +133,25 @@ class SafeAttributePersistenceProvider return CHIP_NO_ERROR; } -protected: /** * Write an attribute value from the attribute store (i.e. not a struct or * list) to non-volatile memory. * * @param [in] aPath the attribute path for the data being written. - * @param [in] aValue the data to write. The value should be stored as-is. + * @param [in] aValue the data to write. The value will be stored as-is. */ virtual CHIP_ERROR SafeWriteValue(const ConcreteAttributePath & aPath, const ByteSpan & aValue) = 0; /** - * Read an attribute value from non-volatile memory. - * It can be assumed that this method will never be called upon to read - * an attribute of type string or long-string. + * Read an attribute value as a raw sequence of bytes from non-volatile memory. * * @param [in] aPath the attribute path for the data being persisted. * @param [in,out] aValue where to place the data. The size of the buffer - * will be equal to `aValue.size()`. The callee is expected to adjust - * aValue's size to the actual number of bytes read. + * will be equal to `aValue.size()`. On success aValue.size() + * will be the actual number of bytes read. + * + * @retval CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND if no stored value exists for the attribute + * @retval CHIP_ERROR_BUFFER_TOO_SMALL aValue.size() is too small to hold the value. */ virtual CHIP_ERROR SafeReadValue(const ConcreteAttributePath & aPath, MutableByteSpan & aValue) = 0; }; diff --git a/src/app/tests/TestAttributePersistenceProvider.cpp b/src/app/tests/TestAttributePersistenceProvider.cpp index 7ae40b52402fe0..1b8c7ee3d3872c 100644 --- a/src/app/tests/TestAttributePersistenceProvider.cpp +++ b/src/app/tests/TestAttributePersistenceProvider.cpp @@ -59,7 +59,13 @@ TEST_F(TestAttributePersistenceProvider, TestStorageAndRetrivalByteSpans) MutableByteSpan valueReadBack(getArray); err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack); EXPECT_EQ(err, CHIP_NO_ERROR); - EXPECT_TRUE(std::equal(valueReadBack.begin(), valueReadBack.end(), value.begin(), value.end())); + EXPECT_TRUE(valueReadBack.data_equal(value)); + + uint8_t getArrayThatIsLongerThanNeeded[10]; + MutableByteSpan valueReadBack2(getArrayThatIsLongerThanNeeded); + err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBack2); + EXPECT_EQ(err, CHIP_NO_ERROR); + EXPECT_TRUE(valueReadBack2.data_equal(value)); // Finishing persistenceProvider.Shutdown(); @@ -320,6 +326,7 @@ TEST_F(TestAttributePersistenceProvider, TestBufferTooSmallErrors) err = persistenceProvider.SafeReadValue(TestConcretePath, valueReadBackByteSpan8); EXPECT_EQ(err, CHIP_ERROR_BUFFER_TOO_SMALL); + // TODO: ReadScalarValue() does not take a buffer, so expecting CHIP_ERROR_BUFFER_TOO_SMALL is bad API // Fail to get value as uint8_t uint8_t valueReadBack8; err = persistenceProvider.ReadScalarValue(TestConcretePath, valueReadBack8);