Repository navigation
Fixed out-of-bounds read in string_replace with nested list elements - #6483
Open
Dhanu-dynamic wants to merge 1 commit into
Open
Dhanu-dynamic wants to merge 1 commit into
Dhanu-dynamic wants to merge 1 commit into
Conversation
StringReplaceWithLists(), the string_replace(..., "lists") policy
function, passes each element of the match and substitute arrays
directly to JsonPrimitiveGetAsString():
const char *m = JsonPrimitiveGetAsString(JsonAt(match_list, i));
const char *s = JsonPrimitiveGetAsString(JsonAt(substitute_list, i));
That function returns primitive.value, which in the JsonElement union
aliases container.children (a Seq *). For an element that is itself an
array or object it therefore returns the internal container pointer
reinterpreted as a char *, and SearchAndReplace() then reads it as a
NUL-terminated string. FnCallStringReplace() only checks that the two
arguments are arrays of equal length, not that every element is a
primitive, so a nested element reaches the unchecked call. These lists
commonly come from data containers (readjson, parsejson, mergedata,
module output), so JSON with a nested element triggers it.
With NDEBUG the read runs off the Seq object into adjacent heap memory
and those bytes, including internal pointers, end up in the returned
string; otherwise the assertion in JsonPrimitiveGetAsString() aborts,
which cf-promises hits during policy validation.
Reject any non-primitive element before the conversion, reusing the
function's existing warn-and-fail path.
Changelog: Title
Signed-off-by: Dhanraj E <dhanraj@labs.digiscrypt.com>
|
Thank you for submitting a PR! Maybe @craigcomstock can review this? |
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.
string_replace(string, match, substitute, "lists")passes every element of the match andsubstitute arrays to
JsonPrimitiveGetAsString()inStringReplaceWithLists(). For a primitivethat returns
primitive.value, but for a nested array or object that field aliasescontainer.children(aSeq *), so the internal pointer comes back as achar *andSearchAndReplace()reads it as a string.FnCallStringReplace()only checks that both argumentsare arrays of equal length, not that the elements are primitives, so a nested element reaches the
call. The lists are usually built from data containers (
readjson,parsejson,mergedata,module output), so JSON carrying a nested element reaches it.
In a release build (NDEBUG) this is an out-of-bounds heap read whose bytes, including internal
pointers, are copied into the returned string, and it can crash depending on the heap layout. In a
debug build the
assertinJsonPrimitiveGetAsString()aborts instead, whichcf-promiseshitswhile validating the policy.
The fix rejects any non-primitive element in
StringReplaceWithLists()before the conversion,using the warn-and-fail path the function already has.
Repro:
string_replace("abcb", '["b"]', '[["Z"]]', "lists"). Withjson.cbuilt-DNDEBUGandASan the result contains heap bytes where
bwas; in the normal debug buildcf-promises -caborts in
JsonPrimitiveGetAsString. The new unit test inevalfunction_test.caborts before thechange and passes after, and checks that a valid flat-list replace still returns
aZcZ.