Merge pull request #18543 from owen-mc/go/misc-improvements-rs-cors

Go: miscellaneous improvements rs cors models
github · Jan 29, 2025 · 168fe4a · 168fe4a
2 parents 54efb0a + d472dfe
commit 168fe4a
Show file tree

Hide file tree

Showing 7 changed files with 188 additions and 87 deletions.
diff --git 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
@@ -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,20 +134,16 @@ 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;
 
     RsOptions() {
       this = v.getBaseVariable().getSourceVariable() and
-      exists(Type t | t.hasQualifiedName(packagePath(), "Options") | v.getType() = t)
+      v.getType().hasQualifiedName(packagePath(), "Options")
     }
 
-    /**
-     * Get variable declaration of Options
-     */
+    /** Gets the SSA variable declaration of Options. */
     SsaWithFields getV() { result = v }
   }
 }
diff --git 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,17 +23,38 @@ 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")
 	})
 	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
@@ -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
@@ -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
@@ -1 +1,2 @@
-experimental/CWE-942/CorsMisconfiguration.ql
+query: experimental/CWE-942/CorsMisconfiguration.ql
+postprocess: utils/test/InlineExpectationsTestQuery.ql