Block PHP object injection when decoding term descriptions - #990
Merged
Conversation
Edit Flow stores pseudo term-meta as a base64-encoded serialized array in term descriptions and read it back with maybe_unserialize(), which would instantiate any object present in the payload. The callers only check the result with is_array() afterwards, so they could not prevent the object from being constructed first. Gate the decode with is_serialized() and unserialize with allowed_classes => false, so no object is ever instantiated while a plain description still passes through untouched. The encode side is unchanged, so existing stored data keeps working without a migration. Fixes VIPPLUG-9.
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
Edit Flow stores pseudo term-meta (custom-status position and viewability, user-group membership, and similar) as a base64-encoded serialized array tucked into a term's description field, and read it back through the shared
EF_Module::get_unencoded_description()helper usingmaybe_unserialize( base64_decode( ... ) ). Becausemaybe_unserialize()will happily instantiate any object encoded in the payload, that helper was a PHP object-injection surface, and the callers only inspect the result withis_array()after the unserialise has already run, so they could not prevent an object being constructed.The decode is now gated with
is_serialized()and usesunserialize()witharray( 'allowed_classes' => false ), so no object can ever be instantiated. A plain, unencoded description still passes through unchanged, and the encode side is untouched, so existing stored data continues to read correctly with no migration.An alternative was to migrate the format to JSON, but
allowed_classes => falsefully neutralises the object-injection threat with zero back-compat risk and without leaving a permanent legacy read path behind, so that is the route taken here.Test plan
composer csandparallel-lintpass on the changed files.ModuleTestclass passes.Fixes VIPPLUG-9.