Reject tensors with ndim > GGUF_TENSOR_MAX_DIM (fix OOB write in #25)#26
Open
ValheruEldarr wants to merge 1 commit into
Open
Reject tensors with ndim > GGUF_TENSOR_MAX_DIM (fix OOB write in #25)#26ValheruEldarr wants to merge 1 commit into
ValheruEldarr wants to merge 1 commit into
Conversation
…x OOB write, antirez#25) gguf_get_tensor read the tensor dimension count (ndim) from the file and wrote that many entries into the fixed uint64_t dim[GGUF_TENSOR_MAX_DIM] array, guarded only by an assert() that is compiled out under -DNDEBUG (the usual release build). A .gguf declaring ndim > 8 therefore caused an out-of-bounds write. Return 0 (stop iteration) instead. For valid tensors (ndim <= 8; the GGUF maximum is 4) behavior is unchanged.
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
1 participant
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.
Fixes the out-of-bounds write reported in #25 (CWE-787).
gguf_get_tensorread the tensor dimension countndimstraight from the file and wrote that many 64-bit values into the fixeduint64_t dim[GGUF_TENSOR_MAX_DIM]array (== 8). The only guard was anassert(tensor->ndim <= GGUF_TENSOR_MAX_DIM), which is compiled out under-DNDEBUG(the usual release build), so a.ggufdeclaringndim > 8wrote pastdim[].This returns 0 (stops the tensor iteration, exactly like the existing
*type >= GGUF_TYPE_COUNTrejection a few lines below) whenndim > GGUF_TENSOR_MAX_DIM, instead of writing out of bounds.Verified: built with
-DNDEBUG -fsanitize=address,undefined, the PoC from #25 (gguf-tools show poison.gguf) no longer reports a stack-buffer-overflow WRITE or theindex 8 out of bounds for type 'uint64_t[8]'UBSan error. For valid tensors (ndim <= 8; the GGUF spec maximum is 4) the new branch is not taken, so behavior is unchanged.Note: #25 also reports out-of-bounds reads in the KV / metadata parsing (for example
gguf_get_keyindexingctx->databy an unchecked in-filestr->len). Those require bounding eachctx->data + offsetaccess againstctx->sizeand are a larger, separate change; this PR addresses the memory-corruption write only.