Skip to content

Commit 06e91c1

Browse files
aibaarsaibaars
authored
Merge pull request #322 from github/request-without-validation
rb/request-without-cert-validation
2 parents 398ed4c + ceef976 commit 06e91c1

29 files changed

+938
-128
lines changed

‎ql/lib/codeql/ruby/Concepts.qll

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -419,6 +419,15 @@ module HTTP {
419419

420420
/** Gets a string that identifies the framework used for this request. */
421421
string getFramework() { result = super.getFramework() }
422+
423+
/**
424+
* Holds if this request is made using a mode that disables SSL/TLS
425+
* certificate validation, where `disablingNode` represents the point at
426+
* which the validation was disabled.
427+
*/
428+
predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
429+
super.disablesCertificateValidation(disablingNode)
430+
}
422431
}
423432

424433
/** Provides a class for modeling new HTTP requests. */
@@ -435,6 +444,13 @@ module HTTP {
435444

436445
/** Gets a string that identifies the framework used for this request. */
437446
abstract string getFramework();
447+
448+
/**
449+
* Holds if this request is made using a mode that disables SSL/TLS
450+
* certificate validation, where `disablingNode` represents the point at
451+
* which the validation was disabled.
452+
*/
453+
abstract predicate disablesCertificateValidation(DataFlow::Node disablingNode);
438454
}
439455
}
440456

‎ql/lib/codeql/ruby/ast/Literal.qll

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -194,6 +194,13 @@ class BooleanLiteral extends Literal, TBooleanLiteral {
194194

195195
/** Holds if the Boolean literal is `false` or `FALSE`. */
196196
predicate isFalse() { none() }
197+
198+
/** Gets the value of this Boolean literal. */
199+
boolean getValue() {
200+
this.isTrue() and result = true
201+
or
202+
this.isFalse() and result = false
203+
}
197204
}
198205

199206
private class TrueLiteral extends BooleanLiteral, TTrueLiteral {
@@ -750,7 +757,7 @@ class HashLiteral extends Literal, THashLiteral {
750757
final override string getAPrimaryQlClass() { result = "HashLiteral" }
751758

752759
/**
753-
* Gets the `n`th element in this array literal.
760+
* Gets the `n`th element in this hash literal.
754761
*
755762
* In the following example, the 0th element is a `Pair`, and the 1st element
756763
* is a `HashSplatExpr`.
@@ -761,7 +768,7 @@ class HashLiteral extends Literal, THashLiteral {
761768
*/
762769
final Expr getElement(int n) { toGenerated(result) = g.getChild(n) }
763770

764-
/** Gets an element in this array literal. */
771+
/** Gets an element in this hash literal. */
765772
final Expr getAnElement() { result = this.getElement(_) }
766773

767774
/** Gets a key-value `Pair` in this hash literal. */

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

Lines changed: 103 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -18,29 +18,113 @@ private import codeql.ruby.ApiGraphs
1818
* https://github.com/excon/excon/blob/master/README.md
1919
*/
2020
class ExconHttpRequest extends HTTP::Client::Request::Range {
21-
DataFlow::Node request;
22-
DataFlow::CallNode responseBody;
21+
DataFlow::Node requestUse;
22+
API::Node requestNode;
23+
API::Node connectionNode;
2324

2425
ExconHttpRequest() {
25-
exists(API::Node requestNode | request = requestNode.getAnImmediateUse() |
26-
requestNode =
27-
[
28-
// one-off requests
29-
API::getTopLevelMember("Excon"),
30-
// connection re-use
31-
API::getTopLevelMember("Excon").getInstance()
32-
]
33-
.getReturn([
34-
// Excon#request exists but Excon.request doesn't.
35-
// This shouldn't be a problem - in real code the latter would raise NoMethodError anyway.
36-
"get", "head", "delete", "options", "post", "put", "patch", "trace", "request"
37-
]) and
38-
responseBody = requestNode.getAMethodCall("body") and
39-
this = request.asExpr().getExpr()
40-
)
26+
requestUse = requestNode.getAnImmediateUse() and
27+
connectionNode =
28+
[
29+
// one-off requests
30+
API::getTopLevelMember("Excon"),
31+
// connection re-use
32+
API::getTopLevelMember("Excon").getInstance(),
33+
API::getTopLevelMember("Excon").getMember("Connection").getInstance()
34+
] and
35+
requestNode =
36+
connectionNode
37+
.getReturn([
38+
// Excon#request exists but Excon.request doesn't.
39+
// This shouldn't be a problem - in real code the latter would raise NoMethodError anyway.
40+
"get", "head", "delete", "options", "post", "put", "patch", "trace", "request"
41+
]) and
42+
this = requestUse.asExpr().getExpr()
4143
}
4244

43-
override DataFlow::Node getResponseBody() { result = responseBody }
45+
override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }
46+
47+
override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
48+
// Check for `ssl_verify_peer: false` in the options hash.
49+
exists(DataFlow::Node arg, int i |
50+
i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i)
51+
|
52+
argSetsVerifyPeer(arg, false, disablingNode)
53+
)
54+
or
55+
// Or we see a call to `Excon.defaults[:ssl_verify_peer] = false` before the
56+
// request, and no `ssl_verify_peer: true` in the explicit options hash for
57+
// the request call.
58+
exists(DataFlow::CallNode disableCall |
59+
setsDefaultVerification(disableCall, false) and
60+
disableCall.asExpr().getASuccessor+() = requestUse.asExpr() and
61+
disablingNode = disableCall and
62+
not exists(DataFlow::Node arg, int i |
63+
i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i)
64+
|
65+
argSetsVerifyPeer(arg, true, _)
66+
)
67+
)
68+
}
4469

4570
override string getFramework() { result = "Excon" }
4671
}
72+
73+
/**
74+
* Holds if `arg` represents an options hash that contains the key
75+
* `:ssl_verify_peer` with `value`, where `kvNode` is the data-flow node for
76+
* this key-value pair.
77+
*/
78+
predicate argSetsVerifyPeer(DataFlow::Node arg, boolean value, DataFlow::Node kvNode) {
79+
// Either passed as an individual key:value argument, e.g.:
80+
// Excon.get(..., ssl_verify_peer: false)
81+
isSslVerifyPeerPair(arg.asExpr().getExpr(), value) and
82+
kvNode = arg
83+
or
84+
// Or as a single hash argument, e.g.:
85+
// Excon.get(..., { ssl_verify_peer: false, ... })
86+
exists(DataFlow::LocalSourceNode optionsNode, Pair p |
87+
p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and
88+
isSslVerifyPeerPair(p, value) and
89+
optionsNode.flowsTo(arg) and
90+
kvNode.asExpr().getExpr() = p
91+
)
92+
}
93+
94+
/**
95+
* Holds if `callNode` sets `Excon.defaults[:ssl_verify_peer]` or
96+
* `Excon.ssl_verify_peer` to `value`.
97+
*/
98+
private predicate setsDefaultVerification(DataFlow::CallNode callNode, boolean value) {
99+
callNode = API::getTopLevelMember("Excon").getReturn("defaults").getAMethodCall("[]=") and
100+
isSslVerifyPeerLiteral(callNode.getArgument(0)) and
101+
hasBooleanValue(callNode.getArgument(1), value)
102+
or
103+
callNode = API::getTopLevelMember("Excon").getAMethodCall("ssl_verify_peer=") and
104+
hasBooleanValue(callNode.getArgument(0), value)
105+
}
106+
107+
private predicate isSslVerifyPeerLiteral(DataFlow::Node node) {
108+
exists(DataFlow::LocalSourceNode literal |
109+
literal.asExpr().getExpr().(SymbolLiteral).getValueText() = "ssl_verify_peer" and
110+
literal.flowsTo(node)
111+
)
112+
}
113+
114+
/** Holds if `node` can contain `value`. */
115+
private predicate hasBooleanValue(DataFlow::Node node, boolean value) {
116+
exists(DataFlow::LocalSourceNode literal |
117+
literal.asExpr().getExpr().(BooleanLiteral).getValue() = value and
118+
literal.flowsTo(node)
119+
)
120+
}
121+
122+
/** Holds if `p` is the pair `ssl_verify_peer: <value>`. */
123+
private predicate isSslVerifyPeerPair(Pair p, boolean value) {
124+
exists(DataFlow::Node key, DataFlow::Node valueNode |
125+
key.asExpr().getExpr() = p.getKey() and valueNode.asExpr().getExpr() = p.getValue()
126+
|
127+
isSslVerifyPeerLiteral(key) and
128+
hasBooleanValue(valueNode, value)
129+
)
130+
}

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

Lines changed: 117 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,127 @@ private import codeql.ruby.ApiGraphs
1414
* ```
1515
*/
1616
class FaradayHttpRequest extends HTTP::Client::Request::Range {
17-
DataFlow::Node request;
18-
DataFlow::CallNode responseBody;
17+
DataFlow::Node requestUse;
18+
API::Node requestNode;
19+
API::Node connectionNode;
1920

2021
FaradayHttpRequest() {
21-
exists(API::Node requestNode |
22-
requestNode =
23-
[
24-
// one-off requests
25-
API::getTopLevelMember("Faraday"),
26-
// connection re-use
27-
API::getTopLevelMember("Faraday").getInstance()
28-
].getReturn(["get", "head", "delete", "post", "put", "patch", "trace"]) and
29-
responseBody = requestNode.getAMethodCall("body") and
30-
request = requestNode.getAnImmediateUse() and
31-
this = request.asExpr().getExpr()
32-
)
22+
connectionNode =
23+
[
24+
// one-off requests
25+
API::getTopLevelMember("Faraday"),
26+
// connection re-use
27+
API::getTopLevelMember("Faraday").getInstance()
28+
] and
29+
requestNode =
30+
connectionNode.getReturn(["get", "head", "delete", "post", "put", "patch", "trace"]) and
31+
requestUse = requestNode.getAnImmediateUse() and
32+
this = requestUse.asExpr().getExpr()
3333
}
3434

35-
override DataFlow::Node getResponseBody() { result = responseBody }
35+
override DataFlow::Node getResponseBody() { result = requestNode.getAMethodCall("body") }
36+
37+
override predicate disablesCertificateValidation(DataFlow::Node disablingNode) {
38+
// `Faraday::new` takes an options hash as its second argument, and we're
39+
// looking for
40+
// `{ ssl: { verify: false } }`
41+
// or
42+
// `{ ssl: { verify_mode: OpenSSL::SSL::VERIFY_NONE } }`
43+
exists(DataFlow::Node arg, int i |
44+
i > 0 and arg = connectionNode.getAUse().(DataFlow::CallNode).getArgument(i)
45+
|
46+
// Either passed as an individual key:value argument, e.g.:
47+
// Faraday.new(..., ssl: {...})
48+
isSslOptionsPairDisablingValidation(arg.asExpr().getExpr()) and
49+
disablingNode = arg
50+
or
51+
// Or as a single hash argument, e.g.:
52+
// Faraday.new(..., { ssl: {...} })
53+
exists(DataFlow::LocalSourceNode optionsNode, Pair p |
54+
p = optionsNode.asExpr().getExpr().(HashLiteral).getAKeyValuePair() and
55+
isSslOptionsPairDisablingValidation(p) and
56+
optionsNode.flowsTo(arg) and
57+
disablingNode.asExpr().getExpr() = p
58+
)
59+
)
60+
}
3661

3762
override string getFramework() { result = "Faraday" }
3863
}
64+
65+
/**
66+
* Holds if the pair `p` contains the key `:ssl` for which the value is a hash
67+
* containing either `verify: false` or
68+
* `verify_mode: OpenSSL::SSL::VERIFY_NONE`.
69+
*/
70+
private predicate isSslOptionsPairDisablingValidation(Pair p) {
71+
exists(DataFlow::Node key, DataFlow::Node value |
72+
key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue()
73+
|
74+
isSymbolLiteral(key, "ssl") and
75+
(isHashWithVerifyFalse(value) or isHashWithVerifyModeNone(value))
76+
)
77+
}
78+
79+
/** Holds if `node` represents the symbol literal with the given `valueText`. */
80+
private predicate isSymbolLiteral(DataFlow::Node node, string valueText) {
81+
exists(DataFlow::LocalSourceNode literal |
82+
literal.asExpr().getExpr().(SymbolLiteral).getValueText() = valueText and
83+
literal.flowsTo(node)
84+
)
85+
}
86+
87+
/**
88+
* Holds if `node` represents a hash containing the key-value pair
89+
* `verify: false`.
90+
*/
91+
private predicate isHashWithVerifyFalse(DataFlow::Node node) {
92+
exists(DataFlow::LocalSourceNode hash |
93+
isVerifyFalsePair(hash.asExpr().getExpr().(HashLiteral).getAKeyValuePair()) and
94+
hash.flowsTo(node)
95+
)
96+
}
97+
98+
/**
99+
* Holds if `node` represents a hash containing the key-value pair
100+
* `verify_mode: OpenSSL::SSL::VERIFY_NONE`.
101+
*/
102+
private predicate isHashWithVerifyModeNone(DataFlow::Node node) {
103+
exists(DataFlow::LocalSourceNode hash |
104+
isVerifyModeNonePair(hash.asExpr().getExpr().(HashLiteral).getAKeyValuePair()) and
105+
hash.flowsTo(node)
106+
)
107+
}
108+
109+
/**
110+
* Holds if the pair `p` has the key `:verify_mode` and the value
111+
* `OpenSSL::SSL::VERIFY_NONE`.
112+
*/
113+
private predicate isVerifyModeNonePair(Pair p) {
114+
exists(DataFlow::Node key, DataFlow::Node value |
115+
key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue()
116+
|
117+
isSymbolLiteral(key, "verify_mode") and
118+
value = API::getTopLevelMember("OpenSSL").getMember("SSL").getMember("VERIFY_NONE").getAUse()
119+
)
120+
}
121+
122+
/**
123+
* Holds if the pair `p` has the key `:verify` and the value `false`.
124+
*/
125+
private predicate isVerifyFalsePair(Pair p) {
126+
exists(DataFlow::Node key, DataFlow::Node value |
127+
key.asExpr().getExpr() = p.getKey() and value.asExpr().getExpr() = p.getValue()
128+
|
129+
isSymbolLiteral(key, "verify") and
130+
isFalse(value)
131+
)
132+
}
133+
134+
/** Holds if `node` can contain the Boolean value `false`. */
135+
private predicate isFalse(DataFlow::Node node) {
136+
exists(DataFlow::LocalSourceNode literal |
137+
literal.asExpr().getExpr().(BooleanLiteral).isFalse() and
138+
literal.flowsTo(node)
139+
)
140+
}

0 commit comments

Comments
 (0)