fix: exclude valid OMS signatures from content analysis#261
fix: exclude valid OMS signatures from content analysis#261major-security wants to merge 1 commit into
Conversation
OMS bundles necessarily contain long base64-encoded payload, signature, and certificate fields. Generic obfuscated-code checks can misclassify these fields as hidden executable content. Recognize root-level skill.oms.sig files with the minimal OMS v0.3 DSSE/in-toto structure and inventory them as oms_signature components without sending their contents through static or LLM analyzers. Preserve normal scanning for malformed, nested, oversized, or unrecognized signature files. Add a pinned real-world fixture, regression coverage, completeness accounting, and documentation. Recognition is structural only and does not verify signatures, certificates, transparency logs, or signer identity. Signed-off-by: major-security <[email protected]>
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Requesting changes. The OMS false-positive goal is reasonable, but the exemption is forgeable and the report overstates coverage. A structurally plausible, unverified bundle can hide attacker-controlled content from every analyzer, while the report still claims the excluded file was fully scanned.
Please require schema-strict parsing and analyze all non-opaque/decoded content, or cryptographically verify a trusted, subject-bound bundle before exempting it. Coverage metadata must also disclose the exclusion.
| return False | ||
|
|
||
| statement = _decode_base64_json(envelope.get("payload")) | ||
| return bool( |
There was a problem hiding this comment.
Blocking: this predicate trusts only attacker-controlled shape: type strings, any nonempty base64 sig, and any dict verificationMaterial. It does not verify the signature/chain, constrain extra fields, or bind the bundle to a trusted subject. Because the caller later removes the whole file from analysis, a forged bundle can place malicious or prompt-injection content in extra verification/predicate fields and bypass both static and LLM analyzers. Require strict schema/content handling or real cryptographic verification, and add an adversarial forged-bundle regression.
| missing_components = [c for c in components if c not in file_cache and c not in excluded] | ||
| # Intentionally excluded metadata is fully accounted for by the scan even | ||
| # though analyzers do not inspect its content. | ||
| scanned_components = total_components - len(missing_components) |
There was a problem hiding this comment.
Blocking: OMS-excluded content is still counted as scanned_components, so a file inspected by no analyzer can produce 100% coverage and is_complete=true. Keep coverage honest by excluding it from the scanned count and surfacing an explicit exclusion/limitation in report metadata.
|
There is a design tradeoff encoded here that we can debate: how much of the signature validation do we want to encoded in the obfuscated code checker? Because a signature contains many b64 encoded fields, we need to exempt valid signatures from the obfuscated code checker. The PR currently validates that the the skill.oms.sig file is a valid sigstore bundle (is a valid artifiact at the top level). An OMS signature also contains file digests for each file in covered by the signature in in-toto predicates. The PR as-is doesn't currently validate that the digest payloads in the signature file match the content of the file on disk. One one hand, an attacker could put obfuscated non-digest content in these in-toto predicates. On the other hand, if they did, the signature verification would fail upon signature checking. So it's a tradeoff of how much of the signature checking logic we build into the obfuscated content checker. Alternatively, SkillSpector could call out to the OMS signature verifier. |
rng1995
left a comment
There was a problem hiding this comment.
[Automated SkillSpector Review]
Re-review after author reply — requesting changes. No new commits since the previous review (head is still 44c08a9), so this re-review evaluates the author's reply against the two open blockers. The reply is a thoughtful framing of the design tradeoff, but both issues remain unaddressed in code.
Prior-issue checklist
-
Forgeable OMS exemption (
src/skillspector/nodes/build_context.py,_is_valid_oms_signature) — Not resolved. Code is unchanged. The author's reply acknowledges the gap directly (the PR "doesn't currently validate that the digest payloads in the signature file match the content of the file on disk" and "an attacker could put obfuscated non-digest content in these in-toto predicates"), and argues that downstream signature verification would fail on such a bundle. That argument does not hold within SkillSpector's threat model: SkillSpector itself performs no verification (recognition is explicitly structural-only), so a forged bundle produces a clean scan with 100% coverage, and any external verifier is out of scope and not guaranteed to run before the skill is installed. The bypass surface is also broader than predicate fields:verificationMaterialonly needs to be any dict (arbitrary base64 blobs or extra keys pass), extra top-level bundle keys are unconstrained, and the decoded statement'ssubject/predicatecontents are unconstrained — all of it hidden from both static and LLM analyzers, including prompt-injection text. Acceptable resolutions remain: (a) verify the in-toto digests against the on-disk files and schema-constrain/scan all non-opaque decoded content, (b) call out to a real OMS/sigstore verifier (the author's own suggested alternative), or (c) narrow the exemption to suppressing only the obfuscation/entropy checks on recognized fields rather than removing the whole file from analysis. An adversarial forged-bundle regression test is still required. -
Coverage overstatement (
src/skillspector/nodes/report.py,_build_analysis_completeness) — Not resolved. The reply does not address this point, and the code is unchanged: the excluded file is counted inscanned_components, yieldingcoverage_percent: 100.0,is_complete: true, andlimitations: null(asserted bytests/nodes/test_analysis_completeness.py::test_intentionally_excluded_component_counts_as_covered), with no field in the report output disclosing the exclusion. A consumer cannot tell that a file was inspected by zero analyzers. At minimum, surface the exclusion explicitly inanalysis_completeness(e.g. anexcluded_componentsfield and/or a limitations entry) instead of silently folding it into the scanned count.
On the tradeoff framing
The reply's question — how much signature validation belongs in the scanner — is fair, and full cryptographic verification may indeed be out of scope for this PR. But the current middle ground (structural recognition + total exemption) is the one combination that creates a bypass: it grants the exemption on attacker-controlled shape alone. Either strengthen the recognition (digest/verifier check) or weaken the exemption (scan decoded content; suppress only the entropy heuristics). Happy to re-review once either direction lands in code.
OMS bundles necessarily contain long base64-encoded payload, signature, and certificate fields. Generic obfuscated-code checks can misclassify these fields as hidden executable content.
Recognize root-level skill.oms.sig files with the minimal OMS v0.3 DSSE/in-toto structure and inventory them as oms_signature components without sending their contents through static or LLM analyzers.
Preserve normal scanning for malformed, nested, oversized, or unrecognized signature files. Add a pinned real-world fixture, regression coverage, completeness accounting, and documentation.
Recognition is structural only and does not verify signatures, certificates, transparency logs, or signer identity.