rework to pedantic operations

Signed-off-by: Eliott Bouhana <[email protected]>

internal/appsec/dyngo/operation.go

@@ -22,13 +22,18 @@ package dyngo
 
 import (
 	"context"
-	"gopkg.in/DataDog/dd-trace-go.v1/internal/orchestrion"
 	"sync"
+	"sync/atomic"
 
-	"go.uber.org/atomic"
-	"gopkg.in/DataDog/dd-trace-go.v1/internal/log"
+	"gopkg.in/DataDog/dd-trace-go.v1/internal/orchestrion"
 )
 
+// LogError is the function used to log errors in the dyngo package.
+// This is required because we really want to be able to log errors from dyngo
+// but the log package depend on too much packages that we want to instrument.
+// So we need to do this to avoid dependency cycles.
+var LogError func(string, ...any)
+
 // Operation interface type allowing to register event listeners to the
 // operation. The event listeners will be automatically removed from the
 // operation once it finishes so that it no longer can be called on finished
@@ -316,8 +321,8 @@ func (b *dataBroadcaster) clear() {
 
 func emitData[T any](b *dataBroadcaster, v T) {
 	defer func() {
-		if r := recover(); r != nil {
-			log.Error("appsec: recovered from an unexpected panic from an event listener: %+v", r)
+		if r := recover(); r != nil && LogError != nil {
+			LogError("appsec: recovered from an unexpected panic from an event listener: %+v", r)
 		}
 	}()
 	b.mu.RLock()
@@ -347,8 +352,8 @@ func (r *eventRegister) clear() {
 
 func emitEvent[O Operation, T any](r *eventRegister, op O, v T) {
 	defer func() {
-		if r := recover(); r != nil {
-			log.Error("appsec: recovered from an unexpected panic from an event listener: %+v", r)
+		if r := recover(); r != nil && LogError != nil {
+			LogError("appsec: recovered from an unexpected panic from an event listener: %+v", r)
 		}
 	}()
 	r.mu.RLock()

internal/appsec/emitter/ossec/lfi.go

@@ -6,53 +6,36 @@
 package ossec
 
 import (
-	"context"
-	"os"
-	"sync"
+	"io/fs"
 
-	"gopkg.in/DataDog/dd-trace-go.v1/appsec/events"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo"
-	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/ossec/types"
-	"gopkg.in/DataDog/dd-trace-go.v1/internal/log"
 )
 
-var badInputContextOnce sync.Once
-
-const readMask = os.O_RDONLY | os.O_RDWR
-
-func ProtectOpen(ctx context.Context, path string, flags int) error {
-	if flags&readMask == 0 { // We only care about read operations
-		return nil
+type (
+	// OpenOperation type embodies any kind of function calls that will result in a call to an open(2) syscall
+	OpenOperation struct {
+		dyngo.Operation
+		blockErr error
 	}
 
-	parent, _ := dyngo.FromContext(ctx)
-	if parent == nil { // No parent operation => we can't monitor the request
-		badInputContextOnce.Do(func() {
-			log.Debug("appsec: file opening monitoring attempt ignored: could not find the handler " +
-				"instrumentation metadata in the request context: the request handler is not being monitored by a " +
-				"middleware function or the incoming request context has not be forwarded correctly to the `os.Open` call")
-		})
-		return nil
+	// OpenOperationArgs is the arguments for an open operation
+	OpenOperationArgs struct {
+		// Path is the path to the file to be opened
+		Path string
+		// Flags are the flags passed to the open(2) syscall
+		Flags int
+		// Perms are the permissions passed to the open(2) syscall if the creation of a file is required
+		Perms fs.FileMode
 	}
 
-	op := &types.OpenOperation{
-		Operation: dyngo.NewOperation(parent),
-	}
-
-	var err *events.BlockingSecurityEvent
-	dyngo.OnData(op, func(e *events.BlockingSecurityEvent) {
-		err = e
-	})
-
-	dyngo.StartOperation(op, types.OpenOperationArgs{
-		Path: path,
-	})
-	dyngo.FinishOperation(op, types.OpenOperationRes{})
-
-	if err != nil {
-		log.Debug("appsec: malicious local file inclusion attack blocked by the WAF on path: %s", path)
-		return err
+	// OpenOperationRes is the result of an open operation
+	OpenOperationRes struct {
+		// File is the file descriptor returned by the open(2) syscall
+		File *any
+		// Err is the error returned by the function
+		Err *error
 	}
+)
 
-	return nil
-}
+func (OpenOperationArgs) IsArgOf(*OpenOperation)   {}
+func (OpenOperationRes) IsResultOf(*OpenOperation) {}

internal/appsec/listener/ossec/lfi.go

@@ -6,11 +6,14 @@
 package ossec
 
 import (
+	"os"
+
 	"github.com/DataDog/appsec-internal-go/limiter"
 	waf "github.com/DataDog/go-libddwaf/v3"
 
+	"gopkg.in/DataDog/dd-trace-go.v1/appsec/events"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo"
-	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/ossec/types"
+	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/ossec"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/sharedsec"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/trace"
 )
@@ -19,8 +22,31 @@ const (
 	ServerIOFSFileAddr = "server.io.fs.file"
 )
 
-func RegisterOpenListener(op dyngo.Operation, events *trace.SecurityEventsHolder, wafCtx *waf.Context, limiter limiter.Limiter) {
-	dyngo.On(op, sharedsec.MakeWAFRunListener(events, wafCtx, limiter, func(args types.OpenOperationArgs) waf.RunAddressData {
+func RegisterOpenListener(op dyngo.Operation, eventsHolder *trace.SecurityEventsHolder, wafCtx *waf.Context, limiter limiter.Limiter) {
+	println("RegisterOpenListener")
+	runWAF := sharedsec.MakeWAFRunListener(eventsHolder, wafCtx, limiter, func(args ossec.OpenOperationArgs) waf.RunAddressData {
 		return waf.RunAddressData{Ephemeral: map[string]any{ServerIOFSFileAddr: args.Path}}
-	}))
+	})
+
+	dyngo.On(op, func(op *ossec.OpenOperation, args ossec.OpenOperationArgs) {
+		// We only care about read operations. We don't want to scan for write operations
+		// os.O_RDONLY is not a flag it's simply 0, so we can't do a simple bit mask
+		// If os.O_CREATE is set, we also want to monitor the operation because the file must not exist
+		if (args.Flags&os.O_RDWR == 0 && args.Flags != os.O_RDONLY) || args.Flags&os.O_CREATE != 0 {
+			return
+		}
+
+		dyngo.OnData(op, func(e *events.BlockingSecurityEvent) {
+			println("dyngo.OnData")
+			dyngo.OnFinish(op, func(op *ossec.OpenOperation, res ossec.OpenOperationRes) {
+				println("dyngo.OnFinish")
+				if res.Err != nil {
+					*res.Err = e
+				}
+			})
+		})
+
+		println("runWAF")
+		runWAF(op, args)
+	})
 }

internal/appsec/waf_test.go

@@ -6,28 +6,34 @@
 package appsec_test
 
 import (
+	"context"
 	"database/sql"
 	"encoding/json"
 	"fmt"
 	"io"
+	"io/fs"
 	"log"
 	"math/rand"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
 	"os"
+	"strconv"
 	"strings"
 	"testing"
 
 	internal "github.com/DataDog/appsec-internal-go/appsec"
 	waf "github.com/DataDog/go-libddwaf/v3"
+
 	pAppsec "gopkg.in/DataDog/dd-trace-go.v1/appsec"
 	"gopkg.in/DataDog/dd-trace-go.v1/appsec/events"
 	sqltrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/database/sql"
 	httptrace "gopkg.in/DataDog/dd-trace-go.v1/contrib/net/http"
 	"gopkg.in/DataDog/dd-trace-go.v1/ddtrace/mocktracer"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/config"
+	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo"
+	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/emitter/ossec"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/listener/httpsec"
 
 	_ "github.com/glebarez/go-sqlite"
@@ -642,7 +648,115 @@ func TestRASPSQLi(t *testing.T) {
 			})
 		}
 	}
+}
+
+func TestRASPLFI(t *testing.T) {
+	t.Setenv("DD_APPSEC_RULES", "testdata/rasp.json")
+	appsec.Start()
+	defer appsec.Stop()
+
+	if !appsec.RASPEnabled() {
+		t.Skip("RASP needs to be enabled for this test")
+	}
+
+	// Simulate what orchestrion does
+	WrappedOpen := func(ctx context.Context, path string, flags int) (file *os.File, err error) {
+		parent, _ := dyngo.FromContext(ctx)
+		op := &ossec.OpenOperation{
+			Operation: dyngo.NewOperation(parent),
+		}
+
+		dyngo.StartOperation(op, &ossec.OpenOperationArgs{
+			Path:  path,
+			Flags: flags,
+			Perms: fs.FileMode(0),
+		})
+
+		var x any = file
+		defer dyngo.FinishOperation(op, &ossec.OpenOperationRes{
+			File: &x,
+			Err:  &err,
+		})
 
+		// Open the file
+		file = &os.File{}
+
+		return
+	}
+
+	mux := httptrace.NewServeMux()
+	mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) {
+		// Subsequent spans inherit their parent from context.
+		path := r.URL.Query().Get("path")
+		block := r.URL.Query().Get("block")
+		if block == "true" {
+			// Make sure we don't scan writing operations
+			file, err := WrappedOpen(r.Context(), path, os.O_WRONLY)
+			require.NoError(t, err)
+			require.NotNil(t, file)
+
+			file, err = WrappedOpen(r.Context(), path, os.O_CREATE|os.O_RDWR)
+			require.NoError(t, err)
+			require.NotNil(t, file)
+
+			// Make sure we scan reading operations
+			file, err = WrappedOpen(r.Context(), path, os.O_RDONLY)
+			require.ErrorIs(t, err, &events.BlockingSecurityEvent{})
+			require.Nil(t, file)
+
+			return
+		}
+
+		_, err := WrappedOpen(r.Context(), "/tmp/test", os.O_RDWR)
+		require.NoError(t, err)
+		w.WriteHeader(204)
+	})
+	srv := httptest.NewServer(mux)
+	defer srv.Close()
+
+	for _, tc := range []struct {
+		name  string
+		path  string
+		block bool
+	}{
+		{
+			name:  "no-error",
+			path:  "",
+			block: false,
+		},
+		{
+			name:  "passwd",
+			path:  "/etc/passwd",
+			block: true,
+		},
+		{
+			name:  "shadow",
+			path:  "/etc/shadow",
+			block: true,
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			mt := mocktracer.Start()
+			defer mt.Stop()
+
+			req, err := http.NewRequest("GET", srv.URL+"?path="+tc.path+"&block="+strconv.FormatBool(tc.block), nil)
+			require.NoError(t, err)
+			res, err := srv.Client().Do(req)
+			require.NoError(t, err)
+			defer res.Body.Close()
+
+			spans := mt.FinishedSpans()
+			require.Len(t, spans, 1)
+
+			if tc.block {
+				require.Equal(t, 403, res.StatusCode)
+				require.Contains(t, spans[0].Tag("_dd.appsec.json"), "rasp-930-100")
+				require.Contains(t, spans[0].Tags(), "_dd.stack")
+			} else {
+				require.Equal(t, 204, res.StatusCode)
+			}
+		})
+	}
 }
 
 // BenchmarkSampleWAFContext benchmarks the creation of a WAF context and running the WAF on a request/response pair

internal/log/log.go

@@ -15,6 +15,7 @@ import (
 	"sync"
 	"time"
 
+	"gopkg.in/DataDog/dd-trace-go.v1/internal/appsec/dyngo"
 	"gopkg.in/DataDog/dd-trace-go.v1/internal/version"
 )
 
@@ -102,6 +103,11 @@ func init() {
 	if v := os.Getenv("DD_LOGGING_RATE"); v != "" {
 		setLoggingRate(v)
 	}
+
+	// This is required because we really want to be able to log errors from dyngo
+	// but the log package depend on too much packages that we want to instrument.
+	// So we need to do this to avoid dependency cycles.
+	dyngo.LogError = Error
 }
 
 func setLoggingRate(v string) {