Skip to content

fix: suppress Sigstore certificate payload false positives - #337

Closed
koriyoshi2041 wants to merge 1 commit into
NVIDIA:mainfrom
koriyoshi2041:fix-sigstore-certificate-false-positives
Closed

fix: suppress Sigstore certificate payload false positives#337
koriyoshi2041 wants to merge 1 commit into
NVIDIA:mainfrom
koriyoshi2041:fix-sigstore-certificate-false-positives

Conversation

@koriyoshi2041

Copy link
Copy Markdown
Contributor

Summary

  • mask only certificate rawBytes inside structurally valid Sigstore v0.3 skill.oms.sig bundles before static pattern matching
  • preserve byte-for-character offsets so findings in every other bundle field keep correct locations
  • keep malformed bundles, differently named files, and non-certificate payloads fully visible to analyzers

Why this approach

Signed skills currently receive SC3 and MP2 findings from base64-encoded DER certificates. Skipping the whole signature file would create a forgeable analysis bypass; this instead implements the narrow field-level suppression suggested in the review of #261. Unknown fields and attacker-controlled content remain scanned.

Closes #336.

Validation

  • uv run pytest tests/nodes/analyzers/test_static_runner_filtering.py tests/nodes/analyzers/test_static_patterns.py tests/nodes/analyzers/test_mp2_regex_backtracking.py -q — 139 passed
  • uv run ruff check src/skillspector/nodes/analyzers/static_runner.py tests/nodes/analyzers/test_static_runner_filtering.py
  • uv run ruff format --check src/skillspector/nodes/analyzers/static_runner.py tests/nodes/analyzers/test_static_runner_filtering.py
  • git diff --check

Risk

The exemption is limited to the exact certificate chain path in a valid v0.3 bundle named skill.oms.sig. It does not verify the signature or attest trust; it only prevents opaque certificate bytes from reaching generic content heuristics.

Signed-off-by: kigland <shuaizhicheng336@gmail.com>
@Hokonoken

Copy link
Copy Markdown

Heads-up from an independent check, in case it saves a rebase surprise: #261 merged to main later the same day this PR was opened (ee9612c, after its review-hardening pass), so main now carries the structural-recognizer approach — a recognized root-level bundle is inventoried as an oms_signature component and excluded from content analysis; anything failing the DSSE structure check is scanned normally.

Measured on three real catalog bundles (they carry dsseEnvelope + tlogEntries beyond the certificate chain), --no-llm, per-file SC3 count on skill.oms.sig:

pre-recognizer base this PR main (#261)
real bundle 4 1 0

So the certificate masking here does reduce the noise (4→1) and clears #336's synthetic bundle, but each real bundle keeps one SC3 HIGH (line 1) — presumably the base64 content outside the certificate fields the mask covers. #261's recognizer takes them to 0.

For context, the design lineage still looked live rather than settled at merge: #261's review flagged that a structural-only exemption is forgeable (a plausible unverified bundle could hide content from every analyzer), and field-level masking — this PR's shape — is the direction that avoids whole-file exclusion. If maintainers want that hardening on top of #261, this branch would need a rebase onto the recognizer plus coverage of the non-certificate base64 fields. Maintainers' call — just flagging the merge and the real-bundle numbers.

@keshprad

keshprad commented Aug 5, 2026

Copy link
Copy Markdown
Member

Hey sorry yes I noticed these two PRs had some overlap. I meant to also put a comment here yesterday.

Thanks for the thoughtful alternative, especially the offset-preserving masking and tests that keep unrelated bundle fields visible.

We had an internal review with our security SMEs, we chose the design implemented in #261 instead. The agreed boundary is that a recognized root-level skill.oms.sig is detached signing metadata, not skill content: SkillSpector performs lightweight structural recognition, excludes that artifact from content analysis, and makes the exclusion explicit in the inspection ledger and report.

We intentionally do not treat that recognition as signature authentication. Full verification needs publisher-trust configuration and adds a tight dependency on the signing ecosystem, while still not eliminating all publisher-controlled metadata fields. Authenticity of signatures should be established by a downstream signature-verification workflow.

Given that decision, certificate-only masking would create a different, static-analyzer-specific policy while other analysis stages see different content. We prefer the single, transparent scope boundary in #261. We’ll therefore proceed with #261 rather than this alternative.

Thanks again for the careful implementation and for spelling out the trade-offs.

@keshprad

keshprad commented Aug 5, 2026

Copy link
Copy Markdown
Member

I'm closing this PR but pls do feel free to followup if you think we should still consider this.

@keshprad keshprad closed this Aug 5, 2026
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.

SC3 flags skill.oms.sig as obfuscated code, once per certificate, on every signed skill

3 participants