Skip to content

False positive in go/log-injection聽#9016

Closed
Closed
False positive in go/log-injection#9016
Assignees
owen-mc
Labels
Gofalse-positive

Description

@antoineco
antoineco
opened on May 3, 2022

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

  1. antoineco
    added
    false-positive
    on May 3, 2022
  2. aeisenberg
    added
    Go
    on May 3, 2022

  3. aeisenberg commented on May 3, 2022

    @aeisenberg
    aeisenberg
    on May 3, 2022
    Contributor

    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.

  4. antoineco commented on May 3, 2022

    @antoineco
    antoineco
    on May 3, 2022
    Author

    @aeisenberg thanks for looking into this!

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

  5. aeisenberg commented on May 3, 2022

    @aeisenberg
    aeisenberg
    on May 3, 2022
    Contributor

    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.

  6. owen-mc commented on May 4, 2022

    @owen-mc
    owen-mc
    on May 4, 2022
    Contributor

    @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.

  7. antoineco commented on May 4, 2022

    @antoineco
    antoineco
    on May 4, 2022
    Author

    @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.

  8. owen-mc commented on May 4, 2022

    @owen-mc
    owen-mc
    on May 4, 2022
    Contributor

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

  9. aeisenberg
    assigned
    owen-mc
    on May 4, 2022

  10. owen-mc commented on Jan 17, 2023

    @owen-mc
    owen-mc
    on Jan 17, 2023
    Contributor

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

  11. owen-mc commented on Jan 19, 2023

    @owen-mc
    owen-mc
    on Jan 19, 2023
    Contributor

    #11910 has now been merged.

  12. owen-mc
    closed this as completedon Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

Gofalse-positive

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions