Merge pull request #74 from sauyon/http-formvalue

HTTP Library Improvements

ql/src/semmle/go/frameworks/HTTP.qll

@@ -10,7 +10,30 @@ private module StdlibHttp {
     DataFlow::FieldReadNode {
     UserControlledRequestField() {
       exists(string fieldName | this.getField().hasQualifiedName("net/http", "Request", fieldName) |
-        fieldName = "Body" or fieldName = "Form" or fieldName = "Header" or fieldName = "URL"
+        fieldName = "Body" or
+        fieldName = "GetBody" or
+        fieldName = "Form" or
+        fieldName = "PostForm" or
+        fieldName = "MultipartForm" or
+        fieldName = "Header" or
+        fieldName = "Trailer" or
+        fieldName = "URL"
+      )
+    }
+  }
+
+  private class UserControlledRequestMethod extends UntrustedFlowSource::Range,
+    DataFlow::MethodCallNode {
+    UserControlledRequestMethod() {
+      exists(string methName | this.getTarget().hasQualifiedName("net/http", "Request", methName) |
+        methName = "Cookie" or
+        methName = "Cookies" or
+        methName = "FormFile" or
+        methName = "FormValue" or
+        methName = "MultipartReader" or
+        methName = "PostFormValue" or
+        methName = "Referer" or
+        methName = "UserAgent"
       )
     }
   }

ql/src/semmle/go/frameworks/Stdlib.qll

@@ -95,6 +95,19 @@ module Fmt {
   }
 }
 
+/** Provides models of commonly used functions in the `io` package. */
+module Io {
+  private class ReaderRead extends TaintTracking::FunctionModel, Method {
+    ReaderRead() {
+      exists(Method im | im.hasQualifiedName("io", "Reader", "Read") | this.implements(im))
+    }
+
+    override predicate hasTaintFlow(FunctionInput inp, FunctionOutput outp) {
+      inp.isReceiver() and outp.isParameter(0)
+    }
+  }
+}
+
 /** Provides models of commonly used functions in the `io/ioutil` package. */
 module IoUtil {
   private class IoUtilFileSystemAccess extends FileSystemAccess::Range, DataFlow::CallNode {

ql/src/semmle/go/security/OpenUrlRedirectCustomizations.qll

@@ -39,29 +39,36 @@ module OpenUrlRedirect {
     UnsafeUrlMethod() { this.getName() = "Query" }
   }
 
-  /**
-   * A function that trims the right hand side of a string, considered to preserve the safeness
-   * of taint flow from the full request URL.
-   */
-  class StringRightTrimmer extends Strings::Trimmer {
-    StringRightTrimmer() {
-      this.hasQualifiedName("strings", "TrimSuffix") or
-      this.hasQualifiedName("strings", "TrimRight") or
-      this.hasQualifiedName("strings", "TrimRightFunc")
-    }
-  }
-
   /**
    * A source of third-party user input, considered as a flow source for URL redirects.
    */
   class UntrustedFlowAsSource extends Source, UntrustedFlowSource {
     UntrustedFlowAsSource() {
-      // exclude request headers, as they are generally not attacker-controllable for open redirect
-      // exploits
-      not this
-          .(DataFlow::FieldReadNode)
-          .getField()
-          .hasQualifiedName("net/http", "Request", "Header")
+      // exclude some fields and methods of URLs that are generally not attacker-controllable for
+      // open redirect exploits
+      not exists(Field f, string fieldName |
+        f.hasQualifiedName("net/http", "Request", fieldName) and
+        this = f.getARead()
+      |
+        fieldName = "Body" or
+        fieldName = "GetBody" or
+        fieldName = "PostForm" or
+        fieldName = "MultipartForm" or
+        fieldName = "Header" or
+        fieldName = "Trailer"
+      ) and
+      not exists(Method m, string methName |
+        m.hasQualifiedName("net/http", "Request", methName) and
+        this = m.getACall()
+      |
+        methName = "Cookie" or
+        methName = "Cookies" or
+        methName = "FormValue" or
+        methName = "MultipartReader" or
+        methName = "PostFormValues" or
+        methName = "Referer" or
+        methName = "UserAgent"
+      )
     }
   }
 

ql/test/library-tests/semmle/go/frameworks/HTTP/UntrustedFlowSources.expected

@@ -13,3 +13,4 @@
 | server.go:9:6:9:13 | selection of Header |
 | server.go:10:6:10:13 | selection of Header |
 | server.go:13:6:13:11 | selection of Body |
+| server.go:14:15:14:23 | selection of GetBody |

ql/test/library-tests/semmle/go/frameworks/TaintSteps/TaintStep.expected

@@ -5,19 +5,23 @@
 | crypto.go:11:18:11:57 | call to Open | crypto.go:11:2:11:57 | ... := ...[0] |
 | crypto.go:11:18:11:57 | call to Open | crypto.go:11:2:11:57 | ... := ...[1] |
 | crypto.go:11:42:11:51 | ciphertext | crypto.go:11:2:11:57 | ... := ...[0] |
-| main.go:10:12:10:26 | call to Marshal | main.go:10:2:10:26 | ... := ...[0] |
-| main.go:10:12:10:26 | call to Marshal | main.go:10:2:10:26 | ... := ...[1] |
-| main.go:10:25:10:25 | v | main.go:10:2:10:26 | ... := ...[0] |
-| main.go:12:14:12:52 | call to MarshalIndent | main.go:12:2:12:52 | ... := ...[0] |
-| main.go:12:14:12:52 | call to MarshalIndent | main.go:12:2:12:52 | ... := ...[1] |
-| main.go:12:33:12:33 | v | main.go:12:2:12:52 | ... := ...[0] |
-| main.go:13:25:13:25 | b | main.go:13:9:13:41 | composite literal |
-| main.go:13:28:13:30 | err | main.go:13:9:13:41 | composite literal |
-| main.go:13:33:13:34 | b2 | main.go:13:9:13:41 | composite literal |
-| main.go:13:37:13:40 | err2 | main.go:13:9:13:41 | composite literal |
-| main.go:18:18:18:42 | call to DecodeString | main.go:18:2:18:42 | ... := ...[0] |
-| main.go:18:18:18:42 | call to DecodeString | main.go:18:2:18:42 | ... := ...[1] |
-| main.go:18:35:18:41 | encoded | main.go:18:2:18:42 | ... := ...[0] |
-| main.go:22:25:22:31 | decoded | main.go:22:9:22:48 | composite literal |
-| main.go:22:34:22:36 | err | main.go:22:9:22:48 | composite literal |
-| main.go:22:39:22:47 | reEncoded | main.go:22:9:22:48 | composite literal |
+| main.go:11:12:11:26 | call to Marshal | main.go:11:2:11:26 | ... := ...[0] |
+| main.go:11:12:11:26 | call to Marshal | main.go:11:2:11:26 | ... := ...[1] |
+| main.go:11:25:11:25 | v | main.go:11:2:11:26 | ... := ...[0] |
+| main.go:13:14:13:52 | call to MarshalIndent | main.go:13:2:13:52 | ... := ...[0] |
+| main.go:13:14:13:52 | call to MarshalIndent | main.go:13:2:13:52 | ... := ...[1] |
+| main.go:13:33:13:33 | v | main.go:13:2:13:52 | ... := ...[0] |
+| main.go:14:25:14:25 | b | main.go:14:9:14:41 | composite literal |
+| main.go:14:28:14:30 | err | main.go:14:9:14:41 | composite literal |
+| main.go:14:33:14:34 | b2 | main.go:14:9:14:41 | composite literal |
+| main.go:14:37:14:40 | err2 | main.go:14:9:14:41 | composite literal |
+| main.go:19:18:19:42 | call to DecodeString | main.go:19:2:19:42 | ... := ...[0] |
+| main.go:19:18:19:42 | call to DecodeString | main.go:19:2:19:42 | ... := ...[1] |
+| main.go:19:35:19:41 | encoded | main.go:19:2:19:42 | ... := ...[0] |
+| main.go:23:25:23:31 | decoded | main.go:23:9:23:48 | composite literal |
+| main.go:23:34:23:36 | err | main.go:23:9:23:48 | composite literal |
+| main.go:23:39:23:47 | reEncoded | main.go:23:9:23:48 | composite literal |
+| main.go:28:2:28:4 | implicit dereference | main.go:26:15:26:17 | definition of req |
+| main.go:28:2:28:4 | implicit dereference | main.go:28:2:28:9 | selection of Body |
+| main.go:28:2:28:4 | req | main.go:28:2:28:4 | implicit dereference |
+| main.go:28:2:28:9 | selection of Body | main.go:27:2:27:2 | definition of b |

ql/test/library-tests/semmle/go/frameworks/TaintSteps/main.go

@@ -3,6 +3,7 @@ package main
 import (
 	"encoding/hex"
 	"encoding/json"
+	"net/http"
 )
 
 func jsonTest(v interface{}) []interface{} {
@@ -21,3 +22,9 @@ func hexTest(encoded string) []interface{} {
 	reEncoded := hex.EncodeToString(decoded)
 	return [](interface{}){decoded, err, reEncoded}
 }
+
+func readTest(req *http.Request) string {
+	b := make([]byte, 8)
+	req.Body.Read(b)
+	return string(b)
+}