Merge pull request #16180 from owen-mc/go/tweak-go-tainted-path-addit…

…ions Go: Tweak go tainted path additions
github · Apr 11, 2024 · 1e8315d · 1e8315d
2 parents e6fdc75 + a7c5e84
commit 1e8315d
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 32 deletions.
diff --git a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll
@@ -88,13 +88,41 @@ module TaintedPath {
     }
   }
 
-  /**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 read from the field `Filename` of the type `mime/multipart.FileHeader`,
+   * considered as a sanitizer for path traversal.
+   *
+   * The only way to create a `mime/multipart.FileHeader` is to create a
+   * `mime/multipart.Form`, which creates the `Filename` field of each
+   * `mime/multipart.FileHeader` by calling `Part.FileName`, which calls
+   * `path/filepath.Base` on its return value. In general `path/filepath.Base`
+   * is not a sanitizer for path traversal, but in this specific case where the
+   * output is going to be used as a filename rather than a directory name, it
+   * is adequate.
+   */
+  class MimeMultipartFileHeaderFilenameSanitizer extends Sanitizer {
+    MimeMultipartFileHeaderFilenameSanitizer() {
+      this.(DataFlow::FieldReadNode)
+          .getField()
+          .hasQualifiedName("mime/multipart", "FileHeader", "Filename")
+    }
+  }
+
+  /**
+   * A call to `mime/multipart.Part.FileName`, considered as a sanitizer
+   * against path traversal.
+   *
+   * `Part.FileName` calls `path/filepath.Base` on its return value. In
+   * general `path/filepath.Base` is not a sanitizer for path traversal, but in
+   * this specific case where the output is going to be used as a filename
+   * rather than a directory name, it is adequate.
+   */
+  class MimeMultipartPartFileNameSanitizer extends Sanitizer {
+    MimeMultipartPartFileNameSanitizer() {
+      this =
+        any(Method m | m.hasQualifiedName("mime/multipart", "Part", "FileName"))
+            .getACall()
+            .getResult()
     }
   }
 
@@ -120,15 +148,8 @@ 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()
-      )
-    }
+  class DotDotReplaceAll extends StringOps::ReplaceAll, Sanitizer {
+    DotDotReplaceAll() { this.getReplacedString() = ["..", "."] }
   }
 
   /**

diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected
@@ -1,20 +1,20 @@
 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 |  |
-| TaintedPath.go:67:39:67:56 | ...+... | TaintedPath.go:67:28:67:57 | call to Clean | provenance |  |
+| TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:14:18:14:30 | call to Query | provenance |  |
+| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:17:29:17:40 | tainted_path | provenance |  |
+| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:21:57:21:68 | tainted_path | provenance |  |
+| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:68:39:68:56 | ...+... | provenance |  |
+| TaintedPath.go:21:57:21:68 | tainted_path | TaintedPath.go:21:28:21:69 | call to Join | provenance |  |
+| TaintedPath.go:68:39:68:56 | ...+... | TaintedPath.go:68:28:68: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 |
-| TaintedPath.go:67:28:67:57 | call to Clean | semmle.label | call to Clean |
-| TaintedPath.go:67:39:67:56 | ...+... | semmle.label | ...+... |
+| TaintedPath.go:14:18:14:22 | selection of URL | semmle.label | selection of URL |
+| TaintedPath.go:14:18:14:30 | call to Query | semmle.label | call to Query |
+| TaintedPath.go:17:29:17:40 | tainted_path | semmle.label | tainted_path |
+| TaintedPath.go:21:28:21:69 | call to Join | semmle.label | call to Join |
+| TaintedPath.go:21:57:21:68 | tainted_path | semmle.label | tainted_path |
+| TaintedPath.go:68:28:68:57 | call to Clean | semmle.label | call to Clean |
+| TaintedPath.go:68:39:68: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 |
-| 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 |
+| TaintedPath.go:17:29:17:40 | tainted_path | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:17:29:17:40 | tainted_path | This path depends on a $@. | TaintedPath.go:14:18:14:22 | selection of URL | user-provided value |
+| TaintedPath.go:21:28:21:69 | call to Join | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:21:28:21:69 | call to Join | This path depends on a $@. | TaintedPath.go:14:18:14:22 | selection of URL | user-provided value |
+| TaintedPath.go:68:28:68:57 | call to Clean | TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:68:28:68:57 | call to Clean | This path depends on a $@. | TaintedPath.go:14:18:14:22 | selection of URL | user-provided value |
diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go
@@ -2,6 +2,7 @@ package main
 
 import (
 	"io/ioutil"
+	"mime/multipart"
 	"net/http"
 	"path"
 	"path/filepath"
@@ -76,4 +77,20 @@ func handler(w http.ResponseWriter, r *http.Request) {
 	}
 	data, _ = ioutil.ReadFile(files[0].Filename)
 	w.Write(data)
+
+	// GOOD: Part.FileName sanitized by filepath.Base
+	r.ParseMultipartForm(32 << 20)
+	file, _, err := r.FormFile("uploadfile")
+	if err != nil {
+		return
+	}
+	reader := multipart.NewReader(file, "foo")
+
+	// Read the first part
+	part, err := reader.NextPart()
+	if err != nil {
+		return
+	}
+
+	data, _ = ioutil.ReadFile(part.FileName())
 }