Add extra sanitizer Part.FileName()

Author: owen-mc

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

@@ -108,6 +108,24 @@ module TaintedPath {
     }
   }
 
+  /**
+   * 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()
+    }
+  }
+
   /**
    * A check of the form `!strings.Contains(nd, "..")`, considered as a sanitizer guard for
    * path traversal.

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:16:18:16:22 | selection of URL | TaintedPath.go:16:18:16:30 | call to Query | provenance |  |
+| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:19:29:19:40 | tainted_path | provenance |  |
+| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:23:57:23:68 | tainted_path | provenance |  |
+| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:70:39:70:56 | ...+... | provenance |  |
+| TaintedPath.go:23:57:23:68 | tainted_path | TaintedPath.go:23:28:23:69 | call to Join | provenance |  |
+| TaintedPath.go:70:39:70:56 | ...+... | TaintedPath.go:70:28:70: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:16:18:16:22 | selection of URL | semmle.label | selection of URL |
+| TaintedPath.go:16:18:16:30 | call to Query | semmle.label | call to Query |
+| TaintedPath.go:19:29:19:40 | tainted_path | semmle.label | tainted_path |
+| TaintedPath.go:23:28:23:69 | call to Join | semmle.label | call to Join |
+| TaintedPath.go:23:57:23:68 | tainted_path | semmle.label | tainted_path |
+| TaintedPath.go:70:28:70:57 | call to Clean | semmle.label | call to Clean |
+| TaintedPath.go:70:39:70: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:19:29:19:40 | tainted_path | TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:19:29:19:40 | tainted_path | This path depends on a $@. | TaintedPath.go:16:18:16:22 | selection of URL | user-provided value |
+| TaintedPath.go:23:28:23:69 | call to Join | TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:23:28:23:69 | call to Join | This path depends on a $@. | TaintedPath.go:16:18:16:22 | selection of URL | user-provided value |
+| TaintedPath.go:70:28:70:57 | call to Clean | TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:70:28:70:57 | call to Clean | This path depends on a $@. | TaintedPath.go:16:18:16:22 | selection of URL | user-provided value |

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

@@ -1,7 +1,10 @@
 package main
 
 import (
+	"fmt"
+	"io"
 	"io/ioutil"
+	"mime/multipart"
 	"net/http"
 	"path"
 	"path/filepath"
@@ -76,4 +79,21 @@ 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())
+	// fmt.Println("Content-Type:", part.Header.Get("Content-Type"))
 }