From 4e4cd52f6300da905256df3c5a532a018ff756b1 Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 22 Mar 2024 11:44:06 +0000 Subject: [PATCH 1/3] Go: Update query help for `go/path-injection` to include example fixes. --- go/ql/src/Security/CWE-022/TaintedPath.qhelp | 51 +++++++++++++++---- go/ql/src/Security/CWE-022/TaintedPathGood.go | 20 ++++++++ .../src/Security/CWE-022/TaintedPathGood2.go | 23 +++++++++ 3 files changed, 84 insertions(+), 10 deletions(-) create mode 100644 go/ql/src/Security/CWE-022/TaintedPathGood.go create mode 100644 go/ql/src/Security/CWE-022/TaintedPathGood2.go diff --git a/go/ql/src/Security/CWE-022/TaintedPath.qhelp b/go/ql/src/Security/CWE-022/TaintedPath.qhelp index 061ffbe34712..3a3948c425dc 100644 --- a/go/ql/src/Security/CWE-022/TaintedPath.qhelp +++ b/go/ql/src/Security/CWE-022/TaintedPath.qhelp @@ -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.

+

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

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

+

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

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

- @@ -43,6 +55,25 @@ password file. This file would then be sent back to the user, giving them access information.

+

+If the input should only be a file name, you can check that it doesn't contain any +path separators or ".." sequences. +

+ +

+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, we can make it safe by verifying +that the path is within a specific directory that is considered safe. +This can be done by resolving the input with respect to that directory, and then checking +that the resulting path is still within it. +

+ +

+Note that /home/user is just an example, you should replace it with the actual +safe directory in your application. +

 diff --git a/go/ql/src/Security/CWE-022/TaintedPathGood.go b/go/ql/src/Security/CWE-022/TaintedPathGood.go new file mode 100644 index 000000000000..50c1359a9030 --- /dev/null +++ b/go/ql/src/Security/CWE-022/TaintedPathGood.go @@ -0,0 +1,20 @@ +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 + 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) +} diff --git a/go/ql/src/Security/CWE-022/TaintedPathGood2.go b/go/ql/src/Security/CWE-022/TaintedPathGood2.go new file mode 100644 index 000000000000..0136adf11dcb --- /dev/null +++ b/go/ql/src/Security/CWE-022/TaintedPathGood2.go @@ -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) +} From bc9396e0e66b5bd7dc689aeea6058108701b1d5c Mon Sep 17 00:00:00 2001 From: Max Schaefer Date: Fri, 22 Mar 2024 13:19:36 +0000 Subject: [PATCH 2/3] Address suggestions from review. --- go/ql/src/Security/CWE-022/TaintedPath.qhelp | 4 +++- go/ql/src/Security/CWE-022/TaintedPathGood.go | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/go/ql/src/Security/CWE-022/TaintedPath.qhelp b/go/ql/src/Security/CWE-022/TaintedPath.qhelp index 3a3948c425dc..3b54e80cd97c 100644 --- a/go/ql/src/Security/CWE-022/TaintedPath.qhelp +++ b/go/ql/src/Security/CWE-022/TaintedPath.qhelp @@ -72,7 +72,9 @@ that the resulting path is still within it.

Note that /home/user is just an example, you should replace it with the actual -safe directory in your application. +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.

diff --git a/go/ql/src/Security/CWE-022/TaintedPathGood.go b/go/ql/src/Security/CWE-022/TaintedPathGood.go index 50c1359a9030..8a46c76b26a0 100644 --- a/go/ql/src/Security/CWE-022/TaintedPathGood.go +++ b/go/ql/src/Security/CWE-022/TaintedPathGood.go @@ -11,6 +11,7 @@ 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 From 034ed1722781b0bd219855af192a793d48077d1c Mon Sep 17 00:00:00 2001 From: Max Schaefer <54907921+max-schaefer@users.noreply.github.com> Date: Fri, 22 Mar 2024 15:24:29 +0000 Subject: [PATCH 3/3] Apply suggestions from code review Co-authored-by: Ben Ahmady <32935794+subatoi@users.noreply.github.com> --- go/ql/src/Security/CWE-022/TaintedPath.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ql/src/Security/CWE-022/TaintedPath.qhelp b/go/ql/src/Security/CWE-022/TaintedPath.qhelp index 3b54e80cd97c..94edec4e4f4b 100644 --- a/go/ql/src/Security/CWE-022/TaintedPath.qhelp +++ b/go/ql/src/Security/CWE-022/TaintedPath.qhelp @@ -64,9 +64,9 @@ path separators or ".." sequences. 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, we can make it safe by verifying +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. -This can be done by resolving the input with respect to that directory, and then checking +You can do this by resolving the input with respect to that directory, and then checking that the resulting path is still within it.