Skip to content

Fix EvtVarTypeAnsiString conversion in winlogbeat#44026

Merged
marc-gr merged 6 commits intoelastic:mainfrom
AltairQ:main
Jul 8, 2025
Merged

Fix EvtVarTypeAnsiString conversion in winlogbeat#44026
marc-gr merged 6 commits intoelastic:mainfrom
AltairQ:main

Conversation

@AltairQ
Copy link
Copy Markdown
Contributor

@AltairQ AltairQ commented Apr 22, 2025

Handle null terminator in ANSIBytesToString

WHAT:

Trim AnsiStringVal to the null terminator in the ANSIBytesToString function.

WHY:

In winlogbeat/sys/wineventlog/syscall_windows.go, there's a recurring pattern of _EvtGet* WinAPI functions called twice, first with a nil buffer pointer, to establish the required buffer size. Indeed, looking at e.g. documentation for EvtGetEventMetadataProperty this seems to be the right way. However, despite the phrase "required buffer size" in the docs, the value returned via EventMetadataPropertyBufferUsed is not guaranteed to exactly describe the retrieved property size.

The EvtVariant.Data method makes an implicit assumption that variable-length event data types fill the buf[offset:] byte slice exactly, factoring in further golang-specific interpretation. This certainly isn't true for both flavours of strings, for two reasons:

  • The null terminator is not part of a string in golang.
  • wevtapi.dll!EvtGetEventMetadataProperty occasionally returns a buffer size rounded up to align to 4 or 8

I have no clue why inflating the returned buffer size happens, but I have oberved it with my very own eyes, tracing a v9.0.0 winlogbeat on a Win10 machine with default sysmon+winlogbeat configuration. I have it on good authority that a similar thing happens on Windows Server machines.

Anyway, this detail makes no difference for consumers written in languages with first-class null-terminated strings. According to the EVT_VARIANT documentation, both StringVal and AnsiStringVal are null terminated.
The parsing logic for StringVal/EvtVarTypeString is not affected, because it relies on common.UTF16ToUTF8Bytes, which explicitly handles the (wide) null terminator. However, the EvtVarTypeAnsiString case passes the byte slice directly to a Decoder from golang.org/x/text/encoding, which happily interprets null bytes as U+0000.

The end result is that events with fields with inType="win:AnsiString" contain extra code points beyond the correct string; at the very least, a trailing U+0000. This is especially severe for empty strings.
It's easiest to spot with the Microsoft-Windows-Kernel-Boot event provider (enabled and emitting events by default), event code 27, field LoadOptions. The decompiled manifest confirms that its type is AnsiString.

(Please note that I'm not a golang developer. This was the first project I've ever compiled.)

Checklist

  • [Hopefully] My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Disruptive User Impact

I can't imagine anyone relies on this bug.

How to test this PR locally

  • Start Winlogbeat (v9.0 / current main) with the default configuration, but edited to use output.file
  • Check the output files for events containing stray \u0000 codepoints -- see the Logs section
  • Compile the patched version
  • Run it again, first deleting the registry
  • Observe no stray null characters

Testing using an EVTX file would've been better, sure, but I couldn't get it to work. I'd be happy to provide one.

Related issues

Logs

With the bug

Example of an affected event, generated as described above. Notice the stray null byte in winlog.event_data.LoadOptions.

{
    "@timestamp": "2025-04-08T23:11:57.099Z",
    "@metadata": {
        "beat": "winlogbeat",
        "type": "_doc",
        "version": "9.0.0"
    },
    "host": {
        "hostname": "(REDACTED)",
        "architecture": "x86_64",
        "os": {
            "build": "19045.5737",
            "type": "windows",
            "platform": "windows",
            "version": "10.0",
            "family": "windows",
            "name": "Windows 10 Pro",
            "kernel": "10.0.19041.5737 (WinBuild.160101.0800)"
        },
        "id": "b7f83483-6ced-4ed7-8a45-ff70f45f821f",
        "ip": [
            "(REDACTED)"
        ],
        "mac": [
            "(REDACTED)"
        ],
        "name": "(REDACTED)"
    },
    "winlog": {
        "provider_guid": "{15CA44FF-4D7A-4BAA-BBA5-0998955E531E}",
        "provider_name": "Microsoft-Windows-Kernel-Boot",
        "computer_name": "(REDACTED)",
        "user": {
            "identifier": "S-1-5-18",
            "domain": "NT AUTHORITY",
            "name": "SYSTEM",
            "type": "Well Known Group"
        },
        "event_data": {
            "BootType": "0",
            "LoadOptions": " NOEXECUTE=OPTIN  NOVGA\u0000"
        },
        "channel": "System",
        "version": 1,
        "event_id": "27",
        "opcode": "Info",
        "record_id": 1351,
        "process": {
            "pid": 4,
            "thread": {
                "id": 8
            }
        },
        "task": "BootType"
    },
    "event": {
        "code": "27",
        "kind": "event",
        "provider": "Microsoft-Windows-Kernel-Boot",
        "action": "BootType",
        "created": "2025-04-22T13:13:08.659Z"
    },
    "log": {
        "level": "information"
    },
    "message": "The boot type was 0.",
    "ecs": {
        "version": "8.0.0"
    },
    "agent": {
        "name": "(REDACTED)",
        "type": "winlogbeat",
        "version": "9.0.0",
        "ephemeral_id": "f2c28050-e72b-4c51-9851-c4162f40e256",
        "id": "12c3d9d7-8121-42d3-864d-c0e2cc957ab6"
    }
}

Another, more severe example, from the same machine and the same event provider (just the relevant portion):

    "winlog": {
        "provider_name": "Microsoft-Windows-Kernel-Boot",
        "opcode": "Info",
        "provider_guid": "{15CA44FF-4D7A-4BAA-BBA5-0998955E531E}",
        "computer_name": "(REDACTED)",
        "event_data": {
            "BootType": "2",
            "LoadOptions": "\u0000\u0000i\u0000c\u0000r\u0000"
        },
        "process": {
            "pid": 4,
            "thread": {
                "id": 16028
            }
        },
        "event_id": "27",
        "record_id": 1786,
        "task": "BootType",
        "version": 1,
        "channel": "System"
    },

I strongly suspect this is a leak from a re-used buffer, and what we're seing in winlog.event_data.LoadOptions are the remnants of "Microsoft" encoded with UTF-16LE.

Patched

Just the winlog part:

"winlog": {
    "user": {
        "identifier": "S-1-5-18",
        "domain": "NT AUTHORITY",
        "name": "SYSTEM",
        "type": "Well Known Group"
    },
    "event_data": {
        "BootType": "0",
        "LoadOptions": " NOEXECUTE=OPTIN  NOVGA"
    },
    "version": 1,
    "record_id": 10,
    "opcode": "Info",
    "provider_guid": "{15CA44FF-4D7A-4BAA-BBA5-0998955E531E}",
    "channel": "System",
    "task": "BootType",
    "event_id": "27",
    "provider_name": "Microsoft-Windows-Kernel-Boot",
    "computer_name": "(REDACTED)",
    "process": {
        "pid": 4,
        "thread": {
            "id": 8
        }
    }
}

And the analogous event to the second "bugged" example; notice the LoadOptions are missing completely, since the field is empty.

"winlog": {
    "event_id": "27",
    "record_id": 2067,
    "task": "BootType",
    "provider_guid": "{15CA44FF-4D7A-4BAA-BBA5-0998955E531E}",
    "provider_name": "Microsoft-Windows-Kernel-Boot",
    "version": 1,
    "event_data": {
        "BootType": "2"
    },
    "channel": "System",
    "opcode": "Info",
    "computer_name": "(REDACTED)",
    "process": {
        "pid": 4,
        "thread": {
            "id": 23660
        }
    }
}

Bonus

How the ~same events look in Event Viewer:
image

image

@botelastic botelastic Bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 22, 2025
@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Apr 22, 2025

This pull request does not have a backport label.
If this is a bug or security fix, could you label this PR @AltairQ? 🙏.
For such, you'll need to label your PR with:

  • The upcoming major version of the Elastic Stack
  • The upcoming minor version of the Elastic Stack (if you're not pushing a breaking change)

To fixup this pull request, you need to add the backport labels for the needed
branches, such as:

  • backport-8./d is the label to automatically backport to the 8./d branch. /d is the digit
  • backport-active-all is the label that automatically backports to all active branches.
  • backport-active-8 is the label that automatically backports to all active minor branches for the 8 major.
  • backport-active-9 is the label that automatically backports to all active minor branches for the 9 major.

@AltairQ
Copy link
Copy Markdown
Contributor Author

AltairQ commented Apr 22, 2025

I would gladly label this PR with the appropriate labels, if I could edit them. Is there some special markdown syntax I'm not aware of?

@AltairQ AltairQ marked this pull request as ready for review April 29, 2025 00:44
@AltairQ AltairQ requested a review from a team as a code owner April 29, 2025 00:44
@andrewkroh andrewkroh added Winlogbeat bugfix Team:Security-Windows Platform Windows Platform Team in Security Solution labels Apr 29, 2025
@botelastic botelastic Bot removed the needs_team Indicates that the issue/PR needs a Team:* label label Apr 29, 2025
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/sec-windows-platform (Team:Security-Windows Platform)

@andrewkroh andrewkroh added backport-active-8 Automated backport with mergify to all the active 8.[0-9]+ branches backport-active-9 Automated backport with mergify to all the active 9.[0-9]+ branches labels Apr 29, 2025
@marc-gr
Copy link
Copy Markdown
Contributor

marc-gr commented May 27, 2025

Thanks for the contribution!

Testing using an EVTX file would've been better, sure, but I couldn't get it to work. I'd be happy to provide one.

If you could add an evtx to the PR that is able to be commited (properly sanitized, etc) I am happy to set the tests up for you.

If not possible let me know any way and will move on with the PR

@AltairQ
Copy link
Copy Markdown
Contributor Author

AltairQ commented May 27, 2025

Sure, see attached a sample EVTX (.dmp extension added to please GitHub):
kernel-boot event 27 samples.evtx.dmp
I've prepared it by manually saving (using Event Viewer) 4 events from the same provider as in the first post. I didn't tweak it afterwards -- I don't think it's particularly sensitive.

I followed this guide but I still couldn't get it to ingest any events, so I can't claim the same bug will be triggered. I've at least peeked inside the file and the LoadOptions values are stored as narrow strings, so I hope this will work, assuming same datapaths are taken.
Let me know if you need anything else!

@marc-gr marc-gr enabled auto-merge (squash) July 7, 2025 11:02
@marc-gr
Copy link
Copy Markdown
Contributor

marc-gr commented Jul 8, 2025

/test

@marc-gr marc-gr merged commit 6e1d4ea into elastic:main Jul 8, 2025
32 checks passed
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jul 8, 2025

@Mergifyio backport 8.17 8.18 8.19 9.0 9.1

@mergify
Copy link
Copy Markdown
Contributor

mergify Bot commented Jul 8, 2025

mergify Bot pushed a commit that referenced this pull request Jul 8, 2025
* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)
mergify Bot pushed a commit that referenced this pull request Jul 8, 2025
* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)
mergify Bot pushed a commit that referenced this pull request Jul 8, 2025
* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)
mergify Bot pushed a commit that referenced this pull request Jul 8, 2025
* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)
mergify Bot pushed a commit that referenced this pull request Jul 8, 2025
* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)
marc-gr added a commit that referenced this pull request Jul 8, 2025
* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------


(cherry picked from commit 6e1d4ea)

Co-authored-by: Altair <AltairQ@users.noreply.github.com>
Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
marc-gr added a commit that referenced this pull request Jul 8, 2025
* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------


(cherry picked from commit 6e1d4ea)

Co-authored-by: Altair <AltairQ@users.noreply.github.com>
Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
marc-gr added a commit that referenced this pull request Jul 8, 2025
…beat (#45228)

* Fix EvtVarTypeAnsiString conversion in winlogbeat (#44026)

* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Altair <AltairQ@users.noreply.github.com>
Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
marc-gr added a commit that referenced this pull request Jul 8, 2025
…beat (#45229)

* Fix EvtVarTypeAnsiString conversion in winlogbeat (#44026)

* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Altair <AltairQ@users.noreply.github.com>
Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
marc-gr added a commit that referenced this pull request Jul 8, 2025
…eat (#45232)

* Fix EvtVarTypeAnsiString conversion in winlogbeat (#44026)

* Naive first fix

* Fix import order

* Update strings_windows.go

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
(cherry picked from commit 6e1d4ea)

* Update CHANGELOG.next.asciidoc

---------

Co-authored-by: Altair <AltairQ@users.noreply.github.com>
Co-authored-by: Marc Guasch <marc-gr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-active-8 Automated backport with mergify to all the active 8.[0-9]+ branches backport-active-9 Automated backport with mergify to all the active 9.[0-9]+ branches bugfix Team:Security-Windows Platform Windows Platform Team in Security Solution Winlogbeat

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants