Merge pull request github#3183 from asger-semmle/js/bad-url-scheme-check

Approved by esbena

Author: semmle-qlci

change-notes/1.24/analysis-javascript.md

@@ -86,6 +86,7 @@
 | Useless regular-expression character escape (`js/useless-regexp-character-escape`) | Fewer false positive results | This query now distinguishes escapes in strings and regular expression literals. |
 | Identical operands (`js/redundant-operation`) | Fewer results | This query now recognizes cases where the operands change a value using ++/-- expressions. |
 | Superfluous trailing arguments (`js/superfluous-trailing-arguments`) | Fewer results | This query now recognizes cases where a function uses the `Function.arguments` value to process a variable number of parameters. |
+| Incomplete URL scheme check (`js/incomplete-url-scheme-check`) | More results | This query now recognizes more variations of URL scheme checks. |
 
 ## Changes to libraries
 

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

@@ -12,11 +12,46 @@
  */
 
 import javascript
-import semmle.javascript.dataflow.internal.AccessPaths
 
 /** A URL scheme that can be used to represent executable code. */
 class DangerousScheme extends string {
   DangerousScheme() { this = "data:" or this = "javascript:" or this = "vbscript:" }
+
+  /** Gets the name of this scheme without the `:`. */
+  string getWithoutColon() { this = result + ":" }
+
+  /** Gets the name of this scheme, with or without the `:`. */
+  string getWithOrWithoutColon() { result = this or result = getWithoutColon() }
+}
+
+/** Returns a node that refers to the scheme of `url`. */
+DataFlow::SourceNode schemeOf(DataFlow::Node url) {
+  // url.split(":")[0]
+  exists(DataFlow::MethodCallNode split |
+    split.getMethodName() = "split" and
+    split.getArgument(0).getStringValue() = ":" and
+    result = split.getAPropertyRead("0") and
+    url = split.getReceiver()
+  )
+  or
+  // url.getScheme(), url.getProtocol(), getScheme(url), getProtocol(url)
+  exists(DataFlow::CallNode call |
+    result = call and
+    (call.getCalleeName() = "getScheme" or call.getCalleeName() = "getProtocol")
+  |
+    call.getNumArgument() = 1 and
+    url = call.getArgument(0)
+    or
+    call.getNumArgument() = 0 and
+    url = call.getReceiver()
+  )
+  or
+  // url.scheme, url.protocol
+  exists(DataFlow::PropRead prop |
+    result = prop and
+    (prop.getPropertyName() = "scheme" or prop.getPropertyName() = "protocol") and
+    url = prop.getBase()
+  )
 }
 
 /** Gets a data-flow node that checks `nd` against the given `scheme`. */
@@ -27,6 +62,20 @@ DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
     sw.getSubstring().mayHaveStringValue(scheme)
   )
   or
+  // check of the form `array.includes(getScheme(nd))`
+  exists(InclusionTest test, DataFlow::ArrayCreationNode array | test = result |
+    schemeOf(nd).flowsTo(test.getContainedNode()) and
+    array.flowsTo(test.getContainerNode()) and
+    array.getAnElement().mayHaveStringValue(scheme.getWithOrWithoutColon())
+  )
+  or
+  // check of the form `getScheme(nd) === scheme`
+  exists(EqualityTest test, Expr op1, Expr op2 | test.flow() = result |
+    test.hasOperands(op1, op2) and
+    schemeOf(nd).flowsToExpr(op1) and
+    op2.mayHaveStringValue(scheme.getWithOrWithoutColon())
+  )
+  or
   // propagate through trimming, case conversion, and regexp replace
   exists(DataFlow::MethodCallNode stringop |
     stringop.getMethodName().matches("trim%") or
@@ -42,14 +91,14 @@ DataFlow::Node schemeCheck(DataFlow::Node nd, DangerousScheme scheme) {
 }
 
 /** Gets a data-flow node that checks an instance of `ap` against the given `scheme`. */
-DataFlow::Node schemeCheckOn(AccessPath ap, DangerousScheme scheme) {
-  result = schemeCheck(ap.getAnInstance().flow(), scheme)
+DataFlow::Node schemeCheckOn(DataFlow::SourceNode root, string path, DangerousScheme scheme) {
+  result = schemeCheck(AccessPath::getAReferenceTo(root, path), scheme)
 }
 
-from AccessPath ap, int n
+from DataFlow::SourceNode root, string path, int n
 where
   n = strictcount(DangerousScheme s) and
-  strictcount(DangerousScheme s | exists(schemeCheckOn(ap, s))) < n
-select schemeCheckOn(ap, "javascript:"),
+  strictcount(DangerousScheme s | exists(schemeCheckOn(root, path, s))) < n
+select schemeCheckOn(root, path, "javascript:"),
   "This check does not consider " +
-    strictconcat(DangerousScheme s | not exists(schemeCheckOn(ap, s)) | s, " and ") + "."
+    strictconcat(DangerousScheme s | not exists(schemeCheckOn(root, path, s)) | s, " and ") + "."

javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.expected

@@ -1 +1,5 @@
-| IncompleteUrlSchemeCheck.js:3:9:3:35 | u.start ... ript:") | This check does not consider data: and vbscript:. |
+| IncompleteUrlSchemeCheck.js:5:9:5:35 | u.start ... ript:") | This check does not consider data: and vbscript:. |
+| IncompleteUrlSchemeCheck.js:16:9:16:39 | badProt ... otocol) | This check does not consider vbscript:. |
+| IncompleteUrlSchemeCheck.js:23:9:23:43 | badProt ... scheme) | This check does not consider vbscript:. |
+| IncompleteUrlSchemeCheck.js:30:9:30:43 | badProt ... scheme) | This check does not consider vbscript:. |
+| IncompleteUrlSchemeCheck.js:37:9:37:31 | scheme  ... script" | This check does not consider data: and vbscript:. |

javascript/ql/test/query-tests/Security/CWE-020/IncompleteUrlSchemeCheck.js

@@ -1,6 +1,47 @@
+import * as dummy from 'dummy';
+
 function sanitizeUrl(url) {
     let u = decodeURI(url).trim().toLowerCase();
-    if (u.startsWith("javascript:"))
+    if (u.startsWith("javascript:")) // NOT OK
+        return "about:blank";
+    return url;
+}
+
+let badProtocols = ['javascript:', 'data:'];
+let badProtocolNoColon = ['javascript', 'data'];
+let badProtocolsGood = ['javascript:', 'data:', 'vbscript:'];
+
+function test2(url) {
+    let protocol = new URL(url).protocol;
+    if (badProtocols.includes(protocol)) // NOT OK
+        return "about:blank";
+    return url;
+}
+
+function test3(url) {
+    let scheme = goog.uri.utils.getScheme(url);
+    if (badProtocolNoColon.includes(scheme)) // NOT OK
+        return "about:blank";
+    return url;
+}
+
+function test4(url) {
+    let scheme = url.split(':')[0];
+    if (badProtocolNoColon.includes(scheme)) // NOT OK
+        return "about:blank";
+    return url;
+}
+
+function test5(url) {
+    let scheme = url.split(':')[0];
+    if (scheme === "javascript") // NOT OK
+        return "about:blank";
+    return url;
+}
+
+function test6(url) {
+    let protocol = new URL(url).protocol;
+    if (badProtocolsGood.includes(protocol)) // OK
         return "about:blank";
     return url;
 }