simple-binary-encoding icon indicating copy to clipboard operation
simple-binary-encoding copied to clipboard

C++ bounds checks are insufficient

Open amluto opened this issue 2 years ago • 0 comments

This is a followup to #130. with my apologies for ignoring this for so long.

A very recent simple-binary-encoding build from the CME templates generated MDIncrementalRefreshBook46.h, which does indeed contain some bounds checking, but I think it's incomplete. In the generated file, there's a class MDIncrementalRefreshBook46::NoOrderIDEntries, and that contains:

        inline void wrapForDecode(
            char *buffer,
            std::uint64_t *pos,
            const std::uint64_t actingVersion,
            const std::uint64_t bufferLength)
        {
            GroupSize8Byte dimensions(buffer, *pos, bufferLength, actingVersion);
            m_buffer = buffer;
            m_bufferLength = bufferLength;
            m_blockLength = dimensions.blockLength();
            m_count = dimensions.numInGroup();
            m_index = 0;
            m_actingVersion = actingVersion;
            m_initialPosition = *pos;
            m_positionPtr = pos;
            *m_positionPtr = *m_positionPtr + 8;
        }

And next() looks like this:

        inline NoOrderIDEntries &next()
        {
            if (m_index >= m_count)
            {
                throw std::runtime_error("index >= count [E108]");
            }
            m_offset = *m_positionPtr;
            if (SBE_BOUNDS_CHECK_EXPECT(((m_offset + m_blockLength) > m_bufferLength), false))
            {
                throw std::runtime_error("buffer too short for next group index [E108]");
            }
            *m_positionPtr = m_offset + m_blockLength;
            ++m_index;

            return *this;
        }

dimensions.blockLength() is an untrusted value, although it happens to be an sbe_uint16_t despite being stored in an sbe_uint64_t field, so it's constrained. This dodges a bullet: otherwise m_offset + m_blockLength could overflow. I didn't check whether other classes might not be so lucky, though.

But there's a more fundamental issue: nothing seems to check that m_blockLength is appropriate. For example:


        SBE_NODISCARD std::uint64_t orderID() const SBE_NOEXCEPT
        {
            std::uint64_t val;
            std::memcpy(&val, m_buffer + m_offset + 0, sizeof(std::uint64_t));
            return SBE_LITTLE_ENDIAN_ENCODE_64(val);
        }

If there are fewer than 8 bytes in the buffer, this will overflow. But m_blockLength could easily be 0 or 1 or 7, and the bounds check in next() would pass with fewer than 8 bytes in the buffer.

This is extra complicated because of the actingVersion mechanism:

        SBE_NODISCARD std::uint64_t mDOrderPriority() const SBE_NOEXCEPT
        {
            if (m_actingVersion < 7)
            {
                return UINT64_C(0xffffffffffffffff);
            }

            std::uint64_t val;
            std::memcpy(&val, m_buffer + m_offset + 8, sizeof(std::uint64_t));
            return SBE_LITTLE_ENDIAN_ENCODE_64(val);
        }

blockLength == 8 is valid for NoOrderIDEntries if actingVersion < 7, but if actingVersion >= 7, then this requires a blockLength of at least 16! And checking that from user code (as opposed to the generated headers) would get very awkward very quickly.

amluto avatar Jan 24 '24 19:01 amluto