-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Go: Update go/path-injection
docs to include more sanitizers
#20507
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
base: main
Are you sure you want to change the base?
Conversation
QHelp previews: go/ql/src/Security/CWE-022/TaintedPath.qhelpUncontrolled data used in path expressionAccessing files using paths constructed from user-controlled data can allow an attacker to access unexpected resources. This can result in sensitive information being revealed or deleted, or an attacker being able to influence behavior by modifying unexpected files. 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. RecommendationValidate user input before using it to construct a file path. 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. 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. Note that removing "../" sequences is not 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. 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. Sanitizer functions from the Go standard library: The Go standard library provides several functions that can help sanitize paths:
ExampleIn the first example, a file name is read from an HTTP request and then used to access a file. However, a malicious user could enter a file name which is an absolute path, such as "/etc/passwd". In the second example, it appears that the user is restricted to opening a file within the package main
import (
"io/ioutil"
"net/http"
"path/filepath"
)
func handler(w http.ResponseWriter, r *http.Request) {
path := r.URL.Query()["path"][0]
// BAD: This could read any file on the file system
data, _ := ioutil.ReadFile(path)
w.Write(data)
// BAD: This could still read any file on the file system
data, _ = ioutil.ReadFile(filepath.Join("/home/user/", path))
w.Write(data)
} If the input should only be a file name, you can check that it doesn't contain any path separators or ".." sequences. 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)
} Note that this approach is only suitable if the input is expected to be a single file name. 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. 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)
} Using Go standard library sanitizers: You can also use Go's built-in path validation functions for additional safety: package main
import (
"net/http"
"os"
"path/filepath"
"strings"
)
func handleFileWithSanitizers(w http.ResponseWriter, r *http.Request) {
userPath := r.URL.Query().Get("file")
safeDir := "/home/user/documents"
// Method 1: Using filepath.IsLocal to validate the path is local
if !filepath.IsLocal(userPath) {
http.Error(w, "Invalid path: must be local", http.StatusBadRequest)
return
}
// Method 2: Using strings.Contains to check for path traversal sequences
if strings.Contains(userPath, "..") {
http.Error(w, "Invalid path: contains path traversal", http.StatusBadRequest)
return
}
// Method 3: Using filepath.Rel to ensure path is within safe directory
relPath, err := filepath.Rel(safeDir, filepath.Join(safeDir, userPath))
if err != nil || strings.HasPrefix(relPath, "..") {
http.Error(w, "Invalid path: outside safe directory", http.StatusBadRequest)
return
}
// Method 4: Using filepath.Clean with absolute prefix for normalization
cleanPath := filepath.Clean("/" + userPath)
if strings.Contains(cleanPath, "..") {
http.Error(w, "Invalid path after normalization", http.StatusBadRequest)
return
}
// Method 5: Using strings.HasPrefix for additional validation
finalPath := filepath.Join(safeDir, userPath)
if !strings.HasPrefix(finalPath, safeDir) {
http.Error(w, "Invalid path: must be within safe directory", http.StatusBadRequest)
return
}
// Safe to open file
file, err := os.Open(finalPath)
if err != nil {
http.Error(w, "File not found", http.StatusNotFound)
return
}
defer file.Close()
// Process file...
} Note that References |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Updates the Go path injection documentation to include additional sanitizer functions from the Go standard library. The changes provide developers with more comprehensive options for preventing path traversal vulnerabilities.
- Adds a new example file demonstrating 5 different sanitization methods
- Updates the documentation to include descriptions of standard library functions for path validation
- Expands the recommendation section with detailed explanations of each sanitizer function
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
TaintedPathSanitizers.go | New example file showing multiple Go standard library sanitization methods |
TaintedPath.qhelp | Updated documentation with sanitizer function descriptions and reference to new example |
// Method 3: Using filepath.Rel to ensure path is within safe directory | ||
relPath, err := filepath.Rel(safeDir, filepath.Join(safeDir, userPath)) | ||
if err != nil || strings.HasPrefix(relPath, "..") { | ||
http.Error(w, "Invalid path: outside safe directory", http.StatusBadRequest) | ||
return | ||
} | ||
|
||
// Method 4: Using filepath.Clean with absolute prefix for normalization | ||
cleanPath := filepath.Clean("/" + userPath) | ||
if strings.Contains(cleanPath, "..") { | ||
http.Error(w, "Invalid path after normalization", http.StatusBadRequest) | ||
return | ||
} | ||
|
||
// Method 5: Using strings.HasPrefix for additional validation | ||
finalPath := filepath.Join(safeDir, userPath) | ||
if !strings.HasPrefix(finalPath, safeDir) { | ||
http.Error(w, "Invalid path: must be within safe directory", http.StatusBadRequest) | ||
return | ||
} | ||
|
||
// Safe to open file | ||
file, err := os.Open(finalPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sanitization method has a logical flaw. The filepath.Join(safeDir, userPath)
operation happens before validation, which means a malicious userPath
like ../../../etc/passwd
could escape the safe directory before filepath.Rel
is called. Consider validating userPath
first or using filepath.Rel(safeDir, userPath)
directly to check if the user path would escape the safe directory.
// Method 3: Using filepath.Rel to ensure path is within safe directory | |
relPath, err := filepath.Rel(safeDir, filepath.Join(safeDir, userPath)) | |
if err != nil || strings.HasPrefix(relPath, "..") { | |
http.Error(w, "Invalid path: outside safe directory", http.StatusBadRequest) | |
return | |
} | |
// Method 4: Using filepath.Clean with absolute prefix for normalization | |
cleanPath := filepath.Clean("/" + userPath) | |
if strings.Contains(cleanPath, "..") { | |
http.Error(w, "Invalid path after normalization", http.StatusBadRequest) | |
return | |
} | |
// Method 5: Using strings.HasPrefix for additional validation | |
finalPath := filepath.Join(safeDir, userPath) | |
if !strings.HasPrefix(finalPath, safeDir) { | |
http.Error(w, "Invalid path: must be within safe directory", http.StatusBadRequest) | |
return | |
} | |
// Safe to open file | |
file, err := os.Open(finalPath) | |
// Robust: Resolve absolute path and ensure it's within safeDir | |
absSafeDir, err := filepath.Abs(safeDir) | |
if err != nil { | |
http.Error(w, "Internal error: cannot resolve safe directory", http.StatusInternalServerError) | |
return | |
} | |
absFilePath, err := filepath.Abs(filepath.Join(absSafeDir, userPath)) | |
if err != nil { | |
http.Error(w, "Invalid path", http.StatusBadRequest) | |
return | |
} | |
rel, err := filepath.Rel(absSafeDir, absFilePath) | |
if err != nil || strings.HasPrefix(rel, "..") || filepath.IsAbs(rel) { | |
http.Error(w, "Invalid path: outside safe directory", http.StatusBadRequest) | |
return | |
} | |
// Safe to open file | |
file, err := os.Open(absFilePath) |
Copilot uses AI. Check for mistakes.
|
||
// Method 5: Using strings.HasPrefix for additional validation | ||
finalPath := filepath.Join(safeDir, userPath) | ||
if !strings.HasPrefix(finalPath, safeDir) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This validation method is vulnerable to bypass. An attacker could use a path like ../documents
which, when joined with /home/user/documents
, becomes /home/user/documents/../documents
and still has the safe directory as a prefix even though it references a parent directory. This check should occur after filepath.Clean()
or use a more robust validation method.
if !strings.HasPrefix(finalPath, safeDir) { | |
absFinalPath, err := filepath.Abs(finalPath) | |
if err != nil { | |
http.Error(w, "Invalid path: could not resolve absolute path", http.StatusBadRequest) | |
return | |
} | |
absSafeDir, err := filepath.Abs(safeDir) | |
if err != nil { | |
http.Error(w, "Invalid safe directory", http.StatusInternalServerError) | |
return | |
} | |
if !strings.HasPrefix(absFinalPath, absSafeDir+string(os.PathSeparator)) && absFinalPath != absSafeDir { |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorrect: the result of filepath.Join
is Clean
'd.
The Go standard library provides several functions that can help sanitize paths: | ||
</p> | ||
<ul> | ||
<li><code>filepath.Rel(basepath, targpath)</code> - Returns a relative path from basepath to targpath. If the path is not contained within basepath, it returns an error, making it useful for validating that a path stays within expected boundaries.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This description is incorrect. filepath.Rel()
does not return an error when the target path is outside the base path. It returns a relative path that may contain ..
sequences to reach the target. The error checking should focus on whether the result contains ..
prefixes, not just if an error is returned.
<li><code>filepath.Rel(basepath, targpath)</code> - Returns a relative path from basepath to targpath. If the path is not contained within basepath, it returns an error, making it useful for validating that a path stays within expected boundaries.</li> | |
<li><code>filepath.Rel(basepath, targpath)</code> - Returns a relative path from basepath to targpath. If the target path is outside the base path, the result may contain <code>..</code> components; it does <b>not</b> return an error in this case. To validate that a path stays within expected boundaries, check that the result does not start with <code>..</code> or contain <code>..</code> components.</li> |
Copilot uses AI. Check for mistakes.
This was done by copilot coding agent, but in the wrong repo, so I'm transferring it manually.
Open questions: is this too long-winded? Should we have so many examples? Should we try and indicate where each one is useful?