go: reverseproxy-director and shared-url-struct-mutation

go/lang/security/reverseproxy-director.go

@@ -0,0 +1,65 @@
+package main
+
+import (
+	"log"
+	"net/http"
+	"net/http/httputil"
+	"net/url"
+)
+
+func NewProxy(targetHost string) (*httputil.ReverseProxy, error) {
+	url, err := url.Parse(targetHost)
+	if err != nil {
+		return nil, err
+	}
+
+	proxy := httputil.NewSingleHostReverseProxy(url)
+
+	originalDirector := proxy.Director
+	// ruleid: reverseproxy-director
+	proxy.Director = func(req *http.Request) {
+		originalDirector(req)
+		modifyRequest(req)
+	}
+	return proxy, nil
+}
+
+func modifyRequest(req *http.Request) {
+	req.Header.Set("Extra-Header", "nice")
+}
+
+func ProxyRequestHandler(proxy *httputil.ReverseProxy) func(http.ResponseWriter, *http.Request) {
+	return func(w http.ResponseWriter, r *http.Request) {
+		proxy.ServeHTTP(w, r)
+	}
+}
+
+type Fake struct {
+	Director string
+}
+
+func extraCases() {
+	rp := &httputil.ReverseProxy{
+		// ruleid: reverseproxy-director
+		Director: func(req *http.Request) {
+			modifyRequest(req)
+		},
+	}
+	_ = rp
+
+	f := Fake{
+		// ok: reverseproxy-director
+		Director: "abcd",
+	}
+	_ = f
+}
+
+func main() {
+	proxy, err := NewProxy("https://example.com")
+	if err != nil {
+		panic(err)
+	}
+
+	http.HandleFunc("/", ProxyRequestHandler(proxy))
+	log.Fatal(http.ListenAndServe(":8080", nil))
+}

go/lang/security/reverseproxy-director.yaml

@@ -0,0 +1,33 @@
+rules:
+- id: reverseproxy-director
+  message: >-
+    ReverseProxy can remove headers added by Director. Consider using ReverseProxy.Rewrite
+    instead of ReverseProxy.Director.
+  languages: [go]
+  severity: WARNING
+  patterns:
+  - pattern-inside: |
+      import "net/http/httputil"
+      ...
+  - pattern-either:
+      - pattern: $PROXY.Director = $FUNC
+      - patterns:
+          - pattern-inside: |
+              httputil.ReverseProxy{
+                  ...
+              }
+          - pattern: |
+              Director: $FUNC
+  metadata:
+    cwe:
+    - "CWE-115: Misinterpretation of Input"
+    category: security
+    subcategory:
+    - audit
+    technology:
+      - go
+    confidence: MEDIUM
+    likelihood: LOW
+    impact: LOW
+    references:
+      - https://github.com/golang/go/issues/50580

go/lang/security/shared-url-struct-mutation.go

@@ -0,0 +1,118 @@
+package main
+
+import (
+	"net/http"
+	"net/url"
+)
+
+var redirectURL, _ = url.Parse("https://example.com")
+
+func getRedirectToken() (string, error) {
+	return "abcd", nil
+}
+
+func handler1(w http.ResponseWriter, r *http.Request) {
+	u := redirectURL
+	q := u.Query()
+
+	// opaque process that might fail
+	token, err := getRedirectToken()
+	if err != nil {
+		q.Set("error", err.Error())
+	} else {
+		q.Set("token", token)
+	}
+	// ruleid: shared-url-struct-mutation
+	u.RawQuery = q.Encode()
+	r.URL.RawQuery = q.Encode()
+
+	http.Redirect(w, r, u.String(), http.StatusFound)
+}
+
+func handler2(w http.ResponseWriter, r *http.Request) {
+	u, _ := url.Parse("https://example.com")
+
+	q := u.Query()
+
+	// opaque process that might fail
+	token, err := getRedirectToken()
+	if err != nil {
+		q.Set("error", err.Error())
+	} else {
+		q.Set("token", token)
+	}
+	// ok: shared-url-struct-mutation
+	u.RawQuery = q.Encode()
+
+	http.Redirect(w, r, u.String(), http.StatusFound)
+}
+
+func handler3(w http.ResponseWriter, r *http.Request) {
+	u := url.URL{
+		Scheme: "https",
+		Host:   "example.com",
+		Path:   "/",
+	}
+	q := u.Query()
+
+	// opaque process that might fail
+	token, err := getRedirectToken()
+	if err != nil {
+		q.Set("error", err.Error())
+	} else {
+		q.Set("token", token)
+	}
+
+	u.RawQuery = q.Encode()
+
+	http.Redirect(w, r, u.String(), http.StatusFound)
+}
+
+func handler4(w http.ResponseWriter, r *http.Request) {
+	var u *url.URL
+	if true {
+		u, _ = url.Parse("https://example.com")
+	}
+
+	if u != nil {
+
+		q := u.Query()
+
+		// opaque process that might fail
+		token, err := getRedirectToken()
+		if err != nil {
+			q.Set("error", err.Error())
+		} else {
+			q.Set("token", token)
+		}
+		// ok: shared-url-struct-mutation
+		u.RawQuery = q.Encode()
+
+		http.Redirect(w, r, u.String(), http.StatusFound)
+	}
+	http.Redirect(w, r, "https://google.com", http.StatusFound)
+}
+
+func extraCases(w http.ResponseWriter, r *http.Request) {
+	var x struct {
+		y []struct {
+			Path string
+		}
+	}
+	// ok: shared-url-struct-mutation
+	r.URL.RawQuery = "abcd"
+	// ok: shared-url-struct-mutation
+	x.y[0].Path = "abcd"
+
+	a, _ := url.ParseRequestURI("https://example.com")
+	// ok: shared-url-struct-mutation
+	a.RawQuery = "abcd"
+}
+
+func main() {
+	http.HandleFunc("/1", handler1)
+	http.HandleFunc("/2", handler2)
+	http.HandleFunc("/3", handler3)
+	http.HandleFunc("/4", handler4)
+	http.ListenAndServe(":7777", nil)
+}

go/lang/security/shared-url-struct-mutation.yaml

@@ -0,0 +1,52 @@
+rules:
+- id: shared-url-struct-mutation
+  message: >-
+    Shared URL struct may have been accidentally mutated. Ensure that
+    this behavior is intended.
+  languages: [go]
+  severity: WARNING
+  patterns:
+  - pattern-inside: |
+      import "net/url"
+      ...
+  - pattern-not-inside: |
+      ... = url.Parse(...)
+      ...
+  - pattern-not-inside: |
+      ... = url.ParseRequestURI(...)
+      ...
+  - pattern-not-inside: |
+      ... = url.URL{...}
+      ...
+  - pattern-not-inside: |
+      var $URL *$X.URL
+      ...
+  - pattern-either:
+      - pattern: $URL.RawQuery = ...
+      - pattern: $URL.Path = ...
+      - pattern: $URL.RawPath = ...
+      - pattern: $URL.Fragment = ...
+      - pattern: $URL.RawFragment = ...
+      - pattern: $URL.Scheme = ...
+      - pattern: $URL.Opaque = ...
+      - pattern: $URL.Host = ...
+      - pattern: $URL.User = ...
+  - metavariable-pattern:
+      metavariable: $URL
+      patterns:
+        - pattern-not: $X.$Y
+        - pattern-not: $X[...]
+  metadata:
+    cwe:
+    - "CWE-436: Interpretation Conflict"
+    category: security
+    subcategory:
+    - audit
+    technology:
+      - go
+    confidence: LOW
+    likelihood: LOW
+    impact: LOW
+    references:
+      - https://github.com/golang/go/issues/63777
+