From de2ed83b55239c9ac1530f0143778375a89aeb8e Mon Sep 17 00:00:00 2001 From: Chris Smowton Date: Fri, 28 Jan 2022 19:32:58 +0000 Subject: [PATCH] Note that `filepath.Clean("/" + e)` is a sanitizer against path traversal attacks. --- .../go/security/TaintedPathCustomizations.qll | 14 ++++++++++++++ .../query-tests/Security/CWE-022/TaintedPath.go | 5 +++++ 2 files changed, 19 insertions(+) diff --git a/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/ql/lib/semmle/go/security/TaintedPathCustomizations.qll index 2c0fba9d..4845fd46 100644 --- a/ql/lib/semmle/go/security/TaintedPathCustomizations.qll +++ b/ql/lib/semmle/go/security/TaintedPathCustomizations.qll @@ -76,6 +76,20 @@ module TaintedPath { } } + /** + * A call to `filepath.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 + concatNode = cleanCall.getArgument(0) and + concatNode.getOperand(0).asExpr().(StringLit).getValue() = "/" and + this = cleanCall.getResult() + ) + } + } + /** * A check of the form `!strings.Contains(nd, "..")`, considered as a sanitizer guard for * path traversal. diff --git a/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/ql/test/query-tests/Security/CWE-022/TaintedPath.go index 79b5c22c..a0b5ee0c 100644 --- a/ql/test/query-tests/Security/CWE-022/TaintedPath.go +++ b/ql/test/query-tests/Security/CWE-022/TaintedPath.go @@ -51,4 +51,9 @@ func handler(w http.ResponseWriter, r *http.Request) { data, _ = ioutil.ReadFile(filepath.Join("/home/user/", path)) w.Write(data) } + + // GOOD: Sanitized by filepath.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)) + w.Write(data) }