Skip to content

Commit 1548eca

Browse files
semmle-qlcisemmle-qlci
authored
Merge pull request github#3689 from erik-krogh/https-fix
Approved by mchammer01
2 parents e13353f + 0f5ef2c commit 1548eca

File tree

11 files changed

+290
-0
lines changed

11 files changed

+290
-0
lines changed

‎change-notes/1.25/analysis-javascript.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
| 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. |
3737
| 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. |
3838
| 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. |
39+
| 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. |
3940
| 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. |
4041
| 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. |
4142
| 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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>
7+
Downloading executeables or other sensitive files over an unencrypted connection
8+
can leave a server open to man-in-the-middle attacks (MITM).
9+
Such an attack can allow an attacker to insert arbitrary content
10+
into the downloaded file, and in the worst case, allow the attacker to execute
11+
arbitrary code on the vulnerable system.
12+
</p>
13+
</overview>
14+
<recommendation>
15+
<p>
16+
Use a secure transfer protocol when downloading executables or other sensitive files.
17+
</p>
18+
</recommendation>
19+
<example>
20+
<p>
21+
In this example, a server downloads a shell script from a remote URL using the <code>node-fetch</code>
22+
library, and then executes this shell script.
23+
</p>
24+
<sample src="examples/insecure-download.js" />
25+
<p>
26+
The HTTP protocol is vulnerable to MITM, and thus an attacker could potentially replace the downloaded
27+
shell script with arbitrary code, which gives the attacker complete control over the system.
28+
</p>
29+
<p>
30+
The issue has been fixed in the example below by replacing the HTTP protocol with the HTTPS protocol.
31+
</p>
32+
<sample src="examples/insecure-download.js" />
33+
</example>
34+
35+
<references>
36+
<li>OWASP: <a href="https://owasp.org/www-community/attacks/Man-in-the-middle_attack">Man-in-the-middle attack</a>.</li>
37+
</references>
38+
</qhelp>

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
/**
2+
* @name Download of sensitive file through insecure connection
3+
* @description Downloading executables and other sensitive files over an insecure connection
4+
* opens up for potential man-in-the-middle attacks.
5+
* @kind path-problem
6+
* @problem.severity error
7+
* @precision high
8+
* @id js/insecure-download
9+
* @tags security
10+
* external/cwe/cwe-829
11+
*/
12+
13+
import javascript
14+
import semmle.javascript.security.dataflow.InsecureDownload::InsecureDownload
15+
import DataFlow::PathGraph
16+
17+
from Configuration cfg, DataFlow::PathNode source, DataFlow::PathNode sink
18+
where cfg.hasFlowPath(source, sink)
19+
select sink.getNode(), source, sink, "$@ of sensitive file from $@.",
20+
sink.getNode().(Sink).getDownloadCall(), "Download", source.getNode(), "HTTP source"

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
const fetch = require("node-fetch");
2+
const cp = require("child_process");
3+
4+
fetch('http://mydownload.example.org/myscript.sh')
5+
.then(res => res.text())
6+
.then(script => cp.execSync(script));

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
const fetch = require("node-fetch");
2+
const cp = require("child_process");
3+
4+
fetch('https://mydownload.example.org/myscript.sh')
5+
.then(res => res.text())
6+
.then(script => cp.execSync(script));

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,4 +620,45 @@ module ClientRequest {
620620

621621
override DataFlow::Node getADataNode() { none() }
622622
}
623+
624+
/**
625+
* A call to `nugget` that downloads one of more files to a destination determined by an options object given as the second argument.
626+
*/
627+
class Nugget extends ClientRequest::Range, DataFlow::CallNode {
628+
Nugget() { this = DataFlow::moduleImport("nugget").getACall() }
629+
630+
override DataFlow::Node getUrl() { result = getArgument(0) }
631+
632+
override DataFlow::Node getHost() { none() }
633+
634+
override DataFlow::Node getADataNode() { none() }
635+
}
636+
637+
/**
638+
* A shell execution of `curl` that downloads some file.
639+
*/
640+
class CurlDownload extends ClientRequest::Range {
641+
SystemCommandExecution cmd;
642+
643+
CurlDownload() {
644+
this = cmd and
645+
(
646+
cmd.getACommandArgument().getStringValue() = "curl" or
647+
cmd
648+
.getACommandArgument()
649+
.(StringOps::ConcatenationRoot)
650+
.getConstantStringParts()
651+
.regexpMatch("curl .*")
652+
)
653+
}
654+
655+
override DataFlow::Node getUrl() {
656+
result = cmd.getArgumentList().getALocalSource().getAPropertyWrite().getRhs() or
657+
result = cmd.getACommandArgument().(StringOps::ConcatenationRoot).getALeaf()
658+
}
659+
660+
override DataFlow::Node getHost() { none() }
661+
662+
override DataFlow::Node getADataNode() { none() }
663+
}
623664
}

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
/**
2+
* Provides a taint tracking configuration for reasoning about download of sensitive file through insecure connection.
3+
*
4+
* Note, for performance reasons: only import this file if
5+
* `InsecureDownload::Configuration` is needed, otherwise
6+
* `InsecureDownloadCustomizations` should be imported instead.
7+
*/
8+
9+
import javascript
10+
11+
/**
12+
* Classes and predicates for reasoning about download of sensitive file through insecure connection vulnerabilities.
13+
*/
14+
module InsecureDownload {
15+
import InsecureDownloadCustomizations::InsecureDownload
16+
17+
/**
18+
* A taint tracking configuration for download of sensitive file through insecure connection.
19+
*/
20+
class Configuration extends DataFlow::Configuration {
21+
Configuration() { this = "InsecureDownload" }
22+
23+
override predicate isSource(DataFlow::Node source) { source instanceof Source }
24+
25+
override predicate isSink(DataFlow::Node sink) { sink instanceof Sink }
26+
}
27+
}

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

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
/**
2+
* Provides default sources, sinks and sanitizers for reasoning about
3+
* download of sensitive file through insecure connection, as well as
4+
* extension points for adding your own.
5+
*/
6+
7+
import javascript
8+
9+
/**
10+
* Classes and predicates for reasoning about download of sensitive file through insecure connection vulnerabilities.
11+
*/
12+
module InsecureDownload {
13+
/**
14+
* A data flow source for download of sensitive file through insecure connection.
15+
*/
16+
abstract class Source extends DataFlow::Node { }
17+
18+
/**
19+
* A data flow sink for download of sensitive file through insecure connection.
20+
*/
21+
abstract class Sink extends DataFlow::Node {
22+
/**
23+
* Gets the call that downloads the sensitive file.
24+
*/
25+
abstract DataFlow::Node getDownloadCall();
26+
}
27+
28+
/**
29+
* A sanitizer for download of sensitive file through insecure connection.
30+
*/
31+
abstract class Sanitizer extends DataFlow::Node { }
32+
33+
/**
34+
* A HTTP or FTP URL that refers to a file with a sensitive file extension,
35+
* seen as a source for download of sensitive file through insecure connection.
36+
*/
37+
class SensitiveFileUrl extends Source {
38+
SensitiveFileUrl() {
39+
exists(string str | str = this.getStringValue() |
40+
str.regexpMatch("http://.*|ftp://.*") and
41+
exists(string suffix | suffix = unsafeExtension() |
42+
str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
43+
)
44+
)
45+
}
46+
}
47+
48+
/**
49+
* Gets a file-extension that can potentially be dangerous.
50+
*
51+
* Archives are included, because they often contain source-code.
52+
*/
53+
string unsafeExtension() {
54+
result =
55+
["exe", "dmg", "pkg", "tar.gz", "zip", "sh", "bat", "cmd", "app", "apk", "msi", "dmg",
56+
"tar.gz", "zip", "js", "py", "jar", "war"]
57+
}
58+
59+
/**
60+
* A url downloaded by a client-request, seen as a sink for download of
61+
* sensitive file through insecure connection.a
62+
*/
63+
class ClientRequestURL extends Sink {
64+
ClientRequest request;
65+
66+
ClientRequestURL() { this = request.getUrl() }
67+
68+
override DataFlow::Node getDownloadCall() { result = request }
69+
}
70+
}

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,38 @@
1+
nodes
2+
| insecure-download.js:5:16:5:28 | installer.url |
3+
| insecure-download.js:5:16:5:28 | installer.url |
4+
| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' |
5+
| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' |
6+
| insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
7+
| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
8+
| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
9+
| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
10+
| insecure-download.js:36:9:36:45 | url |
11+
| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" |
12+
| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" |
13+
| insecure-download.js:37:23:37:25 | url |
14+
| insecure-download.js:37:23:37:25 | url |
15+
| insecure-download.js:39:26:39:28 | url |
16+
| insecure-download.js:39:26:39:28 | url |
17+
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
18+
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
19+
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
20+
edges
21+
| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
22+
| insecure-download.js:9:27:9:138 | 'http:/ ... ll.exe' | insecure-download.js:15:18:15:40 | buildTo ... llerUrl |
23+
| insecure-download.js:15:18:15:40 | buildTo ... llerUrl | insecure-download.js:5:16:5:28 | installer.url |
24+
| insecure-download.js:15:18:15:40 | buildTo ... llerUrl | insecure-download.js:5:16:5:28 | installer.url |
25+
| insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" | insecure-download.js:30:12:30:42 | "http:/ ... fe.APK" |
26+
| insecure-download.js:36:9:36:45 | url | insecure-download.js:37:23:37:25 | url |
27+
| insecure-download.js:36:9:36:45 | url | insecure-download.js:37:23:37:25 | url |
28+
| insecure-download.js:36:9:36:45 | url | insecure-download.js:39:26:39:28 | url |
29+
| insecure-download.js:36:9:36:45 | url | insecure-download.js:39:26:39:28 | url |
30+
| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
31+
| insecure-download.js:36:15:36:45 | "http:/ ... fe.APK" | insecure-download.js:36:9:36:45 | url |
32+
| insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" | insecure-download.js:41:12:41:41 | "ftp:// ... fe.APK" |
33+
#select
34+
| 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 |
35+
| 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 |
36+
| 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 |
37+
| 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 |
38+
| 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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Security/CWE-829/InsecureDownload.ql

0 commit comments

Comments
 (0)