Add non-copying string and byte getters on a TLV reader. (#9249)

* Add non-copying string and byte getters on a TLV reader.

We have existing callsites that are doing what GetString/GetBytes say to do and call GetLength() to decide how big a buffer to allocate.  But the return value from GetLength() doesn't have to be sane, so we end up allocating buffers with attacker-controlled sizes.

The changes here are:

1) Add ContiguousBufferTLVReader that is always backed by a single
   buffer, so it can guarantee that the in-place view makes sense.

2) Use the new class in setup payload parsers, fixing various bugs in
   the process.

Fixes #9009

* Addressing review comments

* More review comments

* Bump Darwin build job bootstrap timeout to match other Darwin jobs.

10 minutes keeps timing out.

.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/[email protected] | cat | tail -n1 | awk '{print $NF}'`
                   ln -s /usr/local/Cellar/[email protected]/$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

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 <size_t N>
+    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<const char> 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<const char> 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<const char> & 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.
  *

src/lib/core/CHIPTLVReader.cpp

@@ -27,6 +27,7 @@
 #include <core/CHIPCore.h>
 #include <core/CHIPEncoding.h>
 #include <core/CHIPTLV.h>
+#include <lib/core/CHIPSafeCasts.h>
 #include <support/CHIPMem.h>
 #include <support/CodeUtils.h>
 #include <support/SafeInt.h>
@@ -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<const char> & data)
+{
+    if (!TLVTypeIsUTF8String(ElementType()))
+    {
+        return CHIP_ERROR_WRONG_TLV_TYPE;
+    }
+
+    const uint8_t * bytes;
+    ReturnErrorOnFailure(GetDataPtr(bytes)); // Does length sanity checks
+    data = Span<const char>(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

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<const char> 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<const char> 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<uint8_t>(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<const uint8_t *>(testString), static_cast<uint32_t>(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<uint8_t>(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<const char *>(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()
 };

src/setup_payload/AdditionalDataPayloadParser.cpp

@@ -31,14 +31,15 @@
 #include <core/CHIPTLVData.hpp>
 #include <core/CHIPTLVUtilities.hpp>
 #include <protocols/Protocols.h>
+#include <setup_payload/AdditionalDataPayloadGenerator.h>
 #include <support/CodeUtils.h>
 
 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<const char> 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());