Skip to content

Commit f4555ed

Browse files
MathiasVPMathiasVP
authored
Merge pull request #7242 from geoffw0/sslquery
2 parents 27f40e0 + e9ce296 commit f4555ed

8 files changed

Lines changed: 264 additions & 0 deletions

File tree

‎cpp/change-notes/2021-11-25-certificate-result-conflation.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,2 @@
1+
lgtm,codescanning
2+
* A new query `cpp/certificate-result-conflation` has been added for C/C++. The query flags unsafe use of OpenSSL and similar libraries.

‎cpp/ql/src/Security/CWE/CWE-295/SSLResultConflation.qhelp‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<!DOCTYPE qhelp PUBLIC
2+
"-//Semmle//qhelp//EN"
3+
"qhelp.dtd">
4+
<qhelp>
5+
<overview>
6+
<p>When checking the result of SSL certificate verification, accepting any error code may allow an attacker to impersonate someone who is trusted.</p>
7+
8+
</overview>
9+
<recommendation>
10+
11+
<p>When checking an SSL certificate with <code>SSL_get_verify_result</code>, only <code>X509_V_OK</code> is a success code. If there is any other result the certificate should not be accepted.</p>
12+
13+
</recommendation>
14+
<example>
15+
16+
<p>In this example the error code <code>X509_V_ERR_CERT_HAS_EXPIRED</code> is treated the same as an OK result. An expired certificate should not be accepted as it is more likely to be compromised than a valid certificate.</p>
17+
18+
<sample src="SSLResultConflationBad.cpp" />
19+
20+
<p>In the corrected example, only a result of <code>X509_V_OK</code> is accepted.</p>
21+
22+
<sample src="SSLResultConflationGood.cpp" />
23+
24+
</example>
25+
<references>
26+
27+
</references>
28+
</qhelp>

‎cpp/ql/src/Security/CWE/CWE-295/SSLResultConflation.ql‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
/**
2+
* @name Certificate result conflation
3+
* @description Only accept SSL certificates that pass certificate verification.
4+
* @kind problem
5+
* @problem.severity error
6+
* @security-severity 7.5
7+
* @precision medium
8+
* @id cpp/certificate-result-conflation
9+
* @tags security
10+
* external/cwe/cwe-295
11+
*/
12+
13+
import cpp
14+
import semmle.code.cpp.controlflow.Guards
15+
import semmle.code.cpp.dataflow.DataFlow
16+
17+
/**
18+
* A call to `SSL_get_verify_result`.
19+
*/
20+
class SSLGetVerifyResultCall extends FunctionCall {
21+
SSLGetVerifyResultCall() { getTarget().getName() = "SSL_get_verify_result" }
22+
}
23+
24+
/**
25+
* Data flow from a call to `SSL_get_verify_result` to a guard condition
26+
* that references the result.
27+
*/
28+
class VerifyResultConfig extends DataFlow::Configuration {
29+
VerifyResultConfig() { this = "VerifyResultConfig" }
30+
31+
override predicate isSource(DataFlow::Node source) {
32+
source.asExpr() instanceof SSLGetVerifyResultCall
33+
}
34+
35+
override predicate isSink(DataFlow::Node sink) {
36+
exists(GuardCondition guard | guard.getAChild*() = sink.asExpr())
37+
}
38+
}
39+
40+
from
41+
VerifyResultConfig config, DataFlow::Node source, DataFlow::Node sink1, DataFlow::Node sink2,
42+
GuardCondition guard, Expr c1, Expr c2, boolean testIsTrue
43+
where
44+
config.hasFlow(source, sink1) and
45+
config.hasFlow(source, sink2) and
46+
guard.comparesEq(sink1.asExpr(), c1, 0, false, testIsTrue) and // (value != c1) => testIsTrue
47+
guard.comparesEq(sink2.asExpr(), c2, 0, false, testIsTrue) and // (value != c2) => testIsTrue
48+
c1.getValue().toInt() = 0 and
49+
c2.getValue().toInt() != 0
50+
select guard, "This expression conflates OK and non-OK results from $@.", source, source.toString()

‎cpp/ql/src/Security/CWE/CWE-295/SSLResultConflationBad.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// ...
2+
3+
if (cert = SSL_get_peer_certificate(ssl))
4+
{
5+
result = SSL_get_verify_result(ssl);
6+
7+
if ((result == X509_V_OK) || (result == X509_V_ERR_CERT_HAS_EXPIRED)) // BAD (conflates OK and a non-OK codes)
8+
{
9+
do_ok();
10+
} else {
11+
do_error();
12+
}
13+
}

‎cpp/ql/src/Security/CWE/CWE-295/SSLResultConflationGood.cpp‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
// ...
2+
3+
if (cert = SSL_get_peer_certificate(ssl))
4+
{
5+
result = SSL_get_verify_result(ssl);
6+
7+
if (result == X509_V_OK) // GOOD
8+
{
9+
do_ok();
10+
} else {
11+
do_error();
12+
}
13+
}

‎cpp/ql/test/query-tests/Security/CWE/CWE-295/SSLResultConflation.expected‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
| test.cpp:18:9:18:38 | ... \|\| ... | This expression conflates OK and non-OK results from $@. | test.cpp:100:18:100:38 | call to SSL_get_verify_result | call to SSL_get_verify_result |
2+
| test.cpp:38:7:38:36 | ... \|\| ... | This expression conflates OK and non-OK results from $@. | test.cpp:36:16:36:36 | call to SSL_get_verify_result | call to SSL_get_verify_result |
3+
| test.cpp:54:7:54:47 | ... \|\| ... | This expression conflates OK and non-OK results from $@. | test.cpp:52:16:52:36 | call to SSL_get_verify_result | call to SSL_get_verify_result |
4+
| test.cpp:62:7:62:36 | ... \|\| ... | This expression conflates OK and non-OK results from $@. | test.cpp:60:16:60:36 | call to SSL_get_verify_result | call to SSL_get_verify_result |
5+
| test.cpp:70:7:70:36 | ... && ... | This expression conflates OK and non-OK results from $@. | test.cpp:68:16:68:36 | call to SSL_get_verify_result | call to SSL_get_verify_result |
6+
| test.cpp:83:7:83:40 | ... \|\| ... | This expression conflates OK and non-OK results from $@. | test.cpp:78:16:78:36 | call to SSL_get_verify_result | call to SSL_get_verify_result |
7+
| test.cpp:87:7:87:38 | ... \|\| ... | This expression conflates OK and non-OK results from $@. | test.cpp:7:57:7:77 | call to SSL_get_verify_result | call to SSL_get_verify_result |
8+
| test.cpp:107:13:107:42 | ... \|\| ... | This expression conflates OK and non-OK results from $@. | test.cpp:105:16:105:36 | call to SSL_get_verify_result | call to SSL_get_verify_result |

‎cpp/ql/test/query-tests/Security/CWE/CWE-295/SSLResultConflation.qlref‎

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

‎cpp/ql/test/query-tests/Security/CWE/CWE-295/test.cpp‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
2+
struct SSL {
3+
// ...
4+
};
5+
6+
int SSL_get_verify_result(const SSL *ssl);
7+
int get_verify_result_indirect(const SSL *ssl) { return SSL_get_verify_result(ssl); }
8+
9+
int something_else(const SSL *ssl);
10+
11+
bool is_ok(int result)
12+
{
13+
return (result == 0); // GOOD
14+
}
15+
16+
bool is_maybe_ok(int result)
17+
{
18+
return (result == 0) || (result == 1); // BAD (conflates OK and a non-OK codes)
19+
}
20+
21+
void test1_1(SSL *ssl)
22+
{
23+
{
24+
int result = SSL_get_verify_result(ssl);
25+
26+
if (result == 0) // GOOD
27+
{
28+
}
29+
30+
if (result == 1) // GOOD
31+
{
32+
}
33+
}
34+
35+
{
36+
int result = SSL_get_verify_result(ssl);
37+
38+
if ((result == 0) || (result == 1)) // BAD (conflates OK and a non-OK codes)
39+
{
40+
}
41+
}
42+
43+
{
44+
int result = SSL_get_verify_result(ssl);
45+
46+
if ((result == 1) || (result == 2)) // GOOD (both results are non-OK)
47+
{
48+
}
49+
}
50+
51+
{
52+
int result = SSL_get_verify_result(ssl);
53+
54+
if ((result == 0) || (false) || (result == 2)) // BAD (conflates OK and a non-OK codes)
55+
{
56+
}
57+
}
58+
59+
{
60+
int result = SSL_get_verify_result(ssl);
61+
62+
if ((0 == result) || (1 == result)) // BAD (conflates OK and a non-OK codes)
63+
{
64+
}
65+
}
66+
67+
{
68+
int result = SSL_get_verify_result(ssl);
69+
70+
if ((result != 0) && (result != 1)) // BAD (conflates OK and a non-OK codes)
71+
{
72+
} else {
73+
// conflation occurs here
74+
}
75+
}
76+
77+
{
78+
int result = SSL_get_verify_result(ssl);
79+
int result_cpy = result;
80+
int result2 = get_verify_result_indirect(ssl);
81+
int result3 = something_else(ssl);
82+
83+
if ((result == 0) || (result_cpy == 1)) // BAD (conflates OK and a non-OK codes)
84+
{
85+
}
86+
87+
if ((result2 == 0) || (result2 == 1)) // BAD (conflates OK and a non-OK codes)
88+
{
89+
}
90+
91+
if ((result3 == 0) || (result3 == 1)) // GOOD (not an SSL result)
92+
{
93+
}
94+
}
95+
96+
if (is_ok(SSL_get_verify_result(ssl)))
97+
{
98+
}
99+
100+
if (is_maybe_ok(SSL_get_verify_result(ssl)))
101+
{
102+
}
103+
104+
{
105+
int result = SSL_get_verify_result(ssl);
106+
107+
bool ok = (result == 0) || (result == 1); // BAD (conflates OK and a non-OK codes)
108+
109+
if (ok) {
110+
}
111+
}
112+
113+
{
114+
int result = SSL_get_verify_result(ssl);
115+
116+
if (result == 1) // BAD (conflates OK and a non-OK codes in `else`) [NOT DETECTED]
117+
{
118+
} else {
119+
}
120+
}
121+
}
122+
123+
void do_good();
124+
125+
void test1_2(SSL *ssl)
126+
{
127+
int result = SSL_get_verify_result(ssl);
128+
129+
if (result == 0) { // GOOD
130+
do_good();
131+
} else if (result == 1) {
132+
throw 1;
133+
} else {
134+
throw 1;
135+
}
136+
}
137+
138+
void test1_3(SSL *ssl)
139+
{
140+
int result = SSL_get_verify_result(ssl);
141+
142+
if (result == 0) { // BAD (error code 1 is treated as OK, not as non-OK) [NOT DETECTED]
143+
do_good();
144+
} else if (result == 1) {
145+
do_good();
146+
} else {
147+
throw 1;
148+
}
149+
}

0 commit comments

Comments
 (0)