Skip to content

C++ bounds checks are insufficient #969

Open
@amluto

Description

@amluto

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions