Merge pull request #11703 from Kwstubbs/go-taintedpath-additions

Go: Add and Modify Sanitizers For TaintedPath

Author: owen-mc

go/ql/lib/change-notes/2024-03-11-addional-gopath-sanitizers.md

@@ -0,0 +1,5 @@
+
+* ---
+category: minorAnalysis
+---
+* Added strings.ReplaceAll, http.ParseMultipartForm sanitizers and remove path sanitizer.

go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll

@@ -5,6 +5,7 @@
 
 import go
 import semmle.go.dataflow.barrierguardutil.RegexpCheck
+import DataFlow
 
 /**
  * Provides extension points for customizing the taint tracking configuration for reasoning about
@@ -74,20 +75,29 @@ module TaintedPath {
   }
 
   /**
-   * A call to `[file]path.Clean("/" + e)`, considered to sanitize `e` against path traversal.
+   * 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", "path/filepath"], "Clean")).getACall() and
+        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()
       )
     }
   }
 
+  /**An call to ParseMultipartForm creates multipart.Form and cleans multipart.Form.FileHeader.Filename using path.Base() */
+  class MultipartClean extends Sanitizer {
+    MultipartClean() {
+      exists(DataFlow::FieldReadNode frn |
+        frn.getField().hasQualifiedName("mime/multipart", "FileHeader", "Filename") and
+        this = frn
+      )
+    }
+  }
+
   /**
    * A check of the form `!strings.Contains(nd, "..")`, considered as a sanitizer guard for
    * path traversal.
@@ -106,6 +116,21 @@ module TaintedPath {
     }
   }
 
+  /**
+   * A replacement of the form `!strings.ReplaceAll(nd, "..")` or `!strings.ReplaceAll(nd, ".")`, considered as a sanitizer for
+   * path traversal.
+   */
+  class DotDotReplace extends Sanitizer {
+    DotDotReplace() {
+      exists(DataFlow::CallNode cleanCall, DataFlow::Node valueNode |
+        cleanCall = any(Function f | f.hasQualifiedName("strings", "ReplaceAll")).getACall() and
+        valueNode = cleanCall.getArgument(1) and
+        valueNode.asExpr().(StringLit).getValue() = ["..", "."] and
+        this = cleanCall.getResult()
+      )
+    }
+  }
+
   /**
    * A node `nd` guarded by a check that ensures it is contained within some root folder,
    * considered as a sanitizer for path traversal.

go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected

@@ -2,18 +2,19 @@ edges
 | TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:13:18:13:30 | call to Query | provenance |  |
 | TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:16:29:16:40 | tainted_path | provenance |  |
 | TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:20:57:20:68 | tainted_path | provenance |  |
+| TaintedPath.go:13:18:13:30 | call to Query | TaintedPath.go:67:39:67:56 | ...+... | provenance |  |
 | TaintedPath.go:20:57:20:68 | tainted_path | TaintedPath.go:20:28:20:69 | call to Join | provenance |  |
-| tst.go:14:2:14:39 | ... := ...[1] | tst.go:17:41:17:56 | selection of Filename | provenance |  |
+| TaintedPath.go:67:39:67:56 | ...+... | TaintedPath.go:67:28:67:57 | call to Clean | provenance |  |
 nodes
 | TaintedPath.go:13:18:13:22 | selection of URL | semmle.label | selection of URL |
 | TaintedPath.go:13:18:13:30 | call to Query | semmle.label | call to Query |
 | 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 |
 | TaintedPath.go:20:57:20:68 | tainted_path | semmle.label | tainted_path |
-| tst.go:14:2:14:39 | ... := ...[1] | semmle.label | ... := ...[1] |
-| tst.go:17:41:17:56 | selection of Filename | semmle.label | selection of Filename |
+| TaintedPath.go:67:28:67:57 | call to Clean | semmle.label | call to Clean |
+| TaintedPath.go:67:39:67:56 | ...+... | semmle.label | ...+... |
 subpaths
 #select
 | TaintedPath.go:16:29:16:40 | tainted_path | TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:16:29:16:40 | tainted_path | This path depends on a $@. | TaintedPath.go:13:18:13:22 | selection of URL | user-provided value |
 | TaintedPath.go:20:28:20:69 | call to Join | TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:20:28:20:69 | call to Join | This path depends on a $@. | TaintedPath.go:13:18:13:22 | selection of URL | user-provided value |
-| tst.go:17:41:17:56 | selection of Filename | tst.go:14:2:14:39 | ... := ...[1] | tst.go:17:41:17:56 | selection of Filename | This path depends on a $@. | tst.go:14:2:14:39 | ... := ...[1] | user-provided value |
+| TaintedPath.go:67:28:67:57 | call to Clean | TaintedPath.go:13:18:13:22 | selection of URL | TaintedPath.go:67:28:67:57 | call to Clean | This path depends on a $@. | TaintedPath.go:13:18:13:22 | selection of URL | user-provided value |

go/ql/test/query-tests/Security/CWE-022/TaintedPath.go

@@ -31,6 +31,10 @@ func handler(w http.ResponseWriter, r *http.Request) {
 		w.Write(data)
 	}
 
+	// GOOD: Sanitized by strings.ReplaceAll and replaces all .. with empty string
+	data, _ = ioutil.ReadFile(strings.ReplaceAll(tainted_path, "..", ""))
+	w.Write(data)
+
 	// GOOD: This can only read inside the provided safe path
 	_, err := filepath.Rel("/home/user/safepath", tainted_path)
 	if err == nil {
@@ -53,10 +57,23 @@ func handler(w http.ResponseWriter, r *http.Request) {
 		w.Write(data)
 	}
 
-	// GOOD: Sanitized by [file]path.Clean with a prepended '/' forcing interpretation
+	// 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("/" + tainted_path))
 	w.Write(data)
+
+	// BAD: Sanitized by path.Clean with a prepended '/' forcing interpretation
+	// as an absolute path, however is not sufficient for Windows paths.
 	data, _ = ioutil.ReadFile(path.Clean("/" + tainted_path))
 	w.Write(data)
+
+	// GOOD: Multipart.Form.FileHeader.Filename sanitized by filepath.Base when calling ParseMultipartForm
+	r.ParseMultipartForm(32 << 20)
+	form := r.MultipartForm
+	files, ok := form.File["files"]
+	if !ok {
+		return
+	}
+	data, _ = ioutil.ReadFile(files[0].Filename)
+	w.Write(data)
 }