JS: Detect insecure downloads that are saved to sensitive files.

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

@@ -68,6 +68,11 @@ class ClientRequest extends DataFlow::InvokeNode {
    * wrapped in a promise object.
    */
   DataFlow::Node getAResponseDataNode() { result = getAResponseDataNode(_, _) }
+
+  /**
+   * Gets a data-flow node that determines where in the file-system the result of the request should be saved.
+   */
+  DataFlow::Node getASavePath() { result = self.getASavePath() }
 }
 
 deprecated class CustomClientRequest = ClientRequest::Range;
@@ -103,6 +108,11 @@ module ClientRequest {
      * See the decription of `responseType` in `ClientRequest::getAResponseDataNode`.
      */
     DataFlow::Node getAResponseDataNode(string responseType, boolean promise) { none() }
+
+    /**
+     * Gets a data-flow node that determines where in the file-system the result of the request should be saved.
+     */
+    DataFlow::Node getASavePath() { none() }
   }
 
   /**
@@ -180,6 +190,14 @@ module ClientRequest {
     }
 
     override DataFlow::Node getADataNode() { result = getArgument(1) }
+
+    override DataFlow::Node getASavePath() {
+      exists(DataFlow::CallNode write |
+        write = DataFlow::moduleMember("fs", "createWriteStream").getACall() and
+        write = this.getAMemberCall("pipe").getArgument(0).getALocalSource() and
+        result = write.getArgument(0)
+      )
+    }
   }
 
   /** Gets the string `url` or `uri`. */
@@ -632,6 +650,10 @@ module ClientRequest {
     override DataFlow::Node getHost() { none() }
 
     override DataFlow::Node getADataNode() { none() }
+
+    override DataFlow::Node getASavePath() {
+      result = this.getArgument(1).getALocalSource().getAPropertyWrite("target").getRhs()
+    }
   }
 
   /**

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

@@ -20,8 +20,12 @@ module InsecureDownload {
   class Configuration extends DataFlow::Configuration {
     Configuration() { this = "InsecureDownload" }
 
-    override predicate isSource(DataFlow::Node source) { source instanceof Source }
+    override predicate isSource(DataFlow::Node source, DataFlow::FlowLabel label) {
+      source.(Source).getALabel() = label
+    }
 
-    override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
+    override predicate isSink(DataFlow::Node sink, DataFlow::FlowLabel label) {
+      sink.(Sink).getALabel() = label
+    }
   }
 }

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

@@ -13,7 +13,12 @@ module InsecureDownload {
   /**
    * A data flow source for download of sensitive file through insecure connection.
    */
-  abstract class Source extends DataFlow::Node { }
+  abstract class Source extends DataFlow::Node {
+    /**
+     * Gets a flow-label for this source.
+     */
+    abstract DataFlow::FlowLabel getALabel();
+  }
 
   /**
    * A data flow sink for download of sensitive file through insecure connection.
@@ -23,28 +28,67 @@ module InsecureDownload {
      * Gets the call that downloads the sensitive file.
      */
     abstract DataFlow::Node getDownloadCall();
+
+    /**
+     * Gets a flow-label where this sink is vulnerable.
+     */
+    abstract DataFlow::FlowLabel getALabel();
   }
 
   /**
    * A sanitizer for download of sensitive file through insecure connection.
    */
   abstract class Sanitizer extends DataFlow::Node { }
 
+  /**
+   * Flow-labels for reasoning about download of sensitive file through insecure connection.
+   */
+  module Label {
+    /**
+     * A flow-label for file URLs that are both sensitive and downloaded over an insecure connection.
+     */
+    class SensitiveInsecureURL extends DataFlow::FlowLabel {
+      SensitiveInsecureURL() { this = "sensitiveInsecure" }
+    }
+
+    /**
+     * A flow-label for a URL that is downloaded over an insecure connection.
+     */
+    class InsecureURL extends DataFlow::FlowLabel {
+      InsecureURL() { this = "insecure" }
+    }
+  }
+
   /**
    * 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 {
+    string str;
+
     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
-        )
-      )
+      str = this.getStringValue() and
+      str.regexpMatch("http://.*|ftp://.*")
+    }
+
+    override DataFlow::FlowLabel getALabel() {
+      result instanceof Label::InsecureURL
+      or
+      hasUnsafeExtension(str) and
+      result instanceof Label::SensitiveInsecureURL
     }
   }
 
+  /**
+   * Holds if `str` is a string that ends with an unsafe file extension.
+   */
+  bindingset[str]
+  predicate hasUnsafeExtension(string str) {
+    exists(string suffix | suffix = unsafeExtension() |
+      str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
+    )
+  }
+
   /**
    * Gets a file-extension that can potentially be dangerous.
    *
@@ -58,13 +102,48 @@ module InsecureDownload {
 
   /**
    * A url downloaded by a client-request, seen as a sink for download of
-   * sensitive file through insecure connection.a
+   * sensitive file through insecure connection.
    */
   class ClientRequestURL extends Sink {
     ClientRequest request;
 
     ClientRequestURL() { this = request.getUrl() }
 
     override DataFlow::Node getDownloadCall() { result = request }
+
+    override DataFlow::FlowLabel getALabel() {
+      result instanceof Label::SensitiveInsecureURL
+      or
+      hasUnsafeExtension(request.getASavePath().getStringValue()) and
+      result instanceof Label::InsecureURL
+    }
+  }
+
+  /**
+   * Gets a node for the response from `request`, type-tracked using `t`.
+   */
+  DataFlow::SourceNode clientRequestResponse(DataFlow::TypeTracker t, ClientRequest request) {
+    t.start() and
+    result = request.getAResponseDataNode()
+    or
+    exists(DataFlow::TypeTracker t2 | result = clientRequestResponse(t2, request).track(t2, t))
+  }
+
+  /**
+   * A url that is downloaded through an insecure connection, where the result ends up being saved to a sensitive location.
+   */
+  class FileWriteSink extends Sink {
+    ClientRequest request;
+    FileSystemWriteAccess write;
+
+    FileWriteSink() {
+      this = request.getUrl() and
+      clientRequestResponse(DataFlow::TypeTracker::end(), request).flowsTo(write.getADataNode()) and
+      hasUnsafeExtension(write.getAPathArgument().getStringValue())
+    }
+
+    override DataFlow::FlowLabel getALabel() { result instanceof Label::InsecureURL }
+
+    override DataFlow::Node getDownloadCall() { result = request }
   }
 }

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

@@ -17,6 +17,12 @@ nodes
 | 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" |
+| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
+| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
+| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
+| insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
+| insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
+| insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
 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 |
@@ -30,9 +36,13 @@ edges
 | 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" |
+| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" |
+| insecure-download.js:52:11:52:45 | "http:/ ... nknown" | insecure-download.js:52:11:52:45 | "http:/ ... nknown" |
 #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 |
+| insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | $@ of sensitive file from $@. | insecure-download.js:48:5:48:71 | nugget( ... => { }) | Download | insecure-download.js:48:12:48:38 | "http:/ ... unsafe" | HTTP source |
+| insecure-download.js:52:11:52:45 | "http:/ ... nknown" | insecure-download.js:52:11:52:45 | "http:/ ... nknown" | insecure-download.js:52:11:52:45 | "http:/ ... nknown" | $@ of sensitive file from $@. | insecure-download.js:52:5:54:6 | $.get(" ... \\n    }) | Download | insecure-download.js:52:11:52:45 | "http:/ ... nknown" | HTTP source |

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

@@ -40,3 +40,20 @@ function baz() {
 
     nugget("ftp://example.org/unsafe.APK") // NOT OK
 }
+
+const fs = require("fs");
+var writeFileAtomic = require("write-file-atomic");
+
+function test() {
+    nugget("http://example.org/unsafe", {target: "foo.exe"}, () => { }) // NOT OK
+
+    nugget("http://example.org/unsafe", {target: "foo.safe"}, () => { }) // OK
+
+    $.get("http://example.org/unsafe.unknown", function( data ) {
+        writeFileAtomic('unsafe.exe', data, {}, function (err) {}); // NOT OK
+    });
+
+    $.get("http://example.org/unsafe.unknown", function( data ) {
+        writeFileAtomic('foo.safe', data, {}, function (err) {}); // OK
+    });
+}