G304: error persists after running filePath.Clean on a separate line from os.OpenFile

[ericcornelissen]

Summary

Due to the recent addition of #488 I got a new error in my code. I wanted to fix this in accordance with the docs for G304 but to my surprise my solution (below) didn't work:

cleanFilePath := filePath.Clean(filePath)
osFile, err := os.OpenFile(cleanFilePath, flag, mode)

Only after rewriting this as:

osFile, err := os.OpenFile(filePath.Clean(filePath), flag, mode)

did gosec recognize the solution.

Steps to reproduce the behavior

Create a sample project with the file below and run gosec

package main

import (
    "os"
    "path/filepath"
)

func main() {
    repoFile := "path_of_file"
    cleanRepoFile := filepath.Clean(repoFile)
    byContext, err := os.OpenFile(cleanRepoFile, os.O_RDONLY, 0600)
    if err != nil {
        panic(err)
    }
}

gosec version

The gosec GitHub Action as linked above.

Go version (output of 'go version')

Same as in the gosec GitHub Action.

Operating system / Environment

The gosec GitHub Action container.

Expected behavior

No warning if the variable is actually the result of filepath.Clean.

Actual behavior

A warning even if the variable is actually the result of filepath.Clean.

[benma]

I would be more interested why filepath.Clean would fix any security issue? It just returns an equivalent path but in a canonical form. If an attacker controlled the filename, then Clean() does not help, or am I missing something?

[ericcornelissen]

That is a fair question which I don't know the answer to (though I think you're right). This has also been discussed in #439 (in more depth). I would argue that comment deserves a separate issue, but I haven't opened such an issue as the rule is actually not relevant to my current context, but I discovered this bug in the process of realizing that 🙃 (as an aside, to contribute to that discussion, the note about filepath.Clean was added to the website/docs in securego/securego.github.io#20)

[ccojocar]

There is a bug, see the attached pull request. The variable assignment is not resolved in order to check if the filepath.Clean is called on the provided input.