diff --git a/.github/workflows/build.yaml b/.github/workflows/build.yaml index dfc6a2245777a4..58aeca9363cd7c 100644 --- a/.github/workflows/build.yaml +++ b/.github/workflows/build.yaml @@ -242,7 +242,7 @@ jobs: # uses: github/codeql-action/analyze@v1 build_darwin: name: Build on Darwin (clang, python_lib) - timeout-minutes: 60 + timeout-minutes: 75 env: BUILD_TYPE: clang @@ -266,7 +266,7 @@ jobs: OPEN_SSL_VERSION=`ls -la /usr/local/Cellar/openssl@1.1 | cat | tail -n1 | awk '{print $NF}'` ln -s /usr/local/Cellar/openssl@1.1/$OPEN_SSL_VERSION/lib/pkgconfig/* . - name: Bootstrap - timeout-minutes: 10 + timeout-minutes: 25 run: scripts/build/gn_bootstrap.sh - name: Uploading bootstrap logs uses: actions/upload-artifact@v2 diff --git a/src/lib/core/CHIPTLV.h b/src/lib/core/CHIPTLV.h index f78713fce32a67..e1ea9c6cff1901 100644 --- a/src/lib/core/CHIPTLV.h +++ b/src/lib/core/CHIPTLV.h @@ -844,6 +844,75 @@ class DLL_EXPORT TLVReader TLVElementType ElementType() const; }; +/** + * A TLVReader that is guaranteed to be backed by a single contiguous buffer. + * This allows it to expose some additional methods that allow consumers to + * directly access the data in that buffer in a safe way that is guaranteed to + * work as long as the reader object stays in scope. + */ +class ContiguousBufferTLVReader : public TLVReader +{ +public: + ContiguousBufferTLVReader() : TLVReader() {} + + /** + * Init with input buffer as ptr + length pair. + */ + void Init(const uint8_t * data, size_t dataLen) { TLVReader::Init(data, dataLen); } + + /** + * Init with input buffer as ByteSpan. + */ + void Init(const ByteSpan & data) { Init(data.data(), data.size()); } + + /** + * Init with input buffer as byte array. + */ + template + void Init(const uint8_t (&data)[N]) + { + Init(data, N); + } + + /** + * Allow opening a container, with a new ContiguousBufferTLVReader reading + * that container. See TLVReader::OpenContainer for details. + */ + CHIP_ERROR OpenContainer(ContiguousBufferTLVReader & containerReader); + + /** + * Get the value of the current UTF8 string as a Span pointing + * into the TLV data. Consumers may need to copy the data elsewhere as + * needed (e.g. before releasing the reader and its backing buffer if they + * plan to use the data after that point). + * + * @param[out] data A Span representing the string data. + * + * @retval #CHIP_NO_ERROR If the method succeeded. + * @retval #CHIP_ERROR_WRONG_TLV_TYPE If the current element is not a TLV UTF8 string, or + * the reader is not positioned on an element. + * @retval #CHIP_ERROR_TLV_UNDERRUN If the underlying TLV encoding ended prematurely (i.e. the string length was "too big"). + * + */ + CHIP_ERROR GetStringView(Span & data); + + /** + * Get the value of the current octet string as a ByteSpan pointing into the + * TLV data. Consumers may need to copy the data elsewhere as needed + * (e.g. before releasing the reader and its backing buffer if they plan to + * use the data after that point). + * + * @param[out] data A ByteSpan representing the string data. + * + * @retval #CHIP_NO_ERROR If the method succeeded. + * @retval #CHIP_ERROR_WRONG_TLV_TYPE If the current element is not a TLV octet string, or + * the reader is not positioned on an element. + * @retval #CHIP_ERROR_TLV_UNDERRUN If the underlying TLV encoding ended prematurely (i.e. the string length was "too big"). + * + */ + CHIP_ERROR GetByteView(ByteSpan & data); +}; + /** * Provides a memory efficient encoder for writing data in CHIP TLV format. * diff --git a/src/lib/core/CHIPTLVReader.cpp b/src/lib/core/CHIPTLVReader.cpp index 07ec7b261912f6..6b1a0ec1d086db 100644 --- a/src/lib/core/CHIPTLVReader.cpp +++ b/src/lib/core/CHIPTLVReader.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -896,5 +897,39 @@ CHIP_ERROR TLVReader::FindElementWithTag(const uint64_t tag, TLVReader & destRea return err; } +CHIP_ERROR ContiguousBufferTLVReader::OpenContainer(ContiguousBufferTLVReader & containerReader) +{ + // We are going to initialize containerReader by calling our superclass + // OpenContainer method. The superclass only knows how to initialize + // members the superclass knows about, so we assert that we don't have any + // extra members that need initializing. If such members ever get added, + // they would need to be initialized in this method. + static_assert(sizeof(ContiguousBufferTLVReader) == sizeof(TLVReader), "We have state the superclass is not initializing?"); + return TLVReader::OpenContainer(containerReader); +} + +CHIP_ERROR ContiguousBufferTLVReader::GetStringView(Span & data) +{ + if (!TLVTypeIsUTF8String(ElementType())) + { + return CHIP_ERROR_WRONG_TLV_TYPE; + } + + const uint8_t * bytes; + ReturnErrorOnFailure(GetDataPtr(bytes)); // Does length sanity checks + data = Span(Uint8::to_const_char(bytes), GetLength()); + return CHIP_NO_ERROR; +} + +CHIP_ERROR ContiguousBufferTLVReader::GetByteView(ByteSpan & data) +{ + if (!TLVTypeIsByteString(ElementType())) + { + return CHIP_ERROR_WRONG_TLV_TYPE; + } + + return Get(data); +} + } // namespace TLV } // namespace chip diff --git a/src/lib/core/tests/TestCHIPTLV.cpp b/src/lib/core/tests/TestCHIPTLV.cpp index ef76642bcdc7d9..12af215c55c323 100644 --- a/src/lib/core/tests/TestCHIPTLV.cpp +++ b/src/lib/core/tests/TestCHIPTLV.cpp @@ -3812,6 +3812,188 @@ static void TLVReaderFuzzTest(nlTestSuite * inSuite, void * inContext) } } +static void AssertCanReadString(nlTestSuite * inSuite, ContiguousBufferTLVReader & reader, const char * expectedString) +{ + Span str; + CHIP_ERROR err = reader.GetStringView(str); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, str.size() == strlen(expectedString)); + NL_TEST_ASSERT(inSuite, strncmp(str.data(), expectedString, str.size()) == 0); +} + +static void AssertCannotReadString(nlTestSuite * inSuite, ContiguousBufferTLVReader & reader) +{ + Span str; + CHIP_ERROR err = reader.GetStringView(str); + NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR); +} + +static void CheckGetStringView(nlTestSuite * inSuite, void * inContext) +{ + uint8_t buf[256]; + const char testString[] = "This is a test"; + { + TLVWriter writer; + writer.Init(buf); + CHIP_ERROR err = writer.PutString(CommonTag(0), testString); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // First check that basic read from entire buffer works. + ContiguousBufferTLVReader reader; + reader.Init(buf); + reader.Next(); + AssertCanReadString(inSuite, reader, testString); + + // Now check that read from a buffer bounded by the number of bytes + // written works. + reader.Init(buf, writer.GetLengthWritten()); + reader.Next(); + AssertCanReadString(inSuite, reader, testString); + + // Now check that read from a buffer bounded by fewer than the number of + // bytes written fails. + reader.Init(buf, writer.GetLengthWritten() - 1); + reader.Next(); + AssertCannotReadString(inSuite, reader); + } + + { + // Check that an integer cannot be read as a string. + TLVWriter writer; + writer.Init(buf); + CHIP_ERROR err = writer.Put(CommonTag(0), static_cast(5)); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ContiguousBufferTLVReader reader; + reader.Init(buf); + reader.Next(); + AssertCannotReadString(inSuite, reader); + } + + { + // Check that an octet string cannot be read as a string. + TLVWriter writer; + writer.Init(buf); + CHIP_ERROR err = + writer.PutBytes(CommonTag(0), reinterpret_cast(testString), static_cast(strlen(testString))); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ContiguousBufferTLVReader reader; + reader.Init(buf); + reader.Next(); + AssertCannotReadString(inSuite, reader); + } + + { + // Check that a manually constructed string can be read as a string. + const uint8_t shortString[] = { CHIP_TLV_UTF8_STRING_2ByteLength(CHIP_TLV_TAG_COMMON_PROFILE_2Bytes(0), 2, 'a', 'b') }; + ContiguousBufferTLVReader reader; + reader.Init(shortString); + reader.Next(); + AssertCanReadString(inSuite, reader, "ab"); + } + + { + // Check that a manually constructed string with bogus length cannot be read as a string. + const uint8_t shortString[] = { CHIP_TLV_UTF8_STRING_2ByteLength(CHIP_TLV_TAG_COMMON_PROFILE_2Bytes(0), 3, 'a', 'b') }; + ContiguousBufferTLVReader reader; + reader.Init(shortString); + reader.Next(); + AssertCannotReadString(inSuite, reader); + } +} + +static void AssertCanReadBytes(nlTestSuite * inSuite, ContiguousBufferTLVReader & reader, const ByteSpan & expectedBytes) +{ + ByteSpan bytes; + CHIP_ERROR err = reader.GetByteView(bytes); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + NL_TEST_ASSERT(inSuite, bytes.data_equal(expectedBytes)); +} + +static void AssertCannotReadBytes(nlTestSuite * inSuite, ContiguousBufferTLVReader & reader) +{ + ByteSpan bytes; + CHIP_ERROR err = reader.GetByteView(bytes); + NL_TEST_ASSERT(inSuite, err != CHIP_NO_ERROR); +} + +static void CheckGetByteView(nlTestSuite * inSuite, void * inContext) +{ + uint8_t buf[256]; + const uint8_t testBytes[] = { 'T', 'h', 'i', 's', 'i', 's', 'a', 't', 'e', 's', 't', '\0' }; + { + TLVWriter writer; + writer.Init(buf); + CHIP_ERROR err = writer.PutBytes(CommonTag(0), testBytes, sizeof(testBytes)); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + // First check that basic read from entire buffer works. + ContiguousBufferTLVReader reader; + reader.Init(buf); + reader.Next(); + AssertCanReadBytes(inSuite, reader, ByteSpan(testBytes)); + + // Now check that read from a buffer bounded by the number of bytes + // written works. + reader.Init(buf, writer.GetLengthWritten()); + reader.Next(); + AssertCanReadBytes(inSuite, reader, ByteSpan(testBytes)); + + // Now check that read from a buffer bounded by fewer than the number of + // bytes written fails. + reader.Init(buf, writer.GetLengthWritten() - 1); + reader.Next(); + AssertCannotReadBytes(inSuite, reader); + } + + { + // Check that an integer cannot be read as an octet string. + TLVWriter writer; + writer.Init(buf); + CHIP_ERROR err = writer.Put(CommonTag(0), static_cast(5)); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ContiguousBufferTLVReader reader; + reader.Init(buf); + reader.Next(); + AssertCannotReadBytes(inSuite, reader); + } + + { + // Check that an string cannot be read as an octet string. + TLVWriter writer; + writer.Init(buf); + CHIP_ERROR err = writer.PutString(CommonTag(0), reinterpret_cast(testBytes)); + NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR); + + ContiguousBufferTLVReader reader; + reader.Init(buf); + reader.Next(); + AssertCannotReadBytes(inSuite, reader); + } + + { + // Check that a manually constructed octet string can be read as octet string. + const uint8_t shortBytes[] = { CHIP_TLV_BYTE_STRING_2ByteLength(CHIP_TLV_TAG_COMMON_PROFILE_2Bytes(0), 2, 1, 2) }; + ContiguousBufferTLVReader reader; + reader.Init(shortBytes); + reader.Next(); + const uint8_t expectedBytes[] = { 1, 2 }; + AssertCanReadBytes(inSuite, reader, ByteSpan(expectedBytes)); + } + + { + // Check that a manually constructed octet string with bogus length + // cannot be read as an octet string. + const uint8_t shortBytes[] = { CHIP_TLV_BYTE_STRING_2ByteLength(CHIP_TLV_TAG_COMMON_PROFILE_2Bytes(0), 3, 1, 2) }; + ContiguousBufferTLVReader reader; + reader.Init(shortBytes); + reader.Next(); + AssertCannotReadBytes(inSuite, reader); + } +} + // Test Suite /** @@ -3842,6 +4024,8 @@ static const nlTest sTests[] = NL_TEST_DEF("CHIP TLV ByteSpan", CheckCHIPTLVByteSpan), NL_TEST_DEF("CHIP TLV Check reserve", CheckCloseContainerReserve), NL_TEST_DEF("CHIP TLV Reader Fuzz Test", TLVReaderFuzzTest), + NL_TEST_DEF("CHIP TLV GetStringView Test", CheckGetStringView), + NL_TEST_DEF("CHIP TLV GetByteView Test", CheckGetByteView), NL_TEST_SENTINEL() }; diff --git a/src/setup_payload/AdditionalDataPayloadParser.cpp b/src/setup_payload/AdditionalDataPayloadParser.cpp index f183359a82f35b..6834291fe0c558 100644 --- a/src/setup_payload/AdditionalDataPayloadParser.cpp +++ b/src/setup_payload/AdditionalDataPayloadParser.cpp @@ -31,14 +31,15 @@ #include #include #include +#include #include namespace chip { CHIP_ERROR AdditionalDataPayloadParser::populatePayload(SetupPayloadData::AdditionalDataPayload & outPayload) { - TLV::TLVReader reader; - TLV::TLVReader innerReader; + TLV::ContiguousBufferTLVReader reader; + TLV::ContiguousBufferTLVReader innerReader; reader.Init(mPayloadBufferData, mPayloadBufferLength); ReturnErrorOnFailure(reader.Next(TLV::kTLVType_Structure, TLV::AnonymousTag)); @@ -49,9 +50,12 @@ CHIP_ERROR AdditionalDataPayloadParser::populatePayload(SetupPayloadData::Additi ReturnErrorOnFailure(innerReader.Next(TLV::kTLVType_UTF8String, TLV::ContextTag(SetupPayloadData::kRotatingDeviceIdTag))); // Get the value of the rotating device id - char rotatingDeviceId[SetupPayloadData::kRotatingDeviceIdLength]; - ReturnErrorOnFailure(innerReader.GetString(rotatingDeviceId, sizeof(rotatingDeviceId))); - outPayload.rotatingDeviceId = std::string(rotatingDeviceId); + Span rotatingDeviceId; + ReturnErrorOnFailure(innerReader.GetStringView(rotatingDeviceId)); + + // This test uses <, not <=, because kHexMaxLength includes the null-terminator. + VerifyOrReturnError(rotatingDeviceId.size() < RotatingDeviceId::kHexMaxLength, CHIP_ERROR_INVALID_STRING_LENGTH); + outPayload.rotatingDeviceId = std::string(rotatingDeviceId.data(), rotatingDeviceId.size()); // Verify the end of the container ReturnErrorOnFailure(reader.VerifyEndOfContainer()); diff --git a/src/setup_payload/QRCodeSetupPayloadParser.cpp b/src/setup_payload/QRCodeSetupPayloadParser.cpp index 664d01bb54e2a2..3c6a482d672cbd 100644 --- a/src/setup_payload/QRCodeSetupPayloadParser.cpp +++ b/src/setup_payload/QRCodeSetupPayloadParser.cpp @@ -65,7 +65,8 @@ static CHIP_ERROR readBits(std::vector buf, size_t & index, uint64_t & return CHIP_NO_ERROR; } -static CHIP_ERROR openTLVContainer(TLV::TLVReader & reader, TLV::TLVType type, uint64_t tag, TLV::TLVReader & containerReader) +static CHIP_ERROR openTLVContainer(TLV::ContiguousBufferTLVReader & reader, TLV::TLVType type, uint64_t tag, + TLV::ContiguousBufferTLVReader & containerReader) { VerifyOrReturnError(reader.GetType() == type, CHIP_ERROR_INVALID_ARGUMENT); VerifyOrReturnError(reader.GetTag() == tag, CHIP_ERROR_INVALID_ARGUMENT); @@ -77,17 +78,13 @@ static CHIP_ERROR openTLVContainer(TLV::TLVReader & reader, TLV::TLVType type, u return CHIP_NO_ERROR; } -static CHIP_ERROR retrieveOptionalInfoString(TLV::TLVReader & reader, OptionalQRCodeInfo & info) +static CHIP_ERROR retrieveOptionalInfoString(TLV::ContiguousBufferTLVReader & reader, OptionalQRCodeInfo & info) { - const uint32_t valLength = reader.GetLength(); - chip::Platform::ScopedMemoryBuffer value; - value.Alloc(valLength + 1); - VerifyOrReturnError(value, CHIP_ERROR_NO_MEMORY); - - ReturnErrorOnFailure(reader.GetString(value.Get(), valLength + 1)); + Span data; + ReturnErrorOnFailure(reader.GetStringView(data)); info.type = optionalQRCodeInfoTypeString; - info.data = std::string(value.Get()); + info.data = std::string(data.data(), data.size()); return CHIP_NO_ERROR; } @@ -136,7 +133,8 @@ static CHIP_ERROR retrieveOptionalInfoUInt64(TLV::TLVReader & reader, OptionalQR return CHIP_NO_ERROR; } -static CHIP_ERROR retrieveOptionalInfo(TLV::TLVReader & reader, OptionalQRCodeInfo & info, optionalQRCodeInfoType type) +static CHIP_ERROR retrieveOptionalInfo(TLV::ContiguousBufferTLVReader & reader, OptionalQRCodeInfo & info, + optionalQRCodeInfoType type) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -156,7 +154,8 @@ static CHIP_ERROR retrieveOptionalInfo(TLV::TLVReader & reader, OptionalQRCodeIn return err; } -static CHIP_ERROR retrieveOptionalInfo(TLV::TLVReader & reader, OptionalQRCodeInfoExtension & info, optionalQRCodeInfoType type) +static CHIP_ERROR retrieveOptionalInfo(TLV::ContiguousBufferTLVReader & reader, OptionalQRCodeInfoExtension & info, + optionalQRCodeInfoType type) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -184,7 +183,7 @@ static CHIP_ERROR retrieveOptionalInfo(TLV::TLVReader & reader, OptionalQRCodeIn return err; } -CHIP_ERROR QRCodeSetupPayloadParser::retrieveOptionalInfos(SetupPayload & outPayload, TLV::TLVReader & reader) +CHIP_ERROR QRCodeSetupPayloadParser::retrieveOptionalInfos(SetupPayload & outPayload, TLV::ContiguousBufferTLVReader & reader) { CHIP_ERROR err = CHIP_NO_ERROR; while (err == CHIP_NO_ERROR) @@ -242,7 +241,7 @@ CHIP_ERROR QRCodeSetupPayloadParser::parseTLVFields(SetupPayload & outPayload, u { return CHIP_ERROR_INVALID_ARGUMENT; } - TLV::TLVReader rootReader; + TLV::ContiguousBufferTLVReader rootReader; rootReader.Init(tlvDataStart, tlvDataLengthInBytes); ReturnErrorOnFailure(rootReader.Next()); @@ -251,7 +250,7 @@ CHIP_ERROR QRCodeSetupPayloadParser::parseTLVFields(SetupPayload & outPayload, u return CHIP_ERROR_INVALID_ARGUMENT; } - TLV::TLVReader innerStructureReader; + TLV::ContiguousBufferTLVReader innerStructureReader; ReturnErrorOnFailure(openTLVContainer(rootReader, TLV::kTLVType_Structure, TLV::AnonymousTag, innerStructureReader)); ReturnErrorOnFailure(innerStructureReader.Next()); err = retrieveOptionalInfos(outPayload, innerStructureReader); diff --git a/src/setup_payload/QRCodeSetupPayloadParser.h b/src/setup_payload/QRCodeSetupPayloadParser.h index 48088b612cb168..17c8fbebb6039d 100644 --- a/src/setup_payload/QRCodeSetupPayloadParser.h +++ b/src/setup_payload/QRCodeSetupPayloadParser.h @@ -45,7 +45,7 @@ class QRCodeSetupPayloadParser CHIP_ERROR populatePayload(SetupPayload & outPayload); private: - CHIP_ERROR retrieveOptionalInfos(SetupPayload & outPayload, TLV::TLVReader & reader); + CHIP_ERROR retrieveOptionalInfos(SetupPayload & outPayload, TLV::ContiguousBufferTLVReader & reader); CHIP_ERROR populateTLV(SetupPayload & outPayload, const std::vector & buf, size_t & index); CHIP_ERROR parseTLVFields(chip::SetupPayload & outPayload, uint8_t * tlvDataStart, size_t tlvDataLengthInBytes); };