From dffd67030c078f19600a3948f14f8efb30de1df5 Mon Sep 17 00:00:00 2001 From: baradika Date: Sat, 29 Aug 2026 15:42:02 +0700 Subject: [PATCH] upb: reject non-delimited unknowns when promoting to message extensions upb_MiniTable_ParseUnknownMessage consumed an unknown field as a length-delimited message without first verifying its wire type. upb_Message_FindUnknown2 matches unknowns by field number only and deliberately does not constrain the wire type, so an unknown of any other wire type could be returned for a message-typed extension's field number and then be misinterpreted as length-delimited. In that case the field's own payload is read as the message length while the data pointer is already at the end of the unknown field, leaving the size passed to upb_Decode under attacker control and producing an out-of-bounds read. Check the wire type immediately after reading the tag and return the existing parse error for non-delimited types. Only the delimited wire type carries a length prefix, so this rejects precisely the inputs that the length-prefix assumption does not hold for. upb_Message_FindUnknown2 is intentionally left unchanged: its generic field-number semantics are required by its other callers. Add regression tests for a zero-length varint payload (silently promoted to an empty message before this change) and for a 0xFFFFFFFF payload (out-of-bounds read caught by ASan), plus a control case confirming a well-formed length-delimited unknown still promotes. --- upb/message/promote.c | 11 ++++- upb/message/promote_test.cc | 85 +++++++++++++++++++++++++++++++++++++ 2 files changed, 95 insertions(+), 1 deletion(-) diff --git a/upb/message/promote.c b/upb/message/promote.c index 46020d4a4621a..a744c88f7cf2a 100644 --- a/upb/message/promote.c +++ b/upb/message/promote.c @@ -54,8 +54,17 @@ static upb_UnknownToMessageRet upb_MiniTable_ParseUnknownMessage( uint32_t tag; uint64_t message_len = 0; // This assumes that the unknown field is a message type, so it is - // a delimited wire type. + // a delimited wire type. Verify that instead of assuming it: + // `upb_Message_FindUnknown2` matches on field number alone, so an unknown + // of any other wire type can arrive here, and only the delimited wire + // type carries a length prefix. Reading one from, say, a varint would + // make `message_len` attacker-controlled and `upb_Decode` below would + // read out of bounds. ptr = upb_WireReader_ReadTag(ptr, &tag, NULL); + if (upb_WireReader_GetWireType(tag) != kUpb_WireType_Delimited) { + ret.status = kUpb_UnknownToMessage_ParseError; + return ret; + } ptr = upb_WireReader_ReadVarint(ptr, &message_len, NULL); data = ptr; size = message_len; diff --git a/upb/message/promote_test.cc b/upb/message/promote_test.cc index e467ae56c2465..d668953ecc55b 100644 --- a/upb/message/promote_test.cc +++ b/upb/message/promote_test.cc @@ -708,4 +708,89 @@ TEST(GeneratedCode, PromoteNonCanonicalExtensionWithDifferentMinitable) { upb_FindUnknownRet found = upb_Message_FindUnknown(UPB_UPCAST(msg), 1547, 0); EXPECT_EQ(kUpb_FindUnknown_NotPresent, found.status); } +// Regression test for a wire-type confusion bug in extension promotion. +// +// `upb_Message_FindUnknown2` matches unknown fields on field number alone and +// does not constrain the wire type, but `upb_MiniTable_ParseUnknownMessage` +// treats the bytes that follow the tag as a length prefix. A varint field at a +// message-typed extension's field number therefore turns its own payload into +// `message_len`, leaving `data` at the end of the field and `size` under +// attacker control; `upb_Decode` then reads out of bounds. +// +// Field 1547 (`model_ext`) encoded with wire type 0 (varint) instead of wire +// type 2 (length-delimited): +// d8 60 -> tag: field 1547, wire type 0 +// -> payload; promotion misreads this as `message_len` +TEST(GeneratedCode, PromoteExtensionRejectsNonDelimitedWireType) { + // Payload 0: once the payload is misread as a length prefix, `size` is 0 + // and the promotion silently succeeds with an empty message unless the wire + // type is checked. A bare bounds check cannot catch this, since 0 <= 0. + const char kZeroLength[] = {'\xd8', '\x60', '\x00'}; + upb::Arena arena; + upb_test_ModelWithExtensions* msg = + upb_test_ModelWithExtensions_new(arena.ptr()); + EXPECT_EQ(kUpb_DecodeStatus_Ok, + upb_Decode(kZeroLength, sizeof(kZeroLength), UPB_UPCAST(msg), + &upb_0test__ModelWithExtensions_msg_init, nullptr, 0, + arena.ptr())); + // The field reached unknowns (the host does not know field 1547). + EXPECT_EQ(kUpb_FindUnknown_Ok, + upb_Message_FindUnknown(UPB_UPCAST(msg), 1547, 0).status); + + upb_MessageValue val; + EXPECT_EQ(kUpb_GetExtension_ParseError, + upb_Message_GetOrPromoteExtension( + UPB_UPCAST(msg), upb_test_ModelExtension1_model_ext_ext, 0, + arena.ptr(), &val)); + EXPECT_EQ(0, upb_Message_ExtensionCount(UPB_UPCAST(msg))); + + // Payload 0xFFFFFFFF: the misread length makes `upb_Decode` read far past + // the end of the unknown field. This must be rejected, not read. + const char kHugeLength[] = {'\xd8', '\x60', '\xff', '\xff', + '\xff', '\xff', '\x0f'}; + upb::Arena arena2; + upb_test_ModelWithExtensions* msg2 = + upb_test_ModelWithExtensions_new(arena2.ptr()); + EXPECT_EQ(kUpb_DecodeStatus_Ok, + upb_Decode(kHugeLength, sizeof(kHugeLength), UPB_UPCAST(msg2), + &upb_0test__ModelWithExtensions_msg_init, nullptr, 0, + arena2.ptr())); + EXPECT_EQ(kUpb_GetExtension_ParseError, + upb_Message_GetOrPromoteExtension( + UPB_UPCAST(msg2), upb_test_ModelExtension1_model_ext_ext, 0, + arena2.ptr(), &val)); +} + +// Control case: a well-formed length-delimited unknown at the extension's +// field number still promotes, confirming the check above rejects only +// genuinely malformed input. +TEST(GeneratedCode, PromoteExtensionStillAcceptsValidDelimitedUnknown) { + upb::Arena arena; + upb_test_ModelWithExtensions* msg = + upb_test_ModelWithExtensions_new(arena.ptr()); + + // Field 1547, wire type 2, length 5; body is field 25 (str) = "Hi": + // ca 01 -> tag: field 25, wire type 2 + // 02 -> length 2 + // 48 69 -> "Hi" + const char kBody[] = {'\xca', '\x01', '\x02', '\x48', '\x69'}; + const char kWireBytes[] = {'\xda', '\x60', sizeof(kBody), kBody[0], + kBody[1], kBody[2], kBody[3], kBody[4]}; + + EXPECT_EQ(kUpb_DecodeStatus_Ok, + upb_Decode(kWireBytes, sizeof(kWireBytes), UPB_UPCAST(msg), + &upb_0test__ModelWithExtensions_msg_init, nullptr, 0, + arena.ptr())); + + upb_MessageValue val; + ASSERT_EQ(kUpb_GetExtension_Ok, + upb_Message_GetOrPromoteExtension( + UPB_UPCAST(msg), upb_test_ModelExtension1_model_ext_ext, 0, + arena.ptr(), &val)); + upb_StringView str = + upb_test_ModelExtension1_str((upb_test_ModelExtension1*)val.msg_val); + EXPECT_EQ(absl::string_view(str.data, str.size), "Hi"); + EXPECT_EQ(1, upb_Message_ExtensionCount(UPB_UPCAST(msg))); +} + } // namespace