omerbenamram / evtx

A Fast (and safe) parser for the Windows XML Event Log (EVTX) format
Apache License 2.0
625 stars 61 forks source link

(bugfix) Fix off-by-6 bug in assemble.rs #238

Closed dgmcdona closed 3 months ago

dgmcdona commented 3 months ago

This fixes an off-by-six bug in assemble.rs that was causing string-cache miss backup parsing to fail. When the string cache is populated in string_cache.rs, from the given offset in the chunk header, a BinXmlNameLink (which is 6 total bytes in size) is read from the cursor, followed by the BinXmlName. On cache misses, assemble.rs was not reading the BinXmlNameLink which was in turn failing to advance the cursor by the number of bytes required.

This commit adds 6 to the offset in assemble.rs, fixing this problem.

See https://github.com/omerbenamram/evtx/blob/6f374c8d42370397a6fd45d69ebb9b46dfbf73ae/src/string_cache.rs#L24

omerbenamram commented 3 months ago

Hi, thanks for this - do you have a sample that triggers this?

dgmcdona commented 3 months ago

You can see the effects of this by overwriting one of the string table entries with null bytes. Here's an example from samples/security.evtx. I made the following change to two bytes at offset 0x1094:

radiff2 -x samples/security.evtx  ~/Downloads/security_bad_string_table.evtx
  offset     0 1 2 3 4 5 6 7 8 9 A B C D E F 0123456789ABCDEF    0 1 2 3 4 5 6 7 8 9 A B C D E F 0123456789ABCDEF
0x00000000  456c6646696c65000000000000000000 ElfFile.........   456c6646696c65000000000000000000 ElfFile.........
0x00000010  1900000000000000b208000000000000 ................   1900000000000000b208000000000000 ................
...
0x00001090! ec070000f3050000e804000000000000 ................   ec07000000000000e804000000000000 ................
0x000010a0  0000000000000000660900003d030000 ........f...=...   0000000000000000660900003d030000 ........f...=...
0x000010b0  00000000000000004071000000000000 ........@q......   00000000000000004071000000000000 ........@q......

This causes a change in the JSON output when parsed using the current release:

diff <(jq . sec_good.jsonl | gron) <(jq . sec_bad.jsonl | gron)
13c13
< json.Event.System.Execution["#attributes"].ThreadID = 460;
---
> json.Event.System.Execution["#attributes"][""] = 460;
22d21
< json.Event.System.Task = 12288;
26a26
> json.Event.System[""] = 12288;

This fix produces an identical JSON file to the original since the string cache miss is now handled correctly. I can add a corrupt version of the existing sample + an insta snapshot test if you'd like.

dgmcdona commented 3 months ago

I went ahead and added the test that I mentioned above.

omerbenamram commented 3 months ago

Thank you!