Skip to content

Commit ba1d43d

Browse files
hmachmac
authored
Merge pull request github#8658 from hmac/hmac/insecure-download
Ruby: Add InsecureDownload query
2 parents f4453f4 + 992cc51 commit ba1d43d

File tree

12 files changed

+421
-4
lines changed

12 files changed

+421
-4
lines changed

‎ruby/ql/lib/codeql/ruby/frameworks/http_clients/NetHttp.qll

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,10 @@ private import codeql.ruby.DataFlow
2121
class NetHttpRequest extends HTTP::Client::Request::Range {
2222
private DataFlow::CallNode request;
2323
private DataFlow::Node responseBody;
24+
private API::Node requestNode;
2425

2526
NetHttpRequest() {
26-
exists(API::Node requestNode, string method |
27+
exists(string method |
2728
request = requestNode.getAnImmediateUse() and
2829
this = request.asExpr().getExpr()
2930
|
@@ -48,10 +49,19 @@ class NetHttpRequest extends HTTP::Client::Request::Range {
4849
}
4950

5051
/**
51-
* Gets the node representing the URL of the request.
52-
* Currently unused, but may be useful in future, e.g. to filter out certain requests.
52+
* Gets a node that contributes to the URL of the request.
5353
*/
54-
override DataFlow::Node getAUrlPart() { result = request.getArgument(0) }
54+
override DataFlow::Node getAUrlPart() {
55+
result = request.getArgument(0)
56+
or
57+
// Net::HTTP.new(...).get(...)
58+
exists(API::Node new |
59+
new = API::getTopLevelMember("Net").getMember("HTTP").getInstance() and
60+
requestNode = new.getReturn(_)
61+
|
62+
result = new.getAnImmediateUse().(DataFlow::CallNode).getArgument(0)
63+
)
64+
}
5565

5666
override DataFlow::Node getResponseBody() { result = responseBody }
5767

‎ruby/ql/lib/codeql/ruby/security/InsecureDownloadCustomizations.qll

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,197 @@
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+
private import ruby
8+
private import codeql.ruby.DataFlow
9+
private import codeql.ruby.Concepts
10+
private import codeql.ruby.typetracking.TypeTracker
11+
private import codeql.ruby.frameworks.Files
12+
13+
/**
14+
* Classes and predicates for reasoning about download of sensitive file through insecure connection vulnerabilities.
15+
*/
16+
module InsecureDownload {
17+
/**
18+
* A data flow source for download of sensitive file through insecure connection.
19+
*/
20+
abstract class Source extends DataFlow::Node {
21+
/**
22+
* Gets a flow-label for this source.
23+
*/
24+
abstract DataFlow::FlowState getALabel();
25+
}
26+
27+
/**
28+
* A data flow sink for download of sensitive file through insecure connection.
29+
*/
30+
abstract class Sink extends DataFlow::Node {
31+
/**
32+
* Gets the call that downloads the sensitive file.
33+
*/
34+
abstract DataFlow::Node getDownloadCall();
35+
36+
/**
37+
* Gets a flow-label where this sink is vulnerable.
38+
*/
39+
abstract DataFlow::FlowState getALabel();
40+
}
41+
42+
/**
43+
* A sanitizer for download of sensitive file through insecure connection.
44+
*/
45+
abstract class Sanitizer extends DataFlow::Node { }
46+
47+
/**
48+
* Flow-labels for reasoning about download of sensitive file through insecure connection.
49+
*/
50+
module Label {
51+
/**
52+
* A flow-label for a URL that is downloaded over an insecure connection.
53+
*/
54+
class Insecure extends DataFlow::FlowState {
55+
Insecure() { this = "insecure" }
56+
}
57+
58+
/**
59+
* A flow-label for a URL that is sensitive.
60+
*/
61+
class Sensitive extends DataFlow::FlowState {
62+
Sensitive() { this = "sensitive" }
63+
}
64+
65+
/**
66+
* A flow-label for file URLs that are both sensitive and downloaded over an insecure connection.
67+
*/
68+
class SensitiveInsecure extends DataFlow::FlowState {
69+
SensitiveInsecure() { this = "sensitiveInsecure" }
70+
}
71+
}
72+
73+
/**
74+
* A HTTP or FTP URL.
75+
*/
76+
class InsecureUrl extends DataFlow::Node {
77+
string str;
78+
79+
InsecureUrl() {
80+
str = this.asExpr().getConstantValue().getString() and
81+
str.regexpMatch("http://.*|ftp://.*")
82+
}
83+
}
84+
85+
/**
86+
* A HTTP or FTP URL that refers to a file with a sensitive file extension,
87+
* seen as a source for downloads of sensitive files through an insecure connection.
88+
*/
89+
class InsecureFileUrl extends Source, InsecureUrl {
90+
override DataFlow::FlowState getALabel() {
91+
result instanceof Label::Insecure
92+
or
93+
hasUnsafeExtension(str) and
94+
result instanceof Label::SensitiveInsecure
95+
}
96+
}
97+
98+
/**
99+
* A string containing a sensitive file extension,
100+
* seen as a source for downloads of sensitive files through an insecure connection.
101+
*/
102+
class SensitiveFileName extends Source {
103+
SensitiveFileName() { hasUnsafeExtension(this.asExpr().getConstantValue().getString()) }
104+
105+
override DataFlow::FlowState getALabel() { result instanceof Label::Sensitive }
106+
}
107+
108+
/**
109+
* Holds if `str` is a string that ends with an unsafe file extension.
110+
*/
111+
bindingset[str]
112+
predicate hasUnsafeExtension(string str) {
113+
exists(string suffix | suffix = unsafeExtension() |
114+
str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
115+
)
116+
}
117+
118+
/**
119+
* Gets a file-extension that can potentially be dangerous.
120+
*
121+
* Archives are included, because they often contain source-code.
122+
*/
123+
string unsafeExtension() {
124+
result =
125+
[
126+
"exe", "dmg", "pkg", "tar.gz", "zip", "sh", "bat", "cmd", "app", "apk", "msi", "dmg",
127+
"tar.gz", "zip", "js", "py", "jar", "war"
128+
]
129+
}
130+
131+
/**
132+
* A response from an outgoing HTTP request.
133+
* This is a sink if there are both insecure and sensitive parts of the URL.
134+
* In other words, if the URL is HTTP and the extension is in `unsafeExtension()`.
135+
*/
136+
private class HttpResponseAsSink extends Sink {
137+
private HTTP::Client::Request req;
138+
139+
HttpResponseAsSink() {
140+
this = req.getAUrlPart() and
141+
// If any part of the URL has an unsafe extension, we consider all parts of the URL to be sinks.
142+
hasUnsafeExtension(req.getAUrlPart().asExpr().getConstantValue().getString())
143+
}
144+
145+
override DataFlow::Node getDownloadCall() { result.asExpr().getExpr() = req }
146+
147+
override DataFlow::FlowState getALabel() {
148+
result instanceof Label::SensitiveInsecure
149+
or
150+
any(req.getAUrlPart()) instanceof InsecureUrl and result instanceof Label::Sensitive
151+
}
152+
}
153+
154+
/**
155+
* Gets a node for the response from `request`, type-tracked using `t`.
156+
*/
157+
DataFlow::LocalSourceNode clientRequestResponse(TypeTracker t, HTTP::Client::Request request) {
158+
t.start() and
159+
result = request.getResponseBody()
160+
or
161+
exists(TypeTracker t2 | result = clientRequestResponse(t2, request).track(t2, t))
162+
}
163+
164+
/**
165+
* A url that is downloaded through an insecure connection, where the result ends up being saved to a sensitive location.
166+
*/
167+
class FileWriteSink extends Sink {
168+
HTTP::Client::Request request;
169+
170+
FileWriteSink() {
171+
// For example, in:
172+
//
173+
// ```rb
174+
// f = File.open("foo.exe")
175+
// f.write(Excon.get(...).body) # $BAD=
176+
// ```
177+
//
178+
// `f` is the `FileSystemAccess` and the call `f.write` is the `IO::FileWriter`.
179+
// The call `Excon.get` is the `HTTP::Client::Request`.
180+
//
181+
// The `file = write` alternative models this case:
182+
// ```rb
183+
// File.write("foo.exe", Excon.get(...).body)
184+
// ```
185+
exists(IO::FileWriter write, FileSystemAccess file |
186+
this = request.getAUrlPart() and
187+
clientRequestResponse(TypeTracker::end(), request).flowsTo(write.getADataNode()) and
188+
(file.(DataFlow::LocalSourceNode).flowsTo(write.getReceiver()) or file = write) and
189+
hasUnsafeExtension(file.getAPathArgument().asExpr().getConstantValue().getString())
190+
)
191+
}
192+
193+
override DataFlow::FlowState getALabel() { result instanceof Label::Insecure }
194+
195+
override DataFlow::Node getDownloadCall() { result.asExpr().getExpr() = request }
196+
}
197+
}

‎ruby/ql/lib/codeql/ruby/security/InsecureDownloadQuery.qll

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/**
2+
* Provides a dataflow configuration for reasoning about the 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+
private import ruby
10+
private import codeql.ruby.DataFlow
11+
import InsecureDownloadCustomizations::InsecureDownload
12+
13+
/**
14+
* A taint tracking configuration for download of sensitive file through insecure connection.
15+
*/
16+
class Configuration extends DataFlow::Configuration {
17+
Configuration() { this = "InsecureDownload" }
18+
19+
override predicate isSource(DataFlow::Node source, DataFlow::FlowState label) {
20+
source.(Source).getALabel() = label
21+
}
22+
23+
override predicate isSink(DataFlow::Node sink, DataFlow::FlowState label) {
24+
sink.(Sink).getALabel() = label
25+
}
26+
27+
override predicate isBarrier(DataFlow::Node node) {
28+
super.isBarrier(node) or
29+
node instanceof Sanitizer
30+
}
31+
}

‎ruby/ql/src/change-notes/2022-04-14-rb-insecure-download.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: newQuery
3+
---
4+
* Added a new query, `rb/insecure-download`. The query finds cases where executables and other sensitive files are downloaded over an insecure connection, which may allow for man-in-the-middle attacks.

‎ruby/ql/src/queries/security/cwe-829/InsecureDownload.qhelp

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
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 and then executes the script.
22+
</p>
23+
<sample src="examples/insecure_download.rb" />
24+
<p>
25+
The HTTP protocol is vulnerable to MITM, and thus an attacker could potentially replace the downloaded
26+
shell script with arbitrary code, which gives the attacker complete control over the system.
27+
</p>
28+
<p>
29+
The issue has been fixed in the example below by replacing the HTTP protocol with the HTTPS protocol.
30+
</p>
31+
<sample src="examples/secure_download.rb" />
32+
</example>
33+
34+
<references>
35+
<li>Wikipedia: <a href="https://en.wikipedia.org/wiki/Man-in-the-middle_attack">Man-in-the-middle attack</a></li>
36+
</references>
37+
</qhelp>

‎ruby/ql/src/queries/security/cwe-829/InsecureDownload.ql

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

‎ruby/ql/src/queries/security/cwe-829/examples/insecure_download.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
require "net/http"
2+
3+
script = Net::HTTP.new("http://mydownload.example.org").get("/myscript.sh").body
4+
system(script)

‎ruby/ql/src/queries/security/cwe-829/examples/secure_download.rb

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
require "net/http"
2+
3+
script = Net::HTTP.new("https://mydownload.example.org").get("/myscript.sh").body
4+
system(script)

‎ruby/ql/test/library-tests/frameworks/http_clients/HttpClients.expected

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,13 @@
4848
| NetHttp.rb:6:8:6:50 | call to post | Net::HTTP | NetHttp.rb:6:23:6:36 | call to parse | NetHttp.rb:7:1:7:9 | call to body |
4949
| NetHttp.rb:6:8:6:50 | call to post | Net::HTTP | NetHttp.rb:6:23:6:36 | call to parse | NetHttp.rb:8:1:8:14 | call to read_body |
5050
| NetHttp.rb:6:8:6:50 | call to post | Net::HTTP | NetHttp.rb:6:23:6:36 | call to parse | NetHttp.rb:9:1:9:11 | call to entity |
51+
| NetHttp.rb:13:6:13:17 | call to get | Net::HTTP | NetHttp.rb:11:21:11:41 | "https://example.com" | NetHttp.rb:18:1:18:7 | call to body |
5152
| NetHttp.rb:13:6:13:17 | call to get | Net::HTTP | NetHttp.rb:13:14:13:16 | "/" | NetHttp.rb:18:1:18:7 | call to body |
53+
| NetHttp.rb:14:6:14:18 | call to post | Net::HTTP | NetHttp.rb:11:21:11:41 | "https://example.com" | NetHttp.rb:19:1:19:12 | call to read_body |
5254
| NetHttp.rb:14:6:14:18 | call to post | Net::HTTP | NetHttp.rb:14:15:14:17 | "/" | NetHttp.rb:19:1:19:12 | call to read_body |
55+
| NetHttp.rb:15:6:15:17 | call to put | Net::HTTP | NetHttp.rb:11:21:11:41 | "https://example.com" | NetHttp.rb:20:1:20:9 | call to entity |
5356
| NetHttp.rb:15:6:15:17 | call to put | Net::HTTP | NetHttp.rb:15:14:15:16 | "/" | NetHttp.rb:20:1:20:9 | call to entity |
57+
| NetHttp.rb:24:3:24:33 | call to get | Net::HTTP | NetHttp.rb:24:17:24:22 | domain | NetHttp.rb:27:1:27:28 | call to body |
5458
| NetHttp.rb:24:3:24:33 | call to get | Net::HTTP | NetHttp.rb:24:29:24:32 | path | NetHttp.rb:27:1:27:28 | call to body |
5559
| OpenURI.rb:3:9:3:41 | call to open | OpenURI | OpenURI.rb:3:21:3:40 | "http://example.com" | OpenURI.rb:4:1:4:10 | call to read |
5660
| OpenURI.rb:6:9:6:34 | call to open | OpenURI | OpenURI.rb:6:14:6:33 | "http://example.com" | OpenURI.rb:7:1:7:15 | call to readlines |

‎ruby/ql/test/query-tests/security/cwe-829/InsecureDownload.expected

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
failures
2+
edges
3+
| insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | insecure_download.rb:33:15:33:17 | url |
4+
| insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | insecure_download.rb:33:15:33:17 | url |
5+
nodes
6+
| insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | semmle.label | "http://example.org/unsafe.APK" |
7+
| insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | semmle.label | "http://example.org/unsafe.APK" |
8+
| insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | semmle.label | "http://example.org/unsafe.APK" : |
9+
| insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | semmle.label | "http://example.org/unsafe.APK" : |
10+
| insecure_download.rb:33:15:33:17 | url | semmle.label | url |
11+
| insecure_download.rb:33:15:33:17 | url | semmle.label | url |
12+
| insecure_download.rb:37:42:37:68 | "http://example.org/unsafe" | semmle.label | "http://example.org/unsafe" |
13+
| insecure_download.rb:41:37:41:63 | "http://example.org/unsafe" | semmle.label | "http://example.org/unsafe" |
14+
| insecure_download.rb:43:22:43:56 | "http://example.org/unsafe.unk..." | semmle.label | "http://example.org/unsafe.unk..." |
15+
| insecure_download.rb:53:65:53:78 | "/myscript.sh" | semmle.label | "/myscript.sh" |
16+
subpaths
17+
#select
18+
| insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | $@ | insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | "http://example.org/unsafe.APK" |
19+
| insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | $@ | insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | "http://example.org/unsafe.APK" |
20+
| insecure_download.rb:33:15:33:17 | url | insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | insecure_download.rb:33:15:33:17 | url | $@ | insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | "http://example.org/unsafe.APK" : |
21+
| insecure_download.rb:33:15:33:17 | url | insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | insecure_download.rb:33:15:33:17 | url | $@ | insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | "http://example.org/unsafe.APK" : |
22+
| insecure_download.rb:33:15:33:17 | url | insecure_download.rb:33:15:33:17 | url | insecure_download.rb:33:15:33:17 | url | $@ | insecure_download.rb:33:15:33:17 | url | url |
23+
| insecure_download.rb:33:15:33:17 | url | insecure_download.rb:33:15:33:17 | url | insecure_download.rb:33:15:33:17 | url | $@ | insecure_download.rb:33:15:33:17 | url | url |
24+
| insecure_download.rb:37:42:37:68 | "http://example.org/unsafe" | insecure_download.rb:37:42:37:68 | "http://example.org/unsafe" | insecure_download.rb:37:42:37:68 | "http://example.org/unsafe" | $@ | insecure_download.rb:37:42:37:68 | "http://example.org/unsafe" | "http://example.org/unsafe" |
25+
| insecure_download.rb:41:37:41:63 | "http://example.org/unsafe" | insecure_download.rb:41:37:41:63 | "http://example.org/unsafe" | insecure_download.rb:41:37:41:63 | "http://example.org/unsafe" | $@ | insecure_download.rb:41:37:41:63 | "http://example.org/unsafe" | "http://example.org/unsafe" |
26+
| insecure_download.rb:43:22:43:56 | "http://example.org/unsafe.unk..." | insecure_download.rb:43:22:43:56 | "http://example.org/unsafe.unk..." | insecure_download.rb:43:22:43:56 | "http://example.org/unsafe.unk..." | $@ | insecure_download.rb:43:22:43:56 | "http://example.org/unsafe.unk..." | "http://example.org/unsafe.unk..." |
27+
| insecure_download.rb:53:65:53:78 | "/myscript.sh" | insecure_download.rb:53:65:53:78 | "/myscript.sh" | insecure_download.rb:53:65:53:78 | "/myscript.sh" | $@ | insecure_download.rb:53:65:53:78 | "/myscript.sh" | "/myscript.sh" |

0 commit comments

Comments
 (0)