fix(security): YAML criterion path containment + SQL read-only gate [2 HIGH]#68
Merged
Merged
Conversation
Closes both HIGH findings from the post-PR-67 certification audit.
1. resolvePath escape in internal/engine/criteria.go (HIGH).
resolvePath returned c.Path verbatim when filepath.IsAbs was true,
letting a YAML criterion read arbitrary host files. A poisoned
repo's vxd.yaml could ship
- kind: file_contains
path: /etc/shadow
value: root
The CriterionResult.Detail then flowed back through the WebSocket
command_result response, exfiltrating the file contents to anyone
with dashboard read access. The same path also accepted `..`
traversals that collapsed back inside workDir via the OS at open
time. Now: absolute paths rejected outright, joined+cleaned form
must remain a descendant of cleaned workDir, empty paths rejected.
resolvePath now returns (string, error) so the criterion handlers
surface the rejection in the standard CriterionResult.Detail
channel. Four new tests pin the boundary (absolute, traversal,
contained relative, empty).
2. evaluateSQLQueryReturns SQL injection (HIGH).
The YAML-supplied SQL for the sql_query_returns criterion was
passed straight to conn.Query with no classifier and no read-only
transaction. vxd db sql already had a three-layer defence
(ValidateSQLForReadOnly + BEGIN READ ONLY + --write gating); the
same logic now protects the QA path.
Moved internal/cli/sql_safety.go + its tests to a new
internal/sqlsafety package so both engine and cli can call the
exact same classifier without cli↔engine import inversion. cli/db.go
updated to import sqlsafety; behaviour unchanged for vxd db sql.
engine/criteria_db.go now calls sqlsafety.ValidateSQLForReadOnly
(no --write override — QA criteria are never expected to mutate)
and wraps execution in pgx.BeginTx with AccessMode: pgx.ReadOnly.
Postgres itself rejects any DDL/DML that slipped past the classifier.
Eight new tests in criteria_db_safety_test.go cover plain DROP,
INSERT, multi-statement with SELECT prefix, CTE-DELETE-RETURNING,
EXPLAIN ANALYZE INSERT, empty-after-stripping, plus four positive
cases (SELECT/SHOW/VALUES/TABLE) confirming the gate passes legitimate
SQL.
All 28 packages still pass with -count=1.
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.
Summary
Closes both HIGH findings from the post-PR-67 certification audit. Both are exploitable through a malicious or copy-pasted `vxd.yaml` with zero privileges beyond the normal submission path.
YAML criterion path containment
Four new tests: `TestResolvePath_RejectsAbsolute`, `_RejectsTraversal`, `_AcceptsContainedRelative`, `_RejectsEmpty`.
SQL read-only gate
Eight new tests cover plain DROP, INSERT, multi-statement with SELECT prefix, CTE-DELETE-RETURNING, EXPLAIN ANALYZE INSERT, empty-after-stripping, plus four positive cases (SELECT/SHOW/VALUES/TABLE).
Test plan