Merge branch 'main' into js/shared-dataflow-merged

javascript/ql/src/CHANGELOG.md

@@ -1,3 +1,56 @@
+## 0.8.9
+
+### Bug Fixes
+
+* The left operand of the `&&` operator no longer propagates data flow by default.
+
+## 0.8.8
+
+No user-facing changes.
+
+## 0.8.7
+
+### Minor Analysis Improvements
+
+* Added support for [doT](https://github.com/olado/doT) templates. 
+
+## 0.8.6
+
+No user-facing changes.
+
+## 0.8.5
+
+No user-facing changes.
+
+## 0.8.4
+
+### Minor Analysis Improvements
+
+* Added django URLs to detected "safe" URL patterns in `js/unsafe-external-link`. 
+
+## 0.8.3
+
+### Query Metadata Changes
+
+* Lower the security severity of log-injection to medium.
+* Increase the security severity of XSS to high.
+
+## 0.8.2
+
+### Minor Analysis Improvements
+
+* Added modeling for importing `express-rate-limit` using a named import.
+
+## 0.8.1
+
+### Minor Analysis Improvements
+
+* Added the `AmdModuleDefinition::Range` class, making it possible to define custom aliases for the AMD `define` function.
+
+## 0.8.0
+
+No user-facing changes.
+
 ## 0.7.5
 
 ### Bug Fixes

javascript/ql/src/DOM/TargetBlank.ql

@@ -44,7 +44,9 @@ predicate hasDynamicHrefHostAttributeValue(DOM::ElementDefinition elem) {
       // ... that does not start with a fixed host or a relative path (common formats)
       not url.regexpMatch("(?i)((https?:)?//)?[-a-z0-9.]*/.*") and
       // .. that is not a call to `url_for` in a Flask / nunjucks application
-      not url.regexpMatch("\\{\\{\\s*url(_for)?\\(.+\\).*")
+      not url.regexpMatch("\\{\\{\\s*url(_for)?\\(.+\\).*") and
+      // .. that is not a call to `url` in a Django application
+      not url.regexpMatch("\\{%\\s*url.*")
     )
   )
 }

javascript/ql/src/Diagnostics/ExtractedFiles.ql

@@ -1,6 +1,6 @@
 /**
- * @name Successfully extracted files
- * @description Lists all files in the source code directory that were extracted without encountering an error in the file.
+ * @name Extracted files
+ * @description Lists all files in the source code directory that were extracted.
  * @kind diagnostic
  * @id js/diagnostics/successfully-extracted-files
  * @tags successfully-extracted-files
@@ -9,7 +9,5 @@
 import javascript
 
 from File f
-where
-  not exists(Error e | e.isFatal() and e.getFile() = f) and
-  exists(f.getRelativePath())
+where exists(f.getRelativePath())
 select f, ""

javascript/ql/src/Expressions/TypoDatabase.qll

@@ -1,4 +0,0 @@
-import codeql.typos.TypoDatabase as DB
-
-/** DEPRECATED: Use the `codeql/typos` pack instead. */
-deprecated predicate typos = DB::typos/2;

javascript/ql/src/LanguageFeatures/SpuriousArguments.ql

@@ -46,7 +46,8 @@ class SpuriousArguments extends Expr {
 
   SpuriousArguments() {
     this = invk.getArgument(maxArity(invk)).asExpr() and
-    not invk.isIncomplete()
+    not invk.isIncomplete() and
+    not invk.getAstNode() instanceof TaggedTemplateExpr
   }
 
   /**

javascript/ql/src/Performance/ReDoSReferences.inc.qhelp

@@ -10,7 +10,7 @@
 		<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/ReDoS">ReDoS</a>.</li>
 		<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Time_complexity">Time complexity</a>.</li>
 		<li>James Kirrage, Asiri Rathnayake, Hayo Thielecke:
-		<a href="http://www.cs.bham.ac.uk/~hxt/research/reg-exp-sec.pdf">Static Analysis for Regular Expression Denial-of-Service Attack</a>.
+		<a href="https://arxiv.org/abs/1301.0849">Static Analysis for Regular Expression Denial-of-Service Attack</a>.
 		</li>
 	</references>
 </qhelp>

javascript/ql/src/RegExp/IdentityReplacement.ql

@@ -3,7 +3,7 @@
  * @description Replacing a substring with itself has no effect and may indicate a mistake.
  * @kind problem
  * @problem.severity warning
- * @security-severity 7.8
+ * @security-severity 5.0
  * @id js/identity-replacement
  * @precision very-high
  * @tags correctness

javascript/ql/src/Security/CWE-022/TaintedPath.qhelp

@@ -13,40 +13,47 @@ attacker being able to influence behavior by modifying unexpected files.
 
 <recommendation>
 <p>
-Validate user input before using it to construct a file path, either using an off-the-shelf library
-like the <code>sanitize-filename</code> npm package, or by performing custom validation.
+Validate user input before using it to construct a file path.
 </p>
 
 <p>
-Ideally, follow these rules:
+The validation method you should use depends on whether you want to allow the user to specify complex paths with multiple components that may span multiple folders, or only simple filenames without a path component.
 </p>
 
-<ul>
-<li>Do not allow more than a single "." character.</li>
-<li>Do not allow directory separators such as "/" or "\" (depending on the file system).</li>
-<li>Do not rely on simply replacing problematic sequences such as "../". For example, after
-applying this filter to ".../...//", the resulting string would still be "../".</li>
-<li>Use a whitelist of known good patterns.</li>
-</ul>
+<p>
+In the former case, a common strategy is to make sure that the constructed file path is contained within a safe root folder.
+First, normalize the path using <code>path.resolve</code> or <code>fs.realpathSync</code> to remove any ".." segments.
+You should always normalize the file path since an unnormalized path that starts with the root folder can still be used to access files outside the root folder.
+Then, after you have normalized the path, check that the path starts with the root folder.
+</p>
+
+<p>
+In the latter case, you can use a library like the <code>sanitize-filename</code> npm package to eliminate any special characters from the file path.
+Note that it is <i>not</i> sufficient to only remove "../" sequences: for example, applying this filter to ".../...//" would still result in the string "../".
+</p>
+
+<p>
+Finally, the simplest (but most restrictive) option is to use an allow list of safe patterns and make sure that the user input matches one of these patterns.
+</p>
 </recommendation>
 
 <example>
 <p>
-In the first example, a file name is read from an HTTP request and then used to access a file.
-However, a malicious user could enter a file name which is an absolute path, such as
-<code>"/etc/passwd"</code>.
-</p>
-
-<p>
-In the second example, it appears that the user is restricted to opening a file within the
-<code>"user"</code> home directory. However, a malicious user could enter a file name containing
-special characters. For example, the string <code>"../../etc/passwd"</code> will result in the code
-reading the file located at <code>"/home/user/../../etc/passwd"</code>, which is the system's
-password file. This file would then be sent back to the user, giving them access to all the
-system's passwords.
+In the first (bad) example, the code reads the file name from an HTTP request, then accesses that file within a root folder.
+A malicious user could enter a file name containing "../" segments to navigate outside the root folder and access sensitive files.
 </p>
 
 <sample src="examples/TaintedPath.js" />
+
+<p>
+The second (good) example shows how to avoid access to sensitive files by sanitizing the file path.
+First, the code resolves the file name relative to a root folder, normalizing the path and removing any "../" segments in the process.
+Then, the code calls <code>fs.realpathSync</code> to resolve any symbolic links in the path.
+Finally, the code checks that the normalized path starts with the path of the root folder, ensuring the file is contained within the root folder.
+</p>
+
+<sample src="examples/TaintedPathGood.js" />
+
 </example>
 
 <references>

javascript/ql/src/Security/CWE-022/examples/TaintedPath.js

@@ -1,13 +1,12 @@
-var fs = require('fs'),
-    http = require('http'),
-    url = require('url');
+const fs = require('fs'),
+      http = require('http'),
+      url = require('url');
+
+const ROOT = "/var/www/";
 
 var server = http.createServer(function(req, res) {
-  let path = url.parse(req.url, true).query.path;
+  let filePath = url.parse(req.url, true).query.path;
 
-  // BAD: This could read any file on the file system
-  res.write(fs.readFileSync(path));
-
-  // BAD: This could still read any file on the file system
-  res.write(fs.readFileSync("/home/user/" + path));
-});
+  // BAD: This function uses unsanitized input that can read any file on the file system.
+  res.write(fs.readFileSync(ROOT + filePath, 'utf8'));
+});

javascript/ql/src/Security/CWE-022/examples/TaintedPathGood.js

@@ -0,0 +1,19 @@
+const fs = require('fs'),
+      http = require('http'),
+      path = require('path'),
+      url = require('url');
+
+const ROOT = "/var/www/";
+
+var server = http.createServer(function(req, res) {
+  let filePath = url.parse(req.url, true).query.path;
+
+  // GOOD: Verify that the file path is under the root directory
+  filePath = fs.realpathSync(path.resolve(ROOT, filePath));
+  if (!filePath.startsWith(ROOT)) {
+    res.statusCode = 403;
+    res.end();
+    return;
+  }
+  res.write(fs.readFileSync(filePath, 'utf8'));
+});

javascript/ql/src/Security/CWE-079/ReflectedXss.ql

@@ -4,7 +4,7 @@
  *              a cross-site scripting vulnerability.
  * @kind path-problem
  * @problem.severity error
- * @security-severity 6.1
+ * @security-severity 7.8
  * @precision high
  * @id js/reflected-xss
  * @tags security

javascript/ql/src/Security/CWE-079/StoredXss.ql

@@ -4,7 +4,7 @@
  *              a stored cross-site scripting vulnerability.
  * @kind path-problem
  * @problem.severity error
- * @security-severity 6.1
+ * @security-severity 7.8
  * @precision high
  * @id js/stored-xss
  * @tags security

javascript/ql/src/Security/CWE-079/Xss.ql

@@ -4,7 +4,7 @@
  *              a cross-site scripting vulnerability.
  * @kind path-problem
  * @problem.severity error
- * @security-severity 6.1
+ * @security-severity 7.8
  * @precision high
  * @id js/xss
  * @tags security

javascript/ql/src/Security/CWE-117/LogInjection.ql

@@ -4,7 +4,7 @@
  *              insertion of forged log entries by a malicious user.
  * @kind path-problem
  * @problem.severity error
- * @security-severity 7.8
+ * @security-severity 6.1
  * @precision medium
  * @id js/log-injection
  * @tags security

javascript/ql/src/Security/CWE-327/BadRandomness.ql

@@ -1,5 +1,5 @@
 /**
- * @name Creating biased random numbers from a cryptographically secure source.
+ * @name Creating biased random numbers from a cryptographically secure source
  * @description Some mathematical operations on random numbers can cause bias in
  *              the results and compromise security.
  * @kind problem

javascript/ql/src/Security/CWE-327/BrokenCryptoAlgorithm.ql

@@ -16,9 +16,14 @@ import semmle.javascript.security.dataflow.BrokenCryptoAlgorithmQuery
 import semmle.javascript.security.SensitiveActions
 import BrokenCryptoAlgorithmFlow::PathGraph
 
-from BrokenCryptoAlgorithmFlow::PathNode source, BrokenCryptoAlgorithmFlow::PathNode sink
+from
+  BrokenCryptoAlgorithmFlow::PathNode source, BrokenCryptoAlgorithmFlow::PathNode sink,
+  Source sourceNode, Sink sinkNode
 where
   BrokenCryptoAlgorithmFlow::flowPath(source, sink) and
-  not source.getNode() instanceof CleartextPasswordExpr // flagged by js/insufficient-password-hash
-select sink.getNode(), source, sink, "A broken or weak cryptographic algorithm depends on $@.",
-  source.getNode(), "sensitive data from " + source.getNode().(Source).describe()
+  sourceNode = source.getNode() and
+  sinkNode = sink.getNode() and
+  not sourceNode instanceof CleartextPasswordExpr // flagged by js/insufficient-password-hash
+select sinkNode, source, sink, "$@ depends on $@.", sinkNode.getInitialization(),
+  "A broken or weak cryptographic algorithm", sourceNode,
+  "sensitive data from " + sourceNode.describe()

javascript/ql/src/Summary/LinesOfCode.ql

@@ -4,6 +4,7 @@
  * @description The total number of lines of JavaScript or TypeScript code across all files checked into the repository, except in `node_modules`. This is a useful metric of the size of a database. For all files that were seen during extraction, this query counts the lines of code, excluding whitespace or comments.
  * @kind metric
  * @tags summary
+ *       telemetry
  */
 
 import javascript

javascript/ql/src/change-notes/2024-03-07-lift-cg-restriction.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* The call graph has been improved, leading to more alerts for data flow based queries.

javascript/ql/src/change-notes/released/0.8.0.md

@@ -0,0 +1,3 @@
+## 0.8.0
+
+No user-facing changes.

javascript/ql/src/change-notes/released/0.8.1.md

@@ -1,4 +1,5 @@
----
-category: minorAnalysis
----
+## 0.8.1
+
+### Minor Analysis Improvements
+
 * Added the `AmdModuleDefinition::Range` class, making it possible to define custom aliases for the AMD `define` function.

javascript/ql/src/change-notes/released/0.8.2.md

@@ -0,0 +1,5 @@
+## 0.8.2
+
+### Minor Analysis Improvements
+
+* Added modeling for importing `express-rate-limit` using a named import.

javascript/ql/src/change-notes/released/0.8.3.md

@@ -0,0 +1,6 @@
+## 0.8.3
+
+### Query Metadata Changes
+
+* Lower the security severity of log-injection to medium.
+* Increase the security severity of XSS to high.

javascript/ql/src/change-notes/released/0.8.4.md

@@ -0,0 +1,5 @@
+## 0.8.4
+
+### Minor Analysis Improvements
+
+* Added django URLs to detected "safe" URL patterns in `js/unsafe-external-link`. 

javascript/ql/src/change-notes/released/0.8.5.md

@@ -0,0 +1,3 @@
+## 0.8.5
+
+No user-facing changes.

javascript/ql/src/change-notes/released/0.8.6.md

@@ -0,0 +1,3 @@
+## 0.8.6
+
+No user-facing changes.

javascript/ql/src/change-notes/released/0.8.7.md

@@ -0,0 +1,5 @@
+## 0.8.7
+
+### Minor Analysis Improvements
+
+* Added support for [doT](https://github.com/olado/doT) templates. 

javascript/ql/src/change-notes/released/0.8.8.md

@@ -0,0 +1,3 @@
+## 0.8.8
+
+No user-facing changes.

javascript/ql/src/change-notes/released/0.8.9.md

@@ -0,0 +1,5 @@
+## 0.8.9
+
+### Bug Fixes
+
+* The left operand of the `&&` operator no longer propagates data flow by default.

javascript/ql/src/codeql-pack.release.yml

@@ -1,2 +1,2 @@
 ---
-lastReleaseVersion: 0.7.5
+lastReleaseVersion: 0.8.9

javascript/ql/src/experimental/semmle/javascript/SQL.qll

@@ -0,0 +1,157 @@
+/**
+ * Provides classes for working with SQL connectors.
+ */
+
+import javascript
+
+module ExperimentalSql {
+  /**
+   * Provides SQL injection Sinks for the [TypeORM](https://www.npmjs.com/package/typeorm) package
+   */
+  private module TypeOrm {
+    /**
+     * Gets a `DataSource` instance
+     *
+     * `DataSource` is a pre-defined connection configuration to a specific database.
+     */
+    API::Node dataSource() {
+      result = API::moduleImport("typeorm").getMember("DataSource").getInstance()
+    }
+
+    /**
+     * Gets a `QueryRunner` instance
+     */
+    API::Node queryRunner() { result = dataSource().getMember("createQueryRunner").getReturn() }
+
+    /**
+     * Gets a `*QueryBuilder` instance
+     */
+    API::Node queryBuilderInstance() {
+      // a `*QueryBuilder` instance of a Data Mapper based Entity
+      result =
+        [
+          // Using DataSource
+          dataSource(),
+          // Using repository
+          dataSource().getMember("getRepository").getReturn(),
+          // Using entity manager
+          dataSource().getMember("manager"), queryRunner().getMember("manager")
+        ].getMember("createQueryBuilder").getReturn()
+      or
+      // A `*QueryBuilder` instance of an Active record based Entity
+      result =
+        API::moduleImport("typeorm")
+            .getMember("Entity")
+            .getReturn()
+            .getADecoratedClass()
+            .getMember("createQueryBuilder")
+            .getReturn()
+      or
+      // A WhereExpressionBuilder can be used in complex WHERE expression
+      result =
+        API::moduleImport("typeorm")
+            .getMember(["Brackets", "NotBrackets"])
+            .getParameter(0)
+            .getParameter(0)
+      or
+      // In case of custom query builders
+      result =
+        API::moduleImport("typeorm")
+            .getMember([
+                "SelectQueryBuilder", "InsertQueryBuilder", "RelationQueryBuilder",
+                "UpdateQueryBuilder", "WhereExpressionBuilder"
+              ])
+            .getInstance()
+    }
+
+    /**
+     *  Gets function names which create any type of `QueryBuilder` like `WhereExpressionBuilder` or `InsertQueryBuilder`
+     */
+    string queryBuilderMethods() {
+      result =
+        [
+          "select", "addSelect", "where", "andWhere", "orWhere", "having", "orHaving", "andHaving",
+          "orderBy", "addOrderBy", "distinctOn", "groupBy", "addCommonTableExpression",
+          "leftJoinAndSelect", "innerJoinAndSelect", "leftJoin", "innerJoin", "leftJoinAndMapOne",
+          "innerJoinAndMapOne", "leftJoinAndMapMany", "innerJoinAndMapMany", "orUpdate", "orIgnore",
+          "values", "set"
+        ]
+    }
+
+    /**
+     * Gets function names that the return values of these functions can be the results of a database query run
+     */
+    string queryBuilderResult() {
+      result = ["getOne", "getOneOrFail", "getMany", "getRawOne", "getRawMany", "stream"]
+    }
+
+    /**
+     * Gets a QueryBuilder instance that has a query builder function
+     */
+    API::Node getASuccessorOfBuilderInstance(string queryBuilderMethod) {
+      result.getMember(queryBuilderMethod) = queryBuilderInstance().getASuccessor*()
+    }
+
+    /**
+     *  A call to some successor functions of TypeORM `createQueryBuilder` function which are dangerous
+     */
+    private class QueryBuilderCall extends DatabaseAccess, DataFlow::Node {
+      API::Node queryBuilder;
+
+      QueryBuilderCall() {
+        queryBuilder = getASuccessorOfBuilderInstance(queryBuilderMethods()) and
+        this = queryBuilder.asSource()
+      }
+
+      override DataFlow::Node getAResult() {
+        result = queryBuilder.getMember(queryBuilderResult()).getReturn().asSource()
+      }
+
+      override DataFlow::Node getAQueryArgument() {
+        exists(string memberName | memberName = queryBuilderMethods() |
+          memberName = ["leftJoinAndSelect", "innerJoinAndSelect", "leftJoin", "innerJoin"] and
+          result = queryBuilder.getMember(memberName).getParameter(2).asSink()
+          or
+          memberName =
+            ["leftJoinAndMapOne", "innerJoinAndMapOne", "leftJoinAndMapMany", "innerJoinAndMapMany"] and
+          result = queryBuilder.getMember(memberName).getParameter(3).asSink()
+          or
+          memberName =
+            [
+              "select", "addSelect", "where", "andWhere", "orWhere", "having", "orHaving",
+              "andHaving", "orderBy", "addOrderBy", "distinctOn", "groupBy",
+              "addCommonTableExpression"
+            ] and
+          result = queryBuilder.getMember(memberName).getParameter(0).asSink()
+          or
+          memberName = ["orIgnore", "orUpdate"] and
+          result = queryBuilder.getMember(memberName).getParameter([0, 1]).asSink()
+          or
+          // following functions if use a function as their input fields,called function parameters which are vulnerable
+          memberName = ["values", "set"] and
+          result =
+            queryBuilder.getMember(memberName).getParameter(0).getAMember().getReturn().asSink()
+        )
+      }
+    }
+
+    /**
+     *  A call to the TypeORM `query` function of a `QueryRunner`
+     */
+    private class QueryRunner extends DatabaseAccess, API::CallNode {
+      QueryRunner() { queryRunner().getMember("query").getACall() = this }
+
+      override DataFlow::Node getAResult() { result = this }
+
+      override DataFlow::Node getAQueryArgument() { result = this.getArgument(0) }
+    }
+
+    /** An expression that is passed to the `query` function and hence interpreted as SQL. */
+    class QueryString extends SQL::SqlString {
+      QueryString() {
+        this = any(QueryRunner qr).getAQueryArgument() or
+        this = any(QueryBuilderCall qb).getAQueryArgument()
+      }
+    }
+  }
+}

javascript/ql/src/meta/analysis-quality/UnmodelledSteps.ql

@@ -9,7 +9,6 @@
  */
 
 import javascript
-import meta.MetaMetrics
 private import Expressions.ExprHasNoEffect
 import meta.internal.TaintMetrics
 

javascript/ql/src/qlpack.yml

@@ -1,5 +1,5 @@
 name: codeql/javascript-queries
-version: 0.8.0-dev
+version: 0.8.10-dev
 groups:
   - javascript
   - queries