Skip to content

Go: Update query help for go/path-injection to include example fixes. #16020

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Mar 25, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 43 additions & 10 deletions go/ql/src/Security/CWE-022/TaintedPath.qhelp
Original file line number Diff line number Diff line change
Expand Up @@ -9,23 +9,35 @@ Accessing files using paths constructed from user-controlled data can allow an a
unexpected resources. This can result in sensitive information being revealed or deleted, or an
attacker being able to influence behavior by modifying unexpected files.
</p>
<p>
Paths that are naively constructed from data controlled by a user may be absolute paths,
or may contain unexpected special characters such as "..". Such a path could point anywhere
on the file system.
</p>
</overview>

<recommendation>
<p>
Validate user input before using it to construct a file path, either using an off-the-shelf library
or by performing custom validation.
Validate user input before using it to construct a file path.
</p>
<p>
Common validation methods include checking that the normalized path is relative and
does not contain any ".." components, or checking that the path is contained within a safe folder. The method you should use depends on how the path is used in the application, and whether the path should be a single path component.
</p>
<p>
If the path should be a single path component (such as a file name), you can check for the
existence of any path separators ("/" or "\"), or ".." sequences in the input, and reject
the input if any are found.
</p>
<p>
Note that removing "../" sequences is <i>not</i> sufficient, since the input could still
contain a path separator followed by "..". For example, the input ".../...//" would still
result in the string "../" if only "../" sequences are removed.
</p>
<p>
Ideally, follow these rules:
Finally, the simplest (but most restrictive) option is to use an allow list of safe patterns
and make sure that the user input matches one of these patterns.
</p>
<ul>
<li>Do not allow more than a single "." character.</li>
<li>Do not allow directory separators such as "/" or "\" (depending on the file system).</li>
<li>Do not rely on simply replacing problematic sequences such as "../". For example, after
applying this filter to ".../...//", the resulting string would still be "../".</li>
<li>Use an allowlist of known good patterns.</li>
</ul>
</recommendation>

<example>
Expand All @@ -43,6 +55,27 @@ password file. This file would then be sent back to the user, giving them access
information.
</p>
<sample src="TaintedPath.go" />
<p>
If the input should only be a file name, you can check that it doesn't contain any
path separators or ".." sequences.
</p>
<sample src="TaintedPathGood.go" />
<p>
Note that this approach is only suitable if the input is expected to be a single file name.
</p>
<p>
If the input can be a path with multiple components, you can make it safe by verifying
that the path is within a specific directory that is considered safe.
You can do this by resolving the input with respect to that directory, and then checking
that the resulting path is still within it.
</p>
<sample src="TaintedPathGood2.go" />
<p>
Note that <code>/home/user</code> is just an example, you should replace it with the actual
safe directory in your application. Also, while in this example the path of the safe
directory is absolute, this may not always be the case, and you may need to resolve it
first before checking the input.
</p>
</example>

<references>
Expand Down
21 changes: 21 additions & 0 deletions go/ql/src/Security/CWE-022/TaintedPathGood.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
package main

import (
"io/ioutil"
"net/http"
"path/filepath"
"strings"
)

func handler(w http.ResponseWriter, r *http.Request) {
path := r.URL.Query()["path"][0]

// GOOD: ensure that the filename has no path separators or parent directory references
// (Note that this is only suitable if `path` is expected to have a single component!)
if strings.Contains(path, "/") || strings.Contains(path, "\\") || strings.Contains(path, "..") {
http.Error(w, "Invalid file name", http.StatusBadRequest)
return
}
data, _ := ioutil.ReadFile(filepath.Join("/home/user/", path))
w.Write(data)
}
23 changes: 23 additions & 0 deletions go/ql/src/Security/CWE-022/TaintedPathGood2.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package main

import (
"io/ioutil"
"net/http"
"path/filepath"
"strings"
)

const safeDir = "/home/user/"

func handler(w http.ResponseWriter, r *http.Request) {
path := r.URL.Query()["path"][0]

// GOOD: ensure that the resolved path is within the safe directory
absPath, err := filepath.Abs(filepath.Join(safeDir, path))
if err != nil || !strings.HasPrefix(absPath, safeDir) {
http.Error(w, "Invalid file name", http.StatusBadRequest)
return
}
data, _ := ioutil.ReadFile(absPath)
w.Write(data)
}