Merge branch 'main' into cwe497b

Author: geoffw0

cpp/ql/src/Security/CWE/CWE-319/UseOfHttp.ql

@@ -3,6 +3,7 @@
  * @description Non-HTTPS connections can be intercepted by third parties.
  * @kind path-problem
  * @problem.severity warning
+ * @security-severity 8.1
  * @precision high
  * @id cpp/non-https-url
  * @tags security

cpp/ql/src/Security/CWE/CWE-326/InsufficientKeySize.ql

@@ -4,6 +4,7 @@
  *              allow an attacker to compromise security.
  * @kind path-problem
  * @problem.severity error
+ * @security-severity 7.5
  * @precision high
  * @id cpp/insufficient-key-size
  * @tags security

javascript/ql/lib/semmle/javascript/DynamicPropertyAccess.qll

@@ -181,16 +181,7 @@ class DynamicPropRead extends DataFlow::SourceNode, DataFlow::ValueNode {
    * dst[x][y] = src[y];
    * ```
    */
-  predicate hasDominatingAssignment() {
-    exists(DataFlow::PropWrite write, BasicBlock bb, int i, int j, SsaVariable ssaVar |
-      write = getBase().getALocalSource().getAPropertyWrite() and
-      bb.getNode(i) = write.getWriteNode() and
-      bb.getNode(j) = astNode and
-      i < j and
-      write.getPropertyNameExpr() = ssaVar.getAUse() and
-      astNode.getIndex() = ssaVar.getAUse()
-    )
-  }
+  predicate hasDominatingAssignment() { AccessPath::DominatingPaths::hasDominatingWrite(this) }
 }
 
 /**

javascript/ql/lib/semmle/javascript/GlobalAccessPaths.qll

@@ -532,7 +532,7 @@ module AccessPath {
      */
     cached
     predicate hasDominatingWrite(DataFlow::PropRead read) {
-      Stages::FlowSteps::ref() and
+      Stages::TypeTracking::ref() and
       // within the same basic block.
       exists(ReachableBasicBlock bb, Root root, string path, int ranking |
         read.asExpr() = rankedAccessPath(bb, root, path, ranking, AccessPathRead()) and
@@ -544,6 +544,21 @@ module AccessPath {
         read.asExpr() = getAccessTo(root, path, AccessPathRead()) and
         getAWriteBlock(root, path).strictlyDominates(read.getBasicBlock())
       )
+      or
+      // Dynamic write where the same variable is used to index the read and write (in the same basic block)
+      // For example, this is true for `dst[x]` on line 2 below:
+      // ```js
+      // dst[x] = {};
+      // dst[x][y] = src[y];
+      // ```
+      exists(DataFlow::PropWrite write, BasicBlock bb, int i, int j, SsaVariable ssaVar |
+        write = read.getBase().getALocalSource().getAPropertyWrite() and
+        bb.getNode(i) = write.getWriteNode() and
+        bb.getNode(j) = read.asExpr() and
+        i < j and
+        write.getPropertyNameExpr() = ssaVar.getAUse() and
+        read.getPropertyNameExpr() = ssaVar.getAUse()
+      )
     }
   }
 }

javascript/ql/lib/semmle/javascript/dataflow/TaintTracking.qll

@@ -534,7 +534,10 @@ module TaintTracking {
       or
       // reading from a tainted object yields a tainted result
       succ.(DataFlow::PropRead).getBase() = pred and
-      not AccessPath::DominatingPaths::hasDominatingWrite(succ) and
+      not (
+        AccessPath::DominatingPaths::hasDominatingWrite(succ) and
+        exists(succ.(DataFlow::PropRead).getPropertyName())
+      ) and
       not isSafeClientSideUrlProperty(succ) and
       not ClassValidator::isAccessToSanitizedField(succ)
       or

javascript/ql/lib/semmle/javascript/internal/CachedStages.qll

@@ -212,6 +212,8 @@ module Stages {
       any(DataFlow::Node node).hasUnderlyingType(_)
       or
       any(DataFlow::Node node).hasUnderlyingType(_, _)
+      or
+      AccessPath::DominatingPaths::hasDominatingWrite(_)
     }
   }
 
@@ -235,8 +237,6 @@ module Stages {
     predicate backref() {
       1 = 1
       or
-      AccessPath::DominatingPaths::hasDominatingWrite(_)
-      or
       DataFlow::SharedFlowStep::step(_, _)
     }
   }

javascript/ql/src/Security/CWE-295/DisablingCertificateValidation.ql

@@ -13,26 +13,25 @@
 
 import javascript
 
-/**
- * Gets an options object for a TLS connection.
- */
-DataFlow::ObjectLiteralNode tlsOptions() {
-  exists(DataFlow::InvokeNode invk | result.flowsTo(invk.getAnArgument()) |
-    invk instanceof ClientRequest
-    or
-    invk = DataFlow::moduleMember("https", "Agent").getAnInstantiation()
-    or
-    exists(DataFlow::NewNode new |
-      new = DataFlow::moduleMember("tls", "TLSSocket").getAnInstantiation()
-    |
-      invk = new or
-      invk = new.getAMethodCall("renegotiate")
-    )
-    or
-    invk = DataFlow::moduleMember("tls", ["connect", "createServer"]).getACall()
+/** Gets options argument for a potential TLS connection */
+DataFlow::InvokeNode tlsInvocation() {
+  result instanceof ClientRequest
+  or
+  result = DataFlow::moduleMember("https", "Agent").getAnInstantiation()
+  or
+  exists(DataFlow::NewNode new |
+    new = DataFlow::moduleMember("tls", "TLSSocket").getAnInstantiation()
+  |
+    result = new or
+    result = new.getAMethodCall("renegotiate")
   )
+  or
+  result = DataFlow::moduleMember("tls", ["connect", "createServer"]).getACall()
 }
 
+/** Gets an options object for a TLS connection. */
+DataFlow::ObjectLiteralNode tlsOptions() { result.flowsTo(tlsInvocation().getAnArgument()) }
+
 from DataFlow::PropWrite disable
 where
   exists(DataFlow::SourceNode env |
@@ -41,6 +40,13 @@ where
     disable.getRhs().mayHaveStringValue("0")
   )
   or
-  disable = tlsOptions().getAPropertyWrite("rejectUnauthorized") and
+  (
+    disable = tlsOptions().getAPropertyWrite("rejectUnauthorized")
+    or
+    // the same thing, but with API-nodes if they happen to be available
+    exists(API::Node tlsInvk | tlsInvk.getAnInvocation() = tlsInvocation() |
+      disable.getRhs() = tlsInvk.getAParameter().getMember("rejectUnauthorized").getARhs()
+    )
+  ) and
   disable.getRhs().(AnalyzedNode).getTheBooleanValue() = false
 select disable, "Disabling certificate validation is strongly discouraged."

javascript/ql/test/query-tests/Security/CWE-295/DisablingCertificateValidation.expected

@@ -8,3 +8,4 @@
 | tst.js:39:2:39:29 | rejectU ... ndirect | Disabling certificate validation is strongly discouraged. |
 | tst.js:45:2:45:28 | rejectU ... !!false | Disabling certificate validation is strongly discouraged. |
 | tst.js:48:2:48:26 | rejectU ... : !true | Disabling certificate validation is strongly discouraged. |
+| tst.js:74:9:74:33 | rejectU ... : false | Disabling certificate validation is strongly discouraged. |

javascript/ql/test/query-tests/Security/CWE-295/tst.js

@@ -68,3 +68,10 @@ new https.Agent({
 new https.Agent({
 	rejectUnauthorized: typeof getOptions().rejectUnauthorized === 'boolean' ? getOptions().rejectUnauthorized : undefined // OK
 });
+
+function getSomeunsafeOptions() {
+    return {
+        rejectUnauthorized: false // NOT OK
+    }
+}
+new https.Agent(getSomeunsafeOptions());