Merge branch 'main' into edoardo/3.5-mergeback

javascript/ql/src/Security/CWE-020/IncompleteUrlSchemeCheck.ql

@@ -14,6 +14,7 @@
  */
 
 import javascript
+import semmle.javascript.security.IncompleteBlacklistSanitizer as IncompleteBlacklistSanitizer
 
 /** A URL scheme that can be used to represent executable code. */
 class DangerousScheme extends string {
@@ -55,6 +56,21 @@ DataFlow::SourceNode schemeOf(DataFlow::Node url) {
   )
 }
 
+/**
+ * A chain of replace calls that replaces one or more dangerous schemes.
+ */
+class SchemeReplacementChain extends IncompleteBlacklistSanitizer::StringReplaceCallSequence {
+  SchemeReplacementChain() { this.getAMember().getAReplacedString() instanceof DangerousScheme }
+
+  /**
+   * Gets the source node that the replacement happens on.
+   * The result is the receiver of the first call in the chain.
+   */
+  DataFlow::Node getReplacementSource() {
+    result = this.getReceiver+() and not result instanceof DataFlow::MethodCallNode
+  }
+}
+
 /** Gets a data-flow node that checks `nd` against the given `scheme`. */
 DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
   // check of the form `nd.startsWith(scheme)`
@@ -73,6 +89,11 @@ DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
     schemeOf(nd).flowsTo(candidate)
   )
   or
+  exists(SchemeReplacementChain chain | result = chain |
+    scheme = chain.getAMember().getAReplacedString() and
+    nd = chain.getReplacementSource()
+  )
+  or
   // propagate through trimming, case conversion, and regexp replace
   exists(DataFlow::MethodCallNode stringop |
     stringop.getMethodName().matches("trim%") or

javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.ql

@@ -11,60 +11,4 @@
  *       external/cwe/cwe-020
  */
 
-import javascript
-private import semmle.javascript.dataflow.InferredTypes
-
-/**
- * A check on a string for whether it contains a given substring, possibly with restrictions on the location of the substring.
- */
-class SomeSubstringCheck extends DataFlow::Node {
-  DataFlow::Node substring;
-
-  SomeSubstringCheck() {
-    this.(StringOps::StartsWith).getSubstring() = substring or
-    this.(StringOps::Includes).getSubstring() = substring or
-    this.(StringOps::EndsWith).getSubstring() = substring
-  }
-
-  /**
-   * Gets the substring.
-   */
-  DataFlow::Node getSubstring() { result = substring }
-}
-
-from SomeSubstringCheck check, DataFlow::Node substring, string target, string msg
-where
-  substring = check.getSubstring() and
-  substring.mayHaveStringValue(target) and
-  (
-    // target contains a domain on a common TLD, and perhaps some other URL components
-    target
-        .regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+" + RegExpPatterns::getACommonTld() +
-            "(:[0-9]+)?/?")
-    or
-    // target is a HTTP URL to a domain on any TLD
-    target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/?")
-    or
-    // target is a HTTP URL to a domain on any TLD with path elements, and the check is an includes check
-    check instanceof StringOps::Includes and
-    target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/[a-z0-9/_-]+")
-  ) and
-  (
-    if check instanceof StringOps::StartsWith
-    then msg = "may be followed by an arbitrary host name"
-    else
-      if check instanceof StringOps::EndsWith
-      then msg = "may be preceded by an arbitrary host name"
-      else msg = "can be anywhere in the URL, and arbitrary hosts may come before or after it"
-  ) and
-  // whitelist
-  not (
-    // the leading dot in a subdomain sequence makes the suffix-check safe (if it is performed on the host of the url)
-    check instanceof StringOps::EndsWith and
-    target.regexpMatch("(?i)\\.([a-z0-9-]+)(\\.[a-z0-9-]+)+")
-    or
-    // the trailing port or slash makes the prefix-check safe
-    check instanceof StringOps::StartsWith and
-    target.regexpMatch(".*(:[0-9]+|/)")
-  )
-select check, "'$@' " + msg + ".", substring, target
+import IncompleteUrlSubstringSanitization

javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitization.qll

@@ -0,0 +1,62 @@
+/**
+ * Incomplete URL substring sanitization
+ */
+
+private import IncompleteUrlSubstringSanitizationSpecific
+
+/**
+ * A check on a string for whether it contains a given substring, possibly with restrictions on the location of the substring.
+ */
+class SomeSubstringCheck extends DataFlow::Node {
+  DataFlow::Node substring;
+
+  SomeSubstringCheck() {
+    this.(StringOps::StartsWith).getSubstring() = substring or
+    this.(StringOps::Includes).getSubstring() = substring or
+    this.(StringOps::EndsWith).getSubstring() = substring
+  }
+
+  /**
+   * Gets the substring.
+   */
+  DataFlow::Node getSubstring() { result = substring }
+}
+
+/** Holds if there is an incomplete URL substring sanitization problem */
+query predicate problems(
+  SomeSubstringCheck check, string msg, DataFlow::Node substring, string target
+) {
+  substring = check.getSubstring() and
+  mayHaveStringValue(substring, target) and
+  (
+    // target contains a domain on a common TLD, and perhaps some other URL components
+    target
+        .regexpMatch("(?i)([a-z]*:?//)?\\.?([a-z0-9-]+\\.)+" + RegExpPatterns::getACommonTld() +
+            "(:[0-9]+)?/?")
+    or
+    // target is a HTTP URL to a domain on any TLD
+    target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/?")
+    or
+    // target is a HTTP URL to a domain on any TLD with path elements, and the check is an includes check
+    check instanceof StringOps::Includes and
+    target.regexpMatch("(?i)https?://([a-z0-9-]+\\.)+([a-z]+)(:[0-9]+)?/[a-z0-9/_-]+")
+  ) and
+  (
+    if check instanceof StringOps::StartsWith
+    then msg = "'$@' may be followed by an arbitrary host name."
+    else
+      if check instanceof StringOps::EndsWith
+      then msg = "'$@' may be preceded by an arbitrary host name."
+      else msg = "'$@' can be anywhere in the URL, and arbitrary hosts may come before or after it."
+  ) and
+  // whitelist
+  not (
+    // the leading dot in a subdomain sequence makes the suffix-check safe (if it is performed on the host of the url)
+    check instanceof StringOps::EndsWith and
+    target.regexpMatch("(?i)\\.([a-z0-9-]+)(\\.[a-z0-9-]+)+")
+    or
+    // the trailing port or slash makes the prefix-check safe
+    check instanceof StringOps::StartsWith and
+    target.regexpMatch(".*(:[0-9]+|/)")
+  )
+}

javascript/ql/src/Security/CWE-020/IncompleteUrlSubstringSanitizationSpecific.qll

@@ -0,0 +1,5 @@
+import javascript
+import semmle.javascript.dataflow.InferredTypes
+
+/** Holds if `node` may evaluate to `value` */
+predicate mayHaveStringValue(DataFlow::Node node, string value) { node.mayHaveStringValue(value) }