Added support for same struct and added new test

Author: Kwstubbs

go/ql/lib/semmle/go/frameworks/GinCors.qll

@@ -22,75 +22,102 @@ module GinCors {
    * A write to the value of Access-Control-Allow-Credentials header
    */
   class AllowCredentialsWrite extends DataFlow::ExprNode {
-    GinConfig gc;
+    DataFlow::Node base;
 
     AllowCredentialsWrite() {
-      exists(Field f, Write w, DataFlow::Node base |
+      exists(Field f, Write w |
         f.hasQualifiedName(packagePath(), "Config", "AllowCredentials") and
         w.writesField(base, f, this) and
-        this.getType() instanceof BoolType and
-        (
-          gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
-            base.asInstruction() or
-          gc.getV().getAUse() = base
-        )
+        this.getType() instanceof BoolType
       )
     }
 
+    /**
+     * Get config struct holding header values
+     */
+    DataFlow::Node getBase() { result = base }
+
     /**
      * Get config variable holding header values
      */
-    GinConfig getConfig() { result = gc }
+    GinConfig getConfig() {
+      exists(GinConfig gc |
+        (
+          gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
+            base.asInstruction() or
+          gc.getV().getAUse() = base
+        ) and
+        result = gc
+      )
+    }
   }
 
   /**
    * A write to the value of Access-Control-Allow-Origins header
    */
   class AllowOriginsWrite extends DataFlow::ExprNode {
-    GinConfig gc;
+    DataFlow::Node base;
 
     AllowOriginsWrite() {
-      exists(Field f, Write w, DataFlow::Node base |
+      exists(Field f, Write w |
         f.hasQualifiedName(packagePath(), "Config", "AllowOrigins") and
         w.writesField(base, f, this) and
-        this.asExpr() instanceof SliceLit and
-        (
-          gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
-            base.asInstruction() or
-          gc.getV().getAUse() = base
-        )
+        this.asExpr() instanceof SliceLit
       )
     }
 
+    /**
+     * Get config struct holding header values
+     */
+    DataFlow::Node getBase() { result = base }
+
     /**
      * Get config variable holding header values
      */
-    GinConfig getConfig() { result = gc }
+    GinConfig getConfig() {
+      exists(GinConfig gc |
+        (
+          gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
+            base.asInstruction() or
+          gc.getV().getAUse() = base
+        ) and
+        result = gc
+      )
+    }
   }
 
   /**
    * A write to the value of Access-Control-Allow-Origins of value "*", overriding AllowOrigins
    */
   class AllowAllOriginsWrite extends DataFlow::ExprNode {
-    GinConfig gc;
+    DataFlow::Node base;
 
     AllowAllOriginsWrite() {
-      exists(Field f, Write w, DataFlow::Node base |
+      exists(Field f, Write w |
         f.hasQualifiedName(packagePath(), "Config", "AllowAllOrigins") and
         w.writesField(base, f, this) and
-        this.getType() instanceof BoolType and
-        (
-          gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
-            base.asInstruction() or
-          gc.getV().getAUse() = base
-        )
+        this.getType() instanceof BoolType
       )
     }
 
+    /**
+     * Get config struct holding header values
+     */
+    DataFlow::Node getBase() { result = base }
+
     /**
      * Get config variable holding header values
      */
-    GinConfig getConfig() { result = gc }
+    GinConfig getConfig() {
+      exists(GinConfig gc |
+        (
+          gc.getV().getBaseVariable().getDefinition().(SsaExplicitDefinition).getRhs() =
+            base.asInstruction() or
+          gc.getV().getAUse() = base
+        ) and
+        result = gc
+      )
+    }
   }
 
   /**

go/ql/src/experimental/CWE-942/CorsMisconfiguration.ql

@@ -105,12 +105,17 @@ predicate allowCredentialsIsSetToTrue(DataFlow::ExprNode allowOriginHW) {
   exists(GinCors::AllowCredentialsWrite allowCredentialsGin |
     allowCredentialsGin.getExpr().getBoolValue() = true
   |
-    //flow only goes in one direction so fix this before PR
     allowCredentialsGin.getConfig() = allowOriginHW.(GinCors::AllowOriginsWrite).getConfig() and
     not exists(GinCors::AllowAllOriginsWrite allowAllOrigins |
       allowAllOrigins.getExpr().getBoolValue() = true and
       allowCredentialsGin.getConfig() = allowAllOrigins.getConfig()
     )
+    or
+    allowCredentialsGin.getBase() = allowOriginHW.(GinCors::AllowOriginsWrite).getBase() and
+    not exists(GinCors::AllowAllOriginsWrite allowAllOrigins |
+      allowAllOrigins.getExpr().getBoolValue() = true and
+      allowCredentialsGin.getBase() = allowAllOrigins.getBase()
+    )
   )
 }
 

go/ql/test/experimental/CWE-942/CorsGin.go

@@ -83,3 +83,24 @@ func AllowAllTrue() {
 	})
 	router.Run()
 }
+
+func NoVariableVulnerable() {
+	router := gin.Default()
+	// CORS for https://foo.com origin, allowing:
+	// - PUT and PATCH methods
+	// - Origin header
+	// - Credentials share
+	// - Preflight requests cached for 12 hours
+	router.Use(cors.New(cors.Config{
+		AllowMethods:     []string{"GET", "POST"},
+		AllowHeaders:     []string{"Origin"},
+		ExposeHeaders:    []string{"Content-Length"},
+		AllowOrigins:     []string{"null", "https://foo.com"},
+		AllowCredentials: true,
+		MaxAge:           12 * time.Hour,
+	}))
+	router.GET("/", func(c *gin.Context) {
+		c.String(http.StatusOK, "hello world")
+	})
+	router.Run()
+}

go/ql/test/experimental/CWE-942/CorsMisconfiguration.expected

@@ -1,4 +1,5 @@
 | 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` |
 | 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` |