Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

C++ bounds checks are insufficient #969

Open
amluto opened this issue Jan 24, 2024 · 0 comments
Open

C++ bounds checks are insufficient #969

amluto opened this issue Jan 24, 2024 · 0 comments

Comments

@amluto
Copy link

amluto commented Jan 24, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant