False positive in go/log-injection

[antoineco]

Description of the false positive

📄 Code snippet

CodeQL reports (via GitHub integration) that the following log write receives unsanitized user input:

switch eventType := sanitizeUserInput(event.Type); eventType {
// ...
default:
	h.logger.Warn("Content not supported: ", strconv.Quote(eventType))  // <-- false positive 'go/log-injection'
}

This is untrue.
The user input is explicitly sanitized at the beginning of the switch case (in fact, we fixed it earlier this year thanks to CodeQL! 🙌 ):

var newlineToSpace = strings.NewReplacer("\n", " ", "\r", " ")

// sanitizeUserInput removes unwanted characters from the given string.
// It also guarantees the safe logging of data that potentially originates from
// user input (CWE-117, https://cwe.mitre.org/data/definitions/117.html).
func sanitizeUserInput(s string) string {
	return newlineToSpace.Replace(s)
}

[aeisenberg]

Thank you for raising this issue. This is indeed a false positive. It looks like the current version of the query does not recognize an instance of strings.NewReplacer as a sanitizer. It is looking for a call to Replace or ReplaceAll.

I'll contact the team that maintains the go libraries to see how they can improve the sanitizer recognition.

[antoineco]

@aeisenberg thanks for looking into this!

If there is any way I can help implement that change let me know.

[aeisenberg]

Yes, all of our queries are open source and we welcome external contributions.

The query that is giving you the false positive is here: https://github.com/github/codeql-go/blob/main/ql/lib/semmle/go/security/LogInjectionCustomizations.qll#L50-L59

If you get stuck or want some pointers on how to get started, feel free to join the Security Lab Slack channel. Information on an invite is in the link.

[owen-mc]

@antoineco Thanks for reporting this! I agree with your assessment. This was quite a small fix, and sanitizers aren't the easiest place to start learning CodeQL, so I whipped up a quick PR. The version of CodeQL used in actions is updated every two weeks, so this should be fixed within two weeks of this PR getting merged.

[antoineco]

@owen-mc thanks!
Indeed, I gave it a shot yesterday but tracing back to the the initialization of the Replacer turned out to be difficult due to my lack of experience with the language.

[owen-mc]

@antoineco If you got that far then you were doing well!

[owen-mc]

Sorry, that PR got stuck in limbo. I have now opened #11910 to do the same thing.

[owen-mc]

#11910 has now been merged.