Merge branch 'github:main' into maikypedia/javascript-cors

javascript/ql/src/CHANGELOG.md

@@ -1,3 +1,20 @@
+## 0.7.5
+
+### Bug Fixes
+
+* Fixed an extractor crash that could occur in projects containing TypeScript files larger than 10 MB.
+
+## 0.7.4
+
+### Minor Analysis Improvements
+
+* Files larger than 10 MB are no longer be extracted or analyzed.
+* Imports can now be resolved in more cases, where a non-constant string expression is passed to a `require()` call.
+
+### Bug Fixes
+
+* Fixed an extractor crash that would occur in rare cases when a TypeScript file contains a self-referential namespace alias.
+
 ## 0.7.3
 
 No user-facing changes.

javascript/ql/src/Security/CWE-116/IncompleteMultiCharacterSanitization.qhelp

@@ -1,8 +1,137 @@
 <!DOCTYPE qhelp PUBLIC
-"-//Semmle//qhelp//EN"
-"qhelp.dtd">
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
 <qhelp>
 
-	<include src="IncompleteSanitization.qhelp" />
+<overview>
+<p>
+Sanitizing untrusted input is a common technique for preventing injection attacks and other security 
+vulnerabilities. Regular expressions are often used to perform this sanitization. However, when the 
+regular expression matches multiple consecutive characters, replacing it just once 
+can result in the unsafe text reappearing in the sanitized input.
+</p>
+<p>
+Attackers can exploit this issue by crafting inputs that, when sanitized with an ineffective regular 
+expression, still contain malicious code or content. This can lead to code execution, data exposure, 
+or other vulnerabilities.
+</p>
+</overview>
 
+<recommendation>
+<p>
+To prevent this issue, it is highly recommended to use a well-tested sanitization library whenever 
+possible. These libraries are more likely to handle corner cases and ensure effective sanitization.
+</p>
+
+<p>
+If a library is not an option, you can consider alternative strategies to fix the issue. For example, 
+applying the regular expression replacement repeatedly until no more replacements can be performed, or rewriting the regular 
+expression to match single characters instead of the entire unsafe text.
+</p>
+</recommendation>
+
+<example>
+<p>
+Consider the following JavaScript code that aims to remove all HTML comment start and end tags:
+</p>
+
+<sample language="javascript">
+str.replace(/&lt;!--|--!?&gt;/g, "");   
+</sample>
+
+<p>
+Given the input string "&lt;!&lt;!--- comment ---&gt;&gt;", the output will be "&lt;!-- comment --&gt;", 
+which still contains an HTML comment.
+</p>
+
+<p>
+One possible fix for this issue is to apply the regular expression replacement repeatedly until no
+more replacements can be performed. This ensures that the unsafe text does not re-appear in the sanitized input, effectively 
+removing all instances of the targeted pattern:
+</p>
+
+<sample language="javascript">
+function removeHtmlComments(input) {  
+  let previous;  
+  do {  
+    previous = input;  
+    input = input.replace(/&lt;!--|--!?&gt;/g, "");  
+  } while (input !== previous);  
+  return input;  
+}  
+</sample>
+</example>
+
+<example>
+<p>
+Another example is the following regular expression intended to remove script tags:
+</p>
+
+<sample language="javascript">
+str.replace(/&lt;script\b[^&lt;]*(?:(?!&lt;\/script&gt;)&lt;[^&lt;]*)*&lt;\/script&gt;/g, "");  
+</sample>
+
+<p>
+If the input string is "&lt;scrip&lt;script&gt;is removed&lt;/script&gt;t&gt;alert(123)&lt;/script&gt;", 
+the output will be "&lt;script&gt;alert(123)&lt;/script&gt;", which still contains a script tag.
+</p>
+<p>
+A fix for this issue is to rewrite the regular expression to match single characters 
+("&lt;" and "&gt;") instead of the entire unsafe text. This simplifies the sanitization process 
+and ensures that all potentially unsafe characters are removed:
+</p>
+<sample language="javascript">
+function removeAllHtmlTags(input) {  
+  return input.replace(/&lt;|&gt;/g, "");  
+}
+</sample>
+<p>
+Another potential fix is to use the popular <code>sanitize-html</code> npm library. 
+It keeps most of the safe HTML tags while removing all unsafe tags and attributes.
+</p>
+<sample language="javascript">
+const sanitizeHtml = require("sanitize-html");
+function removeAllHtmlTags(input) {  
+  return sanitizeHtml(input);  
+}
+</sample>
+
+</example>
+
+<example>
+<p>
+Lastly, consider a path sanitizer using the regular expression <code>/\.\.\//</code>:
+</p>
+
+<sample language="javascript">
+str.replace(/\.\.\//g, "");  
+</sample>
+
+<p>
+The regular expression attempts to strip out all occurences of <code>/../</code> from <code>str</code>.
+This will not work as expected: for the string <code>/./.././</code>, for example, it will remove the single
+occurrence of <code>/../</code> in the middle, but the remainder of the string then becomes
+<code>/../</code>, which is another instance of the substring we were trying to remove.
+</p>
+
+<p>
+A possible fix for this issue is to use the "sanitize-filename" npm library for path sanitization. 
+This library is specifically designed to handle path sanitization, and should handle all corner cases
+and ensure effective sanitization:
+</p>
+
+<sample language="javascript">
+const sanitize = require("sanitize-filename");  
+  
+function sanitizePath(input) {  
+  return sanitize(input);  
+}  
+</sample>
+
+</example>
+
+<references>
+<li>OWASP Top 10: <a href="https://www.owasp.org/index.php/Top_10-2017_A1-Injection">A1 Injection</a>.</li>
+<li>Stack Overflow: <a href="https://stackoverflow.com/questions/6659351/removing-all-script-tags-from-html-with-js-regular-expression">Removing all script tags from HTML with JS regular expression</a>.</li>
+</references>
 </qhelp>

javascript/ql/src/Security/CWE-116/IncompleteSanitization.qhelp

@@ -43,18 +43,6 @@ needed, for instance by using prepared statements for SQL queries.
 Otherwise, make sure to use a regular expression with the <code>g</code> flag to ensure that
 all occurrences are replaced, and remember to escape backslashes if applicable.
 </p>
-<p>
-Note, however, that this is generally <i>not</i> sufficient for replacing multi-character strings:
-the <code>String.prototype.replace</code> method only performs one pass over the input string,
-and will not replace further instances of the string that result from earlier replacements.
-</p>
-<p>
-For example, consider the code snippet <code>s.replace(/\/\.\.\//g, "")</code>, which attempts
-to strip out all occurences of <code>/../</code> from <code>s</code>. This will not work as
-expected: for the string <code>/./.././</code>, for example, it will remove the single
-occurrence of <code>/../</code> in the middle, but the remainder of the string then becomes
-<code>/../</code>, which is another instance of the substring we were trying to remove.
-</p>
 </recommendation>
 
 <example>

javascript/ql/src/Security/CWE-400/RemotePropertyInjection.expected

@@ -1 +0,0 @@
-| examples/RemotePropertyInjection.js:8:8:8:11 | prop | A $@ is used as a property name to write to. | examples/RemotePropertyInjection.js:7:13:7:36 | req.que ... trolled | user-provided value |

javascript/ql/src/Security/CWE-601/ServerSideUrlRedirect.qhelp

@@ -16,6 +16,10 @@ To guard against untrusted URL redirection, it is advisable to avoid putting use
 directly into a redirect URL. Instead, maintain a list of authorized
 redirects on the server; then choose from that list based on the user input provided.
 </p>
+<p>
+If this is not possible, then the user input should be validated in some other way,
+for example, by verifying that the target URL is on the same host as the current page.
+</p>
 </recommendation>
 
 <example>
@@ -32,6 +36,21 @@ before doing the redirection:
 </p>
 
 <sample src="examples/ServerSideUrlRedirectGood.js"/>
+
+<p>
+Alternatively, we can check that the target URL does not redirect to a different host
+by parsing it relative to a base URL with a known host and verifying that the host
+stays the same:
+</p>
+
+<sample src="examples/ServerSideUrlRedirectGood2.js"/>
+
+<p>
+Note that as written, the above code will allow redirects to URLs on <code>example.com</code>,
+which is harmless but perhaps not intended. You can substitute your own domain (if known) for
+<code>example.com</code> to prevent this.
+</p>
+
 </example>
 
 <references>

javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirect.js

@@ -1,6 +1,6 @@
 const app = require("express")();
 
-app.get('/some/path', function(req, res) {
+app.get("/redirect", function (req, res) {
   // BAD: a request parameter is incorporated without validation into a URL redirect
-  res.redirect(req.param("target"));
+  res.redirect(req.query["target"]);
 });

javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood.js

@@ -2,9 +2,12 @@ const app = require("express")();
 
 const VALID_REDIRECT = "http://cwe.mitre.org/data/definitions/601.html";
 
-app.get('/some/path', function(req, res) {
+app.get("/redirect", function (req, res) {
   // GOOD: the request parameter is validated against a known fixed string
-  let target = req.param("target");
-  if (VALID_REDIRECT === target)
+  let target = req.query["target"];
+  if (VALID_REDIRECT === target) {
     res.redirect(target);
+  } else {
+    res.redirect("/");
+  }
 });

javascript/ql/src/Security/CWE-601/examples/ServerSideUrlRedirectGood2.js

@@ -0,0 +1,22 @@
+const app = require("express")();
+
+function isLocalUrl(path) {
+  try {
+    return (
+      // TODO: consider substituting your own domain for example.com
+      new URL(path, "https://example.com").origin === "https://example.com"
+    );
+  } catch (e) {
+    return false;
+  }
+}
+
+app.get("/redirect", function (req, res) {
+  // GOOD: check that we don't redirect to a different host
+  let target = req.query["target"];
+  if (isLocalUrl(target)) {
+    res.redirect(target);
+  } else {
+    res.redirect("/");
+  }
+});

javascript/ql/src/change-notes/2023-08-23-fix-cyclic-alias-extraction.md

@@ -1,4 +0,0 @@
----
-category: fix
----
-* Fixed an extractor crash that would occur in rare cases when a TypeScript file contains a self-referential namespace alias.

javascript/ql/src/change-notes/2023-08-23-ignore-huge-files.md

@@ -1,4 +0,0 @@
----
-category: minorAnalysis
----
-* Files larger than 10 MB are no longer be extracted or analyzed.

javascript/ql/src/change-notes/2023-08-23-import-path-string.md

@@ -1,4 +0,0 @@
----
-category: minorAnalysis
----
-* Imports can now be resolved in more cases, where a non-constant string expression is passed to a `require()` call.

javascript/ql/src/change-notes/2023-10-05-amd-range.md

@@ -0,0 +1,4 @@
+---
+category: minorAnalysis
+---
+* Added the `AmdModuleDefinition::Range` class, making it possible to define custom aliases for the AMD `define` function.

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

@@ -0,0 +1,10 @@
+## 0.7.4
+
+### Minor Analysis Improvements
+
+* Files larger than 10 MB are no longer be extracted or analyzed.
+* Imports can now be resolved in more cases, where a non-constant string expression is passed to a `require()` call.
+
+### Bug Fixes
+
+* Fixed an extractor crash that would occur in rare cases when a TypeScript file contains a self-referential namespace alias.

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

@@ -0,0 +1,5 @@
+## 0.7.5
+
+### Bug Fixes
+
+* Fixed an extractor crash that could occur in projects containing TypeScript files larger than 10 MB.

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

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

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

@@ -1,4 +0,0 @@
-/** DEPRECATED: Use `semmle.javascript.Actions` instead. */
-deprecated module Actions {
-  import semmle.javascript.Actions::Actions
-}

javascript/ql/src/qlpack.yml

@@ -1,6 +1,6 @@
 name: codeql/javascript-queries
-version: 0.7.4-dev
-groups: 
+version: 0.8.0-dev
+groups:
   - javascript
   - queries
 suites: codeql-suites