From d196d8c5068bcc10f7fac1a3d18c37d53ebd166f Mon Sep 17 00:00:00 2001 From: alxjsn Date: Tue, 8 Oct 2024 17:59:05 -0500 Subject: [PATCH] go: reverseproxy-director and shared-url-struct-mutation --- go/lang/security/reverseproxy-director.go | 65 ++++++++++ go/lang/security/reverseproxy-director.yaml | 33 +++++ .../security/shared-url-struct-mutation.go | 118 ++++++++++++++++++ .../security/shared-url-struct-mutation.yaml | 52 ++++++++ 4 files changed, 268 insertions(+) create mode 100644 go/lang/security/reverseproxy-director.go create mode 100644 go/lang/security/reverseproxy-director.yaml create mode 100644 go/lang/security/shared-url-struct-mutation.go create mode 100644 go/lang/security/shared-url-struct-mutation.yaml diff --git a/go/lang/security/reverseproxy-director.go b/go/lang/security/reverseproxy-director.go new file mode 100644 index 0000000000..0a7b1762e1 --- /dev/null +++ b/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)) +} diff --git a/go/lang/security/reverseproxy-director.yaml b/go/lang/security/reverseproxy-director.yaml new file mode 100644 index 0000000000..ba210b6c72 --- /dev/null +++ b/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 diff --git a/go/lang/security/shared-url-struct-mutation.go b/go/lang/security/shared-url-struct-mutation.go new file mode 100644 index 0000000000..005d20075b --- /dev/null +++ b/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) +} diff --git a/go/lang/security/shared-url-struct-mutation.yaml b/go/lang/security/shared-url-struct-mutation.yaml new file mode 100644 index 0000000000..0dcd483a15 --- /dev/null +++ b/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 +