Add real EVTX parser (parse_evtx_real)#6
Conversation
…d-only subprocess), structured records (EventID/channel/provider/timestamp/record offset) + failure artifacts, input schema, manifest registration, deterministic tests
There was a problem hiding this comment.
Pull request overview
Adds a new typed tool, parse_evtx_real, that wraps evtx_dump to parse registered EVTX evidence into a normalized JSON artifact, including structured failure artifacts and deterministic unit tests. This extends the existing typed-tool infrastructure beyond mocks while keeping tool inputs schema-validated and evidence-ID based.
Changes:
- Register the new
parse_evtx_realtool in the builtin tool registry and example plugin manifest. - Implement
parse_evtx_realplus helper functions for locatingevtx_dump, invoking it, and normalizing XML event records. - Add unit tests and input schema for the new tool, plus README documentation and manifest-order test updates.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_typed_tools.py | Updates manifest-order expectations to include parse_evtx_real. |
| tests/test_parse_evtx_real.py | Adds deterministic unit tests for binary discovery, schema rejection, failure modes, and basic XML parsing. |
| src/sift_crs/tools.py | Implements parse_evtx_real and supporting helpers (binary lookup, subprocess invocation, XML normalization). |
| schemas/parse_evtx_real.input.schema.json | Defines schema-validated inputs (case_id, evidence_id only). |
| README.md | Documents the new real EVTX parser tool and usage example. |
| examples/plugin_manifest.example.json | Registers parse_evtx_real in the example manifest. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def is_executable_file(path: Path) -> bool: | ||
| return path.is_file() and os.access(path, os.X_OK) |
| completed = subprocess.run( | ||
| command, | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, |
|
|
||
| def write_fake_binary(path: Path, body: str) -> Path: | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| path.write_text("#!/usr/bin/python3\n" + body, encoding="utf-8") |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 612725ed44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "evidence_id": evidence.evidence_id, | ||
| "source_sha256": evidence.sha256, |
There was a problem hiding this comment.
When a registered EVTX file is modified or replaced after registration but before this tool runs, the subprocess parses the changed bytes while the artifact still copies the original ledger hash into source_sha256. The deterministic verifier only compares this field with the ledger value, so findings derived from altered evidence can incorrectly pass hash verification. Re-hash the file immediately before/after parsing and fail the run if it no longer matches the registered hash.
Useful? React with 👍 / 👎.
| for name in EVTX_DUMP_NAMES: | ||
| candidate = Path(directory) / name | ||
| if is_executable_file(candidate): | ||
| return candidate |
There was a problem hiding this comment.
When PATH contains a shadowing or compromised executable named evtx_dump, this lookup selects and executes it with the registered evidence path and unrestricted process permissions. Such an executable can modify evidence or make network calls despite the tool being declared read_only and the manifest declaring network/shell access disabled. Use a trusted/pinned parser and enforce read-only isolation before giving a subprocess access to evidence.
Useful? React with 👍 / 👎.
| completed = subprocess.run( | ||
| command, | ||
| check=False, | ||
| capture_output=True, | ||
| text=True, |
There was a problem hiding this comment.
For large forensic event logs, capture_output=True loads the complete XML dump into memory, after which the code also builds a full ElementTree and a second normalized records list. XML output can be substantially larger than the source EVTX, so an ordinary large log can exhaust process memory and terminate without the structured failure artifact promised by this tool. Stream and incrementally parse the subprocess output instead.
Useful? React with 👍 / 👎.
| capture_output=True, | ||
| text=True, | ||
| timeout=60, |
There was a problem hiding this comment.
Any valid EVTX whose evtx_dump conversion takes longer than 60 seconds is unconditionally killed and reported as a failure. This is a common scenario for large Security or ForwardedEvents logs, especially with the intended pure-Python parser, and callers have no input option to request a longer limit. Make the timeout configurable or base it on a policy suitable for forensic-sized evidence.
Useful? React with 👍 / 👎.
| "record_offset": coerce_int( | ||
| event.attrib.get("offset") or event.attrib.get("record_offset") | ||
| ), |
There was a problem hiding this comment.
For the documented python-evtx evtx_dump, standard event XML does not add offset or record_offset attributes to the <Event> element, so this field is always null on real parser output. The test only passes because its fake XML injects a nonstandard offset attribute, meaning the advertised source record offset is unavailable during real investigations. Obtain the offset from EVTX record metadata or stop claiming that it is populated.
Useful? React with 👍 / 👎.
Adds the real
parse_evtx_realtyped tool wrapping evtx_dump: evidence-ID only, read-only subprocess, structured records (EventID/channel/provider/timestamp/record offset) plus failure artifacts, input schema, manifest registration, deterministic tests. 41 passed, 1 skipped (the live-parse test skips until a sample .evtx is staged). No phase rework; built on main at 2ac2933.