Skip to content

fix(scan): stop dropping exposed secrets, collapse duplicates, and type injection findings correctly - #313

Open
closerforever wants to merge 3 commits into
ArmisSecurity:mainfrom
closerforever:fix/finding-type-dedup-and-secret-gating
Open

fix(scan): stop dropping exposed secrets, collapse duplicates, and type injection findings correctly#313
closerforever wants to merge 3 commits into
ArmisSecurity:mainfrom
closerforever:fix/finding-type-dedup-and-secret-gating

Conversation

@closerforever

Copy link
Copy Markdown

What this is

Three gating-correctness fixes in scan repo / scan image, found while
evaluating whether an Armis AppSec scan is safe to turn on as a blocking
commit gate for an internal Azure Databricks project template.

All three were measured against armis-cli v1.22.0 and the live API. They are
independent of pre-commit — they would matter to any CI use of armis-cli.

The first one is the serious one: a repository whose only problem was two
hard-coded AWS credentials scanned completely clean.

1. Exposed secrets were dropped by the exploitability filter

Fixture containing nothing but two hard-coded AWS keys, scanned on main:

total: 0
filtered_non_exploitable: 2
exit code 0

The exploitability filter graded both secrets low/medium and removed them before
they reached the report. A gate on this is green on committed credentials.

This sits on top of #233 (PPSC-1015), which repaired the filter for the graded
label schema. #233 made the filter work; this PR carves secrets out of it, because
reachability grading does not apply to a literal secret: it is already disclosed to
everyone who can read the repository, and there is no path to reason about.
IsSecretExposure now exempts secret findings from the filter, in both the
repository and the image scanner. Same fixture after: 2 findings, both HIGH, exit
1.

The second half is severity. Where the backend does surface a secret, it can
arrive as severity: INFO with no CWE — so under --fail-on HIGH,CRITICAL, the
setting every CI example in the docs uses, a credential committed to source is
still a green build. --fail-on-secret, on by default, exits non-zero when a
secret is exposed whatever severity was assigned, and explains on stderr why the
scan failed when no finding met the configured threshold. --fail-on-secret=false
restores the old behaviour, and a secret suppressed via .armisignore is ignored
exactly as before, so the escape hatch is unchanged.

ShouldFail and CheckExit keep their severity-only meaning for existing callers;
the new policy lives in ExitPolicy, which scan repo and scan image pass.

2. An injection finding was typed SECRET

DeriveFindingType returned SECRET whenever the code location's has_secret
flag was set, before looking at finding_category. The backend sets that flag
whenever the captured blob contains a secret, not only when the finding is about
the secret.

Measured on a file containing both: a SQL injection at line 14 came back with
has_secret set (a token sat at line 9, inside a snippet starting at line 4),
finding_category: CODE_VULNERABILITY and a CWE-89 title — and was reported as
"type": "SECRET". Anything routing on the type (--group-by, SARIF consumers,
dashboards) mis-sorts it.

An explicit code-vulnerability category is the more specific signal, so it now wins
over the blob-level flag. has_secret still decides for every other category —
and the reason that mattered is fixed properly here: SECRET_EXPOSURE, the
category the repository scanner actually emits for an exposed secret, was missing
from the category switch entirely.

3. Duplicate findings at the same location

One subprocess.run(..., shell=True) line comes back as two findings with the same
file, line and start column, both CWE-78, differing only in the prose of the title
and description and by one character of end column. Same defect, same fix.
Reporting both inflates counts, doubles the suppression work to silence one issue,
and makes triage look twice as large as it is.

Findings are now collapsed on (file, start line, start column, severity, CWE identifiers). Two details the observed data forced:

  • End column is excluded — that is precisely where the duplicates disagree.
  • CWEs are reduced to their identifier, because the backend spells the same one
    differently across findings (CWE-78: … ('OS Command Injection') vs
    CWE-78: … ('Command Injection')).

Package findings are never collapsed: several CVEs in one dependency share a
severity and either a manifest location or no location at all, so they would merge
into one and real results would be lost. Only findings with a file, a line and no
CVEs are candidates.

Within a group the finding with the lexicographically smallest ID is kept, so the
choice does not depend on the order result pages arrived in, and repeated scans of
one commit report the same finding.

Measured on a real repository: 11 findings → 10, the collapsed pair being the
two CWE-78 reports of one line.

Verification

go build ./..., go vet ./..., gofmt -l clean, go test ./... — 22/22 packages
ok, exit 0. New tests: internal/scan/dedupe_test.go,
internal/scan/exploitability_test.go, internal/scan/finding_type_test.go,
internal/output/output_test.go.

One thing this PR cannot fix

ShouldFilterByExploitability reads the backend's Exploitability Level label off
NormalizedTask.Labels, but model.Finding has no Labels field — so the grade
that decided whether a finding was shown never reaches --format json or SARIF. A
consumer cannot see why something was filtered.

Separately, Validation *FindingValidation (TaintPropagation, Confidence,
Exposure) is already modeled and already wired through
normalized_findings.go:150-151 — and was null in 32/32 findings we collected,
because the server does not populate it on the ingest pipeline. The plumbing exists
and sits inert. Surfacing the grade, and populating Validation, is what would let
a consumer calibrate a gate rather than trust or distrust the filter wholesale.
Happy to open that as an issue if useful.

…secret

`DeriveFindingType` returned SECRET whenever the code location's `has_secret`
flag was set, before looking at `finding_category`. The backend sets that flag
whenever the *blob* it captured contains a secret, not only when the finding is
about the secret, so a SQL injection whose snippet happens to start a few lines
above a hard-coded token arrives with `has_secret` set, `finding_category:
CODE_VULNERABILITY` and a CWE-89 title -- and was reported as
`"type": "SECRET"`. Measured on a scan of a file containing both: the CWE-89
finding at line 14 was typed SECRET because the token sat at line 9, inside the
snippet that starts at line 4. Anything routing on the type (`--group-by`, SARIF
consumers, dashboards) mis-sorts it.

An explicit code-vulnerability category is the more specific signal, so it now
wins over the blob-level flag. `has_secret` still decides for every other
category, which is what kept exposed secrets from falling through to the SCA
default -- and the reason that mattered is fixed properly here too:
SECRET_EXPOSURE, the category the repository scanner actually emits for an
exposed secret, was missing from the category switch.
A single `subprocess.run(..., shell=True)` line comes back as two findings with
the same file, line and start column, both carrying CWE-78, differing only in
the prose of their title and description and by one character of end column.
They are the same defect with the same proposed fix. Reporting both inflates
finding counts, doubles the suppression work needed to silence one issue, and
makes triage look twice as large as it is.

Findings are now collapsed on (file, start line, start column, severity, CWE
identifiers). Two details the observed data forced:

- End column is excluded, because that is precisely where the duplicates
  disagree.
- CWEs are reduced to their identifier, because the backend spells the same one
  differently across findings ("CWE-78: ... ('OS Command Injection')" and
  "CWE-78: ... ('Command Injection')").

Package findings are never collapsed: several CVEs in one dependency share a
severity and either a manifest location or no location at all, so they would
merge into one and real results would be lost. Only findings with a file, a line
and no CVEs are candidates.

Within a group the finding with the lexicographically smallest ID is kept, so
the choice does not depend on the order the result pages arrived in and repeated
scans of one commit report the same finding.

Measured on a real repository: 11 findings to 10, with the collapsed pair being
the two CWE-78 reports of one line.
A repository whose only problem was two hard-coded AWS credentials scanned
completely clean: `total: 0`, `filtered_non_exploitable: 2`, exit 0. The
exploitability filter graded both secrets low/medium and dropped them before
they reached the report.

Reachability grading does not apply to a literal secret. It is already disclosed
to everyone who can read the repository; there is no path to reason about. So
`IsSecretExposure` now exempts secret findings from the filter, in both the
repository and the image scanner. On the same fixture that reported nothing:
2 findings, both HIGH, exit 1.

The second half of the same problem is severity. Where the backend does surface
a secret, it can arrive as `severity: INFO` with no CWE, so under
`--fail-on HIGH,CRITICAL` -- the setting every CI example uses -- a credential
committed to source is still a green build. `--fail-on-secret`, on by default,
exits non-zero when a secret is exposed whatever severity was assigned, and
explains on stderr why the scan failed when no finding met the configured
threshold. `--fail-on-secret=false` restores the old behaviour, and a secret
suppressed via .armisignore is ignored as before, so the escape hatch is
unchanged.

`ShouldFail` and `CheckExit` keep their severity-only meaning for existing
callers; the policy lives in the new `ExitPolicy`, which `scan repo` and
`scan image` pass.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant