Skip to content

Commit bb3fb03

Browse files
hmachmac
committed
Ruby: Add InsecureDownload query
This query finds cases where a potentially unsafe file is downloaded over an unsecured connection.
1 parent ce7675e commit bb3fb03

File tree

9 files changed

+404
-0
lines changed

9 files changed

+404
-0
lines changed

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

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,201 @@
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 SensitiveFileUrl extends Source {
103+
string str;
104+
105+
SensitiveFileUrl() {
106+
str = this.asExpr().getConstantValue().getString() and
107+
hasUnsafeExtension(str)
108+
}
109+
110+
override DataFlow::FlowState getALabel() { result instanceof Label::Sensitive }
111+
}
112+
113+
/**
114+
* Holds if `str` is a string that ends with an unsafe file extension.
115+
*/
116+
bindingset[str]
117+
predicate hasUnsafeExtension(string str) {
118+
exists(string suffix | suffix = unsafeExtension() |
119+
str.suffix(str.length() - suffix.length() - 1).toLowerCase() = "." + suffix
120+
)
121+
}
122+
123+
/**
124+
* Gets a file-extension that can potentially be dangerous.
125+
*
126+
* Archives are included, because they often contain source-code.
127+
*/
128+
string unsafeExtension() {
129+
result =
130+
[
131+
"exe", "dmg", "pkg", "tar.gz", "zip", "sh", "bat", "cmd", "app", "apk", "msi", "dmg",
132+
"tar.gz", "zip", "js", "py", "jar", "war"
133+
]
134+
}
135+
136+
/**
137+
* A response from an outgoing HTTP request, considered as a flow sink for
138+
* downloading a sensitive file through an insecure connection.
139+
*/
140+
private class HttpResponseAsSink extends Sink {
141+
private HTTP::Client::Request req;
142+
143+
HttpResponseAsSink() {
144+
this = req.getAUrlPart() and
145+
// If any part of the URL has an unsafe extension, we consider all parts of the URL to be sinks.
146+
hasUnsafeExtension(req.getAUrlPart().asExpr().getConstantValue().getString())
147+
}
148+
149+
override DataFlow::Node getDownloadCall() { result.asExpr().getExpr() = req }
150+
151+
override DataFlow::FlowState getALabel() {
152+
result instanceof Label::SensitiveInsecure
153+
or
154+
any(req.getAUrlPart()) instanceof InsecureUrl and result instanceof Label::Sensitive
155+
}
156+
}
157+
158+
/**
159+
* Gets a node for the response from `request`, type-tracked using `t`.
160+
*/
161+
DataFlow::LocalSourceNode clientRequestResponse(TypeTracker t, HTTP::Client::Request request) {
162+
t.start() and
163+
result = request.getResponseBody()
164+
or
165+
exists(TypeTracker t2 | result = clientRequestResponse(t2, request).track(t2, t))
166+
}
167+
168+
/**
169+
* A url that is downloaded through an insecure connection, where the result ends up being saved to a sensitive location.
170+
*/
171+
class FileWriteSink extends Sink {
172+
HTTP::Client::Request request;
173+
174+
FileWriteSink() {
175+
// For example, in:
176+
//
177+
// ```rb
178+
// f = File.open("foo.exe")
179+
// f.write(Excon.get(...).body) # $BAD=
180+
// ```
181+
//
182+
// `f` is the `FileSystemAccess` and the call `f.write` is the `IO::FileWriter`.
183+
// The call `Excon.get` is the `HTTP::Client::Request`.
184+
//
185+
// The `file = write` alternative models this case:
186+
// ```rb
187+
// File.write("foo.exe", Excon.get(...).body)
188+
// ```
189+
exists(IO::FileWriter write, FileSystemAccess file |
190+
this = request.getAUrlPart() and
191+
clientRequestResponse(TypeTracker::end(), request).flowsTo(write.getADataNode()) and
192+
(file.(DataFlow::LocalSourceNode).flowsTo(write.getReceiver()) or file = write) and
193+
hasUnsafeExtension(file.getAPathArgument().asExpr().getConstantValue().getString())
194+
)
195+
}
196+
197+
override DataFlow::FlowState getALabel() { result instanceof Label::Insecure }
198+
199+
override DataFlow::Node getDownloadCall() { result.asExpr().getExpr() = request }
200+
}
201+
}

‎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 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+
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/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+
* opens up for potential 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/query-tests/security/cwe-829/InsecureDownload.expected

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
failures
2+
| insecure_download.rb:5:36:5:63 | # $BAD= (requires hash flow) | Missing result:BAD= |
3+
edges
4+
| insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | insecure_download.rb:33:15:33:17 | url |
5+
| insecure_download.rb:31:11:31:41 | "http://example.org/unsafe.APK" : | insecure_download.rb:33:15:33:17 | url |
6+
nodes
7+
| insecure_download.rb:27:15:27:45 | "http://example.org/unsafe.APK" | semmle.label | "http://example.org/unsafe.APK" |
8+
| insecure_download.rb:27:15:27:45 | "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:31:11:31:41 | "http://example.org/unsafe.APK" : | semmle.label | "http://example.org/unsafe.APK" : |
11+
| insecure_download.rb:33:15:33:17 | url | semmle.label | url |
12+
| insecure_download.rb:33:15:33:17 | url | semmle.label | url |
13+
| insecure_download.rb:37:42:37:68 | "http://example.org/unsafe" | semmle.label | "http://example.org/unsafe" |
14+
| insecure_download.rb:41:37:41:63 | "http://example.org/unsafe" | semmle.label | "http://example.org/unsafe" |
15+
| insecure_download.rb:43:22:43:56 | "http://example.org/unsafe.unk..." | semmle.label | "http://example.org/unsafe.unk..." |
16+
| insecure_download.rb:53:65:53:78 | "/myscript.sh" | semmle.label | "/myscript.sh" |
17+
subpaths
18+
#select
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: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" |
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: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" : |
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: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 |
25+
| 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" |
26+
| 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" |
27+
| 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..." |
28+
| 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" |

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import ruby
2+
import codeql.ruby.DataFlow
3+
import PathGraph
4+
import TestUtilities.InlineFlowTest
5+
import codeql.ruby.security.InsecureDownloadQuery
6+
7+
class FlowTest extends InlineFlowTest {
8+
override DataFlow::Configuration getValueFlowConfig() { result = any(Configuration config) }
9+
10+
override DataFlow::Configuration getTaintFlowConfig() { none() }
11+
12+
override string getARelevantTag() { result = "BAD" }
13+
14+
override predicate hasActualResult(Location location, string element, string tag, string value) {
15+
tag = "BAD" and
16+
super.hasActualResult(location, element, "hasValueFlow", value)
17+
}
18+
}
19+
20+
from DataFlow::PathNode source, DataFlow::PathNode sink, Configuration conf
21+
where conf.hasFlowPath(source, sink)
22+
select sink, source, sink, "$@", source, source.toString()

0 commit comments

Comments
 (0)