From d195273bf4f2be923d4355fd2e1c48ee1ce758fd Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Mon, 14 Oct 2024 19:48:54 -0700 Subject: [PATCH 1/6] Add mux.Vars() and url.Path sanitizers --- .../go/security/TaintedPathCustomizations.qll | 29 ++ .../2024-10-14-gopathsanitizer.md | 4 + .../Security/CWE-022/TaintedPath.expected | 32 +-- .../Security/CWE-022/TaintedPath.go | 9 + .../test/query-tests/Security/CWE-022/go.mod | 5 + .../vendor/github.com/gorilla/mux/LICENSE | 27 ++ .../vendor/github.com/gorilla/mux/stub.go | 252 ++++++++++++++++++ .../Security/CWE-022/vendor/modules.txt | 3 + 8 files changed, 345 insertions(+), 16 deletions(-) create mode 100644 go/ql/src/change-notes/2024-10-14-gopathsanitizer.md create mode 100644 go/ql/test/query-tests/Security/CWE-022/go.mod create mode 100644 go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/LICENSE create mode 100644 go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/stub.go create mode 100644 go/ql/test/query-tests/Security/CWE-022/vendor/modules.txt diff --git a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll index 953d9810d532..f75a16ceccf8 100644 --- a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll +++ b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll @@ -93,6 +93,35 @@ module TaintedPath { } } + // /** + // * A call to `mux.Vars(path)`, considered to sanitize `path` against path traversal. + // * Only enabled when `SkipClean` is not set true. + // */ + class MuxVarsSanitizer extends Sanitizer { + MuxVarsSanitizer() { + exists(Function m | + m.hasQualifiedName("github.com/gorilla/mux", "Vars") and + this = m.getACall().getResult() + ) and + not exists(CallExpr f | + f.getTarget().hasQualifiedName("github.com/gorilla/mux", "SkipClean") and + f.getArgument(0).toString().toLowerCase() = "true" + ) + } + } + + // /** + // * A read from `net/url` which is sanitized + // */ + class UrlPathSanitizer extends Sanitizer { + UrlPathSanitizer() { + exists(DataFlow::Field fld | + this = fld.getARead() and + fld.hasQualifiedName("net/url", "URL", "Path") + ) + } + } + /** * A read from the field `Filename` of the type `mime/multipart.FileHeader`, * considered as a sanitizer for path traversal. diff --git a/go/ql/src/change-notes/2024-10-14-gopathsanitizer.md b/go/ql/src/change-notes/2024-10-14-gopathsanitizer.md new file mode 100644 index 000000000000..93371d9f229d --- /dev/null +++ b/go/ql/src/change-notes/2024-10-14-gopathsanitizer.md @@ -0,0 +1,4 @@ +--- +category: minorAnalysis +--- +* Added [github.com/gorilla/mux.Vars](https://pkg.go.dev/github.com/gorilla/mux#Vars) to path sanitizers. \ No newline at end of file diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected index 839d35f663ce..950ac856352b 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected @@ -1,25 +1,25 @@ #select -| 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 | +| 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 | edges -| TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:14:18:14:30 | call to Query | provenance | Src:MaD:2 MaD:3 | -| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:17:29:17:40 | tainted_path | provenance | Sink:MaD:1 | -| 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 | FunctionModel Sink:MaD:1 | -| TaintedPath.go:68:39:68:56 | ...+... | TaintedPath.go:68:28:68:57 | call to Clean | provenance | MaD:4 Sink:MaD:1 | +| TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:16:18:16:30 | call to Query | provenance | Src:MaD:2 MaD:3 | +| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:19:29:19:40 | tainted_path | provenance | Sink:MaD:1 | +| 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 | FunctionModel Sink:MaD:1 | +| TaintedPath.go:70:39:70:56 | ...+... | TaintedPath.go:70:28:70:57 | call to Clean | provenance | MaD:4 Sink:MaD:1 | models | 1 | Sink: io/ioutil; ; false; ReadFile; ; ; Argument[0]; path-injection; manual | | 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual | | 3 | Summary: net/url; URL; true; Query; ; ; Argument[receiver]; ReturnValue; taint; manual | | 4 | Summary: path; ; false; Clean; ; ; Argument[0]; ReturnValue; taint; manual | nodes -| 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 | ...+... | +| 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 diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go index e6a1c49f4c5b..b57e293a8243 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go @@ -8,6 +8,8 @@ import ( "path/filepath" "regexp" "strings" + + "github.com/gorilla/mux" ) func handler(w http.ResponseWriter, r *http.Request) { @@ -94,3 +96,10 @@ func handler(w http.ResponseWriter, r *http.Request) { data, _ = ioutil.ReadFile(part.FileName()) } + +// GOOD: Sanitized by Gorilla's cleaner +func GorillaHandler(w http.ResponseWriter, r *http.Request) { + not_tainted_path := mux.Vars(r) + data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path)) + w.Write(data) +} diff --git a/go/ql/test/query-tests/Security/CWE-022/go.mod b/go/ql/test/query-tests/Security/CWE-022/go.mod new file mode 100644 index 000000000000..c173488c7c74 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/go.mod @@ -0,0 +1,5 @@ +module codeql-go-tests/frameworks/Mux + +go 1.14 + +require github.com/gorilla/mux v1.7.4 diff --git a/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/LICENSE b/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/LICENSE new file mode 100644 index 000000000000..6903df6386e9 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/LICENSE @@ -0,0 +1,27 @@ +Copyright (c) 2012-2018 The Gorilla Authors. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/stub.go b/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/stub.go new file mode 100644 index 000000000000..62510300b2df --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/stub.go @@ -0,0 +1,252 @@ +// Code generated by depstubber. DO NOT EDIT. +// This is a simple stub for github.com/gorilla/mux, strictly for use in testing. + +// See the LICENSE file for information about the licensing of the original library. +// Source: github.com/gorilla/mux (exports: ; functions: Vars,NewRouter) + +// Package mux is a stub of github.com/gorilla/mux, generated by depstubber. +package mux + +import ( + http "net/http" + url "net/url" +) + +type BuildVarsFunc func(map[string]string) map[string]string + +type MatcherFunc func(*http.Request, *RouteMatch) bool + +func (_ MatcherFunc) Match(_ *http.Request, _ *RouteMatch) bool { + return false +} + +type MiddlewareFunc func(http.Handler) http.Handler + +func (_ MiddlewareFunc) Middleware(_ http.Handler) http.Handler { + return nil +} + +func NewRouter() *Router { + return nil +} + +type Route struct{} + +func (_ *Route) BuildOnly() *Route { + return nil +} + +func (_ *Route) BuildVarsFunc(_ BuildVarsFunc) *Route { + return nil +} + +func (_ *Route) GetError() error { + return nil +} + +func (_ *Route) GetHandler() http.Handler { + return nil +} + +func (_ *Route) GetHostTemplate() (string, error) { + return "", nil +} + +func (_ *Route) GetMethods() ([]string, error) { + return nil, nil +} + +func (_ *Route) GetName() string { + return "" +} + +func (_ *Route) GetPathRegexp() (string, error) { + return "", nil +} + +func (_ *Route) GetPathTemplate() (string, error) { + return "", nil +} + +func (_ *Route) GetQueriesRegexp() ([]string, error) { + return nil, nil +} + +func (_ *Route) GetQueriesTemplates() ([]string, error) { + return nil, nil +} + +func (_ *Route) Handler(_ http.Handler) *Route { + return nil +} + +func (_ *Route) HandlerFunc(_ func(http.ResponseWriter, *http.Request)) *Route { + return nil +} + +func (_ *Route) Headers(_ ...string) *Route { + return nil +} + +func (_ *Route) HeadersRegexp(_ ...string) *Route { + return nil +} + +func (_ *Route) Host(_ string) *Route { + return nil +} + +func (_ *Route) Match(_ *http.Request, _ *RouteMatch) bool { + return false +} + +func (_ *Route) MatcherFunc(_ MatcherFunc) *Route { + return nil +} + +func (_ *Route) Methods(_ ...string) *Route { + return nil +} + +func (_ *Route) Name(_ string) *Route { + return nil +} + +func (_ *Route) Path(_ string) *Route { + return nil +} + +func (_ *Route) PathPrefix(_ string) *Route { + return nil +} + +func (_ *Route) Queries(_ ...string) *Route { + return nil +} + +func (_ *Route) Schemes(_ ...string) *Route { + return nil +} + +func (_ *Route) SkipClean() bool { + return false +} + +func (_ *Route) Subrouter() *Router { + return nil +} + +func (_ *Route) URL(_ ...string) (*url.URL, error) { + return nil, nil +} + +func (_ *Route) URLHost(_ ...string) (*url.URL, error) { + return nil, nil +} + +func (_ *Route) URLPath(_ ...string) (*url.URL, error) { + return nil, nil +} + +type RouteMatch struct { + Route *Route + Handler http.Handler + Vars map[string]string + MatchErr error +} + +type Router struct { + NotFoundHandler http.Handler + MethodNotAllowedHandler http.Handler + KeepContext bool +} + +func (_ *Router) BuildVarsFunc(_ BuildVarsFunc) *Route { + return nil +} + +func (_ *Router) Get(_ string) *Route { + return nil +} + +func (_ *Router) GetRoute(_ string) *Route { + return nil +} + +func (_ *Router) Handle(_ string, _ http.Handler) *Route { + return nil +} + +func (_ *Router) HandleFunc(_ string, _ func(http.ResponseWriter, *http.Request)) *Route { + return nil +} + +func (_ *Router) Headers(_ ...string) *Route { + return nil +} + +func (_ *Router) Host(_ string) *Route { + return nil +} + +func (_ *Router) Match(_ *http.Request, _ *RouteMatch) bool { + return false +} + +func (_ *Router) MatcherFunc(_ MatcherFunc) *Route { + return nil +} + +func (_ *Router) Methods(_ ...string) *Route { + return nil +} + +func (_ *Router) Name(_ string) *Route { + return nil +} + +func (_ *Router) NewRoute() *Route { + return nil +} + +func (_ *Router) Path(_ string) *Route { + return nil +} + +func (_ *Router) PathPrefix(_ string) *Route { + return nil +} + +func (_ *Router) Queries(_ ...string) *Route { + return nil +} + +func (_ *Router) Schemes(_ ...string) *Route { + return nil +} + +func (_ *Router) ServeHTTP(_ http.ResponseWriter, _ *http.Request) {} + +func (_ *Router) SkipClean(_ bool) *Router { + return nil +} + +func (_ *Router) StrictSlash(_ bool) *Router { + return nil +} + +func (_ *Router) Use(_ ...MiddlewareFunc) {} + +func (_ *Router) UseEncodedPath() *Router { + return nil +} + +func (_ *Router) Walk(_ WalkFunc) error { + return nil +} + +func Vars(_ *http.Request) map[string]string { + return nil +} + +type WalkFunc func(*Route, *Router, []*Route) error diff --git a/go/ql/test/query-tests/Security/CWE-022/vendor/modules.txt b/go/ql/test/query-tests/Security/CWE-022/vendor/modules.txt new file mode 100644 index 000000000000..d96be1fa71b2 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/vendor/modules.txt @@ -0,0 +1,3 @@ +# github.com/gorilla/mux v1.7.4 +## explicit +github.com/gorilla/mux From 1287f1befc2a942458ebd329de02277ed87067d1 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 15 Oct 2024 14:01:14 -0700 Subject: [PATCH 2/6] Address feedback --- .../go/security/TaintedPathCustomizations.qll | 20 +++++++++---------- .../Security/CWE-022/TaintedPath.go | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll index f75a16ceccf8..f505df8b34bf 100644 --- a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll +++ b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll @@ -93,26 +93,26 @@ module TaintedPath { } } - // /** - // * A call to `mux.Vars(path)`, considered to sanitize `path` against path traversal. - // * Only enabled when `SkipClean` is not set true. - // */ + /** + * A call to `mux.Vars(path)`, considered to sanitize `path` against path traversal. + * Only enabled when `SkipClean` is not set true. + */ class MuxVarsSanitizer extends Sanitizer { MuxVarsSanitizer() { exists(Function m | - m.hasQualifiedName("github.com/gorilla/mux", "Vars") and + m.hasQualifiedName(package("github.com/gorilla/mux", ""), "Vars") and this = m.getACall().getResult() ) and not exists(CallExpr f | - f.getTarget().hasQualifiedName("github.com/gorilla/mux", "SkipClean") and - f.getArgument(0).toString().toLowerCase() = "true" + f.getTarget().hasQualifiedName(package("github.com/gorilla/mux", ""), "SkipClean") and + f.getArgument(0).getBoolValue() = true ) } } - // /** - // * A read from `net/url` which is sanitized - // */ + /** + * A read from the field `Path` of the type `net/url.URL`, which is sanitized. + */ class UrlPathSanitizer extends Sanitizer { UrlPathSanitizer() { exists(DataFlow::Field fld | diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go index b57e293a8243..3fed218b7000 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go @@ -99,7 +99,7 @@ func handler(w http.ResponseWriter, r *http.Request) { // GOOD: Sanitized by Gorilla's cleaner func GorillaHandler(w http.ResponseWriter, r *http.Request) { - not_tainted_path := mux.Vars(r) + not_tainted_path := mux.Vars(r)["id"] data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path)) w.Write(data) } From 374b13e1bb3eea75ef7ec3ed87d46f1499256148 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 15 Oct 2024 14:34:11 -0700 Subject: [PATCH 3/6] Remove path sanitizer --- .../semmle/go/security/TaintedPathCustomizations.qll | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll index f505df8b34bf..3459f414bb25 100644 --- a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll +++ b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll @@ -110,18 +110,6 @@ module TaintedPath { } } - /** - * A read from the field `Path` of the type `net/url.URL`, which is sanitized. - */ - class UrlPathSanitizer extends Sanitizer { - UrlPathSanitizer() { - exists(DataFlow::Field fld | - this = fld.getARead() and - fld.hasQualifiedName("net/url", "URL", "Path") - ) - } - } - /** * A read from the field `Filename` of the type `mime/multipart.FileHeader`, * considered as a sanitizer for path traversal. From 8744f158bd5a8dbed353a6d45b82563d11365efb Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 12 Nov 2024 15:44:47 -0800 Subject: [PATCH 4/6] New tests --- .../go/security/TaintedPathCustomizations.qll | 4 +- .../CWE-022/GorillaMuxDefault/MuxClean.go | 22 ++ .../GorillaMuxDefault/TaintedPath.expected | 4 + .../GorillaMuxDefault/TaintedPath.qlref | 2 + .../CWE-022/{ => GorillaMuxDefault}/go.mod | 0 .../vendor/github.com/gorilla/mux/LICENSE | 0 .../vendor/github.com/gorilla/mux/stub.go | 0 .../vendor/modules.txt | 0 .../CWE-022/GorillaMuxSkipClean/MuxClean.go | 22 ++ .../GorillaMuxSkipClean/TaintedPath.expected | 10 + .../GorillaMuxSkipClean/TaintedPath.qlref | 2 + .../CWE-022/GorillaMuxSkipClean/go.mod | 5 + .../vendor/github.com/gorilla/mux/LICENSE | 27 ++ .../vendor/github.com/gorilla/mux/stub.go | 252 ++++++++++++++++++ .../GorillaMuxSkipClean/vendor/modules.txt | 3 + .../Security/CWE-022/TaintedPath.expected | 25 -- .../Security/CWE-022/TaintedPath.go | 9 - 17 files changed, 352 insertions(+), 35 deletions(-) create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.expected create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.qlref rename go/ql/test/query-tests/Security/CWE-022/{ => GorillaMuxDefault}/go.mod (100%) rename go/ql/test/query-tests/Security/CWE-022/{ => GorillaMuxDefault}/vendor/github.com/gorilla/mux/LICENSE (100%) rename go/ql/test/query-tests/Security/CWE-022/{ => GorillaMuxDefault}/vendor/github.com/gorilla/mux/stub.go (100%) rename go/ql/test/query-tests/Security/CWE-022/{ => GorillaMuxDefault}/vendor/modules.txt (100%) create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.qlref create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/go.mod create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/LICENSE create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/stub.go create mode 100644 go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/modules.txt delete mode 100644 go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected diff --git a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll index 3459f414bb25..df601ce1eb84 100644 --- a/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll +++ b/go/ql/lib/semmle/go/security/TaintedPathCustomizations.qll @@ -104,7 +104,9 @@ module TaintedPath { this = m.getACall().getResult() ) and not exists(CallExpr f | - f.getTarget().hasQualifiedName(package("github.com/gorilla/mux", ""), "SkipClean") and + f.getTarget() + .(Method) + .hasQualifiedName(package("github.com/gorilla/mux", ""), "Router", "SkipClean") and f.getArgument(0).getBoolValue() = true ) } diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go new file mode 100644 index 000000000000..25e39a1bfbf3 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go @@ -0,0 +1,22 @@ +// GOOD: Sanitized by Gorilla's cleaner +package main + +import ( + "io/ioutil" + "net/http" + "path/filepath" + + "github.com/gorilla/mux" +) + +func GorillaHandler(w http.ResponseWriter, r *http.Request) { + not_tainted_path := mux.Vars(r)["id"] + data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path)) + w.Write(data) +} + +func main() { + var router = mux.NewRouter() + router.SkipClean(false) + router.HandleFunc("/{category}", GorillaHandler) +} diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.expected new file mode 100644 index 000000000000..e217064d1dfc --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.expected @@ -0,0 +1,4 @@ +edges +nodes +subpaths +#select diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.qlref b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.qlref new file mode 100644 index 000000000000..6de14eaee24d --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/TaintedPath.qlref @@ -0,0 +1,2 @@ +query: Security/CWE-022/TaintedPath.ql +postprocess: TestUtilities/PrettyPrintModels.ql \ No newline at end of file diff --git a/go/ql/test/query-tests/Security/CWE-022/go.mod b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/go.mod similarity index 100% rename from go/ql/test/query-tests/Security/CWE-022/go.mod rename to go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/go.mod diff --git a/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/LICENSE b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/vendor/github.com/gorilla/mux/LICENSE similarity index 100% rename from go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/LICENSE rename to go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/vendor/github.com/gorilla/mux/LICENSE diff --git a/go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/stub.go b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/vendor/github.com/gorilla/mux/stub.go similarity index 100% rename from go/ql/test/query-tests/Security/CWE-022/vendor/github.com/gorilla/mux/stub.go rename to go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/vendor/github.com/gorilla/mux/stub.go diff --git a/go/ql/test/query-tests/Security/CWE-022/vendor/modules.txt b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/vendor/modules.txt similarity index 100% rename from go/ql/test/query-tests/Security/CWE-022/vendor/modules.txt rename to go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/vendor/modules.txt diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go new file mode 100644 index 000000000000..aafc93c75eab --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go @@ -0,0 +1,22 @@ +// GOOD: Sanitized by Gorilla's cleaner +package main + +import ( + "io/ioutil" + "net/http" + "path/filepath" + + "github.com/gorilla/mux" +) + +func GorillaHandler(w http.ResponseWriter, r *http.Request) { + not_tainted_path := mux.Vars(r)["id"] + data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path)) + w.Write(data) +} + +func main() { + var router = mux.NewRouter() + router.SkipClean(true) + router.HandleFunc("/{category}", GorillaHandler) +} diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected new file mode 100644 index 000000000000..f07dfaec8ccd --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected @@ -0,0 +1,10 @@ +edges +| MuxClean.go:13:22:13:32 | call to Vars | MuxClean.go:14:58:14:73 | not_tainted_path | provenance | Src:MaD:524 | +| MuxClean.go:14:58:14:73 | not_tainted_path | MuxClean.go:14:29:14:74 | call to Join | provenance | FunctionModel Sink:MaD:854 | +nodes +| MuxClean.go:13:22:13:32 | call to Vars | semmle.label | call to Vars | +| MuxClean.go:14:29:14:74 | call to Join | semmle.label | call to Join | +| MuxClean.go:14:58:14:73 | not_tainted_path | semmle.label | not_tainted_path | +subpaths +#select +| MuxClean.go:14:29:14:74 | call to Join | MuxClean.go:13:22:13:32 | call to Vars | MuxClean.go:14:29:14:74 | call to Join | This path depends on a $@. | MuxClean.go:13:22:13:32 | call to Vars | user-provided value | diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.qlref b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.qlref new file mode 100644 index 000000000000..6de14eaee24d --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.qlref @@ -0,0 +1,2 @@ +query: Security/CWE-022/TaintedPath.ql +postprocess: TestUtilities/PrettyPrintModels.ql \ No newline at end of file diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/go.mod b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/go.mod new file mode 100644 index 000000000000..c173488c7c74 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/go.mod @@ -0,0 +1,5 @@ +module codeql-go-tests/frameworks/Mux + +go 1.14 + +require github.com/gorilla/mux v1.7.4 diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/LICENSE b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/LICENSE new file mode 100644 index 000000000000..6903df6386e9 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/LICENSE @@ -0,0 +1,27 @@ +Copyright (c) 2012-2018 The Gorilla Authors. All rights reserved. + +Redistribution and use in source and binary forms, with or without +modification, are permitted provided that the following conditions are +met: + + * Redistributions of source code must retain the above copyright +notice, this list of conditions and the following disclaimer. + * Redistributions in binary form must reproduce the above +copyright notice, this list of conditions and the following disclaimer +in the documentation and/or other materials provided with the +distribution. + * Neither the name of Google Inc. nor the names of its +contributors may be used to endorse or promote products derived from +this software without specific prior written permission. + +THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +"AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +(INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/stub.go b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/stub.go new file mode 100644 index 000000000000..62510300b2df --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/github.com/gorilla/mux/stub.go @@ -0,0 +1,252 @@ +// Code generated by depstubber. DO NOT EDIT. +// This is a simple stub for github.com/gorilla/mux, strictly for use in testing. + +// See the LICENSE file for information about the licensing of the original library. +// Source: github.com/gorilla/mux (exports: ; functions: Vars,NewRouter) + +// Package mux is a stub of github.com/gorilla/mux, generated by depstubber. +package mux + +import ( + http "net/http" + url "net/url" +) + +type BuildVarsFunc func(map[string]string) map[string]string + +type MatcherFunc func(*http.Request, *RouteMatch) bool + +func (_ MatcherFunc) Match(_ *http.Request, _ *RouteMatch) bool { + return false +} + +type MiddlewareFunc func(http.Handler) http.Handler + +func (_ MiddlewareFunc) Middleware(_ http.Handler) http.Handler { + return nil +} + +func NewRouter() *Router { + return nil +} + +type Route struct{} + +func (_ *Route) BuildOnly() *Route { + return nil +} + +func (_ *Route) BuildVarsFunc(_ BuildVarsFunc) *Route { + return nil +} + +func (_ *Route) GetError() error { + return nil +} + +func (_ *Route) GetHandler() http.Handler { + return nil +} + +func (_ *Route) GetHostTemplate() (string, error) { + return "", nil +} + +func (_ *Route) GetMethods() ([]string, error) { + return nil, nil +} + +func (_ *Route) GetName() string { + return "" +} + +func (_ *Route) GetPathRegexp() (string, error) { + return "", nil +} + +func (_ *Route) GetPathTemplate() (string, error) { + return "", nil +} + +func (_ *Route) GetQueriesRegexp() ([]string, error) { + return nil, nil +} + +func (_ *Route) GetQueriesTemplates() ([]string, error) { + return nil, nil +} + +func (_ *Route) Handler(_ http.Handler) *Route { + return nil +} + +func (_ *Route) HandlerFunc(_ func(http.ResponseWriter, *http.Request)) *Route { + return nil +} + +func (_ *Route) Headers(_ ...string) *Route { + return nil +} + +func (_ *Route) HeadersRegexp(_ ...string) *Route { + return nil +} + +func (_ *Route) Host(_ string) *Route { + return nil +} + +func (_ *Route) Match(_ *http.Request, _ *RouteMatch) bool { + return false +} + +func (_ *Route) MatcherFunc(_ MatcherFunc) *Route { + return nil +} + +func (_ *Route) Methods(_ ...string) *Route { + return nil +} + +func (_ *Route) Name(_ string) *Route { + return nil +} + +func (_ *Route) Path(_ string) *Route { + return nil +} + +func (_ *Route) PathPrefix(_ string) *Route { + return nil +} + +func (_ *Route) Queries(_ ...string) *Route { + return nil +} + +func (_ *Route) Schemes(_ ...string) *Route { + return nil +} + +func (_ *Route) SkipClean() bool { + return false +} + +func (_ *Route) Subrouter() *Router { + return nil +} + +func (_ *Route) URL(_ ...string) (*url.URL, error) { + return nil, nil +} + +func (_ *Route) URLHost(_ ...string) (*url.URL, error) { + return nil, nil +} + +func (_ *Route) URLPath(_ ...string) (*url.URL, error) { + return nil, nil +} + +type RouteMatch struct { + Route *Route + Handler http.Handler + Vars map[string]string + MatchErr error +} + +type Router struct { + NotFoundHandler http.Handler + MethodNotAllowedHandler http.Handler + KeepContext bool +} + +func (_ *Router) BuildVarsFunc(_ BuildVarsFunc) *Route { + return nil +} + +func (_ *Router) Get(_ string) *Route { + return nil +} + +func (_ *Router) GetRoute(_ string) *Route { + return nil +} + +func (_ *Router) Handle(_ string, _ http.Handler) *Route { + return nil +} + +func (_ *Router) HandleFunc(_ string, _ func(http.ResponseWriter, *http.Request)) *Route { + return nil +} + +func (_ *Router) Headers(_ ...string) *Route { + return nil +} + +func (_ *Router) Host(_ string) *Route { + return nil +} + +func (_ *Router) Match(_ *http.Request, _ *RouteMatch) bool { + return false +} + +func (_ *Router) MatcherFunc(_ MatcherFunc) *Route { + return nil +} + +func (_ *Router) Methods(_ ...string) *Route { + return nil +} + +func (_ *Router) Name(_ string) *Route { + return nil +} + +func (_ *Router) NewRoute() *Route { + return nil +} + +func (_ *Router) Path(_ string) *Route { + return nil +} + +func (_ *Router) PathPrefix(_ string) *Route { + return nil +} + +func (_ *Router) Queries(_ ...string) *Route { + return nil +} + +func (_ *Router) Schemes(_ ...string) *Route { + return nil +} + +func (_ *Router) ServeHTTP(_ http.ResponseWriter, _ *http.Request) {} + +func (_ *Router) SkipClean(_ bool) *Router { + return nil +} + +func (_ *Router) StrictSlash(_ bool) *Router { + return nil +} + +func (_ *Router) Use(_ ...MiddlewareFunc) {} + +func (_ *Router) UseEncodedPath() *Router { + return nil +} + +func (_ *Router) Walk(_ WalkFunc) error { + return nil +} + +func Vars(_ *http.Request) map[string]string { + return nil +} + +type WalkFunc func(*Route, *Router, []*Route) error diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/modules.txt b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/modules.txt new file mode 100644 index 000000000000..d96be1fa71b2 --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/vendor/modules.txt @@ -0,0 +1,3 @@ +# github.com/gorilla/mux v1.7.4 +## explicit +github.com/gorilla/mux diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected deleted file mode 100644 index 950ac856352b..000000000000 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected +++ /dev/null @@ -1,25 +0,0 @@ -#select -| 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 | -edges -| TaintedPath.go:16:18:16:22 | selection of URL | TaintedPath.go:16:18:16:30 | call to Query | provenance | Src:MaD:2 MaD:3 | -| TaintedPath.go:16:18:16:30 | call to Query | TaintedPath.go:19:29:19:40 | tainted_path | provenance | Sink:MaD:1 | -| 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 | FunctionModel Sink:MaD:1 | -| TaintedPath.go:70:39:70:56 | ...+... | TaintedPath.go:70:28:70:57 | call to Clean | provenance | MaD:4 Sink:MaD:1 | -models -| 1 | Sink: io/ioutil; ; false; ReadFile; ; ; Argument[0]; path-injection; manual | -| 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual | -| 3 | Summary: net/url; URL; true; Query; ; ; Argument[receiver]; ReturnValue; taint; manual | -| 4 | Summary: path; ; false; Clean; ; ; Argument[0]; ReturnValue; taint; manual | -nodes -| 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 diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go index 3fed218b7000..e6a1c49f4c5b 100644 --- a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.go @@ -8,8 +8,6 @@ import ( "path/filepath" "regexp" "strings" - - "github.com/gorilla/mux" ) func handler(w http.ResponseWriter, r *http.Request) { @@ -96,10 +94,3 @@ func handler(w http.ResponseWriter, r *http.Request) { data, _ = ioutil.ReadFile(part.FileName()) } - -// GOOD: Sanitized by Gorilla's cleaner -func GorillaHandler(w http.ResponseWriter, r *http.Request) { - not_tainted_path := mux.Vars(r)["id"] - data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path)) - w.Write(data) -} From 460ed30d05d0c20199c10d04fcf816dab1bfdc75 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Tue, 12 Nov 2024 16:08:14 -0800 Subject: [PATCH 5/6] Fixed tests --- .../GorillaMuxSkipClean/TaintedPath.expected | 11 +++++--- .../Security/CWE-022/TaintedPath.expected | 25 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) create mode 100644 go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected index f07dfaec8ccd..887b9858ef36 100644 --- a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/TaintedPath.expected @@ -1,10 +1,13 @@ +#select +| MuxClean.go:14:29:14:74 | call to Join | MuxClean.go:13:22:13:32 | call to Vars | MuxClean.go:14:29:14:74 | call to Join | This path depends on a $@. | MuxClean.go:13:22:13:32 | call to Vars | user-provided value | edges -| MuxClean.go:13:22:13:32 | call to Vars | MuxClean.go:14:58:14:73 | not_tainted_path | provenance | Src:MaD:524 | -| MuxClean.go:14:58:14:73 | not_tainted_path | MuxClean.go:14:29:14:74 | call to Join | provenance | FunctionModel Sink:MaD:854 | +| MuxClean.go:13:22:13:32 | call to Vars | MuxClean.go:14:58:14:73 | not_tainted_path | provenance | Src:MaD:2 | +| MuxClean.go:14:58:14:73 | not_tainted_path | MuxClean.go:14:29:14:74 | call to Join | provenance | FunctionModel Sink:MaD:1 | +models +| 1 | Sink: io/ioutil; ; false; ReadFile; ; ; Argument[0]; path-injection; manual | +| 2 | Source: github.com/gorilla/mux; ; true; Vars; ; ; ReturnValue; remote; manual | nodes | MuxClean.go:13:22:13:32 | call to Vars | semmle.label | call to Vars | | MuxClean.go:14:29:14:74 | call to Join | semmle.label | call to Join | | MuxClean.go:14:58:14:73 | not_tainted_path | semmle.label | not_tainted_path | subpaths -#select -| MuxClean.go:14:29:14:74 | call to Join | MuxClean.go:13:22:13:32 | call to Vars | MuxClean.go:14:29:14:74 | call to Join | This path depends on a $@. | MuxClean.go:13:22:13:32 | call to Vars | user-provided value | diff --git a/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected new file mode 100644 index 000000000000..839d35f663ce --- /dev/null +++ b/go/ql/test/query-tests/Security/CWE-022/TaintedPath.expected @@ -0,0 +1,25 @@ +#select +| 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 | +edges +| TaintedPath.go:14:18:14:22 | selection of URL | TaintedPath.go:14:18:14:30 | call to Query | provenance | Src:MaD:2 MaD:3 | +| TaintedPath.go:14:18:14:30 | call to Query | TaintedPath.go:17:29:17:40 | tainted_path | provenance | Sink:MaD:1 | +| 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 | FunctionModel Sink:MaD:1 | +| TaintedPath.go:68:39:68:56 | ...+... | TaintedPath.go:68:28:68:57 | call to Clean | provenance | MaD:4 Sink:MaD:1 | +models +| 1 | Sink: io/ioutil; ; false; ReadFile; ; ; Argument[0]; path-injection; manual | +| 2 | Source: net/http; Request; true; URL; ; ; ; remote; manual | +| 3 | Summary: net/url; URL; true; Query; ; ; Argument[receiver]; ReturnValue; taint; manual | +| 4 | Summary: path; ; false; Clean; ; ; Argument[0]; ReturnValue; taint; manual | +nodes +| 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 From a94ba25ebe51e1acbd2b2a7042f87f0965a498f9 Mon Sep 17 00:00:00 2001 From: Kevin Stubbings Date: Wed, 13 Nov 2024 14:45:45 -0800 Subject: [PATCH 6/6] Apply suggestions from code review Co-authored-by: Owen Mansel-Chan <62447351+owen-mc@users.noreply.github.com> --- go/ql/src/change-notes/2024-10-14-gopathsanitizer.md | 2 +- .../query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go | 2 +- .../Security/CWE-022/GorillaMuxSkipClean/MuxClean.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go/ql/src/change-notes/2024-10-14-gopathsanitizer.md b/go/ql/src/change-notes/2024-10-14-gopathsanitizer.md index 93371d9f229d..e1577bf3a90f 100644 --- a/go/ql/src/change-notes/2024-10-14-gopathsanitizer.md +++ b/go/ql/src/change-notes/2024-10-14-gopathsanitizer.md @@ -1,4 +1,4 @@ --- category: minorAnalysis --- -* Added [github.com/gorilla/mux.Vars](https://pkg.go.dev/github.com/gorilla/mux#Vars) to path sanitizers. \ No newline at end of file +* Added [github.com/gorilla/mux.Vars](https://pkg.go.dev/github.com/gorilla/mux#Vars) to path sanitizers (disabled if [github.com/gorilla/mix.Router.SkipClean](https://pkg.go.dev/github.com/gorilla/mux#Router.SkipClean) has been called). \ No newline at end of file diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go index 25e39a1bfbf3..a5af6de55803 100644 --- a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxDefault/MuxClean.go @@ -1,4 +1,3 @@ -// GOOD: Sanitized by Gorilla's cleaner package main import ( @@ -9,6 +8,7 @@ import ( "github.com/gorilla/mux" ) +// GOOD: Sanitized by Gorilla's cleaner func GorillaHandler(w http.ResponseWriter, r *http.Request) { not_tainted_path := mux.Vars(r)["id"] data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path)) diff --git a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go index aafc93c75eab..cb3b5d2a7b89 100644 --- a/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go +++ b/go/ql/test/query-tests/Security/CWE-022/GorillaMuxSkipClean/MuxClean.go @@ -1,4 +1,3 @@ -// GOOD: Sanitized by Gorilla's cleaner package main import ( @@ -9,6 +8,7 @@ import ( "github.com/gorilla/mux" ) +// BAD: Gorilla's `Vars` is not a sanitizer as `Router.SkipClean` has been called func GorillaHandler(w http.ResponseWriter, r *http.Request) { not_tainted_path := mux.Vars(r)["id"] data, _ := ioutil.ReadFile(filepath.Join("/home/user/", not_tainted_path))