fix: [cof2misp] fix operator precedence in COF timestamp validation - #894
Open
elhoim wants to merge 1 commit into
Open
fix: [cof2misp] fix operator precedence in COF timestamp validation#894elhoim wants to merge 1 commit into
elhoim wants to merge 1 commit into
Conversation
is_cof_valid_simple checked `not (A and B) or (C and D)`, which Python parses as `(not (A and B)) or (C and D)` rather than the intended `not ((A and B) or (C and D))`. As a result, a record carrying only the valid zone_time_first/zone_time_last pair (and no time_first/time_last) was rejected as invalid, since `not (A and B)` is already True in that case and short-circuits the OR to True, triggering the "missing mandatory fields" error and a False return even though the record is well-formed. A record with both pairs present was also flagged wrong for the same reason. Parenthesised the check as `not ((A and B) or (C and D))` so validation succeeds whenever either timestamp pair is present, matching the intended COF semantics. Verified with py_compile and the full pytest suite against a live modules server on port 6786: 161 passed, 4 skipped, 5 subtests passed, matching the documented baseline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018dfYpyaSZd1nxSRLr8suj8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The defect
In
misp_modules/lib/cof2misp/cof.py,is_cof_valid_simple()checked for the presence of a valid timestamp pair like this:Due to Python operator precedence,
notbinds tighter thanor, so this parses as:The intent was "reject the record unless it has EITHER the
time_first/time_lastpair OR thezone_time_first/zone_time_lastpair" — i.e.not (A or B). Instead, the expression evaluates to(not A) or B, which is true (and so returnsFalse, rejecting the record) wheneverAis false, regardless ofB. Concretely: a record that has onlyzone_time_first/zone_time_last(and lackstime_first/time_last) makesAfalse, sonot AisTrue, and the whole condition isTrue— the record is wrongly rejected as missing required fields, even though it actually has a valid timestamp pair.Impact
Any COF (Common Output Format) DNS record that carries only the
zone_time_first/zone_time_lastfields — a legitimate, documented alternative totime_first/time_last— is silently dropped by thecof2mispconversion before it ever reaches MISP. An analyst importing passive DNS data in this format loses valid records without any indication that they were rejected, other than a generic stderr message that (incorrectly) claims required fields are missing.The fix
Add the missing parentheses so the
orcombines the two "has a valid pair" checks before negation, matching the intendednot (A or B)semantics:No behaviour change beyond correcting this logic error to match the documented/intended validation rule.
Verification
python -m py_compile misp_modules/lib/cof2misp/cof.py— clean.Found during a review of the repository; other findings are being submitted as separate PRs.
🤖 Generated with Claude Code
https://claude.ai/code/session_018dfYpyaSZd1nxSRLr8suj8