diff --git a/vulnhunter-agent/agent/issues_render.py b/vulnhunter-agent/agent/issues_render.py index 76feb63..4a46473 100644 --- a/vulnhunter-agent/agent/issues_render.py +++ b/vulnhunter-agent/agent/issues_render.py @@ -101,6 +101,35 @@ def render_title(f: Finding) -> str: _MD_ESCAPE_CHARS = ("\\", "`", "[", "]") +# Machine-readable footer markers (```` etc.) +# are intentionally NOT html.escaped so the marker regexes stay exact. But the +# values (``f.id``, ``report.results_dir_name``) are attacker-influenced +# LLM/scan-README data, so a value containing ``-->`` would close the comment +# and inject arbitrary markdown/HTML (CWE-79 / CWE-116). Restrict marker values +# to a strict identifier charset — the same class the downstream parsers accept +# (``VULN-\d{3}`` / ``[A-Za-z0-9._-]+_VULNHUNT_RESULTS_...``) — dropping any +# other char. Removing '<' and '>' makes a comment breakout impossible while +# keeping legitimate ids/dir-names intact. +_MARKER_UNSAFE_RE = re.compile(r"[^A-Za-z0-9._-]") + + +def _sanitize_marker_value(value: str, marker: str = "marker") -> str: + """Restrict an HTML-comment marker value to a safe identifier charset so it + cannot break out of the surrounding ```` (CWE-116). + + Stripping can turn a value into a *different* identifier (e.g. a + results-dir name that now points at another directory), so any change is + logged as a warning rather than happening silently.""" + raw = value or "" + cleaned = _MARKER_UNSAFE_RE.sub("", raw) + if cleaned != raw: + logger.warning( + "Stripped out-of-charset characters from %s marker value: %r -> %r", + marker, raw, cleaned, + ) + return cleaned + + def _sanitize_for_issue_body(value: str) -> str: """Neutralize attacker-influenced Finding text before it lands in a GitHub issue body (CWE-79). @@ -118,6 +147,40 @@ def _sanitize_for_issue_body(value: str) -> str: return s +def _sanitize_inline(value: str) -> str: + """``_sanitize_for_issue_body`` for fields rendered on a single line — + the ``### Finding`` table cells and the H2 title / Priority line. + + Additionally escapes ``|`` as ``\\|`` (GFM's in-cell pipe escape; it + renders as a plain ``|`` outside a table too) so a value cannot add or + forge table cells, and collapses CR/LF runs to one space so it cannot end + the row/heading and inject block markdown (PR #9 review, CWE-116). + Multi-line free-form sections keep using ``_sanitize_for_issue_body``. + """ + s = _sanitize_for_issue_body(value).replace("|", "\\|") + return re.sub(r"[\r\n]+", " ", s) + + +_PLACEHOLDER_RE = re.compile(r"\{([A-Z_]+)\}") + + +def _substitute(template: str, fields: dict[str, str]) -> tuple[str, set[str]]: + """Fill ``{NAME}`` placeholders in ONE pass over the template. + + Field-by-field ``str.replace`` re-scanned already-substituted values, so + attacker text containing e.g. ``{IDEMPOTENCY_KEY}`` was expanded by a + later pass. Returns (body, template placeholders with no field).""" + missing: set[str] = set() + + def repl(m: re.Match[str]) -> str: + if m.group(1) in fields: + return fields[m.group(1)] + missing.add(m.group(1)) + return m.group(0) + + return _PLACEHOLDER_RE.sub(repl, template), missing + + def render_body( f: Finding, *, @@ -132,14 +195,17 @@ def render_body( anchor = _github_anchor(f"{f.id}: {f.title}") if f.id else "" deep_link = f"{report_url}#{anchor}" if anchor else report_url fields = { - # Attacker-influenced Finding fields — sanitized (CWE-79). - "TITLE": _sanitize_for_issue_body(f.title or "(untitled)"), - "CWE": _sanitize_for_issue_body(f.cwe or "Unknown CWE"), - "CWE_NAME": _sanitize_for_issue_body(f.cwe_name or "(unspecified)"), - "SEVERITY": _sanitize_for_issue_body(f.severity or "Unspecified"), - "LOCATION": _sanitize_for_issue_body(f.location or "(not specified)"), - "ROOT_CAUSE": _sanitize_for_issue_body(f.root_cause or "(not specified)"), - "DATA_FLOW": _sanitize_for_issue_body(f.data_flow or "(not specified)"), + # Attacker-influenced Finding fields — sanitized (CWE-79). Fields that + # render on a single line (``### Finding`` table cells, the H2 title, + # the Priority line) also get '|' escaped and CR/LF collapsed. + "TITLE": _sanitize_inline(f.title or "(untitled)"), + "CWE": _sanitize_inline(f.cwe or "Unknown CWE"), + "CWE_NAME": _sanitize_inline(f.cwe_name or "(unspecified)"), + "SEVERITY": _sanitize_inline(f.severity or "Unspecified"), + "LOCATION": _sanitize_inline(f.location or "(not specified)"), + "ROOT_CAUSE": _sanitize_inline(f.root_cause or "(not specified)"), + "DATA_FLOW": _sanitize_inline(f.data_flow or "(not specified)"), + # Free-form multi-line sections — newlines are legitimate here. "EXPLOIT_DESCRIPTION": _sanitize_for_issue_body( f.exploit_description or "(not specified in report)" ), @@ -155,14 +221,16 @@ def render_body( "SCAN_DATE": report.scan_date, "REPORT_URL": deep_link, "REPORT_ACCESS_MESSAGE": _report_access_message(), - "IDEMPOTENCY_KEY": f.vulnfix_key, - "VULN_ID": f.id, - "RESULTS_DIR_NAME": report.results_dir_name, + # Marker values are NOT html.escaped (keeps the marker regexes exact) + # but ARE restricted to a safe identifier charset so an attacker- + # influenced value cannot break out of the HTML comment (CWE-116). + "IDEMPOTENCY_KEY": _sanitize_marker_value(f.vulnfix_key, "vulnfix-key"), + "VULN_ID": _sanitize_marker_value(f.id, "vulnhunt-finding-id"), + "RESULTS_DIR_NAME": _sanitize_marker_value( + report.results_dir_name, "vulnhunt-results-dir" + ), } - body = template - for key, value in fields.items(): - body = body.replace("{" + key + "}", value) - leftovers = _find_placeholders(body) + body, leftovers = _substitute(template, fields) if leftovers: logger.warning( "Template placeholders unfilled in rendered body: %s", sorted(leftovers) @@ -170,10 +238,6 @@ def render_body( return body -def _find_placeholders(body: str) -> set[str]: - return set(re.findall(r"\{([A-Z_]+)\}", body)) - - @dataclass(frozen=True) class CleanScanContext: """Values needed to render a clean-scan issue or comment. @@ -251,10 +315,7 @@ def _render_clean_scan(ctx: CleanScanContext, template_path: Path) -> str: "SKILL_VERSION": ctx.skill_version or "unknown", "REPORT_URL_LINE": report_line, } - body = template - for key, value in fields.items(): - body = body.replace("{" + key + "}", value) - leftovers = _find_placeholders(body) + body, leftovers = _substitute(template, fields) if leftovers: logger.warning( "Clean-scan template placeholders unfilled: %s", sorted(leftovers) diff --git a/vulnhunter-agent/tests/test_issues_render.py b/vulnhunter-agent/tests/test_issues_render.py index 666aca6..3ef03bb 100644 --- a/vulnhunter-agent/tests/test_issues_render.py +++ b/vulnhunter-agent/tests/test_issues_render.py @@ -309,3 +309,172 @@ def test_compact_shape(self) -> None: assert "abc1234" in comment assert "757 seconds" in comment assert "2026-07-06T18:42:51Z" in comment + + +class TestMarkerCommentBreakout: + """CANON-21: attacker-influenced marker fields must not break out of the + HTML-comment footer markers (CWE-79 / CWE-116). + + ``f.id`` and ``report.results_dir_name`` are stamped verbatim into + ```` / + ````. Both originate from + LLM/scan-README-derived data, so a value containing ``-->`` would close the + comment early and inject arbitrary markdown/HTML into the rendered issue. + """ + + def test_finding_id_cannot_break_out_of_comment(self) -> None: + body = render_body( + _finding(id="VULN-001 -->" not in body + assert "" not in body + # The finding-id marker line stays a single well-formed comment: exactly + # one closing '-->' on that line and no injected content after it. + marker_line = next( + l for l in body.splitlines() if "vulnhunt-finding-id" in l + ) + assert marker_line.count("-->") == 1 + assert marker_line.rstrip().endswith("-->") + + def test_results_dir_cannot_break_out_of_comment(self) -> None: + body = render_body( + _finding(), + report=_report( + results_dir_name="r_VULNHUNT_RESULTS_x -->xx" not in body + assert "x" not in body + marker_line = next( + l for l in body.splitlines() if "vulnhunt-results-dir" in l + ) + assert marker_line.count("-->") == 1 + assert marker_line.rstrip().endswith("-->") + + def test_benign_finding_id_still_survives(self) -> None: + # Neutralization must not corrupt a legitimate VULN-NNN id so the + # downstream ``vulnhunt-finding-id: (VULN-\d{3})`` parser still matches. + body = render_body( + _finding(id="VULN-042"), + report=_report(), + report_url="https://example/README.md", + ) + assert "" in body + + +_UNESCAPED_PIPE = re.compile(r"(? list[list[str]]: + """Return the cells of each data row of the '### Finding' table.""" + lines = body.splitlines() + start = lines.index("| Field | Value |") + rows = [] + for line in lines[start + 2:]: + if not line.strip(): + break + rows.append([c.strip() for c in _UNESCAPED_PIPE.split(line.strip()[1:-1])]) + return rows + + +class TestFindingTableCellInjection: + """PR #9 review: Finding fields rendered inside the '### Finding' table + (CWE, CWE name, severity, location, root cause, data flow) must not add + cells via '|' or break out of the table / inject headings via CR/LF.""" + + @pytest.mark.parametrize("field", [ + "cwe", "cwe_name", "severity", "location", "root_cause", "data_flow", + ]) + def test_pipe_cannot_forge_cells(self, field: str) -> None: + body = render_body( + _finding(**{field: "a.py:1 | **Severity** | Low"}), + report=_report(), + report_url="https://example/README.md", + ) + rows = _finding_table_rows(body) + assert len(rows) == 5 + assert all(len(r) == 2 for r in rows), rows + + @pytest.mark.parametrize("field", [ + "cwe", "cwe_name", "severity", "location", "root_cause", "data_flow", "title", + ]) + def test_newline_cannot_inject_heading(self, field: str) -> None: + body = render_body( + _finding(**{field: "x\n\n## Fixed — close this issue\r\n| a | b |"}), + report=_report(), + report_url="https://example/README.md", + ) + assert not any(l.startswith("## Fixed") for l in body.splitlines()) + rows = _finding_table_rows(body) + assert len(rows) == 5 + assert all(len(r) == 2 for r in rows), rows + + def test_multiline_free_form_sections_keep_newlines(self) -> None: + body = render_body( + _finding( + exploit_description="Step one.\n\nStep two.", + fix_strategy="1. Parameterize.\n2. Add a test.", + ), + report=_report(), + report_url="https://example/README.md", + ) + assert "Step one.\n\nStep two." in body + assert "1. Parameterize.\n2. Add a test." in body + + def test_legit_location_unchanged(self) -> None: + body = render_body(_finding(), report=_report(), + report_url="https://example/README.md") + assert "| **Location** | src/db.py:42 |" in body + + +class TestSinglePassSubstitution: + """PR #9 review nit: placeholders were replaced field-by-field, so a value + containing '{IDEMPOTENCY_KEY}' etc. was expanded by a later pass.""" + + def test_placeholder_text_in_value_is_not_expanded(self) -> None: + finding = _finding( + root_cause="see {IDEMPOTENCY_KEY} and {REPORT_URL}", + title="{VULN_ID}", + ) + body = render_body(finding, report=_report(), + report_url="https://example/README.md") + assert "see {IDEMPOTENCY_KEY} and {REPORT_URL}" in body + assert body.count(finding.vulnfix_key) == 1 # only the footer marker + assert "## Security Finding: CWE-89 — {VULN_ID}" in body + + def test_placeholder_text_in_value_does_not_warn(self, caplog) -> None: + with caplog.at_level("WARNING", logger="agent.issues_render"): + render_body(_finding(root_cause="{NOT_A_FIELD}"), report=_report(), + report_url="https://example/README.md") + assert "unfilled" not in caplog.text + + def test_clean_scan_value_is_not_expanded(self) -> None: + body = render_clean_scan_body(_clean_scan_ctx(repo_slug="o/{SKILL_VERSION}")) + assert "o/{SKILL_VERSION}" in body + assert "o/3500d0c-clean" not in body # never expanded by a later pass + + +class TestMarkerSanitizeWarns: + """PR #9 review: _sanitize_marker_value silently stripped out-of-charset + chars, which can yield a marker pointing at a different results dir.""" + + def test_changed_marker_value_logs_warning(self, caplog) -> None: + with caplog.at_level("WARNING", logger="agent.issues_render"): + body = render_body( + _finding(), + report=_report(results_dir_name="r_VULNHUNT_RESULTS_a b/../c"), + report_url="https://example/README.md", + ) + assert "" in body + assert "vulnhunt-results-dir" in caplog.text + assert "r_VULNHUNT_RESULTS_a b/../c" in caplog.text + + def test_clean_marker_values_do_not_warn(self, caplog) -> None: + with caplog.at_level("WARNING", logger="agent.issues_render"): + render_body(_finding(), report=_report(), + report_url="https://example/README.md") + assert caplog.text == "" diff --git a/vulnhunter-fix/references/verification-table-rules.md b/vulnhunter-fix/references/verification-table-rules.md index a59e087..1112f8a 100644 --- a/vulnhunter-fix/references/verification-table-rules.md +++ b/vulnhunter-fix/references/verification-table-rules.md @@ -18,6 +18,12 @@ via `render_verification_table`), workers MUST emit these exact 9 headers in this order — any drift (renaming, dropping `#`, adding a `Sink` column) is rejected at delivery time by `scripts/validate-verification.py`. +Every row MUST have exactly nine cells. A literal `|` inside a cell is +written `\|` (GFM's in-cell escape) and a cell never contains a line break; +`render_verification_table` does both via `_escape_table_cell`. The +validator splits only on unescaped pipes and rejects (exit 1, naming the row) +any row whose cell count differs from the header — rows are never skipped. + ## Cell values (REQ-GRA-012) Each cell is one of the following literal strings: diff --git a/vulnhunter-fix/scripts/validate-verification.py b/vulnhunter-fix/scripts/validate-verification.py index 9196a9a..82e101f 100755 --- a/vulnhunter-fix/scripts/validate-verification.py +++ b/vulnhunter-fix/scripts/validate-verification.py @@ -28,6 +28,7 @@ from __future__ import annotations import argparse +import html import json import re import sys @@ -53,7 +54,43 @@ CELL_NA = re.compile(r"^\s*n\s*/\s*a\s*$", re.IGNORECASE) +# GFM splits a table row on pipes that are NOT backslash-escaped; ``\|`` is a +# literal pipe inside a cell. vulnhunter_fix.delivery._escape_table_cell emits +# exactly that escape, plus backslash-escaped markdown punctuation and +# html-escaped ``<>&`` — _decode_cell reverses both so citations and callers +# are checked against their literal text. +_UNESCAPED_PIPE_RE = re.compile(r"(? str: + return html.unescape(_MD_BACKSLASH_ESCAPE_RE.sub(r"\1", cell.strip())) + + +def _split_row(line: str) -> list[str]: + s = line.strip() + if s.startswith("|"): + s = s[1:] + if s.endswith("|") and not s.endswith("\\|"): + s = s[:-1] + return [_decode_cell(c) for c in _UNESCAPED_PIPE_RE.split(s)] + + def _parse_table(text: str) -> tuple[list[str], list[list[str]]] | None: + """Return (header_cells, rows) or None when no verification table exists. + + Fails closed: raises MalformedTableError naming every row whose cell + count differs from the header. Previously such rows were silently + dropped, so an injected ``|`` (extra cells, forged Verdict) or a CR/LF + (short row) removed the row from validation entirely and the gate + exited 0 (PR #9 review). Rows run until the first blank line, matching + GFM, where a leading pipe is optional — so a newline-broken row's + continuation is a row too. + """ lines = text.splitlines() header_idx = None for i, line in enumerate(lines): @@ -62,14 +99,23 @@ def _parse_table(text: str) -> tuple[list[str], list[list[str]]] | None: break if header_idx is None: return None - header_cells = [c.strip() for c in lines[header_idx].strip("|").split("|")] + header_cells = _split_row(lines[header_idx]) rows = [] - for line in lines[header_idx + 2:]: - if not line.strip().startswith("|"): + malformed = [] + for lineno, line in enumerate(lines[header_idx + 2:], start=header_idx + 3): + if not line.strip(): break - cells = [c.strip() for c in line.strip("|").split("|")] - if len(cells) == len(header_cells): - rows.append(cells) + cells = _split_row(line) + if len(cells) != len(header_cells): + malformed.append( + f"row {len(rows) + len(malformed) + 1} (line {lineno}): expected " + f"{len(header_cells)} cells, got {len(cells)} — unescaped '|' or " + f"line break in a cell? {line.strip()[:160]!r}" + ) + continue + rows.append(cells) + if malformed: + raise MalformedTableError("\n".join(malformed)) return header_cells, rows @@ -237,7 +283,12 @@ def validate(pr_body: Path, worktree: Path, sidecars_dir: Path | None, result_pa print(f"{pr_body}: : {exc}", file=sys.stderr) return 2 - parsed = _parse_table(text) + try: + parsed = _parse_table(text) + except MalformedTableError as exc: + for msg in str(exc).splitlines(): + print(f"{pr_body}: : {msg}", file=sys.stderr) + return 1 if not parsed: print(f"{pr_body}:
: 9-column verification table not found", file=sys.stderr) return 1 diff --git a/vulnhunter-fix/tests/test_hand_wave_guard.py b/vulnhunter-fix/tests/test_hand_wave_guard.py index 1b7023e..2442b82 100644 --- a/vulnhunter-fix/tests/test_hand_wave_guard.py +++ b/vulnhunter-fix/tests/test_hand_wave_guard.py @@ -80,3 +80,138 @@ def test_full_tier_forbids_residuals(): tier="FULL", residual_vectors=["something"], ) + + +# --------------------------------------------------------------------------- +# CANON-44 — markdown/HTML injection via residual_vectors (CWE-79/CWE-116) +# +# residual_vectors entries are LLM/finding-derived and were interpolated +# verbatim into the '## Residual Risk' markdown appended to the PR/issue body. +# The hand-wave / consistency guards reject vague or empty entries but never +# escape, so an entry can carry raw HTML or a markdown link into the body. +# --------------------------------------------------------------------------- + + +def test_residual_entry_html_is_neutralized(): + from vulnhunter_fix.delivery import render_residual_risk_section + + payload = "" + section = render_residual_risk_section("VULN-1", "WORKAROUND", [payload]) + # Raw tags must not survive; angle brackets must be escaped. + assert "