Treat zap custom encoders as sanitizers for log-injection checks

[danielriddell21]

Summary

Add an experimental CodeQL helper and query to treat custom zap encoders (types implementing go.uber.org/zap/zapcore.Encoder) as sanitizers for the purposes of log-injection detection. This reduces false positives where applications use a custom encoder to escape or sanitize log field values.

Notes for reviewers

• This change is non-invasive: it adds an experimental suppression query rather than modifying built-in log-injection rules directly.

Risks

• If a type implements zapcore.Encoder but does not actually sanitize input, this change could suppress a genuine finding. Use the whitelist to exclude such types.

[owen-mc]

This PR doesn't make much sense. I don't think the tests would pass.

There is already a query called go/log-injection. It is located at go/ql/src/Security/CWE-117/LogInjection.ql. All sanitizers for it are in go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll. Tests for it are at go/ql/test/query-tests/Security/CWE-117/LogInjection.go. Note that we already include variations on strings.ReplaceAll(s, "\n", "X") as a sanitizer.

[danielriddell21]

This PR doesn't make much sense. I don't think the tests would pass.

There is already a query called go/log-injection. It is located at go/ql/src/Security/CWE-117/LogInjection.ql. All sanitizers for it are in go/ql/lib/semmle/go/security/LogInjectionCustomizations.qll. Tests for it are at go/ql/test/query-tests/Security/CWE-117/LogInjection.go. Note that we already include variations on strings.ReplaceAll(s, "\n", "X") as a sanitizer.

Hi @owen-mc thanks for looking. I am pretty new to codeql.

So I will try and move all the stuff into the correct place when its ready. Am I okay to tag you when It is ready for a review?

[danielriddell21]

My aim is to allow using a zap encoder with a sanitise within as a valid way to suppress a CWE 117

[owen-mc]

Things are in the right place now, but the tests still don't make any sense. May I ask, are you using an LLM coding assistant? Yes, please tag me when you have got the tests passing locally on your machine, or you are stuck and need help with the CodeQL.

[owen-mc]

This type doesn't exist, at least according to https://pkg.go.dev/go.uber.org/zap/zapcore.

[danielriddell21]

Things are in the right place now, but the tests still don't make any sense. May I ask, are you using an LLM coding assistant? Yes, please tag me when you have got the tests passing locally on your machine, or you are stuck and need help with the CodeQL.

Not now, I used it originally so I could understand better what I would have to do, but actually it created a complete mess so I am starting again following your contribution guide

[danielriddell21]

Hi @owen-mc, hope you had good holidays.

I have attempted over the past month to re-approach this but I cannot seem to get the following to be true

package main

import (
"os"
"regexp"
"strings"

"go.uber.org/zap"
"go.uber.org/zap/buffer"
"go.uber.org/zap/zapcore"
)

// SafeEncoder wraps a zapcore.Encoder and sanitizes all string values
// before they are written to the log output. This prevents log injection
// by removing control characters that could be used to forge log entries.
type SafeEncoder struct {
zapcore.Encoder
}

var controlChars = regexp.MustCompile(`[\r\n\t]+`)

func sanitize(s string) string {
s = controlChars.ReplaceAllString(s, " ")
return strings.TrimSpace(s)
}

func (s *SafeEncoder) EncodeEntry(ent zapcore.Entry, fields []zapcore.Field) (*buffer.Buffer, error) {
ent.Message = sanitize(ent.Message)

for i := range fields {
if fields[i].Type == zapcore.StringType {
fields[i].String = sanitize(fields[i].String)
}
}

return s.Encoder.EncodeEntry(ent, fields)
}

func (s *SafeEncoder) Clone() zapcore.Encoder {
return &SafeEncoder{Encoder: s.Encoder.Clone()}
}

func UnsafeLogger() *zap.Logger {
encoder := zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig())
core := zapcore.NewCore(encoder, zapcore.AddSync(os.Stdout), zapcore.InfoLevel)
return zap.New(core)
}

func SafeLogger() *zap.Logger {
baseEncoder := zapcore.NewJSONEncoder(zap.NewProductionEncoderConfig())
safeEncoder := &SafeEncoder{baseEncoder}
core := zapcore.NewCore(safeEncoder, zapcore.AddSync(os.Stdout), zapcore.InfoLevel)
return zap.New(core)
}

func getUserInput() string {
return "user input\nwith\nnewlines"
}

func TestVulnerable() {
logger := UnsafeLogger()
userInput := getUserInput()
// BAD: Logging user input without sanitization
logger.Info(userInput) // $hasValueFlow
}

func TestProtectedByEncoder() {
logger := SafeLogger()
userInput := getUserInput()
// GOOD: Logger uses SafeEncoder which automatically sanitizes
logger.Info(userInput)
}

func TestVulnerableWithFields() {
logger := UnsafeLogger()
userInput := getUserInput()
// BAD: User input in message
logger.Info(userInput, zap.String("key", "value")) // $hasValueFlow
// BAD: User input in field value
logger.Info("message", zap.String("user", userInput)) // $hasValueFlow
}

func TestProtectedWithFields() {
logger := SafeLogger()
userInput := getUserInput()
// GOOD: SafeEncoder sanitizes both message and field values
logger.Info(userInput, zap.String("key", "value"))
logger.Info("message", zap.String("user", userInput))
} 

But i cannot seem get codeql to recognise the encoder. Reading into this it seems to be related to how codeql reads code during the build - I think it doesnt recognise the encoder because this (to my knowledge) gets applied during runtime. But I would appreciate your input

I havent commited anything experimental from my local machine as it hasnt worked

[owen-mc]

Thanks for the detailed test case, which really helps me to understand what you are trying to do. In terms of the broad approach, there are two options, which are fairly equivalent: either restricting the sinks or adding a sanitizer. Since the sink is specified using data extensions (in a *.model.yml file), specifying a sanitizer will be easier. So let's think about what we want this sanitizer to do. It will sanitize arguments to methods called on a safe zap Logger. That's the only place in the path from source to sink where we can have an effect.

Now we get to the hard bit: specifying which zap Loggers are safe. From your test it seems we will need to have a separate data flow config so we can track the flow of safe loggers. (From a performance point of view this is fine as long as it doesn't have too many sources and sinks.) Let's call it SafeZapLoggerFlowConfig. Its sinks will be qualifiers of method calls which have type zap.Logger. What are the sources? Lets see, we want to trace flow back through zap.New(zapcore.Core, ...) (so that will be an additional flow step), it seems the source is a safe Encode as the first argument of zapcore.NewCore. And how will we know if that's a safe Encoder? I don't think we can realistically check that. We often assume that something is safe, e.g. if a user does a regex match on a string then we can't test for all regexes which would check that it's safe, but the mere fact that the user is trying to check the string probably indicates that they are doing it correctly. So we could make the source any first argument to zapcore.NewCore where the EncodeEntry method is defined in the source code. I think you can check for this by checking if getLocation() exists.

Does that make sense?