From e8084233b8ed8414137662c713ffbe0fc9c82f42 Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Tue, 8 Mar 2022 16:42:59 +0000 Subject: [PATCH] Treat path.Clean and filepath.Clean alike re: tainted path sanitization --- .../go/security/TaintedPathCustomizations.qll | 5 +-- .../Security/CWE-022/TaintedPath.expected | 14 ++++----- .../Security/CWE-022/TaintedPath.go | 31 ++++++++++--------- 3 files changed, 27 insertions(+), 23 deletions(-) diff --git a/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/ql/lib/semmle/go/security/TaintedPathCustomizations.qll index 4845fd462e3..5d138e3b018 100644 --- a/ql/lib/semmle/go/security/TaintedPathCustomizations.qll +++ b/ql/lib/semmle/go/security/TaintedPathCustomizations.qll @@ -77,12 +77,13 @@ module TaintedPath { } /** - * A call to `filepath.Clean("/" + e)`, considered to sanitize `e` against path traversal. + * A call to `[file]path.Clean("/" + e)`, considered to sanitize `e` against path traversal. */ class FilepathCleanSanitizer extends Sanitizer { FilepathCleanSanitizer() { exists(DataFlow::CallNode cleanCall, StringOps::Concatenation concatNode | - cleanCall = any(Function f | f.hasQualifiedName("path/filepath", "Clean")).getACall() and + cleanCall = + any(Function f | f.hasQualifiedName(["path", "path/filepath"], "Clean")).getACall() and concatNode = cleanCall.getArgument(0) and concatNode.getOperand(0).asExpr().(StringLit).getValue() = "/" and this = cleanCall.getResult() diff --git a/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/ql/test/query-tests/Security/CWE-022/TaintedPath.expected index 1d0345da41d..430cd9e64b7 100644 --- a/ql/test/query-tests/Security/CWE-022/TaintedPath.expected +++ b/ql/test/query-tests/Security/CWE-022/TaintedPath.expected @@ -1,19 +1,19 @@ edges -| TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:15:29:15:32 | path | -| TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:19:28:19:61 | call to Join | +| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:16:29:16:40 | tainted_path | +| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:20:28:20:69 | call to Join | | tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:47 | implicit dereference : FileHeader | | tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:56 | selection of Filename | | tst.go:17:41:17:47 | implicit dereference : FileHeader | tst.go:17:41:17:47 | implicit dereference : FileHeader | | tst.go:17:41:17:47 | implicit dereference : FileHeader | tst.go:17:41:17:56 | selection of Filename | nodes -| TaintedPath.go:12:10:12:14 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | -| TaintedPath.go:15:29:15:32 | path | semmle.label | path | -| TaintedPath.go:19:28:19:61 | call to Join | semmle.label | call to Join | +| TaintedPath.go:13:18:13:22 | selection of URL : pointer type | semmle.label | selection of URL : pointer type | +| TaintedPath.go:16:29:16:40 | tainted_path | semmle.label | tainted_path | +| TaintedPath.go:20:28:20:69 | call to Join | semmle.label | call to Join | | tst.go:14:2:14:39 | ... := ...[1] : pointer type | semmle.label | ... := ...[1] : pointer type | | tst.go:17:41:17:47 | implicit dereference : FileHeader | semmle.label | implicit dereference : FileHeader | | tst.go:17:41:17:56 | selection of Filename | semmle.label | selection of Filename | subpaths #select -| TaintedPath.go:15:29:15:32 | path | TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:15:29:15:32 | path | This path depends on $@. | TaintedPath.go:12:10:12:14 | selection of URL | a user-provided value | -| TaintedPath.go:19:28:19:61 | call to Join | TaintedPath.go:12:10:12:14 | selection of URL : pointer type | TaintedPath.go:19:28:19:61 | call to Join | This path depends on $@. | TaintedPath.go:12:10:12:14 | selection of URL | a user-provided value | +| TaintedPath.go:16:29:16:40 | tainted_path | TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:16:29:16:40 | tainted_path | This path depends on $@. | TaintedPath.go:13:18:13:22 | selection of URL | a user-provided value | +| TaintedPath.go:20:28:20:69 | call to Join | TaintedPath.go:13:18:13:22 | selection of URL : pointer type | TaintedPath.go:20:28:20:69 | call to Join | This path depends on $@. | TaintedPath.go:13:18:13:22 | selection of URL | a user-provided value | | tst.go:17:41:17:56 | selection of Filename | tst.go:14:2:14:39 | ... := ...[1] : pointer type | tst.go:17:41:17:56 | selection of Filename | This path depends on $@. | tst.go:14:2:14:39 | ... := ...[1] | a user-provided value | diff --git a/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/ql/test/query-tests/Security/CWE-022/TaintedPath.go index a0b5ee0c9c8..e663fc3b11d 100644 --- a/ql/test/query-tests/Security/CWE-022/TaintedPath.go +++ b/ql/test/query-tests/Security/CWE-022/TaintedPath.go @@ -3,57 +3,60 @@ package main import ( "io/ioutil" "net/http" + "path" "path/filepath" "regexp" "strings" ) func handler(w http.ResponseWriter, r *http.Request) { - path := r.URL.Query()["path"][0] + tainted_path := r.URL.Query()["path"][0] // BAD: This could read any file on the file system - data, _ := ioutil.ReadFile(path) + data, _ := ioutil.ReadFile(tainted_path) w.Write(data) // BAD: This could still read any file on the file system - data, _ = ioutil.ReadFile(filepath.Join("/home/user/", path)) + data, _ = ioutil.ReadFile(filepath.Join("/home/user/", tainted_path)) w.Write(data) // GOOD: This can only read inside the provided safe path - sanitized_filepath, _ := filepath.Rel("/home/user/safepath", path) + sanitized_filepath, _ := filepath.Rel("/home/user/safepath", tainted_path) data, _ = ioutil.ReadFile(sanitized_filepath) w.Write(data) // GOOD: This can only read inside the provided safe path - if !strings.Contains(path, "..") { - data, _ = ioutil.ReadFile(path) + if !strings.Contains(tainted_path, "..") { + data, _ = ioutil.ReadFile(tainted_path) w.Write(data) } // GOOD: This can only read inside the provided safe path - _, err := filepath.Rel("/home/user/safepath", path) + _, err := filepath.Rel("/home/user/safepath", tainted_path) if err == nil { - data, _ = ioutil.ReadFile(path) + data, _ = ioutil.ReadFile(tainted_path) w.Write(data) } // GOOD: An attempt has been made to ensure that this can only read inside // the provided safe path - if strings.HasPrefix(path, "/home/user/safepath/") { - data, _ = ioutil.ReadFile(path) + if strings.HasPrefix(tainted_path, "/home/user/safepath/") { + data, _ = ioutil.ReadFile(tainted_path) w.Write(data) } // GOOD: An attempt has been made to ensure that this can only read inside // the provided safe path - matched, _ := regexp.MatchString("\\.\\.", path) + matched, _ := regexp.MatchString("\\.\\.", tainted_path) if !matched { - data, _ = ioutil.ReadFile(filepath.Join("/home/user/", path)) + data, _ = ioutil.ReadFile(filepath.Join("/home/user/", tainted_path)) w.Write(data) } - // GOOD: Sanitized by filepath.Clean with a prepended '/' forcing interpretation + // GOOD: Sanitized by [file]path.Clean with a prepended '/' forcing interpretation // as an absolute path, so that Clean will throw away any leading `..` components. - data, _ = ioutil.ReadFile(filepath.Clean("/" + path)) + data, _ = ioutil.ReadFile(filepath.Clean("/" + tainted_path)) + w.Write(data) + data, _ = ioutil.ReadFile(path.Clean("/" + tainted_path)) w.Write(data) }