Three small hardening notes from a security review: CSV formula-prefix neutralization, full C0 escaping in the JSON reporter, and type diagnostics in gbench/compare.py
- Dominant language
- C++
- Stars
- 10.4k
- Forks
- 1.8k
- Avg merge
- 2d 4h
- Merged PRs (30d)
- 8
Description
## BODY
Three small hardening notes from a security review of the library (static read at
commit `04b5f41e`, 2026-08-28). To state the conclusion first: the review found no
vulnerabilities. Every string that reaches the report writers — benchmark names,
counter names, labels, skip/error messages — is set by the benchmark's author, who
is also the author of the native code the user already builds and runs, so on the
library's own sinks there is no producer distinct from the code author, and the
review closed each of the three notes below with zero delta (no security impact).
They are offered purely as cheap defense-in-depth for the consumer side — results
artifacts and the report-reader tools — where files can travel to readers beyond
their author.
### Note 1 — CSV output: neutralize leading `=`, `+`, `-`, `@` in cell values
Where: `CsvEscape` (`src/csv_reporter.cc:35-49`) wraps a field in quotes and doubles
embedded quotes, but a field beginning with `=`, `+`, `-`, or `@` is otherwise
emitted verbatim. Author-set strings reach CSV cells at the counter-name header
(`src/csv_reporter.cc:81-84`), skip/error messages (`:112-118`), and report labels
(`:155-157`).
Why harden: CSV quoting does not stop spreadsheet applications from re-interpreting
a quoted cell that begins with one of those four characters as a formula (the
classic spreadsheet formula/DDE class — the OWASP CSV Injection page catalogs the
standard examples). Within the library's own trust model this is zero delta: anyone
running a third-party benchmark binary has already executed that author's native
code, which is strictly more capability than any formula payload, and current
spreadsheet builds warn before evaluating such content. But
`--benchmark_out_format=csv` artifacts are often shared onward — a results file
attached to a PR, a CI export opened by a maintainer who did not run the benchmark —
and those readers get no say in the benchmark's counter names. One branch in
`CsvEscape` removes the class for all of them.
Suggested change: in `CsvEscape`, if the field begins with any of `= + - @`, prefix
the cell content with a single quote `'` (or another documented neutral marker).
A small unit test covering the four prefixes plus an embedded quote/CR payload
would pin the behavior.
### Note 2 — JSON output: escape the full C0 control range in `StrEscape`
Where: `StrEscape` (`src/json_reporter.cc:37-69`) escapes `\b \f \n \r \t \" \\`
but passes every other C0 control character through raw inside JSON strings. The
sinks are the same author-set strings: benchmark name (`src/json_reporter.cc:253`),
error/skip messages (`:288-291`), counter keys and label (`:319-342`).
Why harden: RFC 8259 forbids raw U+0000–U+001F inside JSON strings, so a benchmark
name or skip message containing e.g. U+0001 or U+001B makes `--benchmark_out=json`
emit an artifact strict parsers reject. The review again closed this at zero delta —
the strings are author-trusted, and the in-repo consumers (tools/gbench,
tools/compare.py) go through Python's stdlib `json`, whose worst failure is a
`JSONDecodeError` on a developer's own machine. But any third-party tooling that
ingests `benchmark_out` JSON is entitled to the valid JSON the reporter's format
promises, and the fix is local: it cannot perturb any currently well-formed output.
Suggested change: in `StrEscape`, emit `\u00XX` for every remaining byte below
0x20 (a range check alongside the existing switch). Optionally also decide and
document a policy for invalid UTF-8 input bytes (currently passed through raw),
even if the policy is just replacement.
### Note 3 — gbench / compare.py: fail with a diagnostic instead of a traceback on malformed artifacts
Where: the report-reader path is the one place data can arrive from a third party —
a `benchmark_out` JSON supplied as a file. Shape and type mismatches currently
surface as bare exceptions deep in the formatting internals: `len(bc["name"])`
raises TypeError when the name is not a string (`tools/gbench/report.py:82-83`);
string-valued times hit the `{:+16.4f}`-style format specifiers and raise ValueError
(`:425-453`); an unknown `time_unit` yields a None multiplier that later TypeErrors
(`:192`); missing keys raise KeyError in partitioning; deeply nested input reaches
RecursionError via `json.load` (`tools/gbench/util.py:140-154`).
Why harden: this is robustness, not security, and the review closed it at zero
delta — the tool runs in the reader's own process at the reader's own privilege,
Python raises before anything memory-unsafe happens, and "CLI crashes on a
malformed input file" is ordinary issue territory. It still bites in practice:
someone comparing results attached to a PR, or produced by a different or older
benchmark version, gets a stack trace instead of a reason.
Suggested change: one shape check at artifact load time — names are strings, times
are numeric, the expected per-run keys are present, `time_unit` is known — rejecting
with a message that names the offending field, e.g. `a.json: run[3].name is not a
string`. A single check at load keeps the policy out of the formatting internals
and covers compare.py and the gbench classes at once.
### What this is and is not
All three notes come from one review pass and none claims a vulnerability: under
the library's input model the strings involved are author-set, the review's impact
analysis closed each one at zero delta, and the suggested changes are
defense-in-depth for artifact consumers only. Each is small and self-contained;
happy to send PRs for any or all of them.
Contributor guide
Research direction
Start with CsvEscape in src/csv_reporter.cc and StrEscape in src/json_reporter.cc, then trace artifact loading through tools/gbench/report.py and tools/gbench/util.py. Confirm the four CSV prefixes are neutralized, all remaining C0 bytes are escaped in JSON, and malformed benchmark artifacts are rejected with field-specific diagnostics.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, python
- Domain
- cli, security, tooling
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 64/100