mirror of
https://github.com/norandom/OpenUltraSAST.git
synced 2026-10-16 03:49:26 +00:00
feat(reports): say what was looked for, and what cannot be decided
contributor-scan 2.16, implementing the approved Req 5.6. `c/memory` scored zero true positives across 89k lines of C and three distinct classes of false one, for a structural reason: it models string CALLS and libpng overflows through arithmetic. The task allowed retiring the family. Kept, scoped and stated instead -- `strcpy` misuse is real in plenty of C and the family earns its place for that. Retiring it would have removed the disclosure along with the coverage, leaving a C scan silent for a reason nobody could read. **Its own description was the quiet-failure risk.** It claimed "bounds, lifetime, arithmetic", which is exactly what it does not decide. It now names the APIs it models, and a new `limits` field in families.toml says what it cannot: an overflow expressed as an index, a pointer offset, or a memcpy whose size is computed -- which is how most real C overflows are written, and how every one of libpng's is. A limit recorded in a fact table nobody reads is no better than silence, so the report renders it. **What was analysed** lists every family offered to a region whether or not it found anything, because a family that ran and reported nothing is the case a reader is most likely to misread, and it states plainly that silence means no fact table covered the code rather than that the code is safe. The counts are derived from the fact tables the arbiter actually used, so they cannot drift from what the engine looked for; only the limit sentence is authored, because "cannot decide an overflow expressed as an index" is a statement about an abstraction and no table can produce it. A C scan that finds nothing now says: | c | `memory` | 121 | 9 | **`memory` (c)** — Models CALLS, not arithmetic... Gates byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01T7XD7x5KonMdmGRkp63EQG
This commit is contained in:
co-authored by
Claude Opus 5
parent
f488e625c2
commit
af7e33ef3e
@@ -294,7 +294,7 @@
|
||||
- _Requirements: 4.1, 4.4_
|
||||
- _Depends: 2.12_
|
||||
|
||||
- [ ] 2.16 Say what `c/memory` actually covers, or retire it
|
||||
- [x] 2.16 Say what `c/memory` actually covers, or retire it
|
||||
- Its score so far is zero true positives and every finding a false one: 26 from a substring match, 3 from
|
||||
a bound it could not see, 1 from an allocation sized to fit. Each was a real defect in the engine and
|
||||
each is now fixed, but the family has never once been right, and 2.15 explains why -- it models string
|
||||
@@ -304,6 +304,17 @@
|
||||
but it should be a recorded decision with a number behind it, not an assumption.
|
||||
- Observable: either a measured true positive on the 2.15 checkout, or the family's declared scope
|
||||
narrowed in the facts and the docs so no reader takes a silent C scan for a clean bill of health.
|
||||
- **Done 2026-09-09 by the second route: kept, scoped and stated.** `strcpy` misuse is real in plenty of C
|
||||
and the family earns its place for that; what it could not do was claim more. Its description said
|
||||
"bounds, lifetime, arithmetic", which is exactly what it does not decide -- the description was itself
|
||||
the quiet-failure risk. It now names the APIs it models, and a new `limits` field says what it cannot:
|
||||
an overflow expressed as an index, a pointer offset, or a memcpy whose size is computed.
|
||||
- The report renders it as **What was analysed**, for every family offered to a region whether or not it
|
||||
found anything -- a family that ran and reported nothing is the case a reader is most likely to misread.
|
||||
The counts are derived from the fact tables the arbiter used, so they cannot drift from what the engine
|
||||
really looked for; only the limit sentence is authored.
|
||||
- Retiring the family was the alternative and would have been wrong: it would remove the disclosure along
|
||||
with the coverage, leaving a C scan silent for a reason nobody could read.
|
||||
- _Requirements: 3.1, 8.2_
|
||||
- _Depends: 2.15_
|
||||
|
||||
|
||||
@@ -3,7 +3,7 @@ from __future__ import annotations
|
||||
import argparse
|
||||
import json
|
||||
import tempfile
|
||||
from collections.abc import Sequence
|
||||
from collections.abc import Mapping, Sequence
|
||||
from dataclasses import dataclass, replace
|
||||
from pathlib import Path
|
||||
from time import perf_counter
|
||||
@@ -538,6 +538,10 @@ def _run_scan(path: Path, config_path: Path, mode: str, fail_on: str) -> ScanOut
|
||||
wrote_verdicts = True
|
||||
plan = record_completed(plan, Stage.REGRESS)
|
||||
worth_fixing_payload = _worth_fixing_payload(verdict_records) if wrote_verdicts else None
|
||||
# Req 5.6: what each family could see, for the report. Absent when the model layer did not run --
|
||||
# and a scan with no model layer states no limits because it made no claims.
|
||||
coverage_rows = model_payload.get("coverage") if isinstance(model_payload, dict) else None
|
||||
coverage_for_report = coverage_rows if isinstance(coverage_rows, list) and coverage_rows else None
|
||||
runtime.run_stage(
|
||||
"report",
|
||||
lambda: write_markdown_report(
|
||||
@@ -550,6 +554,7 @@ def _run_scan(path: Path, config_path: Path, mode: str, fail_on: str) -> ScanOut
|
||||
overlay=overlay_records if wrote_overlay else None,
|
||||
obligations=obligations_cited or None,
|
||||
obligations_summary=obligations_payload or None,
|
||||
coverage=coverage_for_report,
|
||||
),
|
||||
)
|
||||
runtime.run_stage(
|
||||
@@ -872,7 +877,9 @@ def _model_skipped(reason: str) -> dict[str, object]:
|
||||
return {**_model_payload(None), "skipped": reason}
|
||||
|
||||
|
||||
def _model_payload(result: object, *, unjudged_paths: tuple[str, ...] = ()) -> dict[str, object]:
|
||||
def _model_payload(
|
||||
result: object, *, unjudged_paths: tuple[str, ...] = (), coverage: Sequence[Mapping[str, object]] = ()
|
||||
) -> dict[str, object]:
|
||||
"""What the scan records about the model layer.
|
||||
|
||||
`unjudged_sample` names the regions the budget did not reach, lowest-ranked first. Counting them is the
|
||||
@@ -892,6 +899,7 @@ def _model_payload(result: object, *, unjudged_paths: tuple[str, ...] = ()) -> d
|
||||
"query_seconds": getattr(result, "query_seconds", 0.0),
|
||||
"query_seconds_by_kind": dict(getattr(result, "query_seconds_by_kind", {}) or {}),
|
||||
"arbitrate_seconds": getattr(result, "arbitrate_seconds", 0.0),
|
||||
"coverage": list(coverage),
|
||||
}
|
||||
|
||||
|
||||
@@ -929,6 +937,12 @@ def _run_model_layer(
|
||||
if not regions:
|
||||
return [], _model_skipped("no_regions")
|
||||
|
||||
# Req 5.6: what each family could see, recorded whether or not it found anything. A family that ran
|
||||
# and reported nothing is the case a reader is most likely to misread.
|
||||
from .model.report import coverage_rows
|
||||
|
||||
coverage = coverage_rows(regions)
|
||||
|
||||
resolved = resolve_chat_endpoint(config)
|
||||
client = resolved[0] if resolved else None
|
||||
if client is None:
|
||||
@@ -952,7 +966,7 @@ def _run_model_layer(
|
||||
# The regions the budget did not reach, weakest first -- the evidence an exclusion list is built from.
|
||||
unjudged = tuple(region.path for region in regions[result.regions_scanned :][:10])
|
||||
findings = [_finding_from_model(item) for item in result.findings]
|
||||
return findings, _model_payload(result, unjudged_paths=unjudged)
|
||||
return findings, _model_payload(result, unjudged_paths=unjudged, coverage=coverage)
|
||||
|
||||
|
||||
def _finding_from_model(item: object) -> StaticFinding:
|
||||
|
||||
@@ -82,3 +82,59 @@ def render_findings(findings: Sequence[StaticFinding]) -> str:
|
||||
|
||||
|
||||
__all__ = ["SliceRow", "overfitting_gap", "render", "render_findings"]
|
||||
|
||||
|
||||
def coverage_rows(regions: Sequence[object]) -> list[dict[str, object]]:
|
||||
"""What each family actually modelled for the languages scanned, and what it cannot decide (Req 5.6).
|
||||
|
||||
Derived, not asserted: the count is the size of the fact table the arbiter used, so it cannot drift from
|
||||
what the engine really looked for. The `limits` string is authored, reviewed and committed in
|
||||
`families.toml`, because "cannot decide an overflow expressed as an index" is a statement about an
|
||||
abstraction and no table can produce it.
|
||||
|
||||
Reported for every family that was OFFERED to a region, not only those that produced findings -- a family
|
||||
that ran and found nothing is exactly the case a reader is most likely to misread, and the one Req 5.6
|
||||
exists for.
|
||||
"""
|
||||
from .specs import config_specs, dominance_specs, taint_specs
|
||||
from .taxonomy import load_families
|
||||
|
||||
taxonomy = load_families()
|
||||
limits = {family.id: family.limits for family in taxonomy.families}
|
||||
described = {family.id: family.description for family in taxonomy.families}
|
||||
|
||||
offered: dict[tuple[str, str], int] = {}
|
||||
for region in regions:
|
||||
language = str(getattr(region, "language", ""))
|
||||
for family in getattr(region, "families", ()):
|
||||
offered.setdefault((language, str(family)), 0)
|
||||
offered[(language, str(family))] += 1
|
||||
|
||||
rows: list[dict[str, object]] = []
|
||||
for (language, family), region_count in sorted(offered.items()):
|
||||
modelled = _modelled_count(language, family, taint_specs, dominance_specs, config_specs)
|
||||
rows.append(
|
||||
{
|
||||
"language": language,
|
||||
"family": family,
|
||||
"regions": region_count,
|
||||
"modelled": modelled,
|
||||
"description": described.get(family, ""),
|
||||
"limits": limits.get(family, ""),
|
||||
}
|
||||
)
|
||||
return rows
|
||||
|
||||
|
||||
def _modelled_count(language: str, family: str, taint_specs, dominance_specs, config_specs) -> int: # type: ignore[no-untyped-def]
|
||||
"""How many operations the arbiter for this family can even see in this language."""
|
||||
spec = taint_specs(language=language).get(family)
|
||||
if spec is not None:
|
||||
return len(spec.sinks)
|
||||
spec = dominance_specs(language=language).get(family)
|
||||
if spec is not None:
|
||||
return len(spec.operations)
|
||||
spec = config_specs(language=language).get(family)
|
||||
if spec is not None:
|
||||
return len(spec.settings)
|
||||
return 0
|
||||
|
||||
@@ -31,7 +31,7 @@ VERIFIER_KINDS = ("canary", "static", "none")
|
||||
Relation = Literal["same", "parent_child", "lateral", "fabricated"]
|
||||
DEFAULT_FAMILIES_PATH = Path(__file__).resolve().parents[1] / "ruleset" / "families.toml"
|
||||
|
||||
_FAMILY_FIELDS = frozenset({"id", "description", "cwes", "mechanisms", "verifier", "parent"})
|
||||
_FAMILY_FIELDS = frozenset({"id", "description", "cwes", "mechanisms", "verifier", "parent", "limits"})
|
||||
_TOP_FIELDS = frozenset({"version", "family"})
|
||||
|
||||
|
||||
@@ -47,6 +47,10 @@ class Family:
|
||||
mechanisms: frozenset[str]
|
||||
verifier: str # VERIFIER_KINDS
|
||||
parent: str | None = None # a family this one specializes; partial credit when one side is the other's parent
|
||||
# What this family's abstraction CANNOT decide, in the words a reader needs. Empty means no limit has
|
||||
# been stated, which is not the same as no limit existing -- and a family that has one and does not say
|
||||
# so lets a silent scan read as a clean bill of health (Req 5.6).
|
||||
limits: str = ""
|
||||
|
||||
|
||||
@dataclass(frozen=True)
|
||||
@@ -153,6 +157,7 @@ def _family(source: Path, entry: dict[str, object]) -> Family:
|
||||
mechanisms=frozenset(mechanisms),
|
||||
verifier=verifier,
|
||||
parent=str(parent) if parent else None,
|
||||
limits=str(entry.get("limits", "") or "").strip(),
|
||||
)
|
||||
|
||||
|
||||
|
||||
@@ -22,6 +22,7 @@ def write_markdown_report(
|
||||
mechanisms: Mapping[str, Mapping[str, object]] | None = None,
|
||||
obligations: Mapping[str, Mapping[str, object]] | None = None,
|
||||
obligations_summary: Mapping[str, object] | None = None,
|
||||
coverage: Sequence[Mapping[str, object]] | None = None,
|
||||
) -> None:
|
||||
path.parent.mkdir(parents=True, exist_ok=True)
|
||||
verification_by_id = _verification_by_id(verifications or [])
|
||||
@@ -86,6 +87,8 @@ def write_markdown_report(
|
||||
cited_obligations[finding.finding_id] = info_o
|
||||
_append_obligation_lines(lines, info_o, (mechanisms or {}).get(str(info_o.get("known_fix") or "")))
|
||||
lines.extend(["", finding.rationale, ""])
|
||||
if coverage:
|
||||
_append_coverage(lines, coverage)
|
||||
if patterned:
|
||||
_append_pattern_matches(lines, patterned)
|
||||
if cited:
|
||||
@@ -391,6 +394,36 @@ def _crowded_pattern_rules(findings: Sequence[StaticFinding]) -> list[StaticFind
|
||||
]
|
||||
|
||||
|
||||
def _append_coverage(lines: list[str], coverage: Sequence[Mapping[str, object]]) -> None:
|
||||
"""What was looked for, and what could not be decided (Req 5.6).
|
||||
|
||||
A scan that reports nothing has established that nothing it MODELS was found. That is not the same as
|
||||
finding nothing, and the difference is the whole reason this section exists: a silent report read as a
|
||||
clean bill of health is worse than a noisy one, because the reader acts on it.
|
||||
"""
|
||||
lines.extend(
|
||||
[
|
||||
"## What was analysed",
|
||||
"",
|
||||
"Each family below was offered to at least one region. **A family that reported nothing has "
|
||||
"established only that nothing it models was found** — silence here means no fact table covered "
|
||||
"the code, not that the code is safe.",
|
||||
"",
|
||||
"| language | family | regions | operations modelled |",
|
||||
"| --- | --- | --- | --- |",
|
||||
]
|
||||
)
|
||||
for row in coverage:
|
||||
lines.append(f"| {row.get('language')} | `{row.get('family')}` | {row.get('regions')} | {row.get('modelled')} |")
|
||||
lines.append("")
|
||||
|
||||
stated = [row for row in coverage if str(row.get("limits") or "")]
|
||||
if stated:
|
||||
lines.extend(["### Stated limits", ""])
|
||||
for row in stated:
|
||||
lines.extend([f"**`{row.get('family')}` ({row.get('language')})** — {row.get('limits')}", ""])
|
||||
|
||||
|
||||
def _is_pattern_only(finding: StaticFinding) -> bool:
|
||||
"""Is a text match the only thing behind this finding?
|
||||
|
||||
|
||||
@@ -86,7 +86,8 @@ verifier = "none"
|
||||
|
||||
[[family]]
|
||||
id = "memory"
|
||||
description = "Memory safety in C and C++: bounds, lifetime, arithmetic. Measured on the vendored slice, never gated; specialized systems own this class."
|
||||
description = "Misuse of unbounded string and format APIs in C and C++: a tainted value reaching gets, strcpy, strcat, sprintf or a non-literal format string."
|
||||
limits = """Models CALLS, not arithmetic. It cannot decide an overflow expressed as an index, a pointer offset, or a memcpy whose size is computed -- which is how most real C overflows are written, and how every one of libpng's is. A scan of C code that reports nothing from this family has established that no tainted value reached a modelled string API, and nothing at all about its buffer arithmetic."""
|
||||
cwes = [
|
||||
"CWE-119",
|
||||
"CWE-120",
|
||||
|
||||
@@ -122,3 +122,15 @@ def test_the_shipped_file_must_declare_the_whole_closed_set_in_order(tmp_path: P
|
||||
(tmp_path / "families.toml").write_text(GOOD)
|
||||
with pytest.raises(FamiliesError, match="in that order"):
|
||||
load_families(tmp_path / "families.toml")
|
||||
|
||||
|
||||
def test_the_memory_family_states_what_it_cannot_decide() -> None:
|
||||
"""contributor-scan 2.16. Its description used to claim "bounds, lifetime, arithmetic", which is exactly
|
||||
what it does not do -- the description was itself the quiet-failure risk."""
|
||||
from openultrasast.model.taxonomy import load_families
|
||||
|
||||
memory = next(family for family in load_families().families if family.id == "memory")
|
||||
|
||||
assert "arithmetic" in memory.limits, "the gap is stated"
|
||||
assert "arithmetic" not in memory.description, "and no longer claimed"
|
||||
assert "strcpy" in memory.description, "what it does cover is named"
|
||||
|
||||
@@ -517,3 +517,38 @@ def test_a_reasoned_claim_keeps_its_section_whatever_its_rung(tmp_path: Path) ->
|
||||
write_markdown_report(reasoned, output)
|
||||
|
||||
assert output.read_text().count(f"## {base.title}") == 20
|
||||
|
||||
|
||||
def test_the_report_says_what_was_looked_for_and_what_cannot_be_decided(tmp_path: Path) -> None:
|
||||
"""contributor-scan 2.16, Req 5.6. `c/memory` scored zero true positives across 89k lines of C for a
|
||||
structural reason -- it models string CALLS and libpng overflows through arithmetic. A silent scan
|
||||
reading as a clean bill of health is worse than a noisy one, because the reader acts on it."""
|
||||
output = tmp_path / "report.md"
|
||||
coverage = [
|
||||
{
|
||||
"language": "c",
|
||||
"family": "memory",
|
||||
"regions": 121,
|
||||
"modelled": 9,
|
||||
"description": "Misuse of unbounded string and format APIs",
|
||||
"limits": "Models CALLS, not arithmetic. It cannot decide an overflow expressed as an index.",
|
||||
}
|
||||
]
|
||||
|
||||
write_markdown_report([], output, coverage=coverage)
|
||||
|
||||
text = output.read_text()
|
||||
assert "## What was analysed" in text
|
||||
assert "| c | `memory` | 121 | 9 |" in text, "what it looked for, counted from the fact table itself"
|
||||
assert "### Stated limits" in text
|
||||
assert "cannot decide an overflow expressed as an index" in text
|
||||
assert "silence here means no fact table covered the code, not that the code is safe" in text
|
||||
|
||||
|
||||
def test_a_scan_that_made_no_claims_states_no_limits(tmp_path: Path) -> None:
|
||||
"""A report with no model layer has nothing to disclaim; the section would be noise."""
|
||||
output = tmp_path / "report.md"
|
||||
|
||||
write_markdown_report([_finding()], output)
|
||||
|
||||
assert "## What was analysed" not in output.read_text()
|
||||
|
||||
Reference in New Issue
Block a user