Merge branch 'main' into openssl_keyagreement_instances_and_consumers

2025-12-16 16:53:25 +01:00 · 2025-06-02 20:02:53 +02:00
parent 8b770bfb4d 52aa7e3c7d
commit 7e7dfeb40b
8 changed files with 158 additions and 75 deletions
--- a/.github/workflows/build-ripunzip.yml
+++ b/.github/workflows/build-ripunzip.yml
@@ -6,18 +6,18 @@ on:
      ripunzip-version:
        description: "what reference to checktout from google/runzip"
        required: false
-        default: v1.2.1
+        default: v2.0.2
      openssl-version:
        description: "what reference to checkout from openssl/openssl for Linux"
        required: false
-        default: openssl-3.3.0
+        default: openssl-3.5.0

 jobs:
  build:
    strategy:
      fail-fast: false
      matrix:
-        os: [ubuntu-22.04, macos-13, windows-2019]
+        os: [ubuntu-22.04, macos-13, windows-2022]
    runs-on: ${{ matrix.os }}
    steps:
      - uses: actions/checkout@v4
--- a/.github/workflows/csharp-qltest.yml
+++ b/.github/workflows/csharp-qltest.yml
@@ -36,7 +36,7 @@ jobs:
  unit-tests:
    strategy:
      matrix:
-        os: [ubuntu-latest, windows-2019]
+        os: [ubuntu-latest, windows-latest]
    runs-on: ${{ matrix.os }}
    steps:
      - uses: actions/checkout@v4
--- a/javascript/ql/lib/change-notes/2025-05-30-url-package-taint-step.md
+++ b/javascript/ql/lib/change-notes/2025-05-30-url-package-taint-step.md
@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added taint flow through the `URL` constructor from the `url` package, improving the identification of SSRF vulnerabilities.
--- a/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll
+++ b/javascript/ql/lib/semmle/javascript/security/dataflow/RequestForgeryCustomizations.qll
@@ -82,6 +82,13 @@ module RequestForgery {
      pred = url.getArgument(0)
    )
    or
+    exists(DataFlow::NewNode url |
+      url = API::moduleImport("url").getMember("URL").getAnInstantiation()
+    |
+      succ = url and
+      pred = url.getArgument(0)
+    )
+    or
    exists(HtmlSanitizerCall call |
      pred = call.getInput() and
      succ = call
--- a/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
+++ b/javascript/ql/test/query-tests/Security/CWE-918/RequestForgery.expected
@@ -30,6 +30,9 @@
 | serverSide.js:117:20:117:30 | new ws(url) | serverSide.js:115:25:115:35 | request.url | serverSide.js:117:27:117:29 | url | The $@ of this request depends on a $@. | serverSide.js:117:27:117:29 | url | URL | serverSide.js:115:25:115:35 | request.url | user-provided value |
 | serverSide.js:125:5:128:6 | axios({ ... \\n    }) | serverSide.js:123:29:123:35 | req.url | serverSide.js:127:14:127:20 | tainted | The $@ of this request depends on a $@. | serverSide.js:127:14:127:20 | tainted | URL | serverSide.js:123:29:123:35 | req.url | user-provided value |
 | serverSide.js:131:5:131:20 | axios.get(myUrl) | serverSide.js:123:29:123:35 | req.url | serverSide.js:131:15:131:19 | myUrl | The $@ of this request depends on a $@. | serverSide.js:131:15:131:19 | myUrl | URL | serverSide.js:123:29:123:35 | req.url | user-provided value |
+| serverSide.js:141:3:141:30 | axios.g ... ring()) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:141:13:141:29 | target.toString() | The $@ of this request depends on a $@. | serverSide.js:141:13:141:29 | target.toString() | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
+| serverSide.js:142:3:142:19 | axios.get(target) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:142:13:142:18 | target | The $@ of this request depends on a $@. | serverSide.js:142:13:142:18 | target | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
+| serverSide.js:143:3:143:24 | axios.g ... t.href) | serverSide.js:139:17:139:29 | req.query.url | serverSide.js:143:13:143:23 | target.href | The $@ of this request depends on a $@. | serverSide.js:143:13:143:23 | target.href | URL | serverSide.js:139:17:139:29 | req.query.url | user-provided value |
 edges
 | Request/app/api/proxy/route2.serverSide.ts:4:9:4:15 | { url } | Request/app/api/proxy/route2.serverSide.ts:4:9:4:34 | url | provenance |  |
 | Request/app/api/proxy/route2.serverSide.ts:4:9:4:34 | url | Request/app/api/proxy/route2.serverSide.ts:5:27:5:29 | url | provenance |  |
@@ -106,6 +109,15 @@ edges
 | serverSide.js:123:29:123:35 | req.url | serverSide.js:123:19:123:42 | url.par ... , true) | provenance |  |
 | serverSide.js:130:9:130:45 | myUrl | serverSide.js:131:15:131:19 | myUrl | provenance |  |
 | serverSide.js:130:37:130:43 | tainted | serverSide.js:130:9:130:45 | myUrl | provenance |  |
+| serverSide.js:139:9:139:29 | input | serverSide.js:140:26:140:30 | input | provenance |  |
+| serverSide.js:139:17:139:29 | req.query.url | serverSide.js:139:9:139:29 | input | provenance |  |
+| serverSide.js:140:9:140:31 | target | serverSide.js:141:13:141:18 | target | provenance |  |
+| serverSide.js:140:9:140:31 | target | serverSide.js:142:13:142:18 | target | provenance |  |
+| serverSide.js:140:9:140:31 | target | serverSide.js:143:13:143:18 | target | provenance |  |
+| serverSide.js:140:18:140:31 | new URL(input) | serverSide.js:140:9:140:31 | target | provenance |  |
+| serverSide.js:140:26:140:30 | input | serverSide.js:140:18:140:31 | new URL(input) | provenance | Config |
+| serverSide.js:141:13:141:18 | target | serverSide.js:141:13:141:29 | target.toString() | provenance |  |
+| serverSide.js:143:13:143:18 | target | serverSide.js:143:13:143:23 | target.href | provenance |  |
 nodes
 | Request/app/api/proxy/route2.serverSide.ts:4:9:4:15 | { url } | semmle.label | { url } |
 | Request/app/api/proxy/route2.serverSide.ts:4:9:4:34 | url | semmle.label | url |
@@ -199,4 +211,14 @@ nodes
 | serverSide.js:130:9:130:45 | myUrl | semmle.label | myUrl |
 | serverSide.js:130:37:130:43 | tainted | semmle.label | tainted |
 | serverSide.js:131:15:131:19 | myUrl | semmle.label | myUrl |
+| serverSide.js:139:9:139:29 | input | semmle.label | input |
+| serverSide.js:139:17:139:29 | req.query.url | semmle.label | req.query.url |
+| serverSide.js:140:9:140:31 | target | semmle.label | target |
+| serverSide.js:140:18:140:31 | new URL(input) | semmle.label | new URL(input) |
+| serverSide.js:140:26:140:30 | input | semmle.label | input |
+| serverSide.js:141:13:141:18 | target | semmle.label | target |
+| serverSide.js:141:13:141:29 | target.toString() | semmle.label | target.toString() |
+| serverSide.js:142:13:142:18 | target | semmle.label | target |
+| serverSide.js:143:13:143:18 | target | semmle.label | target |
+| serverSide.js:143:13:143:23 | target.href | semmle.label | target.href |
 subpaths
--- a/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js
+++ b/javascript/ql/test/query-tests/Security/CWE-918/serverSide.js
@@ -133,3 +133,12 @@ var server2 = http.createServer(function(req, res) {
    var myEncodedUrl = `${something}/bla/${encodeURIComponent(tainted)}`; 
    axios.get(myEncodedUrl);
 })
+
+var server2 = http.createServer(function(req, res) {
+  const { URL } = require('url'); 
+  const input = req.query.url; // $Source[js/request-forgery]
+  const target = new URL(input);
+  axios.get(target.toString()); // $Alert[js/request-forgery]
+  axios.get(target); // $Alert[js/request-forgery]
+  axios.get(target.href); // $Alert[js/request-forgery]
+});
--- a/rust/ql/lib/codeql/rust/internal/TypeInference.qll
+++ b/rust/ql/lib/codeql/rust/internal/TypeInference.qll
@@ -207,81 +207,58 @@ private Type inferAssignmentOperationType(AstNode n, TypePath path) {
 }

 /**
- * Holds if the type of `n1` at `path1` is the same as the type of `n2` at
- * `path2` and type information should propagate in both directions through the
- * type equality.
+ * Holds if the type tree of `n1` at `prefix1` should be equal to the type tree
+ * of `n2` at `prefix2` and type information should propagate in both directions
+ * through the type equality.
 */
-bindingset[path1]
-bindingset[path2]
-private predicate typeEquality(AstNode n1, TypePath path1, AstNode n2, TypePath path2) {
-  exists(Variable v |
-    path1 = path2 and
-    n1 = v.getAnAccess()
-  |
-    n2 = v.getPat()
+private predicate typeEquality(AstNode n1, TypePath prefix1, AstNode n2, TypePath prefix2) {
+  prefix1.isEmpty() and
+  prefix2.isEmpty() and
+  (
+    exists(Variable v | n1 = v.getAnAccess() |
+      n2 = v.getPat()
+      or
+      n2 = v.getParameter().(SelfParam)
+    )
    or
-    n2 = v.getParameter().(SelfParam)
-  )
-  or
-  exists(LetStmt let |
-    let.getPat() = n1 and
-    let.getInitializer() = n2 and
-    path1 = path2
-  )
-  or
-  n1 = n2.(ParenExpr).getExpr() and
-  path1 = path2
-  or
-  n1 = n2.(BlockExpr).getStmtList().getTailExpr() and
-  path1 = path2
-  or
-  n1 = n2.(IfExpr).getABranch() and
-  path1 = path2
-  or
-  n1 = n2.(MatchExpr).getAnArm().getExpr() and
-  path1 = path2
-  or
-  exists(BreakExpr break |
-    break.getExpr() = n1 and
-    break.getTarget() = n2.(LoopExpr) and
-    path1 = path2
-  )
-  or
-  exists(AssignmentExpr be |
-    n1 = be.getLhs() and
-    n2 = be.getRhs() and
-    path1 = path2
-  )
-}
-
-bindingset[path1]
-private predicate typeEqualityLeft(AstNode n1, TypePath path1, AstNode n2, TypePath path2) {
-  typeEquality(n1, path1, n2, path2)
-  or
-  n2 =
-    any(DerefExpr pe |
-      pe.getExpr() = n1 and
-      path1.isCons(TRefTypeParameter(), path2)
+    exists(LetStmt let |
+      let.getPat() = n1 and
+      let.getInitializer() = n2
    )
-}
-
-bindingset[path2]
-private predicate typeEqualityRight(AstNode n1, TypePath path1, AstNode n2, TypePath path2) {
-  typeEquality(n1, path1, n2, path2)
-  or
-  n2 =
-    any(DerefExpr pe |
-      pe.getExpr() = n1 and
-      path1 = TypePath::cons(TRefTypeParameter(), path2)
+    or
+    n1 = n2.(ParenExpr).getExpr()
+    or
+    n1 = n2.(BlockExpr).getStmtList().getTailExpr()
+    or
+    n1 = n2.(IfExpr).getABranch()
+    or
+    n1 = n2.(MatchExpr).getAnArm().getExpr()
+    or
+    exists(BreakExpr break |
+      break.getExpr() = n1 and
+      break.getTarget() = n2.(LoopExpr)
    )
+    or
+    exists(AssignmentExpr be |
+      n1 = be.getLhs() and
+      n2 = be.getRhs()
+    )
+  )
+  or
+  n1 = n2.(DerefExpr).getExpr() and
+  prefix1 = TypePath::singleton(TRefTypeParameter()) and
+  prefix2.isEmpty()
 }

 pragma[nomagic]
 private Type inferTypeEquality(AstNode n, TypePath path) {
-  exists(AstNode n2, TypePath path2 | result = inferType(n2, path2) |
-    typeEqualityRight(n, path, n2, path2)
+  exists(TypePath prefix1, AstNode n2, TypePath prefix2, TypePath suffix |
+    result = inferType(n2, prefix2.appendInverse(suffix)) and
+    path = prefix1.append(suffix)
+  |
+    typeEquality(n, prefix1, n2, prefix2)
    or
-    typeEqualityLeft(n2, path2, n, path)
+    typeEquality(n2, prefix2, n, prefix1)
  )
 }

--- a/shared/quantum/codeql/quantum/experimental/Model.qll
+++ b/shared/quantum/codeql/quantum/experimental/Model.qll
@@ -424,6 +424,17 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    final override ConsumerInputDataFlowNode getInputNode() { result = inputNode }
  }

+  final private class SignatureArtifactConsumer extends ArtifactConsumerAndInstance {
+    ConsumerInputDataFlowNode inputNode;
+
+    SignatureArtifactConsumer() {
+      exists(SignatureOperationInstance op | inputNode = op.getSignatureConsumer()) and
+      this = Input::dfn_to_element(inputNode)
+    }
+
+    final override ConsumerInputDataFlowNode getInputNode() { result = inputNode }
+  }
+
  /**
   * An artifact that is produced by an operation, representing a concrete artifact instance rather than a synthetic consumer artifact.
   */
@@ -458,6 +469,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    }

    override DataFlowNode getOutputNode() { result = creator.getOutputArtifact() }
+
+    KeyOperationInstance getCreator() { result = creator }
  }

  /**
@@ -782,6 +795,17 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    abstract ArtifactOutputDataFlowNode getOutputArtifact();
  }

+  /**
+   * A key operation instance representing a signature being generated or verified.
+   */
+  abstract class SignatureOperationInstance extends KeyOperationInstance {
+    /**
+     * Gets the consumer of the signature that is being verified in case of a
+     * verification operation.
+     */
+    abstract ConsumerInputDataFlowNode getSignatureConsumer();
+  }
+
  /**
   * A key-based algorithm instance used in cryptographic operations such as encryption, decryption,
   * signing, verification, and key wrapping.
@@ -1266,6 +1290,7 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    TNonceInput(NonceArtifactConsumer e) or
    TMessageInput(MessageArtifactConsumer e) or
    TSaltInput(SaltArtifactConsumer e) or
+    TSignatureInput(SignatureArtifactConsumer e) or
    TRandomNumberGeneration(RandomNumberGenerationInstance e) { e.flowsTo(_) } or
    // Key Creation Operation union type (e.g., key generation, key load)
    TKeyCreationOperation(KeyCreationOperationInstance e) or
@@ -1327,14 +1352,14 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    /**
     * Returns the child of this node with the given edge name.
     *
-     * This predicate is overriden by derived classes to construct the graph of cryptographic operations.
+     * This predicate is overridden by derived classes to construct the graph of cryptographic operations.
     */
    NodeBase getChild(string edgeName) { none() }

    /**
     * Defines properties of this node by name and either a value or location or both.
     *
-     * This predicate is overriden by derived classes to construct the graph of cryptographic operations.
+     * This predicate is overridden by derived classes to construct the graph of cryptographic operations.
     */
    predicate properties(string key, string value, Location location) { none() }

@@ -1507,6 +1532,20 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    override LocatableElement asElement() { result = instance }
  }

+  /**
+   * A signature input. This may represent a signature, or a signature component
+   * such as the scalar values r and s in ECDSA.
+   */
+  final class SignatureArtifactNode extends ArtifactNode, TSignatureInput {
+    SignatureArtifactConsumer instance;
+
+    SignatureArtifactNode() { this = TSignatureInput(instance) }
+
+    final override string getInternalType() { result = "SignatureInput" }
+
+    override LocatableElement asElement() { result = instance }
+  }
+
  /**
   * A salt input.
   */
@@ -1530,13 +1569,22 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {

    KeyOperationOutputNode() { this = TKeyOperationOutput(instance) }

-    final override string getInternalType() { result = "KeyOperationOutput" }
+    override string getInternalType() { result = "KeyOperationOutput" }

    override LocatableElement asElement() { result = instance }

    override string getSourceNodeRelationship() { none() }
  }

+  class SignOperationOutputNode extends KeyOperationOutputNode {
+    SignOperationOutputNode() {
+      this.asElement().(KeyOperationOutputArtifactInstance).getCreator().getKeyOperationSubtype() =
+        TSignMode()
+    }
+
+    override string getInternalType() { result = "SignatureOutput" }
+  }
+
  /**
   * A source of random number generation.
   */
@@ -2109,6 +2157,7 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
  }

  class SignatureOperationNode extends KeyOperationNode {
+    override SignatureOperationInstance instance;
    string nodeName;

    SignatureOperationNode() {
@@ -2118,6 +2167,21 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
    }

    override string getInternalType() { result = nodeName }
+
+    SignatureArtifactNode getASignatureArtifact() {
+      result.asElement() = instance.getSignatureConsumer().getConsumer()
+    }
+
+    override NodeBase getChild(string key) {
+      result = super.getChild(key)
+      or
+      // [KNOWN_OR_UNKNOWN] - only if we know the type is verify
+      this.getKeyOperationSubtype() = TVerifyMode() and
+      key = "Signature" and
+      if exists(this.getASignatureArtifact())
+      then result = this.getASignatureArtifact()
+      else result = this
+    }
  }

  /**
@@ -2565,6 +2629,8 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
      or
      curveName = "CURVE25519" and keySize = 255 and curveFamily = CURVE25519()
      or
+      curveName = "CURVE448" and keySize = 448 and curveFamily = CURVE448()
+      or
      // TODO: separate these into key agreement logic or sign/verify (ECDSA / ECDH)
      // or
      // curveName = "X25519" and keySize = 255 and curveFamily = CURVE25519()
@@ -2572,8 +2638,6 @@ module CryptographyBase<LocationSig Location, InputSig<Location> Input> {
      // curveName = "ED25519" and keySize = 255 and curveFamily = CURVE25519()
      // or
      // curveName = "ED448" and keySize = 448 and curveFamily = CURVE448()
-      // curveName = "CURVE448" and keySize = 448 and curveFamily = CURVE448()
-      // or
      // or
      // curveName = "X448" and keySize = 448 and curveFamily = CURVE448()
      curveName = "SM2" and keySize in [256, 512] and curveFamily = SM2()