varnish / varnish/tinykvm

ELF loader: section/symbol header fields used as offsets without validation against binary.size()

Open
#100 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
C++
Stars
821
Forks
23
PR merge metrics
No merged PRs in 30d

Description

Summary

Four related defects in lib/tinykvm/machine_elf.cpp. They share one root
cause — section-header and symbol-table fields are used as offsets and counts
without being validated against binary.size(), and the two range checks that
do exist add before they check, so they wrap. Grouping them because the fix is
one coherent piece of work.

All four are reachable on attacker-supplied ELF bytes. Three are reachable
from an ordinary guest fault, because handle_exception calls resolve()
(vcpu_run.cpp:615) to symbolise the faulting address.


1. resolve() trusts snprintf's would-be length (:331)

snprintf returns the length it would have written, not the length it wrote.
The result is passed straight to std::string(result, len), which then copies
len bytes out of a 2048-byte stack buffer.

An ELF with a 2085-character symbol name made resolve() return a string of
length 2094 — the maximum correct value is 2047 — with 0x00055c21… (PIE
base) and 0x00007ffd… (stack) visible in the tail. A 16 MB name gives an ASan
SEGV in memmove from :334.

Worth knowing for anyone re-testing this: ASan does not catch the moderate
over-read.
std::string's large copy compiles to an inline rep movsb,
which is not instrumented. The length check is the evidence.

Fix: if (len > 0 && (size_t)len < sizeof(result)) in both paths of
resolve().

2. section_by_name() indexes the section table unvalidated (:245)

Only the first Elf64_Shdr is bounds-checked; the loop then walks shdr[i]
for i < e_shnum past EOF, and strcmp(strings + shdr[i].sh_name, …) chases
whatever offsets it finds there.

e_shnum = 0xFFFF gives an ASan SEGV at :250/:251, via both the embedder
AddressOf path and the guest-fault → resolve path.

Fix: validate e_shstrndx < e_shnum, check
e_shoff + e_shnum * sizeof(Elf64_Shdr) <= binary.size() with the addition
overflow-guarded, and require sh_name < shstrtab.sh_size.

3. resolve_symbol() / resolve() trust .symtab sh_size and st_name (:272)

sh_size = 0x1000000 declared on a 24-byte .symtab, plus a wild
st_name = 0xFFFFF0, gives an ASan SEGV in strcmp at :278; the
guest-reachable variant SEGVs at :326 via handle_exception. Where the wild
st_name lands on mapped memory instead, snprintf("%s", &strtab[st_name])
turns it into a host-memory-content disclosure channel rather than a crash.

Fix: overflow-checked sh_offset + sh_size <= binary.size() for both .symtab
and .strtab; require st_name < str_hdr->sh_size.

4. is_dynamic_elf() adds before it checks (:25, :36)

A PT_INTERP phdr with p_offset = 0xFFFFFFFFFFFFFFF0 and p_filesz = 0x20
sums to 16, which passes the check at :36. ASan reports a
heap-buffer-overflow READ of size 32 at :40.

This is reached from plain Machine construction — elf_loader calls
is_dynamic_elf() at :74, before its own phdr sanity checks — so it is not
dead code. src/simple.cpp:51 also shows embedders calling it directly on
loaded binaries. The e_phoff + e_phnum * 56 computation at :25 has the same
shape.

Fix: check-then-add guards mirroring the ones already at :93 and :208.


Suggestion

The four fixes are the same fix four times. An overflow-checked range accessor
— something like std::string_view slice(off, len) that throws unless
off <= size && len <= size - off — applied at every point where a header
field becomes an offset would close these and make the next one hard to
reintroduce.

Tests

tests/unit/elfmal.cpp (new file, 5 cases) — cand1 through cand5. Several of
these crash the process rather than failing an assertion, so the cases are run
individually by name filter.

Harness note

fuzz/fuzz.cpp never reaches any of this: it only drives elf_loader, and
the symbol paths are only reachable through AddressOf / address_of /
resolve, which no fuzz target calls. machine->run() is also commented out
at fuzz.cpp:28, so the handle_exception → resolve path has zero coverage.
We're preparing a PR that adds a second fuzz target for exactly this.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in lib/tinykvm/machine_elf.cpp at is_dynamic_elf(), section_by_name(), resolve_symbol(), and resolve(), then review the call paths through vcpu_run.cpp:615, elf_loader, and src/simple.cpp:51. Run the individually filtered cand1–cand5 cases in tests/unit/elfmal.cpp; done means malformed ELF offsets, counts, names, and lengths no longer read outside binary.size() or disclose host memory.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
operating-systems, security
Issue type
Bug
Difficulty
5/5
Estimated time
Over a week
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.