KhronosGroup / KhronosGroup/SPIRV-Reflect
Out-of-bounds read: literal strings are read with unbounded strlen / unchecked offsets
- Dominant language
- C
- Stars
- 871
- Forks
- 188
- Avg merge
- 19m
- Merged PRs (30d)
- 1
Description
# Out-of-bounds read: literal strings are read with unbounded strlen / unchecked offsets
Verified against `main` (`spirv_reflect.c`).
## Description
Five sites take a raw pointer into the SPIR-V words and read a literal string from it without bounding the
read to the module. The asymmetry with the surrounding code is what makes these look like oversights: the
scalar reads on the neighbouring lines go through `CHECKED_READU32` (which bounds-checks via `InRange`),
and the file already has a bounds-safe string reader in `ReadStr()` (it checks
`InRange(p_parser, word_offset + word_count)` and bounds its scan to `word_count * SPIRV_WORD_SIZE`).
These five bypass all of it.
`ParseNodes`, `SpvOpSource`:
```c
if (p_node->word_count >= 5) {
const char* p_source = (const char*)(p_parser->spirv_code + p_node->word_offset + 4);
const size_t source_len = strlen(p_source);
```
`ParseNodes`, `SpvOpSourceContinued`:
```c
case SpvOpSourceContinued: {
const char* p_source = (const char*)(p_parser->spirv_code + p_node->word_offset + 1);
const size_t source_len = strlen(p_source);
```
`ParseNodes`, `SpvOpName` / `SpvOpMemberName`:
```c
uint32_t member_offset = (p_node->op == SpvOpMemberName) ? 1 : 0;
uint32_t name_start = p_node->word_offset + member_offset + 2;
p_node->name = (const char*)(p_parser->spirv_code + name_start);
```
`ParseStrings`, `SpvOpString`:
```c
// String
uint32_t string_start = p_node->word_offset + 2;
p_string->string = (const char*)(p_parser->spirv_code + string_start);
```
Two distinct problems across those sites:
1. **`strlen` is unbounded.** SPIR-V literal strings are NUL terminated and word padded, but a truncated or
malformed module can end without a terminator, and the scan then runs off the end of the module.
2. **The offsets are never range checked.** `name_start` and `string_start` are not validated against
`p_parser->spirv_word_count`, so for a malformed instruction near the tail the pointer is already past
the buffer before anything dereferences it, and it is *stored* and read later. `ParseStrings`' pointer
escapes to the caller through `ParseSource`:
```c
if (p_string->result_id == p_parser->source_file_id) {
p_module->source_file = p_string->string;
```
so `SpvReflectShaderModule::source_file` can be handed back pointing outside the module.
Underlying all of them: `ParseNodes` never checks that an instruction fits in the module. The walk only
validates the *header* word (`node_word_count == 0` is rejected), never
`word_offset + word_count <= spirv_word_count`, so any node can claim a length that overruns the blob.
## Reproduction
Reflect a real module, then truncate it at successive 4-byte boundaries and reflect each truncation. Any
truncation that cuts an `OpName`, `OpString`, `OpSource` or `OpSourceContinued` string mid-instruction
reads past the end. This is visible under the ASan build already in CI
(`SPIRV_REFLECT_ENABLE_ASAN`) on the default copy path, since the module is a `calloc(1, size)` allocation.
It is also observable with `SPV_REFLECT_MODULE_FLAG_NO_COPY` by placing the module so its last byte is the
last byte of a page followed by a `PROT_NONE` / `PAGE_NOACCESS` page, with `NO_COPY` the parser walks the
caller's buffer directly, so there is no allocation slack to absorb the overread.
## Impact
Out-of-bounds reads on caller-supplied data, plus a past-the-end pointer retained on the node and (for
`OpString`) exposed to the caller as `source_file`. Callers that reflect untrusted, truncated or otherwise
malformed SPIR-V can be crashed or can read memory outside the module.
## Notes
Issue #142 ("Crash handling SpvOpSourceContinued", open since 2022) partially overlaps: it covers the
`strcat_s` destination-size problem and the NULL `source_embedded` dereference in the same case, but not
the unbounded `strlen(p_source)`. The `OpSource`, `OpName`/`OpMemberName` and `OpString` sites appear to be
unreported.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in spirv_reflect.c, focusing on ParseNodes and ParseStrings, and compare the affected literal-string sites with the existing bounds-safe ReadStr() and CHECKED_READU32 paths. Use the truncation reproduction under the ASan build, including OpName, OpString, OpSource, and OpSourceContinued cases. Done means malformed or truncated modules no longer read or retain pointers beyond spirv_word_count.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- c
- Domain
- compilers, security
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 58/100