Add field ownership check to SwapFields to prevent OOB memory access#27289
Open
jortles wants to merge 3 commits into
Open
Add field ownership check to SwapFields to prevent OOB memory access#27289jortles wants to merge 3 commits into
jortles wants to merge 3 commits into
Conversation
SwapFieldsImpl() validates that messages belong to this Reflection object, but does not check that each field descriptor in the fields vector actually belongs to the message's descriptor. When a field from a different message type is passed, its index can exceed the bounds of the target's offsets_ array, causing an out-of-bounds read that produces an arbitrary write offset for the subsequent std::swap. Add an ABSL_CHECK_EQ verifying field->containing_type() == descriptor_ for each field before processing, consistent with USAGE_CHECK_MESSAGE_TYPE used by all other Reflection write methods (SetInt32, SetString, etc.). Added regression test SwapFieldsForeignFieldCheck that verifies SwapFields rejects a field from a different message type.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reflection::SwapFieldsImpl()validates that both messages belong to the correct Reflection object (GetReflection() == this), but does not verify that each field descriptor in thefieldsvector belongs to the message's own descriptor. This means a field from a different message type can be passed in, and itsindex()value can exceed the bounds of the target descriptor'soffsets_array.When this happens,
GetFieldOffset()atgenerated_message_reflection.h:127readsoffsets_[field->index()]out of bounds, producing an arbitrary offset value. The subsequentstd::swapthen writes 8 bytes at that offset relative to the message pointer, corrupting adjacent heap memory.Root cause
Every other
Reflectionwrite method (SetInt32,SetString,HasField,ClearField,Swap,RemoveLast,MutableRawRepeatedField, etc.) includesUSAGE_CHECK_MESSAGE_TYPEwhich validatesfield->containing_type() == descriptor_. TheSwapFieldsImplpath was the only write method missing this check.Fix
Add
ABSL_CHECK_EQ(field->containing_type(), descriptor_)for each field at the top of the loop inSwapFieldsImpl, before any field operations. This matches the validation pattern used by all other Reflection methods.Impact
Without this check, passing a foreign field descriptor causes:
offsets_[]array (reads heap data as a field offset)std::swapat the corrupted offset (overwrites adjacent heap objects)Test plan
SwapFieldsForeignFieldCheckdeath test: passes aForeignMessagefield toSwapFieldson aTestAllTypesmessage, verifies the check fires with "does not belong to message type"UsageErrorsdeath testSwapFields*tests should pass unchanged (they only use fields from the correct descriptor)