False positive - Log entries created from user input

[davidhadas]

Description of the issue

in-place sanitization code is not recognized and constantly reports false positives with "Log entries created from user input"

For example, when using the following sanitization, logging sanitized user input produces a false positive.

// contain at most 60 characters
// contain only lowercase alphanumeric characters or '-'
// start with an alphanumeric character
// end with an alphanumeric character
func Sanitize(in string) string {
	if len(in) > 60 {
		return ""
	}
	var alphanumeric bool
	for i, r := range in {
		if (97 <= r && r <= 122) || (48 <= r && r <= 57) {
			alphanumeric = true
			continue
		}
		if r == 45 {
			alphanumeric = false
			// first letter
			if i == 0 {
				return ""
			}
			continue
		}
		return ""
	}
	// last letter
	if !alphanumeric {
		return ""
	}
	return in
}

is there a workaround or a way to signal to CodeQL that this is in-place sanitization and no alert is needed?
That is, without moving to a less efficient non-in-place sanitization...

[tausbn]

Thank you for your question! 🙂

There is a mechanism for extending set of things that are recognised as sanitisers by the CodeQL models, but it is a bit finicky to set up.

First of all, you would need to write a class extending the LogInjection.Sanitizer abstract class (An example of this can be seen a bit further down in the same file.)

Next, you would need to create a custom query pack that includes a version of the query that has makes that class available to the query (usually through the Customizations.qll file.)

Finally, you would need to tell the analysis to use the custom query pack instead of the standard queries, by adding a codeql-config.yml file.

(As you may imagine, this is not the easiest to set up. Needless to say, we're working on making this a bit easier to do.)

If you don't expect the current query to produce any useful results, you can also just disable it. This can be done directly in the configuration for the CodeQL Action. In this case, you would want to exclude the query with the ID go/log-injection.

I should also mention that I forwarded this issue to the team responsible for the Go analysis. It may be possible to identify sanitisers like the one you give automatically, and use this to eliminate the false positives.

[owen-mc]

Hi, I'm on the CodeQL Go team. Thanks for reporting this. The only thing we currently have in this direction is that if you call strings.ReplaceAll (or strings.Replace with the fourth argument < 0, which makes it replace all instances) on a string and the string to be replaced contains \r or \n then we consider the input string to be sanitized. We will consider expanding this to also identify loops through all the characters with a manual check on each character, as contained in your example.