github / codeql

CodeQL: the libraries and queries that power security researchers around the world, as well as code scanning in GitHub Advanced Security
https://codeql.github.com
MIT License
7.52k stars 1.5k forks source link

False positive (?) for sensitive data logging #7992

Closed nfx closed 2 years ago

nfx commented 2 years ago

Description of the issue

First of all, thank you for maintaining these awesome CodeQL checks! helps to automate the reviews a lot and focus on what really matters.

I think there might be a case for a false positive in sensitive data logging rules for the following code

443 | func (c *DatabricksClient) createDebugHeaders(header http.Header, host string) string {
444 |   headers := ""
445 |   if c.DebugHeaders { // <----- by default is false, true only when configured
446 |     if host != "" {
447 |       headers += fmt.Sprintf("\n * Host: %s", escapeNewLines(host))
448 |     }
449 |     for k, v := range header {
450 |       trunc := onlyNBytes(strings.Join(v, ""), c.DebugTruncateBytes)
451 |        headers += fmt.Sprintf("\n * %s: %s", k, escapeNewLines(trunc))
452 |     }
453 |     if len(headers) > 0 {
454 |       headers += "\n"
455 |     }
456 |   }
457 |   return headers
458 | }

...

484 | headers := c.createDebugHeaders(request.Header, c.Host)
485 | log.Printf("[DEBUG] %s %s %s%v", method, escapeNewLines(request.URL.Path),
486 |     headers, c.redactedDump(requestBody)) // lgtm [go/log-injection]
         // ^^ Sensitive data returned by HTTP request headers is logged here

Rationale is: if user explicitly turns on debug logging and wants sensitive data to be logged, then c.DebugHeaders will be true.

GitHub CodeQL alerts suggest not to log sensitive headers at all, but it's possible to dismiss them. I wonder how i can adjust this code or what comment do i need to change/apply to silence this rule.

lgtm.io url: https://lgtm.com/projects/g/databrickslabs/terraform-provider-databricks/snapshot/0cac5eead365aaf9b40e3512918dae6f68413a61/files/common/http.go#x73c089a7bbea9963:1

edoardopirovano commented 2 years ago

Greetings, thanks for getting in touch with us about this!

Unfortunately, there is no way to silence a rule for just one specific instance, although in the Security tab of a GitHub repository you can mark an alert as False positive and you should not be alerted about it again in general (although occasionally changes to the file may cause us to think it is a new alert and bring it up again until it is next dismissed).

If you want to disable this rule entirely, you could tell CodeQL to use a config file:

- uses: github/codeql-action/init@v1
  with:
    config-file: ./.github/codeql/codeql-config.yml

and in that config file specify a query suite:

disable-default-queries: true
queries:
  - uses: ./github/codeql/suite.qls

with the query suite enabling all our default rules except that one:

- from: codeql/go-queries
  apply: codeql-suites/go-security-and-quality.qls
- exclude:
    query filename: CleartextLogging.ql
nfx commented 2 years ago

@edoardopirovano no, i don't want to disable this rule entirely, as it's a very useful one :) disabling rule entirely is bad for code security posture ;)

can we convert this issue into a feature request to add a code comment which disables this rule for a specific line?

edoardopirovano commented 2 years ago

Indeed, we certainly don't recommend disabling rules entirely though it can sometimes be useful if for some reason a specific rule has many false positives on your project (which isn't the case here).

Note that on LGTM you can already suppress alerts with a comment. I see you have already done that for one rule with the lgtm[go/log-injection] comment. Adding a further lgtm[go/clear-text-logging] to that comment would also suppress the other alert that you are still seeing on LGTM. See https://lgtm.com/help/lgtm/alert-suppression for more details.

As for suppressing alerts on GitHub, indeed this is currently not possible with a code comment (although as you note the alerts can be dismissed via the UI). cc. @AlonaHlobina who is our product manager: this may be something we'd want to consider.

nfx commented 2 years ago

thank you, @edoardopirovano - added second exclude, let's see how lgmt.io would react on merge.