JS: add simple query for detecting sensitive files downloaded over insecure connection

change-notes/1.25/analysis-javascript.md

@@ -36,6 +36,7 @@
 | Incomplete HTML attribute sanitization (`js/incomplete-html-attribute-sanitization`) | security, external/cwe/cwe-20, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities due to incomplete sanitization of HTML meta-characters. Results are shown on LGTM by default. |
 | Unsafe expansion of self-closing HTML tag (`js/unsafe-html-expansion`) | security, external/cwe/cwe-079, external/cwe/cwe-116 | Highlights potential XSS vulnerabilities caused by unsafe expansion of self-closing HTML tags. |
 | Unsafe shell command constructed from library input (`js/shell-command-constructed-from-input`) | correctness, security, external/cwe/cwe-078, external/cwe/cwe-088 | Highlights potential command injections due to a shell command being constructed from library inputs. Results are shown on LGTM by default. |
+| Download of sensitive file through insecure connection (`js/insecure-download`) | security, external/cwe/cwe-829 | Highlights downloads of sensitive files through an unencrypted protocol. Results are shown on LGTM by default. |
 | Exposure of private files (`js/exposure-of-private-files`) | security, external/cwe/cwe-200 | Highlights servers that serve private files. Results are shown on LGTM by default. |
 | Creating biased random numbers from a cryptographically secure source (`js/biased-cryptographic-random`) | security, external/cwe/cwe-327 | Highlights mathematical operations on cryptographically secure numbers that can create biased results. Results are shown on LGTM by default. |
 | Storage of sensitive information in build artifact (`js/build-artifact-leak`) | security, external/cwe/cwe-312 | Highlights storage of sensitive information in build artifacts. Results are shown on LGTM by default. |

javascript/ql/src/Security/CWE-829/InsecureDownload.qhelp

@@ -0,0 +1,38 @@
+<!DOCTYPE qhelp PUBLIC
+"-//Semmle//qhelp//EN"
+"qhelp.dtd">
+<qhelp>
+	<overview>
+		<p>
+			Downloading executeables or other sensitive files over an unencrypted connection 
+			can leave a server open to man-in-the-middle attacks (MITM). 
+			Such an attack can allow an attacker to insert arbitrary content 
+			into the downloaded file, and in the worst case, allow the attacker to execute 
+			arbitrary code on the vulnerable system.
+		</p>
+	</overview>
+	<recommendation>
+		<p>
+			Use a secure transfer protocol when downloading executables or other sensitive files.
+		</p>
+	</recommendation>
+	<example>
+		<p>
+			In this example, a server downloads a shell script from a remote URL using the <code>node-fetch</code> 
+			library, and then executes this shell script. 
+		</p>
+		<sample src="examples/insecure-download.js" />
+		<p>
+			The HTTP protocol is vulnerable to MITM, and thus an attacker could potentially replace the downloaded
+			shell script with arbitrary code, which gives the attacker complete control over the system.
+		</p>
+		<p>
+			The issue has been fixed in the example below by replacing the HTTP protocol with the HTTPS protocol.
+		</p>
+		<sample src="examples/insecure-download.js" />
+	</example>
+
+	<references>
+		<li>OWASP: <a href="https://owasp.org/www-community/attacks/Man-in-the-middle_attack">Man-in-the-middle attack</a>.</li>
+	</references>
+</qhelp>

javascript/ql/src/Security/CWE-829/InsecureDownload.ql

@@ -0,0 +1,20 @@
+/**
+ * @name Download of sensitive file through insecure connection
+ * @description Downloading executables and other sensitive files over an insecure connection
+ *              opens up for potential man-in-the-middle attacks.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id js/insecure-download
+ * @tags security
+ *       external/cwe/cwe-829
+ */
+
+import javascript
+import semmle.javascript.security.dataflow.InsecureDownload::InsecureDownload
+import DataFlow::PathGraph
+
+from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink, "$@ of sensitive file from $@.",
+  sink.getNode().(Sink).getDownloadCall(), "Download", source.getNode(), "HTTP source"

javascript/ql/src/Security/CWE-829/examples/insecure-download.js

@@ -0,0 +1,6 @@
+const fetch = require("node-fetch");
+const cp = require("child_process");
+
+fetch('http://mydownload.example.org/myscript.sh')
+    .then(res => res.text())
+    .then(script => cp.execSync(script));

javascript/ql/src/Security/CWE-829/examples/secure-download.js

@@ -0,0 +1,6 @@
+const fetch = require("node-fetch");
+const cp = require("child_process");
+
+fetch('https://mydownload.example.org/myscript.sh')
+    .then(res => res.text())
+    .then(script => cp.execSync(script));

javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll

@@ -620,4 +620,45 @@ module ClientRequest {
 
     override DataFlow::Node getADataNode() { none() }
   }
+
+  /**
+   * A call to `nugget` that downloads one of more files to a destination determined by an options object given as the second argument.
+   */
+  class Nugget extends ClientRequest::Range, DataFlow::CallNode {
+    Nugget() { this = DataFlow::moduleImport("nugget").getACall() }
+
+    override DataFlow::Node getUrl() { result = getArgument(0) }
+
+    override DataFlow::Node getHost() { none() }
+
+    override DataFlow::Node getADataNode() { none() }
+  }
+
+  /**
+   * A shell execution of `curl` that downloads some file.
+   */
+  class CurlDownload extends ClientRequest::Range {
+    SystemCommandExecution cmd;
+
+    CurlDownload() {
+      this = cmd and
+      (
+        cmd.getACommandArgument().getStringValue() = "curl" or
+        cmd
+            .getACommandArgument()
+            .(StringOps::ConcatenationRoot)
+            .getConstantStringParts()
+            .regexpMatch("curl .*")
+      )
+    }
+
+    override DataFlow::Node getUrl() {
+      result = cmd.getArgumentList().getALocalSource().getAPropertyWrite().getRhs() or
+      result = cmd.getACommandArgument().(StringOps::ConcatenationRoot).getALeaf()
+    }
+
+    override DataFlow::Node getHost() { none() }
+
+    override DataFlow::Node getADataNode() { none() }
+  }
 }

javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownload.qll

@@ -0,0 +1,27 @@
+/**
+ * Provides a taint tracking configuration for reasoning about download of sensitive file through insecure connection.
+ *
+ * Note, for performance reasons: only import this file if
+ * `InsecureDownload::Configuration` is needed, otherwise
+ * `InsecureDownloadCustomizations` should be imported instead.
+ */
+
+import javascript
+
+/**
+ * Classes and predicates for reasoning about download of sensitive file through insecure connection vulnerabilities.
+ */
+module InsecureDownload {
+  import InsecureDownloadCustomizations::InsecureDownload
+
+  /**
+   * A taint tracking configuration for download of sensitive file through insecure connection.
+   */
+  class Configuration extends DataFlow::Configuration {
+    Configuration() { this = "InsecureDownload" }
+
+    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+
+    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+  }
+}

javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownloadCustomizations.qll

@@ -0,0 +1,70 @@
+/**
+ * Provides default sources, sinks and sanitizers for reasoning about
+ * download of sensitive file through insecure connection, as well as
+ * extension points for adding your own.
+ */
+
+import javascript
+
+/**
+ * Classes and predicates for reasoning about download of sensitive file through insecure connection vulnerabilities.
+ */
+module InsecureDownload {
+  /**
+   * A data flow source for download of sensitive file through insecure connection.
+   */
+  abstract class Source extends DataFlow::Node { }
+
+  /**
+   * A data flow sink for download of sensitive file through insecure connection.
+   */
+  abstract class Sink extends DataFlow::Node {
+    /**
+     * Gets the call that downloads the sensitive file.
+     */
+    abstract DataFlow::Node getDownloadCall();
+  }
+
+  /**
+   * A sanitizer for download of sensitive file through insecure connection.
+   */
+  abstract class Sanitizer extends DataFlow::Node { }
+
+  /**
+   * A HTTP or FTP URL that refers to a file with a sensitive file extension,
+   * seen as a source for download of sensitive file through insecure connection.
+   */
+  class SensitiveFileUrl extends Source {
+    SensitiveFileUrl() {
+      exists(string str | str = this.getStringValue() |
+        str.regexpMatch("http://.*|ftp://.*") and
+        exists(string suffix | suffix = unsafeExtension() |
+          str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
+        )
+      )
+    }
+  }
+
+  /**
+   * Gets a file-extension that can potentially be dangerous.
+   *
+   * Archives are included, because they often contain source-code.
+   */
+  string unsafeExtension() {
+    result =
+      ["exe", "dmg", "pkg", "tar.gz", "zip", "sh", "bat", "cmd", "app", "apk", "msi", "dmg",
+          "tar.gz", "zip", "js", "py", "jar", "war"]
+  }
+
+  /**
+   * A url downloaded by a client-request, seen as a sink for download of
+   * sensitive file through insecure connection.a
+   */
+  class ClientRequestURL extends Sink {
+    ClientRequest request;
+
+    ClientRequestURL() { this = request.getUrl() }
+
+    override DataFlow::Node getDownloadCall() { result = request }
+  }
+}

javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.expected

@@ -0,0 +1,38 @@
+nodes
+| insecure-download.js:5:16:5:28 | installer.url |
+| insecure-download.js:5:16:5:28 | installer.url |
+| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' |
+| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' |
+| insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
+| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
+| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
+| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
+| insecure-download.js:36:9:36:45 | url |
+| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" |
+| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" |
+| insecure-download.js:37:23:37:25 | url |
+| insecure-download.js:37:23:37:25 | url |
+| insecure-download.js:39:26:39:28 | url |
+| insecure-download.js:39:26:39:28 | url |
+| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
+| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
+| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
+edges
+| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
+| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
+| insecure-download.js:15:18:15:40 | buildTo ... llerUrl | insecure-download.js:5:16:5:28 | installer.url |
+| insecure-download.js:15:18:15:40 | buildTo ... llerUrl | insecure-download.js:5:16:5:28 | installer.url |
+| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
+| insecure-download.js:36:9:36:45 | url | insecure-download.js:37:23:37:25 | url |
+| insecure-download.js:36:9:36:45 | url | insecure-download.js:37:23:37:25 | url |
+| insecure-download.js:36:9:36:45 | url | insecure-download.js:39:26:39:28 | url |
+| insecure-download.js:36:9:36:45 | url | insecure-download.js:39:26:39:28 | url |
+| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
+| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
+| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
+#select
+| insecure-download.js:5:16:5:28 | installer.url | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:5:16:5:28 | installer.url | $@ of sensitive file from $@. | insecure-download.js:5:9:5:44 | nugget( ... => { }) | Download | insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | HTTP source |
+| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | $@ of sensitive file from $@. | insecure-download.js:30:5:30:43 | nugget( ... e.APK") | Download | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | HTTP source |
+| insecure-download.js:37:23:37:25 | url | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:37:23:37:25 | url | $@ of sensitive file from $@. | insecure-download.js:37:5:37:42 | cp.exec ...  () {}) | Download | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | HTTP source |
+| insecure-download.js:39:26:39:28 | url | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:39:26:39:28 | url | $@ of sensitive file from $@. | insecure-download.js:39:5:39:46 | cp.exec ...  () {}) | Download | insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | HTTP source |
+| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | $@ of sensitive file from $@. | insecure-download.js:41:5:41:42 | nugget( ... e.APK") | Download | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | HTTP source |

javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.qlref

@@ -0,0 +1 @@
+Security/CWE-829/InsecureDownload.ql

javascript/ql/test/query-tests/Security/CWE-829/insecure-download.js

@@ -0,0 +1,42 @@
+const nugget = require('nugget');
+
+function foo() {
+    function downloadTools(installer) {
+        nugget(installer.url, {}, () => { }) // NOT OK
+    }
+    var constants = {
+        buildTools: {
+            installerUrl: 'http://download.microsoft.com/download/5/f/7/5f7acaeb-8363-451f-9425-68a90f98b238/visualcppbuildtools_full.exe'
+        }
+    }
+    function getBuildToolsInstallerPath() {
+        const buildTools = constants.buildTools
+        return {
+            url: buildTools.installerUrl
+        }
+    }
+
+    downloadTools(getBuildToolsInstallerPath())
+}
+
+
+const request = require('request');
+
+function bar() {
+    request('http://www.google.com', function () { }); // OK
+
+    nugget("https://download.microsoft.com/download/5/f/7/5f7acaeb-8363-451f-9425-68a90f98b238/visualcppbuildtools_full.exe") // OK
+
+    nugget("http://example.org/unsafe.APK") // NOT OK
+}
+
+var cp = require("child_process")
+
+function baz() {
+    var url = "http://example.org/unsafe.APK";
+    cp.exec("curl " + url, function () {}); // NOT OK
+
+    cp.execFile("curl", [url], function () {}); // NOT OK
+
+    nugget("ftp://example.org/unsafe.APK") // NOT OK
+}