diff --git a/change-notes/1.25/analysis-javascript.md b/change-notes/1.25/analysis-javascript.md index b47d69e3fa3f..b89d7b1bdda3 100644 --- a/change-notes/1.25/analysis-javascript.md +++ b/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. | diff --git a/javascript/ql/src/Security/CWE-829/InsecureDownload.qhelp b/javascript/ql/src/Security/CWE-829/InsecureDownload.qhelp new file mode 100644 index 000000000000..3db0aee40a23 --- /dev/null +++ b/javascript/ql/src/Security/CWE-829/InsecureDownload.qhelp @@ -0,0 +1,38 @@ + + + +

+ 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. +

+ + +

+ Use a secure transfer protocol when downloading executables or other sensitive files. +

+ + +

+ In this example, a server downloads a shell script from a remote URL using the node-fetch + library, and then executes this shell script. +

+ +

+ 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. +

+

+ The issue has been fixed in the example below by replacing the HTTP protocol with the HTTPS protocol. +

+ + + + +
  • OWASP: Man-in-the-middle attack.
    • +     +     diff --git a/javascript/ql/src/Security/CWE-829/InsecureDownload.ql b/javascript/ql/src/Security/CWE-829/InsecureDownload.ql new file mode 100644 index 000000000000..ce764d627a1a --- /dev/null +++ b/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" diff --git a/javascript/ql/src/Security/CWE-829/examples/insecure-download.js b/javascript/ql/src/Security/CWE-829/examples/insecure-download.js new file mode 100644 index 000000000000..96fbd1b2058d --- /dev/null +++ b/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)); \ No newline at end of file diff --git a/javascript/ql/src/Security/CWE-829/examples/secure-download.js b/javascript/ql/src/Security/CWE-829/examples/secure-download.js new file mode 100644 index 000000000000..9194efbf2305 --- /dev/null +++ b/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)); \ No newline at end of file diff --git a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll b/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll index 9d20e821c9ce..0a79fdd52b27 100644 --- a/javascript/ql/src/semmle/javascript/frameworks/ClientRequests.qll +++ b/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() } + } } diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownload.qll b/javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownload.qll new file mode 100644 index 000000000000..d9330949a288 --- /dev/null +++ b/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 } + } +} diff --git a/javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownloadCustomizations.qll b/javascript/ql/src/semmle/javascript/security/dataflow/InsecureDownloadCustomizations.qll new file mode 100644 index 000000000000..c8fa78b03009 --- /dev/null +++ b/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 } + } +} diff --git a/javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.expected b/javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.expected new file mode 100644 index 000000000000..377c61ff0724 --- /dev/null +++ b/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 | diff --git a/javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.qlref b/javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.qlref new file mode 100644 index 000000000000..0b5276834bb4 --- /dev/null +++ b/javascript/ql/test/query-tests/Security/CWE-829/InsecureDownload.qlref @@ -0,0 +1 @@ +Security/CWE-829/InsecureDownload.ql \ No newline at end of file diff --git a/javascript/ql/test/query-tests/Security/CWE-829/insecure-download.js b/javascript/ql/test/query-tests/Security/CWE-829/insecure-download.js new file mode 100644 index 000000000000..76c41d9845f6 --- /dev/null +++ b/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 +}