Skip to content

Enhance parser reliability and fix map keying issues#262

Open
randomaccess3 wants to merge 9 commits into
EricZimmerman:masterfrom
randomaccess3:master
Open

Enhance parser reliability and fix map keying issues#262
randomaccess3 wants to merge 9 commits into
EricZimmerman:masterfrom
randomaccess3:master

Conversation

@randomaccess3

Copy link
Copy Markdown
Contributor

Noticed some of the event logs in TheTechHiveScenario werent parsing properly. They work with evtx_dump (https://github.com/omerbenamram/EVTX) so I gave copilot the task of reviewing the code, comparing with evtx_dump, and re-evaluating against the dataset (https://github.com/randomaccess3/DFIRArtifactMuseum/tree/main/Windows/EventLogs/Win11/TheTechHiveScenario)

Current version:
image

This version:
image

I havent done more testing than this however.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR improves EVTX parsing robustness and correctness when handling real-world logs (including TheTechHiveScenario), with targeted fixes for map keying, safer record parsing, and broader tag/value support.

Changes:

  • Fix event map dictionary keying to include Provider consistently (load + lookup).
  • Harden parsing against malformed EVTX/XML (record boundary checks, safer ampersand handling, safer JSON payload conversion).
  • Add support for additional BinXML/value variants (e.g., BinaryTag.Value2, more ValueType decoding, hex array formatting).

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
EvtxECmd/Program.cs Avoid unconditional XML→JSON conversion; improve error record keying; add best-effort JSON conversion helper.
EvtxECmd/FodyWeavers.xsd Add schema elements for Include/Exclude runtimes.
evtx/Tags/Value.cs Expand supported ValueType decoding to reduce parser failures on non-string value tags.
evtx/Tags/TemplateInstance.cs Fail fast with a clearer exception when templates cannot be resolved.
evtx/Tags/TagBuilder.cs Treat BinaryTag.Value2 the same as Value for tag construction.
evtx/SubstitutionArrayEntry.cs Improve formatting for 32/64-bit hex arrays (but needs bounds safety).
evtx/EventRecord.cs Make XML element parsing more fault-tolerant; fix map key usage; safer ampersand escaping strategy.
evtx/EventLog.cs Key event maps by EventId-Channel-Provider consistently.
evtx/ChunkInfo.cs Add record boundary validation to stop safely on truncated/invalid record structures (but header length check needs correction).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread evtx/SubstitutionArrayEntry.cs
Comment thread evtx/SubstitutionArrayEntry.cs
Comment thread evtx/EventRecord.cs
Comment thread evtx/ChunkInfo.cs
EventRecord.BuildProperties() used a flat XML switch keyed only on
element name, with no awareness of parent/depth. Manifest-based
providers (e.g. Microsoft-Windows-Perflib, Windows Firewall With
Advanced Security) embed an optional <RenderingInfo> element alongside
<System> that reuses several element names (Level, Channel, Provider,
Keywords), but with different shapes/content:

- Keywords under RenderingInfo is a list of <Keyword> child elements
  rather than a single hex string, so reader.ReadElementContentAsString()
  threw 'Element' is an invalid XmlNodeType for every record in
  Microsoft-Windows-Perflib%4Operational.evtx (129 warnings observed).
- Channel/Provider under RenderingInfo silently overwrote the
  authoritative System-derived values later in the same record, since
  RenderingInfo is parsed after System in document order. This was
  previously silent (no warning) and corrupted Channel/Provider for
  Perflib and Windows Firewall/FirewallDiagnostics records (Channel
  ended up as a numeric code like "16"/"21" and Provider was blank).

Fix:
- Track whether the reader is inside <RenderingInfo> and route its
  children to a separate RenderingInfoData DTO instead of the System-
  scoped switch, so System's Channel/Provider/Level/Keywords are never
  clobbered.
- Add ReadRenderingInfoKeywords() to correctly read the nested
  <Keyword> list form (or plain text) instead of assuming text-only
  content, eliminating the XmlNodeType exception.
- Serialize the collected RenderingInfo fields (Level, Opcode, Task,
  Channel, Provider, Keywords, Message) into a single new
  EventRecord.RenderingInfo JSON string property, mirroring how
  Payload holds EventData/UserData as one serialized value, rather
  than discarding this data or spreading it across several new
  columns. This preserves previously-unused signal such as the fully
  rendered Message text and friendly Task name.
- Map the new RenderingInfo property to a CSV column in EvtxECmd.

No Maps files were affected; no map exists for either affected
provider and the defect was upstream of map-based EventData
substitution.

Verified via a git-stash A/B comparison of the full CSV output for the
TheTechHiveScenario sample set (126,392 records): record count and all
pre-existing columns are identical between the pre-fix and post-fix
runs except for Channel/Provider on the previously-corrupted Perflib
and FirewallDiagnostics records, which now hold correct values. The
"Unable to parse XML element Keyword" warning (129 occurrences) no
longer appears.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@randomaccess3

Copy link
Copy Markdown
Contributor Author

Need to redo the copilot stuff as I added a commit here that covers the RenderingInfo that is currently dropped. It sits as an additional object outside of EventData

@randomaccess3

Copy link
Copy Markdown
Contributor Author

@copilot please review the code again

@randomaccess3

Copy link
Copy Markdown
Contributor Author

@copilot can you review the previous comments in this pull request and see if they still apply. Make suggestions as required.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

evtx/EventRecord.cs:567

  • ReadRenderingInfoKeywords calls reader.Read() after ReadSubtree(), but BuildProperties' outer while(reader.Read()) loop will read again. This causes the parser to skip the element immediately following inside .
        // ReadSubtree leaves the original reader positioned on Keywords' end element, so advance past it
        reader.Read();

evtx/SubstitutionArrayEntry.cs:292

  • Array32BitHex parsing loops while index < DataBytes.Length, but BitConverter.ToUInt32 requires at least 4 bytes. If DataBytes isn't a multiple of 4 (corrupt/partial data), this will throw and break parsing.
                while (index < DataBytes.Length)
                {
                    var ul = BitConverter.ToUInt32(DataBytes, index);
                    index += 4;
                    a32h.Add($"0x{ul:X}");
                }

evtx/SubstitutionArrayEntry.cs:303

  • Array64BitHex parsing loops while index < DataBytes.Length, but BitConverter.ToUInt64 requires at least 8 bytes. If DataBytes isn't a multiple of 8, this will throw and abort record parsing.
                while (index < DataBytes.Length)
                {
                    var ul = BitConverter.ToUInt64(DataBytes, index);
                    index += 8;
                    a64h.Add($"0x{ul:X}");
                }

Comment thread evtx/EventRecord.cs Outdated
Comment thread evtx/EventRecord.cs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants