Repository navigation
False positive in go/log-injection聽#9016
Description
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)
}
Activity
aeisenberg commentedon May 3, 2022
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.NewReplaceras a sanitizer. It is looking for a call toReplaceorReplaceAll.I'll contact the team that maintains the go libraries to see how they can improve the sanitizer recognition.
antoineco commentedon May 3, 2022
@aeisenberg thanks for looking into this!
If there is any way I can help implement that change let me know.
aeisenberg commentedon May 3, 2022
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 commentedon May 4, 2022
@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 commentedon May 4, 2022
@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 commentedon May 4, 2022
@antoineco If you got that far then you were doing well!
owen-mc commentedon Jan 17, 2023
Sorry, that PR got stuck in limbo. I have now opened #11910 to do the same thing.
owen-mc commentedon Jan 19, 2023
#11910 has now been merged.