From f928e4cc874ab6bfb0a03c275e72a0de4c2cd01c Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 16 Jan 2025 16:33:02 +0000 Subject: [PATCH 1/3] Improve tests --- go/ql/test/experimental/CWE-942/CorsGin.go | 103 +++++++++++++++--- .../CWE-942/CorsMisconfiguration.expected | 8 +- .../CWE-942/CorsMisconfiguration.go | 10 +- .../CWE-942/CorsMisconfiguration.qlref | 3 +- go/ql/test/experimental/CWE-942/RsCors.go | 60 +++++++++- 5 files changed, 160 insertions(+), 24 deletions(-) diff --git a/go/ql/test/experimental/CWE-942/CorsGin.go b/go/ql/test/experimental/CWE-942/CorsGin.go index 9ba6da6d0629..dff61f2c5c26 100644 --- a/go/ql/test/experimental/CWE-942/CorsGin.go +++ b/go/ql/test/experimental/CWE-942/CorsGin.go @@ -8,10 +8,8 @@ import ( "github.com/gin-gonic/gin" ) -/* -** Function is vulnerable due to AllowAllOrigins = true aka Access-Control-Allow-Origin: null - */ -func vunlnerable() { +// Function is vulnerable due to AllowAllOrigins = true aka Access-Control-Allow-Origin: null +func vulnerable1() { router := gin.Default() // CORS for https://foo.com and null // - PUT and PATCH methods @@ -25,7 +23,7 @@ func vunlnerable() { AllowCredentials: true, MaxAge: 12 * time.Hour, } - config_vulnerable.AllowOrigins = []string{"null", "https://foo.com"} + config_vulnerable.AllowOrigins = []string{"null", "https://foo.com"} // $ Alert router.Use(cors.New(config_vulnerable)) router.GET("/", func(c *gin.Context) { c.String(http.StatusOK, "hello world") @@ -33,9 +31,30 @@ func vunlnerable() { router.Run() } -/* -** Function is safe due to hardcoded origin and AllowCredentials: true - */ +// Function is vulnerable due to AllowAllOrigins = true aka Access-Control-Allow-Origin: null +func vulnerable2() { + router := gin.Default() + // CORS for https://foo.com and null + // - PUT and PATCH methods + // - Origin header + // - Credentials share + // - Preflight requests cached for 12 hours + config_vulnerable := cors.Config{ + AllowMethods: []string{"PUT", "PATCH"}, + AllowHeaders: []string{"Origin"}, + ExposeHeaders: []string{"Content-Length"}, + AllowCredentials: true, + AllowOrigins: []string{"null", "https://foo.com"}, // $ Alert + MaxAge: 12 * time.Hour, + } + router.Use(cors.New(config_vulnerable)) + router.GET("/", func(c *gin.Context) { + c.String(http.StatusOK, "hello world") + }) + router.Run() +} + +// Function is safe due to hardcoded origin and AllowCredentials: true func safe() { router := gin.Default() // CORS for https://foo.com origin, allowing: @@ -58,10 +77,8 @@ func safe() { router.Run() } -/* -** Function is safe due to AllowAllOrigins = true aka Access-Control-Allow-Origin: * - */ -func AllowAllTrue() { +// Function is safe due to AllowAllOrigins = true aka Access-Control-Allow-Origin: * +func AllowAllTrue1() { router := gin.Default() // CORS for "*" origin, allowing: // - PUT and PATCH methods @@ -84,6 +101,30 @@ func AllowAllTrue() { router.Run() } +// Function is safe due to AllowAllOrigins = true aka Access-Control-Allow-Origin: * +func AllowAllTrue2() { + router := gin.Default() + // CORS for "*" origin, allowing: + // - PUT and PATCH methods + // - Origin header + // - Credentials share + // - Preflight requests cached for 12 hours + config_allowall := cors.Config{ + AllowMethods: []string{"PUT", "PATCH"}, + AllowHeaders: []string{"Origin"}, + ExposeHeaders: []string{"Content-Length"}, + AllowAllOrigins: true, + AllowCredentials: true, + MaxAge: 12 * time.Hour, + } + config_allowall.AllowOrigins = []string{"null"} + router.Use(cors.New(config_allowall)) + router.GET("/", func(c *gin.Context) { + c.String(http.StatusOK, "hello world") + }) + router.Run() +} + func NoVariableVulnerable() { router := gin.Default() // CORS for https://foo.com origin, allowing: @@ -95,7 +136,7 @@ func NoVariableVulnerable() { AllowMethods: []string{"GET", "POST"}, AllowHeaders: []string{"Origin"}, ExposeHeaders: []string{"Content-Length"}, - AllowOrigins: []string{"null", "https://foo.com"}, + AllowOrigins: []string{"null", "https://foo.com"}, // $ Alert AllowCredentials: true, MaxAge: 12 * time.Hour, })) @@ -104,3 +145,39 @@ func NoVariableVulnerable() { }) router.Run() } + +var global_config1 = cors.Config{ + AllowMethods: []string{"PUT", "PATCH"}, + AllowHeaders: []string{"Origin"}, + ExposeHeaders: []string{"Content-Length"}, + AllowCredentials: true, + AllowOrigins: []string{"null", "https://foo.com"}, // $ Alert + MaxAge: 12 * time.Hour, +} + +func vulnerableGlobal1() { + router := gin.Default() + router.Use(cors.New(global_config1)) + router.GET("/", func(c *gin.Context) { + c.String(http.StatusOK, "hello world") + }) + router.Run() +} + +var global_config2 = cors.Config{ + AllowMethods: []string{"PUT", "PATCH"}, + AllowHeaders: []string{"Origin"}, + ExposeHeaders: []string{"Content-Length"}, + AllowCredentials: true, + MaxAge: 12 * time.Hour, +} + +func vulnerableGlobal2() { + router := gin.Default() + global_config2.AllowOrigins = []string{"null", "https://foo.com"} // $ MISSING: Alert + router.Use(cors.New(global_config2)) + router.GET("/", func(c *gin.Context) { + c.String(http.StatusOK, "hello world") + }) + router.Run() +} diff --git a/go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected b/go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected index f542f2d098fb..442324639216 100644 --- a/go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected +++ b/go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected @@ -1,8 +1,12 @@ -| CorsGin.go:28:35:28:69 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | -| CorsGin.go:98:21:98:55 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsGin.go:26:35:26:69 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsGin.go:47:21:47:55 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsGin.go:139:21:139:55 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| CorsGin.go:154:20:154:54 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | | CorsMisconfiguration.go:26:4:26:56 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | | CorsMisconfiguration.go:32:4:32:42 | call to Set | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | | CorsMisconfiguration.go:53:4:53:44 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | | CorsMisconfiguration.go:60:4:60:56 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | | CorsMisconfiguration.go:67:5:67:57 | call to Set | access-control-allow-origin header is set to a user-defined value, and access-control-allow-credentials is set to `true` | | RsCors.go:11:21:11:59 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| RsCors.go:31:23:31:61 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | +| RsCors.go:59:20:59:58 | slice literal | access-control-allow-origin header is set to `null`, and access-control-allow-credentials is set to `true` | diff --git a/go/ql/test/experimental/CWE-942/CorsMisconfiguration.go b/go/ql/test/experimental/CWE-942/CorsMisconfiguration.go index 03847bfe521e..506db53a2621 100644 --- a/go/ql/test/experimental/CWE-942/CorsMisconfiguration.go +++ b/go/ql/test/experimental/CWE-942/CorsMisconfiguration.go @@ -23,13 +23,13 @@ func main() { http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // BAD: 'null' origin is allowed, // and Access-Control-Allow-Credentials is set to 'true'. - w.Header().Set("Access-Control-Allow-Origin", "null") + w.Header().Set("Access-Control-Allow-Origin", "null") // $ Alert w.Header().Set("Access-Control-Allow-Credentials", "true") }) http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // BAD: 'null' origin is allowed, // and `Access-Control-Allow-Credentials` is set to 'true': - w.Header().Set(HeaderAllowOrigin, Null) + w.Header().Set(HeaderAllowOrigin, Null) // $ Alert w.Header().Set("Access-Control-Allow-Credentials", "true") }) http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { @@ -50,21 +50,21 @@ func main() { // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, // and `Access-Control-Allow-Credentials` is set to 'true': origin := req.Header.Get("origin") - w.Header().Set(HeaderAllowOrigin, origin) + w.Header().Set(HeaderAllowOrigin, origin) // $ Alert w.Header().Set("Access-Control-Allow-Credentials", "true") }) http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, // and `Access-Control-Allow-Credentials` is set to 'true': origin := req.Header.Get("origin") - w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Origin", origin) // $ Alert w.Header().Set(HeaderAllowCredentials, "true") }) http.HandleFunc("/", func(w http.ResponseWriter, req *http.Request) { // BAD: the `Access-Control-Allow-Origin` header is set using a user-defined value, // and `Access-Control-Allow-Credentials` is set to 'true': if origin := req.Header.Get("Origin"); origin != "" { - w.Header().Set("Access-Control-Allow-Origin", origin) + w.Header().Set("Access-Control-Allow-Origin", origin) // $ Alert } w.Header().Set(HeaderAllowCredentials, "true") }) diff --git a/go/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref b/go/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref index f23401c1091d..5945b4385e90 100644 --- a/go/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref +++ b/go/ql/test/experimental/CWE-942/CorsMisconfiguration.qlref @@ -1 +1,2 @@ -experimental/CWE-942/CorsMisconfiguration.ql +query: experimental/CWE-942/CorsMisconfiguration.ql +postprocess: utils/test/InlineExpectationsTestQuery.ql diff --git a/go/ql/test/experimental/CWE-942/RsCors.go b/go/ql/test/experimental/CWE-942/RsCors.go index dfe56f97b89f..e2601d8ae937 100644 --- a/go/ql/test/experimental/CWE-942/RsCors.go +++ b/go/ql/test/experimental/CWE-942/RsCors.go @@ -6,9 +6,9 @@ import ( "github.com/rs/cors" ) -func rs_vulnerable() { +func rs_vulnerable1() { c := cors.New(cors.Options{ - AllowedOrigins: []string{"null", "http://foo.com:8080"}, + AllowedOrigins: []string{"null", "http://foo.com:8080"}, // $ Alert AllowCredentials: true, // Enable Debugging for testing, consider disabling in production Debug: true, @@ -22,9 +22,26 @@ func rs_vulnerable() { http.ListenAndServe(":8080", c.Handler(handler)) } +func rs_vulnerable2() { + opt := cors.Options{ + AllowCredentials: true, + // Enable Debugging for testing, consider disabling in production + Debug: true, + } + opt.AllowedOrigins = []string{"null", "http://foo.com:8080"} // $ Alert + c := cors.New(opt) + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte("{\"hello\": \"world\"}")) + }) + + http.ListenAndServe(":8080", c.Handler(handler)) +} + func rs_safe() { c := cors.New(cors.Options{ - AllowedOrigins: []string{"http://foo.com:8080"}, + AllowedOrigins: []string{"http://foo.com:8080"}, // GOOD AllowCredentials: true, // Enable Debugging for testing, consider disabling in production Debug: true, @@ -37,3 +54,40 @@ func rs_safe() { http.ListenAndServe(":8080", c.Handler(handler)) } + +var globalCorsOptions1 = cors.Options{ + AllowedOrigins: []string{"null", "http://foo.com:8080"}, // $ Alert + AllowCredentials: true, + // Enable Debugging for testing, consider disabling in production + Debug: true, +} + +func rs_vulnerable_global1() { + c := cors.New(globalCorsOptions1) + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte("{\"hello\": \"world\"}")) + }) + + http.ListenAndServe(":8080", c.Handler(handler)) + +} + +var globalCorsOptions2 cors.Options + +func rs_vulnerable_global2() { + globalCorsOptions2.AllowedOrigins = []string{"null", "http://foo.com:8080"} // $ MISSING: Alert + globalCorsOptions2.AllowCredentials = true + // Enable Debugging for testing, consider disabling in production + globalCorsOptions2.Debug = true + c := cors.New(globalCorsOptions1) + + handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte("{\"hello\": \"world\"}")) + }) + + http.ListenAndServe(":8080", c.Handler(handler)) + +} From 489a87fbae4a9ece2a27f27660a6b49bf5b6e394 Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Thu, 16 Jan 2025 16:58:33 +0000 Subject: [PATCH 2/3] Small QL improvement --- go/ql/lib/semmle/go/frameworks/GinCors.qll | 2 +- go/ql/lib/semmle/go/frameworks/RsCors.qll | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/go/ql/lib/semmle/go/frameworks/GinCors.qll b/go/ql/lib/semmle/go/frameworks/GinCors.qll index a1b2d8367c69..582ea368073b 100644 --- a/go/ql/lib/semmle/go/frameworks/GinCors.qll +++ b/go/ql/lib/semmle/go/frameworks/GinCors.qll @@ -128,7 +128,7 @@ module GinCors { GinConfig() { this = v.getBaseVariable().getSourceVariable() and - exists(Type t | t.hasQualifiedName(packagePath(), "Config") | v.getType() = t) + v.getType().hasQualifiedName(packagePath(), "Config") } /** diff --git a/go/ql/lib/semmle/go/frameworks/RsCors.qll b/go/ql/lib/semmle/go/frameworks/RsCors.qll index aeeb45312c30..1a625c9dc436 100644 --- a/go/ql/lib/semmle/go/frameworks/RsCors.qll +++ b/go/ql/lib/semmle/go/frameworks/RsCors.qll @@ -173,7 +173,7 @@ module RsCors { RsOptions() { this = v.getBaseVariable().getSourceVariable() and - exists(Type t | t.hasQualifiedName(packagePath(), "Options") | v.getType() = t) + v.getType().hasQualifiedName(packagePath(), "Options") } /** From d472dfe4a337c0575589c2067e647bce5da3c10f Mon Sep 17 00:00:00 2001 From: Owen Mansel-Chan Date: Mon, 20 Jan 2025 21:36:30 +0000 Subject: [PATCH 3/3] Fix QLDocs --- go/ql/lib/semmle/go/frameworks/RsCors.qll | 87 +++++++---------------- 1 file changed, 26 insertions(+), 61 deletions(-) diff --git a/go/ql/lib/semmle/go/frameworks/RsCors.qll b/go/ql/lib/semmle/go/frameworks/RsCors.qll index 1a625c9dc436..b9cee2aa459b 100644 --- a/go/ql/lib/semmle/go/frameworks/RsCors.qll +++ b/go/ql/lib/semmle/go/frameworks/RsCors.qll @@ -1,70 +1,52 @@ -/** - * Provides classes for modeling the `github.com/rs/cors` package. - */ +/** Provides classes for modeling the `github.com/rs/cors` package. */ import go -/** - * An abstract class for modeling the Go CORS handler model origin write. - */ +/** An abstract class for modeling the Go CORS handler model origin write. */ abstract class UniversalOriginWrite extends DataFlow::ExprNode { - /** - * Get config variable holding header values - */ + /** Gets the config variable holding header values. */ abstract DataFlow::Node getBase(); - /** - * Get config variable holding header values - */ + /** Gets the config variable holding header values. */ abstract Variable getConfig(); } /** - * An abstract class for modeling the Go CORS handler model allow all origins write. + * An abstract class for modeling the Go CORS handler model allow all origins + * write. */ abstract class UniversalAllowAllOriginsWrite extends DataFlow::ExprNode { - /** - * Get config variable holding header values - */ + /** Gets the config variable holding header values. */ abstract DataFlow::Node getBase(); - /** - * Get config variable holding header values - */ + /** Gets the config variable holding header values. */ abstract Variable getConfig(); } /** - * An abstract class for modeling the Go CORS handler model allow credentials write. + * An abstract class for modeling the Go CORS handler model allow credentials + * write. */ abstract class UniversalAllowCredentialsWrite extends DataFlow::ExprNode { - /** - * Get config struct holding header values - */ + /** Gets the config struct holding header values. */ abstract DataFlow::Node getBase(); - /** - * Get config variable holding header values - */ + /** Gets the config variable holding header values. */ abstract Variable getConfig(); } -/** - * Provides classes for modeling the `github.com/rs/cors` package. - */ +/** Provides classes for modeling the `github.com/rs/cors` package. */ module RsCors { /** Gets the package name `github.com/gin-gonic/gin`. */ string packagePath() { result = package("github.com/rs/cors", "") } - /** - * A new function create a new rs Handler - */ + /** The `New` function that creates a new rs Handler. */ class New extends Function { New() { exists(Function f | f.hasQualifiedName(packagePath(), "New") | this = f) } } /** - * A write to the value of Access-Control-Allow-Credentials header + * A write to the value of Access-Control-Allow-Credentials header. */ class AllowCredentialsWrite extends UniversalAllowCredentialsWrite { DataFlow::Node base; @@ -77,14 +59,10 @@ module RsCors { ) } - /** - * Get options struct holding header values - */ + /** Gets the options struct holding header values. */ override DataFlow::Node getBase() { result = base } - /** - * Get options variable holding header values - */ + /** Gets the options variable holding header values. */ override RsOptions getConfig() { exists(RsOptions gc | ( @@ -97,9 +75,7 @@ module RsCors { } } - /** - * A write to the value of Access-Control-Allow-Origins header - */ + /** A write to the value of Access-Control-Allow-Origins header. */ class AllowOriginsWrite extends UniversalOriginWrite { DataFlow::Node base; @@ -111,14 +87,10 @@ module RsCors { ) } - /** - * Get options struct holding header values - */ + /** Gets the options struct holding header values. */ override DataFlow::Node getBase() { result = base } - /** - * Get options variable holding header values - */ + /** Gets the options variable holding header values. */ override RsOptions getConfig() { exists(RsOptions gc | ( @@ -132,7 +104,8 @@ module RsCors { } /** - * A write to the value of Access-Control-Allow-Origins of value "*", overriding AllowOrigins + * A write to the value of Access-Control-Allow-Origins of value "*", + * overriding `AllowOrigins`. */ class AllowAllOriginsWrite extends UniversalAllowAllOriginsWrite { DataFlow::Node base; @@ -145,14 +118,10 @@ module RsCors { ) } - /** - * Get options struct holding header values - */ + /** Gets the options struct holding header values. */ override DataFlow::Node getBase() { result = base } - /** - * Get options variable holding header values - */ + /** Gets options variable holding header values. */ override RsOptions getConfig() { exists(RsOptions gc | ( @@ -165,9 +134,7 @@ module RsCors { } } - /** - * A variable of type Options that holds the headers to be set. - */ + /** A variable of type Options that holds the headers to be set. */ class RsOptions extends Variable { SsaWithFields v; @@ -176,9 +143,7 @@ module RsCors { v.getType().hasQualifiedName(packagePath(), "Options") } - /** - * Get variable declaration of Options - */ + /** Gets the SSA variable declaration of Options. */ SsaWithFields getV() { result = v } } }