fix(security): preserve exact finding identity - #409
Conversation
Signed-off-by: Nir Paz <npaz@nvidia.com>
Signed-off-by: Nir Paz <npaz@nvidia.com>
Signed-off-by: Nir Paz <npaz@nvidia.com>
rng1995
left a comment
rng1995
left a comment
There was a problem hiding this comment.
Requesting changes because the new identity path still collapses distinct findings and can discard an already-known YARA detection. All three cases reproduce on this exact head; the focused relevant suites pass, which indicates missing regression coverage.
|
|
||
| # --- Data URIs (check first) --- | ||
| for m in _DATA_URI_RE.finditer(text): | ||
| complete_match = m.group() |
There was a problem hiding this comment.
[P1] Include the data-URI payload in the fingerprint. _DATA_URI_RE ends at the comma, so m.group() is only the data:text/...;base64, prefix. Two URIs with different payloads therefore receive the same fingerprint and final deduplication reports only one. Capture/hash the complete URI token (while keeping the displayed preview bounded) and mark its complete range as covered.
| end_lineno: int | None, | ||
| msg_override: str | None = None, | ||
| ) -> None: | ||
| complete_match = get_complete_source_segment(lines, lineno, end_lineno) |
There was a problem hiding this comment.
[P1] Fingerprint the AST node, not its entire line range. get_complete_source_segment(lines, lineno, end_lineno) has no column offsets, so exec("first"); exec("second") produces two findings with the same location and complete match; final deduplication drops one. Use the node's start/end columns (for example ast.get_source_segment) or include column-aware evidence in identity.
| data, | ||
| fingerprint_budget, | ||
| ) | ||
| except _YaraFingerprintLimitError as exc: |
There was a problem hiding this comment.
[P2] Do not discard the current YARA detection when fingerprint hashing reaches its budget. At this point the rule has already matched and preview evidence is available, but this return omits that match entirely. A valid bounded-budget repro returns findings=[] with SIZE_LIMIT. Emit the finding with conservative/incomplete identity, then record the partial-coverage ledger status.
Signed-off-by: Nir Paz <npaz@nvidia.com>
|
Powered by Codex: correction after a five-lens GPT-5.6-sol council on current head
This supersedes the prior mergeable assessment. New, non-duplicate findings:
New anchored review: #409 (review) Recommended action: do not merge until YARA rule identity is part of the normal fingerprint; address the bounded runtime/occurrence regressions in the same identity path. |
|
Fixes pushed. please review and confirm. |
mohgupta-ship-it
left a comment
mohgupta-ship-it
left a comment
There was a problem hiding this comment.
Powered by Codex: five-lens GPT-5.6-sol delta council — REQUEST CHANGES on 863c31d. The prior mergeable assessment is superseded by a reproduced YARA identity collision that changes the published install recommendation; two bounded identity/runtime issues are also anchored below.
| digest.update(len(value).to_bytes(8, "big")) | ||
| digest.update(value) | ||
|
|
||
| update_framed(rule_id.encode("utf-8", errors="surrogatepass")) |
There was a problem hiding this comment.
Powered by Codex: [P1] The normal fingerprint omits match.namespace and match.rule, although the fallback includes both. Two distinct same-category custom rules matching identical harmless bytes therefore received the same fingerprint and deduped 2→1; the reproduced score dropped from 67/HIGH/DO_NOT_INSTALL to 45/MEDIUM/CAUTION. Include namespace and rule name in this domain-separated digest and assert both rule messages and the blocking score survive compaction.
| ) -> None: | ||
| lineno = getattr(ast_node, "lineno", 1) | ||
| end_lineno = getattr(ast_node, "end_lineno", None) | ||
| complete_match = ast.get_source_segment(python_ast.content, ast_node) |
There was a problem hiding this comment.
Powered by Codex: [P2] Calling ast.get_source_segment once per finding re-splits/reconstructs source and is measurably quadratic for many calls on one line: 500/1,000/2,000/4,000 calls took 0.028/0.108/0.420/1.682 seconds. The deadline check cannot interrupt this extraction. Precompute line/byte offsets once per ParsedPythonFile, slice spans from that index, and add a structural large-N regression.
| identity = ( | ||
| ( | ||
| finding.location.file, | ||
| finding.location.start_line, |
There was a problem hiding this comment.
Powered by Codex: [P2] Location identity is line-only here. Two identical P1 matches on the same line are emitted twice by the analyzer but compact to one finding with no distinguishable second occurrence. Preserve a start column or absolute offset in occurrence identity and test two identical same-line matches; no scoring impact is claimed, but the occurrence-preservation contract is currently violated.
3 participants
Summary
Validation
git diff --check