From 32152e879d6be0b56be329d3d202d2e8e24d543b Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Wed, 31 Jan 2018 03:08:35 +0100 Subject: [PATCH 1/2] Winlogbeat: Fix event formatting with missing params There was a crash in the eventlogging module used in legacy Windows versions (XP and 2003 server). It is necessary to account for the case where an event contains fewer parameters than required by its format string. Closes #6234 --- winlogbeat/eventlog/eventlogging.go | 3 +- winlogbeat/eventlog/eventlogging_test.go | 43 ++++++++++ .../sys/eventlogging/eventlogging_windows.go | 55 +++--------- .../sys/eventlogging/stringinserts_windows.go | 86 +++++++++++++++++++ 4 files changed, 142 insertions(+), 45 deletions(-) create mode 100644 winlogbeat/sys/eventlogging/stringinserts_windows.go diff --git a/winlogbeat/eventlog/eventlogging.go b/winlogbeat/eventlog/eventlogging.go index 288cd0400d35..1ca5268b91b0 100644 --- a/winlogbeat/eventlog/eventlogging.go +++ b/winlogbeat/eventlog/eventlogging.go @@ -64,6 +64,7 @@ type eventLogging struct { handle win.Handle // Handle to the event log. readBuf []byte // Buffer for reading in events. formatBuf []byte // Buffer for formatting messages. + insertBuf win.StringInserts // Buffer for parsing insert strings. handles *messageFilesCache // Cached mapping of source name to event message file handles. logPrefix string // Prefix to add to all log entries. @@ -149,7 +150,7 @@ func (l *eventLogging) Read() ([]Record, error) { l.readBuf = l.readBuf[0:numBytesRead] events, _, err := win.RenderEvents( - l.readBuf[:numBytesRead], 0, l.formatBuf, l.handles.get) + l.readBuf[:numBytesRead], 0, l.formatBuf, &l.insertBuf, l.handles.get) if err != nil { return nil, err } diff --git a/winlogbeat/eventlog/eventlogging_test.go b/winlogbeat/eventlog/eventlogging_test.go index cb855463fa2f..702a598fa69b 100644 --- a/winlogbeat/eventlog/eventlogging_test.go +++ b/winlogbeat/eventlog/eventlogging_test.go @@ -475,6 +475,49 @@ func TestReadWhileCleared(t *testing.T) { } } +// Test event messages that include less parameters than required for message +// formating (caused a crash in previous versions) +func TestReadMissingParameters(t *testing.T) { + configureLogp() + log, err := initLog(providerName, sourceName, servicesMsgFile) + if err != nil { + t.Fatal(err) + } + defer func() { + err := uninstallLog(providerName, sourceName, log) + if err != nil { + t.Fatal(err) + } + }() + + var eventID uint32 = 1073748860 + // Missing parameters will be substituted by "(null)" + template := "The %s service entered the (null) state." + msgs := []string{"Windows Update"} + err = log.Report(elog.Info, eventID, msgs) + if err != nil { + t.Fatal(err) + } + + // Read messages: + eventlog, teardown := setupEventLogging(t, 0, map[string]interface{}{"name": providerName}) + defer teardown() + + records, err := eventlog.Read() + if err != nil { + t.Fatal(err) + } + + // Verify the message contents: + assert.Len(t, records, 1) + if len(records) != 1 { + t.FailNow() + } + assert.Equal(t, eventID&0xFFFF, records[0].EventIdentifier.ID) + assert.Equal(t, fmt.Sprintf(template, msgs[0]), + strings.TrimRight(records[0].Message, "\r\n")) +} + func newTestEventLogging(t *testing.T, options map[string]interface{}) EventLog { return newTestEventLog(t, newEventLogging, options) } diff --git a/winlogbeat/sys/eventlogging/eventlogging_windows.go b/winlogbeat/sys/eventlogging/eventlogging_windows.go index 24d5ba72ae24..18a24c7cc78d 100644 --- a/winlogbeat/sys/eventlogging/eventlogging_windows.go +++ b/winlogbeat/sys/eventlogging/eventlogging_windows.go @@ -4,7 +4,6 @@ import ( "bytes" "encoding/binary" "fmt" - "reflect" "strings" "syscall" "time" @@ -100,6 +99,7 @@ func RenderEvents( eventsRaw []byte, lang uint32, buffer []byte, + insertStrings *StringInserts, pubHandleProvider func(string) sys.MessageFiles, ) ([]sys.Event, int, error) { var events []sys.Event @@ -139,21 +139,25 @@ func RenderEvents( event.User = *sid } + if record.numStrings > MaxInsertStrings { + logp.Warn("Record contains %d strings, more than the limit %d. Excess will be ignored.", + record.numStrings, MaxInsertStrings) + record.numStrings = MaxInsertStrings + } // Parse the UTF-16 message insert strings. - stringInserts, stringInsertPtrs, err := parseInsertStrings(record, recordBuf) - if err != nil { + if err = insertStrings.Parse(record, recordBuf); err != nil { event.RenderErr = err.Error() events = append(events, event) continue } - for _, s := range stringInserts { + for _, s := range insertStrings.Strings() { event.EventData.Pairs = append(event.EventData.Pairs, sys.KeyValue{Value: s}) } // Format the parametrized message using the insert strings. event.Message, err = formatMessage(record.sourceName, - record.eventID, lang, stringInsertPtrs, buffer, pubHandleProvider) + record.eventID, lang, insertStrings.Pointer(), buffer, pubHandleProvider) if err != nil { event.RenderErr = err.Error() if errno, ok := err.(syscall.Errno); ok { @@ -180,15 +184,10 @@ func formatMessage( sourceName string, eventID uint32, lang uint32, - stringInserts []uintptr, + stringInserts uintptr, buffer []byte, pubHandleProvider func(string) sys.MessageFiles, ) (string, error) { - var addr uintptr - if len(stringInserts) > 0 { - addr = reflect.ValueOf(&stringInserts[0]).Pointer() - } - messageFiles := pubHandleProvider(sourceName) var lastErr error @@ -208,7 +207,7 @@ func formatMessage( lang, &buffer[0], // Max size allowed is 64k bytes. uint32(len(buffer)/2), // Size of buffer in TCHARS - addr) + stringInserts) // bufferUsed = numChars * sizeof(TCHAR) + sizeof(null-terminator) bufferUsed := int(numChars*2 + 2) if err == syscall.ERROR_INSUFFICIENT_BUFFER { @@ -391,38 +390,6 @@ func parseEventLogRecord(buffer []byte) (eventLogRecord, error) { return record, nil } -// parseInsertStrings parses the insert strings from buffer which should contain -// an eventLogRecord. It returns an array of strings (data is copied and -// converted to UTF-8) and an array of pointers to the null-terminated UTF-16 -// strings within buffer. -func parseInsertStrings(record eventLogRecord, buffer []byte) ([]string, []uintptr, error) { - if record.numStrings < 1 { - return nil, nil, nil - } - - inserts := make([]string, record.numStrings) - insertPtrs := make([]uintptr, record.numStrings) - offset := int(record.stringOffset) - bufferPtr := reflect.ValueOf(&buffer[0]).Pointer() - - for i := 0; i < int(record.numStrings); i++ { - if offset > len(buffer) { - return nil, nil, fmt.Errorf("Failed reading string number %d, "+ - "offset=%d, len(buffer)=%d, record=%+v", i+1, offset, - len(buffer), record) - } - insertStr, length, err := sys.UTF16BytesToString(buffer[offset:]) - if err != nil { - return nil, nil, err - } - inserts[i] = insertStr - insertPtrs[i] = bufferPtr + uintptr(offset) - offset += length - } - - return inserts, insertPtrs, nil -} - func parseSID(record eventLogRecord, buffer []byte) (*sys.SID, error) { if record.userSidLength == 0 { return nil, nil diff --git a/winlogbeat/sys/eventlogging/stringinserts_windows.go b/winlogbeat/sys/eventlogging/stringinserts_windows.go new file mode 100644 index 000000000000..599d6f1ced28 --- /dev/null +++ b/winlogbeat/sys/eventlogging/stringinserts_windows.go @@ -0,0 +1,86 @@ +package eventlogging + +import ( + "fmt" + "reflect" + "unsafe" + + "github.com/elastic/beats/winlogbeat/sys" +) + +const ( + // MaxInsertStrings is the maximum number of strings that can be formatted by + // FormatMessage API. + MaxInsertStrings = 99 +) + +var ( + nullPlaceholder = []byte{'(', 0, 'n', 0, 'u', 0, 'l', 0, 'l', 0, ')', 0, 0, 0} + nullPlaceholderPtr = uintptr(unsafe.Pointer(&nullPlaceholder[0])) +) + +// StringInserts stores the string inserts for an event, as arrays of string +// and pointer to UTF-16 zero-terminated string suitable to be passed to +// the Windows API. The array of pointers has enough entries to ensure that +// a call to FormatMessage will never crash. +type StringInserts struct { + pointers [MaxInsertStrings]uintptr + inserts []string + address uintptr +} + +// Parse parses the insert strings from buffer which should contain +// an eventLogRecord. +func (b *StringInserts) Parse(record eventLogRecord, buffer []byte) error { + if b.inserts == nil { // initialise struct + b.inserts = make([]string, 0, MaxInsertStrings) + b.address = reflect.ValueOf(&b.pointers[0]).Pointer() + } + b.clear() + + n := int(record.numStrings) + if n > MaxInsertStrings { + return fmt.Errorf("number of insert strings in the record (%d) is larger than the limit (%d)", n, MaxInsertStrings) + } + + b.inserts = b.inserts[:n] + if n == 0 { + return nil + } + offset := int(record.stringOffset) + bufferPtr := reflect.ValueOf(&buffer[0]).Pointer() + + for i := 0; i < n; i++ { + if offset > len(buffer) { + return fmt.Errorf("Failed reading string number %d, "+ + "offset=%d, len(buffer)=%d, record=%+v", i+1, offset, + len(buffer), record) + } + insertStr, length, err := sys.UTF16BytesToString(buffer[offset:]) + if err != nil { + return err + } + b.inserts[i] = insertStr + b.pointers[i] = bufferPtr + uintptr(offset) + offset += length + } + + return nil +} + +// Strings returns the array of strings representing the insert strings. +func (b *StringInserts) Strings() []string { + return b.inserts +} + +// Pointer returns a pointer to an array of UTF-16 strings suitable to be +// passed to the FormatMessage API. +func (b *StringInserts) Pointer() uintptr { + return b.address +} + +func (b *StringInserts) clear() { + for i := 0; i < MaxInsertStrings && b.pointers[i] != nullPlaceholderPtr; i++ { + b.pointers[i] = nullPlaceholderPtr + } +} From d5415c29edfafd4a113166a541a91b6e8971eff8 Mon Sep 17 00:00:00 2001 From: Adrian Serrano Date: Fri, 2 Feb 2018 21:12:18 +0100 Subject: [PATCH 2/2] Updated CHANGELOG --- CHANGELOG.asciidoc | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.asciidoc b/CHANGELOG.asciidoc index 0ea7e86aca50..f10516e95abf 100644 --- a/CHANGELOG.asciidoc +++ b/CHANGELOG.asciidoc @@ -128,6 +128,7 @@ https://github.com/elastic/beats/compare/v6.0.0-beta2...master[Check the HEAD di - Fix the registry file. It was not correctly storing event log names, and upon restart it would begin reading at the start of each event log. {issue}5813[5813] - Fix config validation to allow `event_logs.processors`. [pull]6217[6217] +- Fixed a crash under Windows 2003 and XP when an event had less insert strings than required by its format string. {pull}6247[6247] ==== Added