CODEWITHSUNDEEP
go
saidaspen
saidaspen

Reputation: 690

How to handle gosec linter warning: Potential file inclusion via variable

How do I solve the following warning from gosec linter:

::warning: Potential file inclusion via variable,MEDIUM,HIGH (gosec)

The linter is warning me on the first line of this function:

func File2lines(filePath string) ([]string, error) {
    f, err := os.Open(filePath) //Warning here
    if err != nil {
        return nil, err
    }
    defer f.Close()
    return linesFromReader(f)
}

I have tried reading up on local file inclusion, but cannot see how that would be applicable here.

Upvotes: 37

Views: 22246

Answers (5)

VonC
VonC

Reputation: 1324367

Q4 2024: note that the proposal "#67002 os: safer file open functions has been accepted", possibly for Go 1.25 (Q3 2025).

The os package will come with a new Root type, allowing you to safely perform file operations within a specified root directory: any file access cannot escape this directory, even when dealing with symbolic links or directory traversal attempts.

Your File2lines function could therefore use os.Root to mitigate the gosec linter warning about potential file inclusion via variable:

import (
    "os"
)

func File2lines(rootDir, filePath string) ([]string, error) {
    // Open the root directory as a Root
    root, err := os.OpenRoot(rootDir)
    if err != nil {
        return nil, fmt.Errorf("failed to open root directory: %w", err)
    }
    defer root.Close()

    // Open the file within the root directory
    f, err := root.Open(filePath)
    if err != nil {
        return nil, fmt.Errorf("failed to open file: %w", err)
    }
    defer f.Close()

    return linesFromReader(f)
}

The os.OpenRoot(rootDir) creates a Root instance representing the directory at rootDir. All file operations performed through this Root are confined within the rootDir.
And the root.Open(filePath) opens a file relative to the rootDir: If filePath attempts to traverse outside the rootDir (e.g., via ../ sequences or symlinks), the operation fails with an error.

Encapsulating file operations within os.Root should mitigate the risk flagged by gosec: you no longer need to manually sanitize or validate the filePath variable; os.Root manages this for you.

For instance, assuming you are using a getUserInput() function getting input from an untrusted source:

func main() {
    baseDir := "/safe/base/directory"
    userFilePath := getUserInput()

    lines, err := File2lines(baseDir, userFilePath)
    if err != nil {
        log.Fatalf("Error reading file: %v", err)
    }

    // ... process lines
}

As noted by Damien Neil

My current intent is to submit a subset of this proposal for 1.24, with the remainder following in 1.25.

1.24 will contain:

type Root struct { ... }

func OpenRoot(dir string) (*Root, error)
func (*Root) OpenFile
func (*Root) Create
func (*Root) Open
func (*Root) OpenRoot
func (*Root) Close
func (*Root) Mkdir
func (*Root) Remove
func (*Root) Lstat
func (*Root) Stat

func OpenInRoot(dir, name string) (*File, error)

That will leave the following functions for 1.25:

func (*Root) MkdirAll
func (*Root) RemoveAll
func (*Root) Chmod
func (*Root) Chown
func (*Root) Chtimes
func (*Root) Lchown
func (*Root) Readlink
func (*Root) Rename
func (*Root) Symlink
func (*Root) Link
func (*Root) Truncate

The implementation in 1.24 will support all our ports (with the caveats mentioned above for GOOS=js and GOOS=plan9), but not does not take advantage of platform-specific features such as Linux's RESOLVE_BENEATH and Darwin's O_NOFOLLOW_ANY which allow for a more efficient implementation. That will also be a task for 1.25.

Upvotes: 0

Kenny Grant
Kenny Grant

Reputation: 9623

Where does the path come from? If you’re not absolutely sure it can never have user input, best to clean it before use and use a known prefix, e.g.:

filePath = filepath.Join(basePath,filepath.Clean(filePath))
if !strings.HasPrefix(filePath, basePath) {
  return nil, fmt.Errorf("invalid path")
}
f, err := os.Open(filePath)

That should fix the complaint. This is a reasonable precaution anyway even if you think it is safe now, in case later someone uses your function with user data.

EDIT - as rkfcccccc points out below, filepath.Clean is not sufficient in the case of directory traversal in the path (../), so answer above adjusted to check .

Upvotes: 36

rkfcccccc
rkfcccccc

Reputation: 41

The correct solution to this problem i found on this website https://securego.io/docs/rules/g304.html

Someone suggests to do so:

filePath = filepath.Join(basePath,filepath.Clean(filePath))
f, err := os.Open(filePath)

But it's not safe, because basePath could be "/safe/path" and filePath be "../../not-safe" and the result of filepath.Join(basePath,filepath.Clean(filePath)) would be "/not-safe"

The right way

This code panics if /safe/path was removed by an attacker

repoFile := "/safe/path/../../private/path"
repoFile = filepath.Clean(repoFile)
if !strings.HasPrefix(repoFile, "/safe/path/") {
    panic(fmt.Errorf("Unsafe input"))
}
byContext, err := ioutil.ReadFile(repoFile)
if err != nil {
    panic(err)
}
fmt.Printf("%s", string(byContext))}

So it's also important to validate after doing filepath.Clean that your path starts with "/safe/path/" by using strings.HasPrefix

Upvotes: 2

Alex
Alex

Reputation: 705

If you specify the file path with a variable, there is a risk that an unintended file path will be specified. Therefore, you should use filepath.Clean() to clean up possible bad paths.

an easy solution:

f,err := os.Open(filepath.Clean(fname))

Upvotes: 14

hobbs
hobbs

Reputation: 239890

No one said the linter was smart. Looking at the function in isolation, it's impossible to say if there's a security issue. If the function is called with a filePath that's user-supplied and insufficiently validated, and it runs in a context where it can read files that the user would not be able to otherwise (e.g. in a program with elevated privileges, or on a remote server), then there is a probably issue. Otherwise, the only thing to do about the warning is to suppress or ignore it.

Upvotes: 8

Related Questions