Handle non-utf8 strings in protobuf structures

pkg/grpc/tracing/tracing.go

@@ -301,10 +301,6 @@ func (msg *MsgGenericTracepointUnix) HandleMessage() *tetragon.GetEventsResponse
 			tetragonArgs = append(tetragonArgs, &tetragon.KprobeArgument{Arg: &tetragon.KprobeArgument_IntArg{
 				IntArg: v,
 			}})
-		case string:
-			tetragonArgs = append(tetragonArgs, &tetragon.KprobeArgument{Arg: &tetragon.KprobeArgument_StringArg{
-				StringArg: v,
-			}})
 
 		case []byte:
 			tetragonArgs = append(tetragonArgs, &tetragon.KprobeArgument{Arg: &tetragon.KprobeArgument_BytesArg{

pkg/sensors/exec/exec.go

@@ -20,6 +20,7 @@ import (
 	"github.com/cilium/tetragon/pkg/sensors"
 	"github.com/cilium/tetragon/pkg/sensors/exec/procevents"
 	"github.com/cilium/tetragon/pkg/sensors/program"
+	"github.com/cilium/tetragon/pkg/strutils"
 	"github.com/sirupsen/logrus"
 )
 
@@ -136,12 +137,12 @@ func execParse(reader *bytes.Reader) (processapi.MsgProcess, bool, error) {
 		if err != nil {
 			return proc, false, err
 		}
-		proc.Filename = string(data[:])
+		proc.Filename = strutils.UTF8FromBPFBytes(data[:])
 		args = args[unsafe.Sizeof(desc):]
 	} else if exec.Flags&api.EventErrorFilename == 0 {
 		n := bytes.Index(args, []byte{0x00})
 		if n != -1 {
-			proc.Filename = string(args[:n])
+			proc.Filename = strutils.UTF8FromBPFBytes(args[:n])
 			args = args[n+1:]
 		}
 	}
@@ -177,7 +178,7 @@ func execParse(reader *bytes.Reader) (processapi.MsgProcess, bool, error) {
 		cmdArgs = bytes.Split(args, []byte{0x00})
 	}
 
-	proc.Args = string(bytes.Join(cmdArgs[0:], []byte{0x00}))
+	proc.Args = strutils.UTF8FromBPFBytes(bytes.Join(cmdArgs[0:], []byte{0x00}))
 	return proc, false, nil
 }
 

pkg/sensors/exec/exec_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/cilium/tetragon/pkg/sensors/base"
 	"github.com/cilium/tetragon/pkg/sensors/exec/procevents"
 	testsensor "github.com/cilium/tetragon/pkg/sensors/test"
+	"github.com/cilium/tetragon/pkg/strutils"
 	"github.com/cilium/tetragon/pkg/testutils"
 	"github.com/cilium/tetragon/pkg/testutils/perfring"
 	tus "github.com/cilium/tetragon/pkg/testutils/sensors"
@@ -724,5 +725,45 @@ func TestExecParse(t *testing.T) {
 		assert.Equal(t, string(cwd), decCwd)
 	}
 
+	{
+		// - filename (non-utf8)
+		// - args (data event, non-utf8)
+		// - cwd (string)
+
+		var args []byte
+		args = append(args, '\xc3', '\x28', 0, 'a', 'r', 'g', '2', 0)
+		filename := []byte{'p', 'i', 'z', 'z', 'a', '-', '\xc3', '\x28'}
+		cwd := []byte{'/', 'h', 'o', 'm', 'e', '/', '\xc3', '\x28'}
+
+		id := dataapi.DataEventId{Pid: 1, Time: 2}
+		desc := dataapi.DataEventDesc{Error: 0, Pad: 0, Leftover: 0, Size: uint32(len(args[:])), Id: id}
+		err = observer.DataAdd(id, args)
+		assert.NoError(t, err)
+
+		exec.Flags = api.EventDataArgs
+		exec.Size = uint32(processapi.MSG_SIZEOF_EXECVE + len(filename) + binary.Size(desc) + len(cwd) + 1)
+
+		var buf bytes.Buffer
+		binary.Write(&buf, binary.LittleEndian, exec)
+		binary.Write(&buf, binary.LittleEndian, filename)
+		binary.Write(&buf, binary.LittleEndian, []byte{0})
+		binary.Write(&buf, binary.LittleEndian, desc)
+		binary.Write(&buf, binary.LittleEndian, cwd)
+
+		reader := bytes.NewReader(buf.Bytes())
+
+		process, empty, err := execParse(reader)
+		assert.NoError(t, err)
+
+		// execParse check
+		assert.Equal(t, strutils.UTF8FromBPFBytes(filename), process.Filename)
+		assert.Equal(t, strutils.UTF8FromBPFBytes(args)+strutils.UTF8FromBPFBytes(cwd), process.Args)
+		assert.Equal(t, empty, false)
+
+		// ArgsDecoder check
+		decArgs, decCwd := proc.ArgsDecoder(process.Args, process.Flags)
+		assert.Equal(t, "�( arg2", decArgs)
+		assert.Equal(t, strutils.UTF8FromBPFBytes(cwd), decCwd)
+	}
 	observer.DataPurge()
 }

pkg/sensors/tracing/generickprobe.go

@@ -35,6 +35,7 @@ import (
 	"github.com/cilium/tetragon/pkg/sensors"
 	"github.com/cilium/tetragon/pkg/sensors/base"
 	"github.com/cilium/tetragon/pkg/sensors/program"
+	"github.com/cilium/tetragon/pkg/strutils"
 	lru "github.com/hashicorp/golang-lru/v2"
 	"github.com/sirupsen/logrus"
 
@@ -766,7 +767,9 @@ func loadGenericKprobeSensor(bpfDir, mapDir string, load *program.Program, verbo
 		load.LoaderData, load.LoaderData)
 }
 
-func handleGenericKprobeString(r *bytes.Reader) string {
+// 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
 
 	err := binary.Read(r, binary.LittleEndian, &b)
@@ -777,15 +780,17 @@ func handleGenericKprobeString(r *bytes.Reader) string {
 		 * lets just report its on "/" all though pid filtering will mostly
 		 * catch this.
 		 */
-		return "/"
+		logger.GetLogger().WithError(err).Warnf("handleGenericKprobeString: read string size failed")
+		return def
 	}
 	outputStr := make([]byte, b)
 	err = binary.Read(r, binary.LittleEndian, &outputStr)
 	if err != nil {
-		logger.GetLogger().WithError(err).Warnf("String with size %d type err", b)
+		logger.GetLogger().WithError(err).Warnf("handleGenericKprobeString: read string with size %d", b)
+		return def
 	}
 
-	strVal := string(outputStr[:])
+	strVal := strutils.UTF8FromBPFBytes(outputStr[:])
 	lenStrVal := len(strVal)
 	if lenStrVal > 0 && strVal[lenStrVal-1] == '\x00' {
 		strVal = strVal[0 : lenStrVal-1]
@@ -954,7 +959,7 @@ func handleGenericKprobe(r *bytes.Reader) ([]observer.Event, error) {
 			}
 
 			arg.Index = uint64(a.index)
-			arg.Value = handleGenericKprobeString(r)
+			arg.Value = handleGenericKprobeString(r, "/")
 
 			// read the first byte that keeps the flags
 			err := binary.Read(r, binary.LittleEndian, &flags)
@@ -970,7 +975,7 @@ func handleGenericKprobe(r *bytes.Reader) ([]observer.Event, error) {
 			var flags uint32
 
 			arg.Index = uint64(a.index)
-			arg.Value = handleGenericKprobeString(r)
+			arg.Value = handleGenericKprobeString(r, "/")
 
 			// read the first byte that keeps the flags
 			err := binary.Read(r, binary.LittleEndian, &flags)
@@ -982,26 +987,11 @@ func handleGenericKprobe(r *bytes.Reader) ([]observer.Event, error) {
 			arg.Label = a.label
 			unix.Args = append(unix.Args, arg)
 		case gt.GenericFilenameType, gt.GenericStringType:
-			var b int32
 			var arg api.MsgGenericKprobeArgString
 
-			err := binary.Read(r, binary.LittleEndian, &b)
-			if err != nil {
-				logger.GetLogger().WithError(err).Warnf("StringSz type err")
-			}
-			outputStr := make([]byte, b)
-			err = binary.Read(r, binary.LittleEndian, &outputStr)
-			if err != nil {
-				logger.GetLogger().WithError(err).Warnf("String with size %d type err", b)
-			}
-
 			arg.Index = uint64(a.index)
-			strVal := string(outputStr[:])
-			lenStrVal := len(strVal)
-			if lenStrVal > 0 && strVal[lenStrVal-1] == '\x00' {
-				strVal = strVal[0 : lenStrVal-1]
-			}
-			arg.Value = strVal
+			arg.Value = handleGenericKprobeString(r, "")
+
 			arg.Label = a.label
 			unix.Args = append(unix.Args, arg)
 		case gt.GenericCredType:

pkg/sensors/tracing/loader.go

@@ -42,6 +42,7 @@ import (
 	"github.com/cilium/tetragon/pkg/policyfilter"
 	"github.com/cilium/tetragon/pkg/sensors"
 	"github.com/cilium/tetragon/pkg/sensors/program"
+	"github.com/cilium/tetragon/pkg/strutils"
 	"github.com/cilium/tetragon/pkg/tracingpolicy"
 	"golang.org/x/sys/unix"
 )
@@ -168,7 +169,7 @@ func handleLoader(r *bytes.Reader) ([]observer.Event, error) {
 	msg := &tracing.MsgProcessLoaderUnix{
 		ProcessKey: m.ProcessKey,
 		Ktime:      m.Common.Ktime,
-		Path:       string(path),
+		Path:       strutils.UTF8FromBPFBytes(path),
 		Buildid:    m.BuildId[:m.BuildIdSize],
 	}
 	return []observer.Event{msg}, nil

pkg/strutils/strutls.go

@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright Authors of Tetragon
+
+package strutils
+
+import "strings"
+
+// UTF8FromBPFBytes transforms bpf (C) strings to valid utf-8 strings
+//
+// NB(kkourt): strings we get from BPF/kernel are C strings: null-terminated sequence of bytes. They
+// may or may not be valid utf-8 strings. This is true for pathnames (cwd, binary) as well as
+// program arguments. Many of these fields, however, are represented as the protobuf string type,
+// which always has to be utf-8 encoded.
+//
+// This function ensures that by replacing all invalid runes with '�'.
+//
+// Note that this approach means that we loose information.
+// Alternatively, we could use strconf.Quote() or similar to quote the strings but this would add
+// overhead and it will also break the way we 've been representing strings up until now. A better
+// solution would be to update the fields in the proto description to be bytes, and let the proto
+// clients (e.g., tetra CLI and JSON writer) choose their preffered approach.
+func UTF8FromBPFBytes(b []byte) string {
+	return strings.ToValidUTF8(string(b), "�")
+}