From 2ed0f4b5b17a1235cbcef16005370d430b2b5424 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Sat, 16 May 2026 00:41:59 +0300 Subject: [PATCH] wire: validate field number and minimal encoding in decode_tag MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- runtime/pb/wire.lua | 26 +++++++++++++++++----- test/conformance/known_failures.txt | 6 ----- test/conformance_test.lua | 34 +++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/runtime/pb/wire.lua b/runtime/pb/wire.lua index 7214f32381765b5cd4862059a06dec3dc62a2a2e..dda47bfc60616d091d075c4249b2f9da43f663ad 100644 --- a/runtime/pb/wire.lua +++ b/runtime/pb/wire.lua @@ -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 diff --git a/test/conformance/known_failures.txt b/test/conformance/known_failures.txt index 218f356551c11538af099c03efe6fa2be043f8d0..a1294cb1459836241ad4b229b9e8fe2e1fcf2cbe 100644 --- a/test/conformance/known_failures.txt +++ b/test/conformance/known_failures.txt @@ -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 diff --git a/test/conformance_test.lua b/test/conformance_test.lua index 76fb26eff53386ca0e5bdc69afd76c236e20ec33..34f862d2301769af0917658266a1b222768ec062 100644 --- a/test/conformance_test.lua +++ b/test/conformance_test.lua @@ -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,