Repository: corazawaf/coraza (go)

Issue:
Remove hack to prevent GET variables from overriding POST in json processor
In JSON processor, we have this code to remove keys found in a JSON payload from query parameters

https://github.com/corazawaf/coraza/blob/v3/dev/bodyprocessors/json.go#L28

How query parameters and JSON payload get merged is completely app-specific, so removing variables from query from processing seems dangerous, e.g. especially without multiphase evaluation, a log4shell trigger in query parameter seems like it would be allowed if it's together with a JSON payload.

Should we just remove this logic and let the value be present in both locations?

Pull request description:
Don't remove GET keys in json processor
Fixes #696 

Patch:
diff --git a/auditlog/concurrent_writer.go b/auditlog/concurrent_writer.go
index 68fb5a0..1144efe 100644
--- a/auditlog/concurrent_writer.go
+++ b/auditlog/concurrent_writer.go
@@ -23,7 +23,7 @@ type concurrentWriter struct {
 	auditDir      string
 	auditDirMode  fs.FileMode
 	auditFileMode fs.FileMode
-	formatter     LogFormatter
+	formatter     Formatter
 	closer        func() error
 }
 
@@ -88,4 +88,4 @@ func (cl *concurrentWriter) Close() error {
 	return cl.closer()
 }
 
-var _ LogWriter = (*concurrentWriter)(nil)
+var _ Writer = (*concurrentWriter)(nil)
diff --git a/auditlog/formats.go b/auditlog/formats.go
index 0bef25b..b0c7858 100644
--- a/auditlog/formats.go
+++ b/auditlog/formats.go
@@ -97,5 +97,5 @@ func nativeFormatter(al *Log) ([]byte, error) {
 }
 
 var (
-	_ LogFormatter = nativeFormatter
+	_ Formatter = nativeFormatter
 )
diff --git a/auditlog/formats_json.go b/auditlog/formats_json.go
index 3d0bec6..ab2474d 100644
--- a/auditlog/formats_json.go
+++ b/auditlog/formats_json.go
@@ -75,6 +75,6 @@ func legacyJSONFormatter(al *Log) ([]byte, error) {
 }
 
 var (
-	_ LogFormatter = jsonFormatter
-	_ LogFormatter = legacyJSONFormatter
+	_ Formatter = jsonFormatter
+	_ Formatter = legacyJSONFormatter
 )
diff --git a/auditlog/init.go b/auditlog/init.go
index fdbcd23..a51aece 100644
--- a/auditlog/init.go
+++ b/auditlog/init.go
@@ -7,14 +7,14 @@
 package auditlog
 
 func init() {
-	RegisterLogWriter("concurrent", func() LogWriter {
+	RegisterWriter("concurrent", func() Writer {
 		return &concurrentWriter{}
 	})
-	RegisterLogWriter("serial", func() LogWriter {
+	RegisterWriter("serial", func() Writer {
 		return &serialWriter{}
 	})
 
-	RegisterLogFormatter("json", jsonFormatter)
-	RegisterLogFormatter("jsonlegacy", legacyJSONFormatter)
-	RegisterLogFormatter("native", nativeFormatter)
+	RegisterFormatter("json", jsonFormatter)
+	RegisterFormatter("jsonlegacy", legacyJSONFormatter)
+	RegisterFormatter("native", nativeFormatter)
 }
diff --git a/auditlog/init_tinygo.go b/auditlog/init_tinygo.go
index 1751f66..2c3a12e 100644
--- a/auditlog/init_tinygo.go
+++ b/auditlog/init_tinygo.go
@@ -7,14 +7,14 @@
 package auditlog
 
 func init() {
-	RegisterLogWriter("concurrent", func() LogWriter {
+	RegisterWriter("concurrent", func() Writer {
 		return noopWriter{}
 	})
-	RegisterLogWriter("serial", func() LogWriter {
+	RegisterWriter("serial", func() Writer {
 		return noopWriter{}
 	})
 
-	RegisterLogFormatter("json", noopFormater)
-	RegisterLogFormatter("jsonlegacy", noopFormater)
-	RegisterLogFormatter("native", nativeFormatter)
+	RegisterFormatter("json", noopFormater)
+	RegisterFormatter("jsonlegacy", noopFormater)
+	RegisterFormatter("native", nativeFormatter)
 }
diff --git a/auditlog/logger.go b/auditlog/logger.go
index af4466d..08ae507 100644
--- a/auditlog/logger.go
+++ b/auditlog/logger.go
@@ -9,7 +9,7 @@ import (
 	"strings"
 )
 
-// Config is the configuration of a LogWriter.
+// Config is the configuration of a Writer.
 type Config struct {
 	// File is the path to the file to write the raw audit log to.
 	File string
@@ -24,7 +24,7 @@ type Config struct {
 	DirMode fs.FileMode
 
 	// Formatter is the formatter to use when writing formatted audit logs.
-	Formatter LogFormatter
+	Formatter Formatter
 }
 
 // NewConfig returns a Config with default values.
@@ -38,18 +38,18 @@ func NewConfig() Config {
 	}
 }
 
-// LogFormatter is the interface for all log formatters
-// A LogFormatter receives an auditlog and generates "readable" audit log
-type LogFormatter = func(al *Log) ([]byte, error)
+// Formatter is the interface for all log formatters
+// A Formatter receives an auditlog and generates "readable" audit log
+type Formatter = func(al *Log) ([]byte, error)
 
-// LogWriter is the interface for all log writers
-// A LogWriter receives an auditlog and writes it to the output stream
+// Writer is the interface for all log writers
+// A Writer receives an auditlog and writes it to the output stream
 // An output stream may be a file, a socket, an http request, etc
-type LogWriter interface {
+type Writer interface {
 	// Init the writer requires previous preparations
 	Init(Config) error
 	// Write the audit log
-	// Using the LogFormatter is mandatory to generate a "readable" audit log
+	// Using the Formatter is mandatory to generate a "readable" audit log
 	// It is not sent as a bslice because some writers may require some Audit
 	// metadata.
 	Write(*Log) error
@@ -57,20 +57,20 @@ type LogWriter interface {
 	Close() error
 }
 
-type loggerWrapper = func() LogWriter
+type loggerWrapper = func() Writer
 
 var writers = map[string]loggerWrapper{}
-var formatters = map[string]LogFormatter{}
+var formatters = map[string]Formatter{}
 
-// RegisterLogWriter registers a new logger
+// RegisterWriter registers a new logger
 // it can be used for plugins
-func RegisterLogWriter(name string, writer func() LogWriter) {
+func RegisterWriter(name string, writer func() Writer) {
 	writers[name] = writer
 }
 
-// GetLogWriter returns a logger by name
+// GetWriter returns a logger by name
 // It returns an error if it doesn't exist
-func GetLogWriter(name string) (LogWriter, error) {
+func GetWriter(name string) (Writer, error) {
 	logger := writers[strings.ToLower(name)]
 	if logger == nil {
 		return nil, fmt.Errorf("invalid logger %q", name)
@@ -78,15 +78,15 @@ func GetLogWriter(name string) (LogWriter, error) {
 	return logger(), nil
 }
 
-// RegisterLogFormatter registers a new logger format
+// RegisterFormatter registers a new logger format
 // it can be used for plugins
-func RegisterLogFormatter(name string, f func(al *Log) ([]byte, error)) {
+func RegisterFormatter(name string, f func(al *Log) ([]byte, error)) {
 	formatters[name] = f
 }
 
-// GetLogFormatter returns a formatter by name
+// GetFormatter returns a formatter by name
 // It returns an error if it doesn't exist
-func GetLogFormatter(name string) (LogFormatter, error) {
+func GetFormatter(name string) (Formatter, error) {
 	formatter := formatters[strings.ToLower(name)]
 	if formatter == nil {
 		return nil, fmt.Errorf("invalid formatter %q", name)
diff --git a/auditlog/noop_writer.go b/auditlog/noop_writer.go
index ec0975d..6377118 100644
--- a/auditlog/noop_writer.go
+++ b/auditlog/noop_writer.go
@@ -14,4 +14,4 @@ func (noopWriter) Init(Config) error { return nil }
 func (noopWriter) Write(*Log) error  { return nil }
 func (noopWriter) Close() error      { return nil }
 
-var _ LogWriter = (*noopWriter)(nil)
+var _ Writer = (*noopWriter)(nil)
diff --git a/auditlog/serial_writer.go b/auditlog/serial_writer.go
index 2a991ba..979ba56 100644
--- a/auditlog/serial_writer.go
+++ b/auditlog/serial_writer.go
@@ -16,7 +16,7 @@ import (
 type serialWriter struct {
 	closer    func() error
 	log       log.Logger
-	formatter LogFormatter
+	formatter Formatter
 }
 
 func (sl *serialWriter) Init(c Config) error {
@@ -57,4 +57,4 @@ func (sl *serialWriter) Close() error {
 	return sl.closer()
 }
 
-var _ LogWriter = (*serialWriter)(nil)
+var _ Writer = (*serialWriter)(nil)
diff --git a/bodyprocessors/json.go b/bodyprocessors/json.go
index b084e4c..f4326f4 100644
--- a/bodyprocessors/json.go
+++ b/bodyprocessors/json.go
@@ -23,10 +23,7 @@ func (js *jsonBodyProcessor) ProcessRequest(reader io.Reader, v rules.Transactio
 	if err != nil {
 		return err
 	}
-	argsGetCol := v.ArgsGet()
 	for key, value := range data {
-		// TODO: This hack prevent GET variables from overriding POST variables
-		argsGetCol.Remove(key)
 		col.SetIndex(key, 0, value)
 	}
 	return nil
diff --git a/config.go b/config.go
index 9a707c0..3cb98f2 100644
--- a/config.go
+++ b/config.go
@@ -81,8 +81,8 @@ type AuditLogConfig interface {
 	// WithParts configures the parts of the request/response to be logged.
 	WithParts(parts types.AuditLogParts) AuditLogConfig
 
-	// WithLogger configures the auditlog.LogWriter to write logs to.
-	WithLogger(logger auditlog.LogWriter) AuditLogConfig
+	// WithLogger configures the auditlog.Writer to write logs to.
+	WithLogger(logger auditlog.Writer) AuditLogConfig
 }
 
 // NewAuditLogConfig returns a new AuditLogConfig with the default settings.
@@ -208,7 +208,7 @@ func (c *wafConfig) WithResponseBodyMimeTypes(mimeTypes []string) WAFConfig {
 type auditLogConfig struct {
 	relevantOnly bool
 	parts        types.AuditLogParts
-	logger       auditlog.LogWriter
+	logger       auditlog.Writer
 }
 
 func (c *auditLogConfig) LogRelevantOnly() AuditLogConfig {
@@ -223,7 +223,7 @@ func (c *auditLogConfig) WithParts(parts types.AuditLogParts) AuditLogConfig {
 	return ret
 }
 
-func (c *auditLogConfig) WithLogger(logger auditlog.LogWriter) AuditLogConfig {
+func (c *auditLogConfig) WithLogger(logger auditlog.Writer) AuditLogConfig {
 	ret := c.clone()
 	ret.logger = logger
 	return ret
diff --git a/internal/corazawaf/waf.go b/internal/corazawaf/waf.go
index b513c82..9c3fc5f 100644
--- a/internal/corazawaf/waf.go
+++ b/internal/corazawaf/waf.go
@@ -121,7 +121,7 @@ type WAF struct {
 	ErrorLogCb func(rule types.MatchedRule)
 
 	// AuditLogWriter is used to write audit logs
-	AuditLogWriter auditlog.LogWriter
+	AuditLogWriter auditlog.Writer
 }
 
 // NewTransaction Creates a new initialized transaction for this WAF instance
@@ -253,7 +253,7 @@ const _1gb = 1073741824
 func NewWAF() *WAF {
 	logger := debuglog.Noop()
 
-	logWriter, err := auditlog.GetLogWriter("serial")
+	logWriter, err := auditlog.GetWriter("serial")
 	if err != nil {
 		logger.Error().
 			Err(err).
diff --git a/internal/seclang/directives.go b/internal/seclang/directives.go
index 024666b..2ba18d9 100644
--- a/internal/seclang/directives.go
+++ b/internal/seclang/directives.go
@@ -593,7 +593,7 @@ func directiveSecAuditLogType(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	writer, err := auditlog.GetLogWriter(options.Opts)
+	writer, err := auditlog.GetWriter(options.Opts)
 	if err != nil {
 		return err
 	}
@@ -613,7 +613,7 @@ func directiveSecAuditLogFormat(options *DirectiveOptions) error {
 		return errEmptyOptions
 	}
 
-	formatter, err := auditlog.GetLogFormatter(options.Opts)
+	formatter, err := auditlog.GetFormatter(options.Opts)
 	if err != nil {
 		return err
 	}


Test patch:
diff --git a/auditlog/logger_test.go b/auditlog/logger_test.go
index 445b771..ce262ca 100644
--- a/auditlog/logger_test.go
+++ b/auditlog/logger_test.go
@@ -8,7 +8,7 @@ import "testing"
 func TestDefaultWriters(t *testing.T) {
 	ws := []string{"serial", "concurrent"}
 	for _, writer := range ws {
-		if w, err := GetLogWriter(writer); err != nil {
+		if w, err := GetWriter(writer); err != nil {
 			t.Error(err)
 		} else if w == nil {
 			t.Errorf("invalid %s writer", writer)
diff --git a/config_test.go b/config_test.go
index 6afe4a6..1d329e9 100644
--- a/config_test.go
+++ b/config_test.go
@@ -114,7 +114,7 @@ func TestConfigSetters(t *testing.T) {
 }
 
 func TestConfigLogger(t *testing.T) {
-	logger, err := auditlog.GetLogWriter("concurrent")
+	logger, err := auditlog.GetWriter("concurrent")
 	if err != nil {
 		t.Fatal(err)
 	}
diff --git a/testing/engine/json.go b/testing/engine/json.go
index 0848261..a1c554c 100644
--- a/testing/engine/json.go
+++ b/testing/engine/json.go
@@ -34,13 +34,13 @@ var _ = profile.RegisterProfile(profile.Profile{
 								101,
 								1100,
 								1101,
+								1102,
 								1010,
 								1000,
 								1011,
 							},
 							NonTriggeredRules: []int{
 								1111,
-								1102,
 								103,
 							},
 							Headers: map[string]string{
@@ -67,7 +67,7 @@ SecRule REQUEST_BODY "456" "id:103, phase:2, log"
 SecRule ARGS:json.test "@eq 123" "id:1100, phase:2, log, block"
 SecRule ARGS:json.test3.2 "@eq 55" "id:1101, phase:2, log, block"
 
-# We test for some vulnerability
+# Both GET and POST can be matched for the same key
 SecRule ARGS:json.test "@eq 456" "id:1102, phase:2, log, block"
 
 SecRule ARGS:json.test3 "@eq 3" "id: 1010, phase:2, log, block"