Skip to content

Commit ca9a90a

Browse files
xcorailxcorail
authored
Merge pull request #55 from theopolis/tls-hostname-verify
OpenSSL hostname verification bug hunting queries
2 parents 5f4789a + f737035 commit ca9a90a

5 files changed

Lines changed: 314 additions & 0 deletions

File tree

‎CodeQL_Queries/cpp/OpenSSL-hostname-validation/BoostAsioMissingVerifyCallback.ql‎

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
1+
/**
2+
* @name BoostAsioMissingVerifyCallback
3+
* @kind problem
4+
* @problem.severity recommendation
5+
* @id cpp/boost-asio-missing-boost-verify-callback
6+
* @tags security
7+
* external/cwe/cwe-273
8+
*/
9+
10+
/*
11+
* The default `verify_callback` https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_verify.html
12+
* does not compare CN/hostnames so a TLS client can be MITMed.
13+
*
14+
* The boost ASIO/SSL library also does not install a hostname verifier callback.
15+
*
16+
* False positives include intentionally skipping checking (for example as the result of
17+
* configuration), using a server context, or using an uncommon strategy for inspecting certs.
18+
*/
19+
20+
import cpp
21+
22+
predicate sslOrDetailNamespace(Namespace n) {
23+
n.getQualifiedName() = "boost::asio::ssl" or
24+
n.getQualifiedName() = "boost::asio::ssl::detail"
25+
}
26+
27+
predicate sslStreamOrEngineType(UserType t) {
28+
t.getSimpleName() = "stream" or
29+
t.getSimpleName() = "engine" or
30+
t.getSimpleName() = "context"
31+
}
32+
33+
class AsioStreamType extends Type {
34+
Type baseType;
35+
36+
AsioStreamType() {
37+
(
38+
// Consider shared_ptr, raw pointers, or base types.
39+
baseType = this.(DerivedType).getBaseType() or
40+
baseType = this.(UserType).getATemplateArgument+() or
41+
baseType = this
42+
) and
43+
sslOrDetailNamespace(baseType.(UserType).getNamespace()) and
44+
sslStreamOrEngineType(baseType)
45+
}
46+
}
47+
48+
class StreamSocketVariable extends Variable {
49+
AsioStreamType type;
50+
51+
StreamSocketVariable() { this.getType() = type }
52+
}
53+
54+
class SetVerifyModeFunctionCall extends FunctionCall {
55+
SetVerifyModeFunctionCall() {
56+
// This can be improved, false positives can be reduced by also checking namespace.
57+
this.getTarget().hasName("set_verify_mode")
58+
}
59+
}
60+
61+
class SetVerifyCallbackFunctionCall extends FunctionCall {
62+
SetVerifyCallbackFunctionCall() {
63+
// This can be improved, false positives can be reduced by also checking namespace.
64+
this.getTarget().hasName("set_verify_callback")
65+
}
66+
}
67+
68+
predicate callsSetVerifyCallback(StreamSocketVariable v) {
69+
exists(SetVerifyCallbackFunctionCall fc, Expr e |
70+
e = fc.getQualifier() and
71+
v = e.(VariableAccess).getTarget()
72+
)
73+
}
74+
75+
from SetVerifyModeFunctionCall fc, StreamSocketVariable v
76+
where
77+
// There is most likely a better method than assuming the only namespace/simple-name
78+
// child access is the qualifier.
79+
v = fc.getQualifier().(VariableAccess).getTarget() and
80+
not callsSetVerifyCallback(v)
81+
select fc,
82+
"boost::asio::ssl context calls set_verify_mode without a callback $@, if this is a client context then hostname validation may be missing",
83+
v, "on the socket (" + v.getName() + ")"

‎CodeQL_Queries/cpp/OpenSSL-hostname-validation/OpenSSLMissingVerifyCallback.ql‎

Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
/**
2+
* @name OpenSSLMissingCNCheck
3+
* @kind path-problem
4+
* @problem.severity recommendation
5+
* @id cpp/openssl-missing-verify-callback
6+
* @tags security
7+
* external/cwe/cwe-273
8+
*/
9+
10+
/*
11+
* The default `verify_callback` https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_set_verify.html
12+
* does not compare CN/hostnames so a TLS client can be MITMed.
13+
*
14+
* There are multiple stategies for verifying server certificates so this query assists with code
15+
* review but does not accurately find vulnerabilities, false positives are likely.
16+
*
17+
* False negatives include using verify_callback to ignore verification, such as always returning
18+
* a 1 without checking `preverify_ok`.
19+
*/
20+
21+
import cpp
22+
import semmle.code.cpp.dataflow.TaintTracking
23+
import OpenSSLVerify
24+
25+
class SslSet1HostFunctionCall extends FunctionCall {
26+
SslSet1HostFunctionCall() { this.getTarget().hasName("SSL_set1_host") }
27+
}
28+
29+
class SslLikeCheckHostnameFunctionCall extends FunctionCall {
30+
SslLikeCheckHostnameFunctionCall() {
31+
this instanceof SslSet1HostFunctionCall or
32+
this instanceof SslCtxSetCertVerifyCallbackFunctionCall
33+
}
34+
}
35+
36+
class SslPointerVariable extends Variable {
37+
SslPointerVariable() {
38+
this.hasDefinition() and this.getUnderlyingType().(PointerType).getBaseType().hasName("SSL")
39+
}
40+
}
41+
42+
class SslCtxPointerVariable extends Variable {
43+
SslCtxPointerVariable() {
44+
this.hasDefinition() and this.getUnderlyingType().(PointerType).getBaseType().hasName("SSL_CTX")
45+
}
46+
}
47+
48+
class SslLikePointerVariable extends Variable {
49+
SslLikePointerVariable() {
50+
this instanceof SslPointerVariable or
51+
this instanceof SslCtxPointerVariable
52+
}
53+
}
54+
55+
class SslCtxNewClientFunctionCall extends FunctionCall {
56+
SslCtxNewClientFunctionCall() {
57+
this.getTarget().hasName("SSL_CTX_new") and
58+
exists(FunctionCall fc |
59+
fc.getTarget().getQualifiedName().matches("%_client_method") and
60+
TaintTracking::localTaint(DataFlow::exprNode(fc), DataFlow::exprNode(this.getArgument(0)))
61+
)
62+
}
63+
}
64+
65+
predicate sslPointerTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
66+
// Propagate the taint from SSL <-> SSL_CTX.
67+
exists(SslLikePointerVariable ctx, FunctionCall fc |
68+
pred.asExpr() = ctx.getAnAccess() and
69+
pred.asExpr() = fc.getArgument(0) and
70+
succ.asExpr() = fc and
71+
// Simple to go back and forth.
72+
(fc.getTarget().hasName("SSL_new") or fc.getTarget().hasName("SSL_get_SSL_CTX"))
73+
)
74+
}
75+
76+
class SslCtxSetVerifyConfiguration extends TaintTracking::Configuration {
77+
SslCtxSetVerifyConfiguration() { this = "SslCtxSetVerifyConfiguration" }
78+
79+
override predicate isSource(DataFlow::Node node) {
80+
node.asExpr() instanceof SslCtxNewClientFunctionCall
81+
}
82+
83+
override predicate isSink(DataFlow::Node node) {
84+
// False positives can be reduced by checking that argument(1) is not VERIFY_NONE.
85+
// This may lead to broader false negatives where code mistakenly sets VERIFY_NONE.
86+
exists(SslLikeSetVerifyFunctionCall fc | node.asExpr() = fc.getArgument(0))
87+
}
88+
89+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
90+
sslPointerTaintStep(pred, succ)
91+
}
92+
}
93+
94+
class SslCtxCheckHostnameConfiguration extends TaintTracking::Configuration {
95+
SslCtxCheckHostnameConfiguration() { this = "SslCtxCheckHostnameConfiguration" }
96+
97+
override predicate isSource(DataFlow::Node node) {
98+
node.asExpr() instanceof SslCtxNewClientFunctionCall
99+
}
100+
101+
override predicate isSink(DataFlow::Node node) {
102+
exists(SslLikeCheckHostnameFunctionCall fc | node.asExpr() = fc.getArgument(0))
103+
}
104+
105+
override predicate isAdditionalTaintStep(DataFlow::Node pred, DataFlow::Node succ) {
106+
sslPointerTaintStep(pred, succ)
107+
}
108+
}
109+
110+
predicate flowsToSetVerify(FunctionCall ctx, FunctionCall fc) {
111+
exists(SslCtxSetVerifyConfiguration conf |
112+
conf.hasFlow(DataFlow::exprNode(ctx), DataFlow::exprNode(fc.getArgument(0)))
113+
)
114+
}
115+
116+
predicate flowsToCheckHostname(FunctionCall ctx) {
117+
exists(SslCtxCheckHostnameConfiguration conf, FunctionCall fc |
118+
conf.hasFlow(DataFlow::exprNode(ctx), DataFlow::exprNode(fc.getArgument(0)))
119+
)
120+
}
121+
122+
from FunctionCall ctx, FunctionCall fc
123+
where
124+
flowsToSetVerify(ctx, fc) and
125+
not flowsToCheckHostname(ctx) and
126+
// Empty verify callback (this will not check CN).
127+
// False positives include intentional skipped verification and any
128+
// additional custom checking via accessing the peer certificate post-handshake.
129+
fc.getArgument(2).getValue() = "0"
130+
select ctx, ctx, fc,
131+
"may be a client context without hostname verification because SSL_set1_host " +
132+
"and other indicators of leaf cert hostname checking are missing"

‎CodeQL_Queries/cpp/OpenSSL-hostname-validation/OpenSSLPreverifyIgnored.ql‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
/**
2+
* @name OpenSSLPreverifyIgnored
3+
* @kind problem
4+
* @problem.severity recommendation
5+
* @id cpp/openssl-preverify-ignored
6+
* @tags security
7+
* external/cwe/cwe-273
8+
*
9+
* The `preverified` variable must be checked in verify_callbacks.
10+
*
11+
* False positives include callbacks used when certificate checking is intentionally
12+
* bypassed or when the checking is performed on client certificates.
13+
*/
14+
15+
import cpp
16+
import OpenSSLVerify
17+
18+
class SetVerifyCallbackFunctionCall extends FunctionCall {
19+
SetVerifyCallbackFunctionCall() { this.getTarget().hasName("set_verify_callback") }
20+
}
21+
22+
class CallbackArg extends Expr {
23+
CallbackArg() {
24+
exists(SetVerifyCallbackFunctionCall fc | this = fc.getArgument(0)) or
25+
exists(SslCtxSetCertVerifyCallbackFunctionCall fc | this = fc.getArgument(1)) or
26+
exists(SslCtxSetVerifyFunctionCall fc | this = fc.getArgument(2)) or
27+
exists(SslSetVerifyFunctionCall fc | this = fc.getArgument(2))
28+
}
29+
}
30+
31+
class CallbackFunc extends Function {
32+
CallbackFunc() {
33+
exists(CallbackArg arg |
34+
// False negatives include lambdas and any allocation expression.
35+
this = arg.getAChild().(FunctionAccess).getTarget() or
36+
this = arg.(FunctionAccess).getTarget()
37+
)
38+
}
39+
}
40+
41+
predicate noPreverifyAccess(Function f) {
42+
// False negatives include the variable being accessed but not influencing the return.
43+
not exists(VariableAccess va | va = f.getParameter(0).getAnAccess())
44+
}
45+
46+
from CallbackFunc f
47+
where noPreverifyAccess(f)
48+
select f, "is used as a certificate verify callback and does not use the preverified variable"

‎CodeQL_Queries/cpp/OpenSSL-hostname-validation/OpenSSLVerify.qll‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
import cpp
2+
3+
/**
4+
* Common utilities for OpenSSL certificate verification.
5+
*/
6+
7+
class SslSetVerifyFunctionCall extends FunctionCall {
8+
SslSetVerifyFunctionCall() { this.getTarget().hasName("SSL_set_verify") }
9+
}
10+
11+
class SslCtxSetVerifyFunctionCall extends FunctionCall {
12+
SslCtxSetVerifyFunctionCall() { this.getTarget().hasName("SSL_CTX_set_verify") }
13+
}
14+
15+
class SslLikeSetVerifyFunctionCall extends FunctionCall {
16+
SslLikeSetVerifyFunctionCall() {
17+
this instanceof SslCtxSetVerifyFunctionCall or
18+
this instanceof SslSetVerifyFunctionCall
19+
}
20+
}
21+
22+
class SslCtxSetCertVerifyCallbackFunctionCall extends FunctionCall {
23+
SslCtxSetCertVerifyCallbackFunctionCall() {
24+
this.getTarget().hasName("SSL_CTX_set_cert_verify_callback")
25+
}
26+
}

‎CodeQL_Queries/cpp/OpenSSL-hostname-validation/README.md‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
# Finding OpenSSL hostname verification bugs
2+
3+
These queries help with bug hunting the [SSL-Conservatory style](https://github.com/iSECPartners/ssl-conservatory) bugs.
4+
5+
## Missing verify callback
6+
7+
A SSL/TLS client can choose from several server certificate verification strategies. One required step is to verify the certificate CN/SAN matches the intended host.
8+
9+
A common way to implement this checking is with a callback set with `SSL_CTX_set_verify` or `SSL_CTX_set_cert_verify_callback`. Thus if `SSL_CTX_set_verify` is used without a callback set it is possible hostname validation does not occur.
10+
11+
False positives include hostname verification strategies where the peer certificate is inspected before sending data over the socket.
12+
13+
## Missing verify callback within Boost ASIO
14+
15+
This is very similar to the above situation. Boost's ASIO library wraps several OpenSSL APIs. Without using `set_verify_callback` the code most likely fails to verify the server certificate hostname.
16+
17+
The percision of this query is much higher.
18+
19+
# Verify callback preverified ignored
20+
21+
Finally in the verify callback method mentioned above, one parameter is a `preverified` result. This is the result of OpenSSL checking various x509 fields, such as expiry time, and verifying a trust chain from leaf cert to a root configured for the context.
22+
23+
A callback can still be correct and not use this value, for example it can reimplement the verification.
24+
25+
There are lots of instances where this function body looks like `{ return 1; }` meaning it ignores the pre-verification and also does not check the hostname.

0 commit comments

Comments
 (0)