~bigbes/tarantool

tarantool-protobuf

2ed0f4b5b17a1235cbcef16005370d430b2b5424 — Eugene Blikh 3 months ago 849b766
wire: validate field number and minimal encoding in decode_tag

Three checks added on the multi-byte path:
1. Field number must be > 0 (IllegalZeroFieldNum_Case_0/1/3).
2. Field number must fit in 29 bits per the protobuf spec
   (BadTag_FieldNumberSlightlyTooHigh, BadTag_FieldNumberTooHigh).
3. Tag varint must be minimally encoded — a trailing 0 byte with more
   than one byte read is overlong (BadTag_OverlongVarint).

The field-number check runs on bit ops over the uint64 cdata returned
by decode_varint rather than after tonumber, otherwise field numbers
above 2^32 alias into the valid range (e.g., fn=2^31+1 was being
recovered as fn=1).

The single-byte fast path picks up the field-zero check directly via
the b >> 3 == 0 condition.

Drops 6 entries from test/conformance/known_failures.txt and adds 4
regression tests pinning each rejection path.
3 files changed, 55 insertions(+), 11 deletions(-)

M runtime/pb/wire.lua
M test/conformance/known_failures.txt
M test/conformance_test.lua
M runtime/pb/wire.lua => runtime/pb/wire.lua +21 -5
@@ 140,13 140,29 @@ local function decode_tag(buf, pos)
    if b < 0x80 then
        local wt = bit.band(b, 7)
        if wt >= 6 then error("illegal wire type " .. wt, 0) end
        return bit.rshift(b, 3), wt, pos + 1
        local fn = bit.rshift(b, 3)
        if fn == 0 then error("illegal field number 0", 0) end
        return fn, wt, pos + 1
    end
    local v, npos = decode_varint(buf, pos)
    v = tonumber(v)
    local wt = bit.band(v, 7)
    local u, npos = decode_varint(buf, pos)
    -- A tag must use the minimum number of bytes to encode its value.
    -- The trailing byte of a multi-byte varint contributes 0 high bits
    -- only when the encoding is overlong (any prior byte already covered
    -- the value).
    if buf:byte(npos - 1) == 0 then
        error("overlong tag varint at offset " .. pos, 0)
    end
    -- Bit ops on the uint64 cdata preserve 64-bit width; going through
    -- tonumber first would truncate field numbers above 2^32.
    local wt = tonumber(bit.band(u, 7))
    if wt >= 6 then error("illegal wire type " .. wt, 0) end
    return bit.rshift(v, 3), wt, npos
    local fn = tonumber(bit.rshift(u, 3))
    if fn == 0 then error("illegal field number 0", 0) end
    -- Field numbers are 29-bit per the protobuf spec.
    if fn > 0x1FFFFFFF then
        error("field number out of range: " .. fn, 0)
    end
    return fn, wt, npos
end
M.decode_tag = decode_tag


M test/conformance/known_failures.txt => test/conformance/known_failures.txt +0 -6
@@ 42,10 42,4 @@ Required.Proto3.JsonInput.Uint64FieldMaxValueNotQuoted.ProtobufOutput
Required.Proto3.JsonInput.ValueAcceptNull.ProtobufOutput
Required.Proto3.JsonInput.WrapperTypesWithNullValue.JsonOutput
Required.Proto3.JsonInput.WrapperTypesWithNullValue.ProtobufOutput
Required.Proto3.ProtobufInput.BadTag_FieldNumberSlightlyTooHigh
Required.Proto3.ProtobufInput.BadTag_FieldNumberTooHigh
Required.Proto3.ProtobufInput.BadTag_OverlongVarint
Required.Proto3.ProtobufInput.IllegalZeroFieldNum_Case_0
Required.Proto3.ProtobufInput.IllegalZeroFieldNum_Case_1
Required.Proto3.ProtobufInput.IllegalZeroFieldNum_Case_3
Required.Proto3.TimestampProtoNegativeNanos.JsonOutput

M test/conformance_test.lua => test/conformance_test.lua +34 -0
@@ 546,6 546,40 @@ core_g.test_message_merge_concatenates_repeated_in_submessage = function()
        {1, 2, 3, 4, 5})
end

-- =========================================================================
-- Fix 7: tag validation in decode_tag. Field number 0 is illegal; field
-- numbers > 2^29-1 are illegal; tag varints must be minimally encoded.
-- The uint64-cdata bit ops are required so very high field numbers don't
-- alias back into the valid range after a 32-bit truncation.
-- =========================================================================

core_g.test_field_number_zero_rejected = function()
    -- IllegalZeroFieldNum: tag 0x00 = field 0, wt 0. One trailing byte.
    t.assert_not_equals(pb_roundtrip('\x00\x00').parse_error, nil)
end

core_g.test_field_number_far_too_high_rejected = function()
    -- 7-byte tag varint with bits in the highest byte that would put the
    -- field number > 2^29. tonumber+bit.rshift truncation would have
    -- mis-recovered fn=10 — the uint64 bit ops catch it.
    local input = '\xd2\x80\x80\x80\x80\x80\x0f\xd2\t'
    t.assert_not_equals(pb_roundtrip(input).parse_error, nil)
end

core_g.test_field_number_slightly_too_high_rejected = function()
    -- 5-byte tag with fn = 2^31+1, still > 2^29-1 → reject. The 32-bit
    -- truncation aliased this to fn=1 pre-fix.
    local input = '\x88\x80\x80\x80\x40\xd2\t'
    t.assert_not_equals(pb_roundtrip(input).parse_error, nil)
end

core_g.test_overlong_tag_varint_rejected = function()
    -- Tag 1, wt 0 encoded as 5 bytes (canonical is 1). Trailing byte 0
    -- with byte_count > 1 triggers the overlong check.
    local input = '\x88\x80\x80\x80\x00\x01'
    t.assert_not_equals(pb_roundtrip(input).parse_error, nil)
end

core_g.test_oneof_merge_still_clears_sibling_branches = function()
    -- The post-fix merge code must still clear oneof siblings: setting
    -- oneof_uint32 first, then merging two oneof_nested_message entries,