From 94faed19f4afc27d317ff9518f16ad274d4b0cfd Mon Sep 17 00:00:00 2001 From: Mahe Tardy Date: Wed, 26 Jul 2023 09:33:56 +0000 Subject: [PATCH] sensors: harden string parsing from BPF events Initially found this issue by trying to fuzz parts of the handler. You can run the fuzzer with the following command: `go test -fuzz=Fuzz_parseString -run=Fuzz_parseString ./pkg/sensors/tracing/` Also added some unit test cases under `Test_parseString`. Previous parsing had flaws that could make Tetragon panic or be OOM killed in certain circumstances. In reality, the string size was already bound by BPF copy_string function to 1024 but it could have theoretically returned a size of -2 as an error code which would have made it panic. Also remove some code duplication that was rewriting the string parsing instead of using the common function. Signed-off-by: Mahe Tardy --- pkg/sensors/tracing/generickprobe.go | 82 ++++++++++++++++------- pkg/sensors/tracing/generickprobe_test.go | 52 ++++++++++++++ 2 files changed, 109 insertions(+), 25 deletions(-) create mode 100644 pkg/sensors/tracing/generickprobe_test.go diff --git a/pkg/sensors/tracing/generickprobe.go b/pkg/sensors/tracing/generickprobe.go index 6345d6374cb..619e07c896d 100644 --- a/pkg/sensors/tracing/generickprobe.go +++ b/pkg/sensors/tracing/generickprobe.go @@ -7,7 +7,9 @@ import ( "bytes" "context" "encoding/binary" + "errors" "fmt" + "io" "net" "net/http" "path" @@ -774,35 +776,43 @@ func loadGenericKprobeSensor(bpfDir, mapDir string, load *program.Program, verbo load.LoaderData, load.LoaderData) } -// handleGenericKprobeString: reads a string argument. Strings are encoded with their size first. -// def is return in case of an error. -func handleGenericKprobeString(r *bytes.Reader, def string) string { - var b int32 +var errParseStringSize = errors.New("error parsing string size from binary") - err := binary.Read(r, binary.LittleEndian, &b) +// this is from bpf/process/types/basic.h 'MAX_STRING' +const maxStringSize = 1024 + +// parseString parses strings encoded from BPF copy_strings in the form: +// *---------*---------* +// | 4 bytes | N bytes | +// | size | string | +// *---------*---------* +func parseString(r io.Reader) (string, error) { + var size int32 + err := binary.Read(r, binary.LittleEndian, &size) if err != nil { - /* If no size then path walk was not possible and file was either - * a mount point or not a "file" at all which can happen if running - * without any filters and kernel opens an anonymous inode. For this - * lets just report its on "/" all though pid filtering will mostly - * catch this. - */ - logger.GetLogger().WithError(err).Warnf("handleGenericKprobeString: read string size failed") - return def + return "", fmt.Errorf("%w: %s", errParseStringSize, err) + } + + if size < 0 { + return "", errors.New("string size is negative") } - outputStr := make([]byte, b) - err = binary.Read(r, binary.LittleEndian, &outputStr) + + // limit the size of the string to avoid huge memory allocation and OOM kill in case of issue + if size > maxStringSize { + return "", fmt.Errorf("string size too large: %d, max size is %d", size, maxStringSize) + } + stringBuffer := make([]byte, size) + err = binary.Read(r, binary.LittleEndian, &stringBuffer) if err != nil { - logger.GetLogger().WithError(err).Warnf("handleGenericKprobeString: read string with size %d", b) - return def + return "", fmt.Errorf("error parsing string from binary with size %d: %s", size, err) } - strVal := strutils.UTF8FromBPFBytes(outputStr[:]) - lenStrVal := len(strVal) - if lenStrVal > 0 && strVal[lenStrVal-1] == '\x00' { - strVal = strVal[0 : lenStrVal-1] + // remove the trailing '\0' from the C string + if len(stringBuffer) > 0 && stringBuffer[len(stringBuffer)-1] == '\x00' { + stringBuffer = stringBuffer[:len(stringBuffer)-1] } - return strVal + + return strutils.UTF8FromBPFBytes(stringBuffer), nil } func ReadArgBytes(r *bytes.Reader, index int, hasMaxData bool) (*api.MsgGenericKprobeArgBytes, error) { @@ -966,7 +976,19 @@ func handleGenericKprobe(r *bytes.Reader) ([]observer.Event, error) { } arg.Index = uint64(a.index) - arg.Value = handleGenericKprobeString(r, "/") + arg.Value, err = parseString(r) + if err != nil { + if errors.Is(err, errParseStringSize) { + // If no size then path walk was not possible and file was + // either a mount point or not a "file" at all which can + // happen if running without any filters and kernel opens an + // anonymous inode. For this lets just report its on "/" all + // though pid filtering will mostly catch this. + arg.Value = "/" + } else { + logger.GetLogger().WithError(err).Warn("error parsing arg type file") + } + } // read the first byte that keeps the flags err := binary.Read(r, binary.LittleEndian, &flags) @@ -982,7 +1004,14 @@ func handleGenericKprobe(r *bytes.Reader) ([]observer.Event, error) { var flags uint32 arg.Index = uint64(a.index) - arg.Value = handleGenericKprobeString(r, "/") + arg.Value, err = parseString(r) + if err != nil { + if errors.Is(err, errParseStringSize) { + arg.Value = "/" + } else { + logger.GetLogger().WithError(err).Warn("error parsing arg type path") + } + } // read the first byte that keeps the flags err := binary.Read(r, binary.LittleEndian, &flags) @@ -997,7 +1026,10 @@ func handleGenericKprobe(r *bytes.Reader) ([]observer.Event, error) { var arg api.MsgGenericKprobeArgString arg.Index = uint64(a.index) - arg.Value = handleGenericKprobeString(r, "") + arg.Value, err = parseString(r) + if err != nil { + logger.GetLogger().WithError(err).Warn("error parsing arg type string") + } arg.Label = a.label unix.Args = append(unix.Args, arg) diff --git a/pkg/sensors/tracing/generickprobe_test.go b/pkg/sensors/tracing/generickprobe_test.go new file mode 100644 index 00000000000..39d426ebcc8 --- /dev/null +++ b/pkg/sensors/tracing/generickprobe_test.go @@ -0,0 +1,52 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright Authors of Tetragon + +package tracing + +import ( + "bytes" + "testing" +) + +func Fuzz_parseString(f *testing.F) { + f.Fuzz(func(t *testing.T, input []byte) { + reader := bytes.NewReader(input) + parseString(reader) + }) +} + +func Test_parseString(t *testing.T) { + tests := []struct { + name string + input bytes.Reader + want string + wantErr bool + }{ + {"normal", *bytes.NewReader([]byte{6, 0, 0, 0, 'p', 'i', 'z', 'z', 'a', 0}), "pizza", false}, + {"shortened", *bytes.NewReader([]byte{3, 0, 0, 0, 'p', 'i', 'z', 'z', 'a', 0}), "piz", false}, + {"too large", *bytes.NewReader([]byte{0, 0, 0, 1, 'p', 'i', 'z', 'z', 'a', 0}), "", true}, + {"error code -2", *bytes.NewReader([]byte{254, 255, 255, 255, 'p', 'i', 'z', 'z', 'a', 0}), "", true}, + {"negative size", *bytes.NewReader([]byte{253, 255, 255, 255, 'p', 'i', 'z', 'z', 'a', 0}), "", true}, + {"missing content", *bytes.NewReader([]byte{1, 0, 0, 0}), "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseString(&tt.input) + if (err != nil) != tt.wantErr { + t.Errorf("got error = %s, wantErr %t", err, tt.wantErr) + } + if got != tt.want { + t.Errorf("got %q, want %q", got, tt.want) + } + }) + } + t.Run("remove trailing null byte", func(t *testing.T) { + out, err := parseString(bytes.NewReader([]byte{6, 0, 0, 0, 'p', 'i', 'z', 'z', 'a', 0})) + if err != nil { + t.Errorf("unexpected error %v", err) + } + if out != "pizza" { + t.Errorf("got %q, want %q", out, "pizza") + } + }) +}