Security review findings - #8
Open
alvagante wants to merge 2 commits into
Open
Conversation
A security review of the tree found three places where the code did not do what its own comment claimed, plus two smaller ones. All five are fixed here with regression tests; the first two are confirmed exploitable and were verified against live net/url, net/http and git behaviour rather than reasoned about. A File `source` value reached the compiler's file_content endpoint as an unescaped path concatenation. It is the only one in the tree: every other URL this codebase builds escapes its interpolation or is a constant. A `source` is a parameter of a File resource in the candidate catalog, and the candidate catalog is compiled from the change under review, so `puppet:///../../pdb/query/v4/catalogs/<node>` was attacker-shaped input steering an mTLS-authenticated GET. net/url does not remove dot segments from a path it is handed and net/http sends the request line as written, so the traversal survived intact to the wire. Bounded by the transport, which pins scheme and authority and refuses a cross-authority redirect, and by this package never returning retrieved bytes, so it reached other paths on a host PIACE is already authorized against and yielded a digest rather than content. parsePuppetSourceURI now returns an error and refuses an empty, "." or ".." segment and a NUL byte. The inference client had no redirect policy. internal/transport, which carries no credential, has an explicit and tested one; internal/inference, which carries the only bearer token in PIACE, had none, and that asymmetry was the defect. net/http keeps an Authorization header across a redirect whenever the target host is the original host or a subdomain of it, judged on the host alone, so an endpoint answering `302 Location: http://<same host>` got the token in cleartext. checkRedirect now refuses any redirect that leaves https or changes authority. HTTPClient becomes unexported behind SetHTTPClient at the same time: an exported field that five tests and cmd/piace assigned to was a policy that could be dropped by assignment, which is the wrong property for the one client holding a credential. The text report passed control characters through. It is the format that goes to a terminal, and almost everything it interpolates is untrusted: a resource title and a parameter value come from the candidate catalog, a change assessment's summary is prose a model wrote. `ESC[2J ESC[1;1H` followed by a forged outcome line is enough to make a run that exits 30 read as clean to the person deciding whether to merge it. One textf helper now escapes C0, DEL and C1 in every line, preserving only the newlines the format string itself contributes. The JSON and HTML reports are deliberately unchanged: JSON escaping already makes a control character inert and that document's canonical checksum is what explain ties an assessment to, and html/template covers the HTML. Refs reaching git are validated. On a fork pull request the base and head refs are branch names whoever opened the change chose, and a ref of `--output=<path>` reaching `git diff` positionally is an arbitrary file write on the runner holding the catalog-reader identity. As it happens `merge-base` runs first and rejects an unknown option, so that value dead-ends before `diff`, but a guard that depends on the argument order of the first of four commands is not a guard. validateRef refuses a leading "-" before any git command runs, and --end-of-options covers what a future reordering might otherwise let through. Two smaller ones. The inference client read its response body through a LimitReader without the extra byte internal/transport uses to tell a body that reached the limit from one that exceeded it, so an oversized response was reported as "not a chat completion" and sent an operator looking at the wrong thing. And --debug-dump-dir wrote through os.WriteFile, which follows a symlink already sitting at the path and leaves that file's existing mode, applying 0600 only to a file it creates itself; those bodies are the unredacted ones and a dump directory is often /tmp. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
- Anthropic's OpenAI-compatible endpoint documents itself as ignoring `response_format` rather than rejecting it, so a request that relied on structured output alone asked for nothing. TaskPrompt now describes the exact JSON shape unconditionally, and a test holds it to ResponseSchema so the two cannot drift. - Interpret unwraps a Markdown code fence that wraps the whole response, recording a warning, rather than discarding a complete assessment over its packaging. Only a fence that both opens and closes the trimmed reply is removed; backticks inside a rationale are left alone. - Label the impact-estimate section "Potential impact estimate" with a capital P across every output format and its golden files. - Color a successful differences_allowed run green: it shares exit 0 with clean, and yellow is reserved for the advisory medium-risk indication, not for a run that succeeded.
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.
No description provided.