0xrawsec / golang-evtx

GNU General Public License v3.0
158 stars 27 forks source link

Data race #25

Closed steve-offutt closed 4 years ago

steve-offutt commented 4 years ago

Build evtxdump with the following:

go build -race tools/evtxdump/...

Then attempt to dump any EVTX with ./evtxdump $EVTX_FILE

==================
==================
WARNING: DATA RACE
Read at 0x000000c4c5f0 by goroutine 8:
  runtime.slicecopy()
      /usr/local/go/src/runtime/slice.go:197 +0x0
  github.com/0xrawsec/golang-evtx/evtx.UpdateLastElements()
      /home/steve/golang-evtx/evtx/utils.go:81 +0x8c
  github.com/0xrawsec/golang-evtx/evtx.checkFullParsingError()
      /home/steve/golang-evtx/evtx/parser.go:22 +0x40
  github.com/0xrawsec/golang-evtx/evtx.Parse()
      /home/steve/golang-evtx/evtx/parser.go:91 +0x2b6
  github.com/0xrawsec/golang-evtx/evtx.(*TemplateDefinitionData).Parse()
      /home/steve/golang-evtx/evtx/structs.go:718 +0x3d7
  github.com/0xrawsec/golang-evtx/evtx.(*Chunk).ParseTemplateTable()
      /home/steve/golang-evtx/evtx/chunk.go:163 +0x2b1
  github.com/0xrawsec/golang-evtx/evtx.(*File).FetchChunk()
      /home/steve/golang-evtx/evtx/evtx.go:231 +0x5a4
  github.com/0xrawsec/golang-evtx/evtx.(*File).FastEvents.func1.1()
      /home/steve/golang-evtx/evtx/evtx.go:399 +0x186

Previous write at 0x000000c4c5f0 by goroutine 11:
  github.com/0xrawsec/golang-evtx/evtx.UpdateLastElements()
      /home/steve/golang-evtx/evtx/utils.go:82 +0x9c
  github.com/0xrawsec/golang-evtx/evtx.checkFullParsingError()
      /home/steve/golang-evtx/evtx/parser.go:22 +0x40
  github.com/0xrawsec/golang-evtx/evtx.Parse()
      /home/steve/golang-evtx/evtx/parser.go:63 +0x1898
  github.com/0xrawsec/golang-evtx/evtx.Event.GoEvtxMap()
      /home/steve/golang-evtx/evtx/event.go:65 +0x180
  github.com/0xrawsec/golang-evtx/evtx.(*Chunk).Events.func1()
      /home/steve/golang-evtx/evtx/chunk.go:249 +0x16f

Goroutine 8 (running) created at:
  github.com/0xrawsec/golang-evtx/evtx.(*File).FastEvents.func1()
      /home/steve/golang-evtx/evtx/evtx.go:392 +0xe8

Goroutine 11 (running) created at:
  github.com/0xrawsec/golang-evtx/evtx.(*Chunk).Events()
      /home/steve/golang-evtx/evtx/chunk.go:244 +0xcf
  github.com/0xrawsec/golang-evtx/evtx.(*File).FastEvents.func1.1()
      /home/steve/golang-evtx/evtx/evtx.go:404 +0x23d
==================
steve-offutt commented 4 years ago

I cannot share the EVTX that triggered this race detector. However, after some testing it appears in my experience that the race detector is triggered on larger files ~50MB or larger. I just tried running some of the EVTX located here: https://github.com/sbousseaden/EVTX-ATTACK-SAMPLES.git and none seem to trigger it yet.

steve-offutt commented 4 years ago

It appears that the race detector is triggering on the global lastParsedElements. The only place that this variable is being altered is within the function evtx/utils.go > UpdateLastElements()

https://github.com/0xrawsec/golang-evtx/blob/fb1efdb926d27bb62f9b82f532fdd6a66fa14741/evtx/utils.go#L80-L83

According to the comments in globals.go, lastParsedElements is used for debug purposes. Is this still the case? I can confirm that after commenting out the guts on the function UpdateLastElements() the race detector is no longer triggered.

qjerome commented 4 years ago

Hi @steve-offutt,

I am not going to fix this issue because if I do I will remove debugging capability in case of issue. Even though the race is present I don't think it is a big deal since it does not impact functionality of the library and tools. Probably you know it but I would not recommend you to compile with the -race option for everyday use because the tool performance is impacted a lot by the race detector.

Cheers,

qjerome commented 4 years ago

Hi @steve-offutt,

I fixed the data race in a more appropriate way. I used a RWMutex as you suggested for the lastElement structure. Additionnaly, I introduced a debug flag for the parser for which the lastElement structure is updated and used only when the debug flag is set. As a conclusion setting and releasing the mutex does not slow down concurrency in normal circumstances.

Thanks for reporting the issue and have a nice day,

steve-offutt commented 4 years ago

Thank you!