False positive - A secret detected in a go context causes codeql to think all context values are secret

[Jamstah]

Description of the false positive

We use a convenience function to create a logger that includes additional variables taken from a context.Context using the Value call to get the value.

The context itself contains secrets that may be needed. When the logger is created we specify the keys that should be taken from the context to add to the logger.

CodeQL is identifying a Clear-text logging of sensitive information which seems to think that taking any value out of the context, regardless of what is requested, is logging sensitive information.

Code samples or links to source code

This one is detected in the creation of the logger:

https://github.com/distribution/distribution/blob/56a020f7f1fbec903d785b552c97330c58fddae6/internal/dcontext/logger.go#L138
https://github.com/distribution/distribution/security/code-scanning/36

This one is detected when logging an error using context.Errors, which has been populated with a value from the context that is not secret:
https://github.com/distribution/distribution/blob/56a020f7f1fbec903d785b552c97330c58fddae6/registry/handlers/app.go#L831
https://github.com/distribution/distribution/security/code-scanning/23

There are more in the repo that match the same pattern.

[mbg]

Hi @Jamstah 👋

Thanks for reporting this and including the links to the relevant parts of the code and corresponding alerts! That is all very helpful.

This is a tricky one for us to deal with correctly. It is quite a complex problem to identify which elements of your context are accessed (particularly with the loop that takes some items out, depending on which keys are in the array). So we detect that sensitive data flows into the context, but since it's hard to reason about where it flows from there, we taint the entire structure and consequently flag any access.

Fixing false positives is not a current product priority, but we will track this issue internally. I cannot say when it may get looked into though and, even then, it may be a tricky one for us to fix.

In the short term, you can dismiss these alerts if you are happy that they are false positives. If this continues to be a nuisance for you, some options are to either refactor the code to avoid having the sensitive data flow into the context or using a custom CodeQL configuration to exclude the go/clear-text-logging query completely. I appreciate neither of these are ideal though.

[Jamstah]

Thanks for the analysis and options. I haven't worked with CodeQL much, and my knowledge of go contexts is sketchy, so even some confirmation that what I thought was happening is right is really valuable. I was half expecting you to tell me I read it wrong and its horribly insecure ;)

I've got a PR together which removes sensitive data from the context, so I'll propose that to my co-contributors. I expect its a sensible direction for us anyway. If that doesn't go down well, we'll mark them as false positives in the tool and move on.

I don't know if you want to leave this open or close it, but I won't be offended either way.

[mbg]

No problem!

I don't know if you want to leave this open or close it, but I won't be offended either way.

We do want to leave this open so that we can continue to track it until it is resolved at some point of the future.