Merge branch 'main' into alexdenisov/macros

Author: AlexDenisov

cpp/ql/src/Security/CWE/CWE-170/ImproperNullTerminationTainted.ql

@@ -12,79 +12,44 @@
 
 import cpp
 import semmle.code.cpp.commons.NullTermination
-import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
+import semmle.code.cpp.security.FlowSources as FS
+import semmle.code.cpp.dataflow.new.TaintTracking
+import semmle.code.cpp.ir.IR
 
-/** A user-controlled expression that may not be null terminated. */
-class TaintSource extends VariableAccess {
-  TaintSource() {
-    exists(SecurityOptions x, string cause |
-      this.getTarget() instanceof SemanticStackVariable and
-      x.isUserInput(this, cause)
-    |
-      cause = ["read", "fread", "recv", "recvfrom", "recvmsg"]
-    )
-  }
+predicate isSource(FS::FlowSource source, string sourceType) {
+  sourceType = source.getSourceType() and
+  exists(VariableAccess va, Call call |
+    va = source.asDefiningArgument() and
+    call.getAnArgument() = va and
+    va.getTarget() instanceof SemanticStackVariable and
+    call.getTarget().hasGlobalName(["read", "fread", "recv", "recvfrom", "recvmsg"])
+  )
+}
 
-  /**
-   * Holds if `sink` is a tainted variable access that must be null
-   * terminated.
-   */
-  private predicate isSink(VariableAccess sink) {
-    tainted(this, sink) and
-    variableMustBeNullTerminated(sink)
-  }
+predicate isSink(DataFlow::Node sink, VariableAccess va) {
+  va = [sink.asExpr(), sink.asIndirectExpr()] and
+  variableMustBeNullTerminated(va)
+}
 
-  /**
-   * Holds if this source can reach `va`, possibly using intermediate
-   * reassignments.
-   */
-  private predicate sourceReaches(VariableAccess va) {
-    definitionUsePair(_, this, va)
-    or
-    exists(VariableAccess mid, Expr def |
-      this.sourceReaches(mid) and
-      exprDefinition(_, def, mid) and
-      definitionUsePair(_, def, va)
-    )
-  }
+private module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node source) { isSource(source, _) }
 
-  /**
-   * Holds if the sink `sink` is reachable both from this source and
-   * from `va`, possibly using intermediate reassignments.
-   */
-  private predicate reachesSink(VariableAccess va, VariableAccess sink) {
-    this.isSink(sink) and
-    va = sink
+  predicate isBarrier(DataFlow::Node node) {
+    isSink(node) and node.asExpr().getUnspecifiedType() instanceof ArithmeticType
+    or
+    node.asInstruction().(StoreInstruction).getResultType() instanceof ArithmeticType
     or
-    exists(VariableAccess mid, Expr def |
-      this.reachesSink(mid, sink) and
-      exprDefinition(_, def, va) and
-      definitionUsePair(_, def, mid)
-    )
+    mayAddNullTerminator(_, node.asIndirectExpr())
   }
 
-  /**
-   * Holds if `sink` is a tainted variable access that must be null
-   * terminated, and no access which null terminates its contents can
-   * either reach the sink or be reached from the source. (Ideally,
-   * we should instead look for such accesses only on the path from
-   * this source to `sink` found via `tainted(source, sink)`.)
-   */
-  predicate reaches(VariableAccess sink) {
-    this.isSink(sink) and
-    not exists(VariableAccess va |
-      va != this and
-      va != sink and
-      mayAddNullTerminator(_, va)
-    |
-      this.sourceReaches(va)
-      or
-      this.reachesSink(va, sink)
-    )
-  }
+  predicate isSink(DataFlow::Node sink) { isSink(sink, _) }
 }
 
-from TaintSource source, VariableAccess sink
-where source.reaches(sink)
-select sink, "String operation depends on a $@ that may not be null terminated.", source,
-  "user-provided value"
+module Flow = TaintTracking::Global<Config>;
+
+from DataFlow::Node source, DataFlow::Node sink, VariableAccess va, string sourceType
+where
+  Flow::flow(source, sink) and
+  isSource(source, sourceType) and
+  isSink(sink, va)
+select va, "String operation depends on $@ that may not be null terminated.", source, sourceType

cpp/ql/src/Security/CWE/CWE-807/TaintedCondition.ql

@@ -12,30 +12,83 @@
  *       external/cwe/cwe-807
  */
 
-import semmle.code.cpp.ir.dataflow.internal.DefaultTaintTrackingImpl
-import TaintedWithPath
+import cpp
+import semmle.code.cpp.security.Security
+import semmle.code.cpp.security.FlowSources
+import semmle.code.cpp.ir.dataflow.TaintTracking
+import semmle.code.cpp.ir.IR
+import Flow::PathGraph
+
+Expr getExprWithoutNot(Expr expr) {
+  result = expr and not expr instanceof NotExpr
+  or
+  result = getExprWithoutNot(expr.(NotExpr).getOperand()) and expr instanceof NotExpr
+}
 
 predicate sensitiveCondition(Expr condition, Expr raise) {
   raisesPrivilege(raise) and
   exists(IfStmt ifstmt |
-    ifstmt.getCondition() = condition and
+    getExprWithoutNot(ifstmt.getCondition()) = condition and
     raise.getEnclosingStmt().getParentStmt*() = ifstmt
   )
 }
 
-class Configuration extends TaintTrackingConfiguration {
-  override predicate isSink(Element tainted) { sensitiveCondition(tainted, _) }
+private predicate constantInstruction(Instruction instr) {
+  instr instanceof ConstantInstruction
+  or
+  instr instanceof StringConstantInstruction
+  or
+  constantInstruction(instr.(UnaryInstruction).getUnary())
+}
+
+predicate isSource(FlowSource source, string sourceType) { sourceType = source.getSourceType() }
+
+module Config implements DataFlow::ConfigSig {
+  predicate isSource(DataFlow::Node node) { isSource(node, _) }
+
+  predicate isSink(DataFlow::Node node) {
+    sensitiveCondition([node.asExpr(), node.asIndirectExpr()], _)
+  }
+
+  predicate isBarrier(DataFlow::Node node) {
+    // Block flow into binary instructions if both operands are non-constant
+    exists(BinaryInstruction iTo |
+      iTo = node.asInstruction() and
+      not constantInstruction(iTo.getLeft()) and
+      not constantInstruction(iTo.getRight()) and
+      // propagate taint from either the pointer or the offset, regardless of constant-ness
+      not iTo instanceof PointerArithmeticInstruction
+    )
+    or
+    // Block flow through calls to pure functions if two or more operands are non-constant
+    exists(Instruction iFrom1, Instruction iFrom2, CallInstruction iTo |
+      iTo = node.asInstruction() and
+      isPureFunction(iTo.getStaticCallTarget().getName()) and
+      iFrom1 = iTo.getAnArgument() and
+      iFrom2 = iTo.getAnArgument() and
+      not constantInstruction(iFrom1) and
+      not constantInstruction(iFrom2) and
+      iFrom1 != iFrom2
+    )
+  }
 }
 
+module Flow = TaintTracking::Global<Config>;
+
 /*
  * Produce an alert if there is an 'if' statement whose condition `condition`
  * is influenced by tainted data `source`, and the body contains
  * `raise` which escalates privilege.
  */
 
-from Expr source, Expr condition, Expr raise, PathNode sourceNode, PathNode sinkNode
+from
+  Expr raise, string sourceType, DataFlow::Node source, DataFlow::Node sink,
+  Flow::PathNode sourceNode, Flow::PathNode sinkNode
 where
-  taintedWithPath(source, condition, sourceNode, sinkNode) and
-  sensitiveCondition(condition, raise)
-select condition, sourceNode, sinkNode, "Reliance on untrusted input $@ to raise privilege at $@.",
-  source, source.toString(), raise, raise.toString()
+  source = sourceNode.getNode() and
+  sink = sinkNode.getNode() and
+  isSource(source, sourceType) and
+  sensitiveCondition([sink.asExpr(), sink.asIndirectExpr()], raise) and
+  Flow::flowPath(sourceNode, sinkNode)
+select sink, sourceNode, sinkNode, "Reliance on $@ to raise privilege at $@.", source, sourceType,
+  raise, raise.toString()

cpp/ql/test/query-tests/Likely Bugs/Memory Management/ImproperNullTermination/ImproperNullTerminationTainted.expected

@@ -1,2 +1,2 @@
-| test.cpp:466:10:466:15 | buffer | String operation depends on a $@ that may not be null terminated. | test.cpp:465:18:465:23 | buffer | user-provided value |
-| test.cpp:481:10:481:15 | buffer | String operation depends on a $@ that may not be null terminated. | test.cpp:480:9:480:14 | buffer | user-provided value |
+| test.cpp:466:10:466:15 | buffer | String operation depends on $@ that may not be null terminated. | test.cpp:465:18:465:23 | read output argument | buffer read by read |
+| test.cpp:481:10:481:15 | buffer | String operation depends on $@ that may not be null terminated. | test.cpp:480:9:480:14 | fread output argument | string read by fread |

cpp/ql/test/query-tests/Security/CWE/CWE-807/semmle/TaintedCondition/TaintedCondition.expected

@@ -1,13 +1,11 @@
 edges
-| test.cpp:20:29:20:34 | call to getenv | test.cpp:24:10:24:35 | ! ... |
-| test.cpp:20:29:20:34 | call to getenv | test.cpp:24:11:24:16 | call to strcmp |
-| test.cpp:20:29:20:47 | call to getenv | test.cpp:24:10:24:35 | ! ... |
 | test.cpp:20:29:20:47 | call to getenv | test.cpp:24:11:24:16 | call to strcmp |
-subpaths
+| test.cpp:20:29:20:47 | call to getenv indirection | test.cpp:24:11:24:16 | call to strcmp |
 nodes
-| test.cpp:20:29:20:34 | call to getenv | semmle.label | call to getenv |
 | test.cpp:20:29:20:47 | call to getenv | semmle.label | call to getenv |
-| test.cpp:24:10:24:35 | ! ... | semmle.label | ! ... |
+| test.cpp:20:29:20:47 | call to getenv indirection | semmle.label | call to getenv indirection |
 | test.cpp:24:11:24:16 | call to strcmp | semmle.label | call to strcmp |
+subpaths
 #select
-| test.cpp:24:10:24:35 | ! ... | test.cpp:20:29:20:34 | call to getenv | test.cpp:24:10:24:35 | ! ... | Reliance on untrusted input $@ to raise privilege at $@. | test.cpp:20:29:20:34 | call to getenv | call to getenv | test.cpp:25:9:25:27 | ... = ... | ... = ... |
+| test.cpp:24:11:24:16 | call to strcmp | test.cpp:20:29:20:47 | call to getenv | test.cpp:24:11:24:16 | call to strcmp | Reliance on $@ to raise privilege at $@. | test.cpp:20:29:20:47 | call to getenv | an environment variable | test.cpp:25:9:25:27 | ... = ... | ... = ... |
+| test.cpp:24:11:24:16 | call to strcmp | test.cpp:20:29:20:47 | call to getenv indirection | test.cpp:24:11:24:16 | call to strcmp | Reliance on $@ to raise privilege at $@. | test.cpp:20:29:20:47 | call to getenv indirection | an environment variable | test.cpp:25:9:25:27 | ... = ... | ... = ... |

go/ql/lib/change-notes/released/0.7.3.md

@@ -2,7 +2,7 @@
 
 ### Minor Analysis Improvements
 
-* Added the [gin cors](https://github.com/gin-contrib/cors) library to the CorsMisconfiguration.ql query
+* Added the [gin-contrib/cors](https://github.com/gin-contrib/cors) library to the experimental query "CORS misconfiguration" (`go/cors-misconfiguration`).
 
 ### Bug Fixes
 

go/ql/src/experimental/CWE-525/WebCacheDeception.qhelp

@@ -0,0 +1,35 @@
+<!DOCTYPE qhelp PUBLIC "-//Semmle//qhelp//EN" "qhelp.dtd">
+<qhelp>
+    <overview>
+        <p>
+            Web Cache Deception is a security vulnerability where an attacker tricks a web server into caching sensitive information and then accesses that cached data.
+        </p>
+        <p>
+            This attack exploits certain behaviors in caching mechanisms by requesting URLs that trick the server into thinking that a non-cachable page is cachable. If a user then accesses sensitive information on these pages, it could be cached and later retrieved by the attacker.
+        </p>
+    </overview>
+    <recommendation>
+        <p>
+            To prevent Web Cache Deception attacks, web applications should clearly define cacheable and non-cacheable resources. Implementing strict cache controls and validating requested URLs can mitigate the risk of sensitive data being cached.
+        </p>
+    </recommendation>
+    <example>
+        <p>
+            Vulnerable code example: A web server is configured to cache all responses ending in '.css'. An attacker requests 'profile.css', and the server processes 'profile', a sensitive page, and caches it.
+        </p>
+        <sample src="WebCacheDeceptionBad.go" />
+    </example>
+    <example>
+        <p>
+            Secure code example: The server is configured with strict cache controls and URL validation, preventing caching of dynamic or sensitive pages regardless of their URL pattern.
+        </p>
+        <sample src="WebCacheDeceptionGood.go" />
+    </example>
+    <references>
+        <li>
+            OWASP Web Cache Deception Attack:
+            <a href="https://owasp.org/www-community/attacks/Web_Cache_Deception">Understanding Web Cache Deception Attacks</a>
+        </li>
+        <!-- Additional references can be added here -->
+    </references>
+</qhelp>

go/ql/src/experimental/CWE-525/WebCacheDeception.ql

@@ -0,0 +1,27 @@
+/*
+ * @name Web Cache Deception
+ * @description A caching system has been detected on the application and is vulnerable to web cache deception. By manipulating the URL it is possible to force the application to cache pages that are only accessible by an authenticated user. Once cached, these pages can be accessed by an unauthenticated user.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 9
+ * @precision high
+ * @id go/web-cache-deception
+ * @tags security
+ *       external/cwe/cwe-525
+ */
+
+import go
+
+from
+  DataFlow::CallNode httpHandleFuncCall, DataFlow::ReadNode rn, Http::HeaderWrite::Range hw,
+  DeclaredFunction f
+where
+  httpHandleFuncCall.getTarget().hasQualifiedName("net/http", "HandleFunc") and
+  httpHandleFuncCall.getArgument(0).getStringValue().matches("%/") and
+  httpHandleFuncCall.getArgument(1) = rn and
+  rn.reads(f) and
+  f.getParameter(0) = hw.getResponseWriter() and
+  hw.getHeaderName() = "cache-control"
+select httpHandleFuncCall.getArgument(0),
+  "Wildcard Endpoint used with " + httpHandleFuncCall.getArgument(0) + " and '" + hw.getHeaderName()
+    + "' Header is used"

go/ql/src/experimental/CWE-525/WebCacheDeceptionBad.go

@@ -0,0 +1,87 @@
+package bad
+
+import (
+	"fmt"
+	"html/template"
+	"log"
+	"net/http"
+	"os/exec"
+	"strings"
+	"sync"
+)
+
+var sessionMap = make(map[string]string)
+
+var (
+	templateCache = make(map[string]*template.Template)
+	mutex         = &sync.Mutex{}
+)
+
+type Lists struct {
+	Uid       string
+	UserName  string
+	UserLists []string
+	ReadFile  func(filename string) string
+}
+
+func parseTemplateFile(templateName string, tmplFile string) (*template.Template, error) {
+	mutex.Lock()
+	defer mutex.Unlock()
+
+	// Check if the template is already cached
+	if cachedTemplate, ok := templateCache[templateName]; ok {
+		fmt.Println("cached")
+		return cachedTemplate, nil
+	}
+
+	// Parse and store the template in the cache
+	parsedTemplate, _ := template.ParseFiles(tmplFile)
+	fmt.Println("not cached")
+
+	templateCache[templateName] = parsedTemplate
+	return parsedTemplate, nil
+}
+
+func ShowAdminPageCache(w http.ResponseWriter, r *http.Request) {
+
+	if r.Method == "GET" {
+		fmt.Println("cache called")
+		sessionMap[r.RequestURI] = "admin"
+
+		// Check if a session value exists
+		if _, ok := sessionMap[r.RequestURI]; ok {
+			cmd := "mysql -h mysql -u root -prootwolf -e 'select id,name,mail,age,created_at,updated_at from vulnapp.user where name not in (\"" + "admin" + "\");'"
+
+			// mysql -h mysql -u root -prootwolf -e 'select id,name,mail,age,created_at,updated_at from vulnapp.user where name not in ("test");--';echo");'
+			fmt.Println(cmd)
+
+			res, err := exec.Command("sh", "-c", cmd).Output()
+			if err != nil {
+				fmt.Println("err : ", err)
+			}
+
+			splitedRes := strings.Split(string(res), "\n")
+
+			p := Lists{Uid: "1", UserName: "admin", UserLists: splitedRes}
+
+			parsedTemplate, _ := parseTemplateFile("page", "./views/admin/userlists.gtpl")
+			w.Header().Set("Cache-Control", "no-store, no-cache")
+			err = parsedTemplate.Execute(w, p)
+		}
+	} else {
+		http.NotFound(w, nil)
+	}
+
+}
+
+func main() {
+	fmt.Println("Vulnapp server listening : 1337")
+
+	http.Handle("/assets/", http.StripPrefix("/assets/", http.FileServer(http.Dir("assets/"))))
+
+	http.HandleFunc("/adminusers/", ShowAdminPageCache)
+	err := http.ListenAndServe(":1337", nil)
+	if err != nil {
+		log.Fatal("ListenAndServe: ", err)
+	}
+}