Skip to content

Commit 9f8326a

Browse files
MathiasVPMathiasVP
authored
Merge pull request #7243 from geoffw0/sslquery2
C++: New query for SSL certificates not checked
2 parents a077345 + 4b221bd commit 9f8326a

File tree

8 files changed

+316
-0
lines changed

8 files changed

+316
-0
lines changed

‎cpp/change-notes/2021-11-25-certificate-not-checked.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-not-checked` has been added for C/C++. The query flags unsafe use of OpenSSL and similar libraries.

‎cpp/ql/src/Security/CWE/CWE-295/SSLResultNotChecked.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>After fetching an SSL certificate, always check the result of certificate verification.</p>
7+
8+
</overview>
9+
<recommendation>
10+
11+
<p>Always check the result of SSL certificate verification. A certificate that has been revoked may indicate that data is coming from an attacker, whereas a certificate that has expired or was self-signed may indicate an increased likelihood that the data is malicious.</p>
12+
13+
</recommendation>
14+
<example>
15+
16+
<p>In this example, the <code>SSL_get_peer_certificate</code> function is used to get the certificate of a peer. However it is unsafe to use that information without checking if the certificate is valid.</p>
17+
18+
<sample src="SSLResultNotCheckedBad.cpp" />
19+
20+
<p>In the corrected example, we use <code>SSL_get_verify_result</code> to check that certificate verification was successful.</p>
21+
22+
<sample src="SSLResultNotCheckedGood.cpp" />
23+
24+
</example>
25+
<references>
26+
27+
</references>
28+
</qhelp>

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

Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
/**
2+
* @name Certificate not checked
3+
* @description Always check the result of certificate verification after fetching an SSL certificate.
4+
* @kind problem
5+
* @problem.severity error
6+
* @security-severity 7.5
7+
* @precision medium
8+
* @id cpp/certificate-not-checked
9+
* @tags security
10+
* external/cwe/cwe-295
11+
*/
12+
13+
import cpp
14+
import semmle.code.cpp.valuenumbering.GlobalValueNumbering
15+
import semmle.code.cpp.controlflow.IRGuards
16+
17+
/**
18+
* A call to `SSL_get_peer_certificate`.
19+
*/
20+
class SSLGetPeerCertificateCall extends FunctionCall {
21+
SSLGetPeerCertificateCall() {
22+
getTarget().getName() = "SSL_get_peer_certificate" // SSL_get_peer_certificate(ssl)
23+
}
24+
25+
Expr getSSLArgument() { result = getArgument(0) }
26+
}
27+
28+
/**
29+
* A call to `SSL_get_verify_result`.
30+
*/
31+
class SSLGetVerifyResultCall extends FunctionCall {
32+
SSLGetVerifyResultCall() {
33+
getTarget().getName() = "SSL_get_verify_result" // SSL_get_peer_certificate(ssl)
34+
}
35+
36+
Expr getSSLArgument() { result = getArgument(0) }
37+
}
38+
39+
/**
40+
* Holds if the SSL object passed into `SSL_get_peer_certificate` is checked with
41+
* `SSL_get_verify_result` entering `node`.
42+
*/
43+
predicate resultIsChecked(SSLGetPeerCertificateCall getCertCall, ControlFlowNode node) {
44+
exists(Expr ssl, SSLGetVerifyResultCall check |
45+
ssl = globalValueNumber(getCertCall.getSSLArgument()).getAnExpr() and
46+
ssl = check.getSSLArgument() and
47+
node = check
48+
)
49+
}
50+
51+
/**
52+
* Holds if the certificate returned by `SSL_get_peer_certificate` is found to be
53+
* `0` on the edge `node1` to `node2`.
54+
*/
55+
predicate certIsZero(
56+
SSLGetPeerCertificateCall getCertCall, ControlFlowNode node1, ControlFlowNode node2
57+
) {
58+
exists(Expr cert | cert = globalValueNumber(getCertCall).getAnExpr() |
59+
exists(GuardCondition guard, Expr zero |
60+
zero.getValue().toInt() = 0 and
61+
node1 = guard and
62+
(
63+
// if (cert == zero) {
64+
guard.comparesEq(cert, zero, 0, true, true) and
65+
node2 = guard.getATrueSuccessor()
66+
or
67+
// if (cert != zero) { }
68+
guard.comparesEq(cert, zero, 0, false, true) and
69+
node2 = guard.getAFalseSuccessor()
70+
)
71+
)
72+
or
73+
(
74+
// if (cert) { }
75+
node1 = cert
76+
or
77+
// if (!cert) {
78+
node1.(NotExpr).getAChild() = cert
79+
) and
80+
node2 = node1.getASuccessor() and
81+
not cert.(GuardCondition).controls(node2, true) // cert may be false
82+
)
83+
}
84+
85+
/**
86+
* Holds if the SSL object passed into `SSL_get_peer_certificate` has not been checked with
87+
* `SSL_get_verify_result` at `node`. Note that this is only computed at the call to
88+
* `SSL_get_peer_certificate` and at the start and end of `BasicBlock`s.
89+
*/
90+
predicate certNotChecked(SSLGetPeerCertificateCall getCertCall, ControlFlowNode node) {
91+
// cert is not checked at the call to `SSL_get_peer_certificate`
92+
node = getCertCall
93+
or
94+
exists(BasicBlock bb, int pos |
95+
// flow to end of a `BasicBlock`
96+
certNotChecked(getCertCall, bb.getNode(pos)) and
97+
node = bb.getEnd() and
98+
// check for barrier node
99+
not exists(int pos2 |
100+
pos2 > pos and
101+
resultIsChecked(getCertCall, bb.getNode(pos2))
102+
)
103+
)
104+
or
105+
exists(BasicBlock pred, BasicBlock bb |
106+
// flow from the end of one `BasicBlock` to the beginning of a successor
107+
certNotChecked(getCertCall, pred.getEnd()) and
108+
bb = pred.getASuccessor() and
109+
node = bb.getStart() and
110+
// check for barrier bb
111+
not certIsZero(getCertCall, pred.getEnd(), bb.getStart())
112+
)
113+
}
114+
115+
from SSLGetPeerCertificateCall getCertCall, ControlFlowNode node
116+
where
117+
certNotChecked(getCertCall, node) and
118+
node instanceof Function // (function exit)
119+
select getCertCall,
120+
"This " + getCertCall.toString() + " is not followed by a call to SSL_get_verify_result."

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
// ...
2+
3+
X509 *cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is never called)
4+
5+
// ...

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

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

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
| test2.cpp:13:13:13:36 | call to SSL_get_peer_certificate | This call to SSL_get_peer_certificate is not followed by a call to SSL_get_verify_result. |
2+
| test2.cpp:28:13:28:36 | call to SSL_get_peer_certificate | This call to SSL_get_peer_certificate is not followed by a call to SSL_get_verify_result. |
3+
| test2.cpp:61:9:61:32 | call to SSL_get_peer_certificate | This call to SSL_get_peer_certificate is not followed by a call to SSL_get_verify_result. |
4+
| test2.cpp:89:9:89:32 | call to SSL_get_peer_certificate | This call to SSL_get_peer_certificate is not followed by a call to SSL_get_verify_result. |

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

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

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

Lines changed: 147 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,147 @@
1+
2+
struct SSL {
3+
// ...
4+
};
5+
6+
int SSL_get_peer_certificate(const SSL *ssl);
7+
int SSL_get_verify_result(const SSL *ssl);
8+
9+
bool maybe();
10+
11+
bool test2_1(SSL *ssl)
12+
{
13+
int cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is never called)
14+
15+
return true;
16+
}
17+
18+
bool test2_2(SSL *ssl)
19+
{
20+
int cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is always called)
21+
int result = SSL_get_verify_result(ssl);
22+
23+
return (result == 0);
24+
}
25+
26+
bool test2_3(SSL *ssl)
27+
{
28+
int cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result may not be called)
29+
30+
if (maybe())
31+
{
32+
int result = SSL_get_verify_result(ssl);
33+
34+
return (result == 0);
35+
}
36+
37+
return true;
38+
}
39+
40+
bool test2_4(SSL *ssl)
41+
{
42+
int cert, result;
43+
44+
cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
45+
if (cert != 0)
46+
{
47+
result = SSL_get_verify_result(ssl);
48+
if (result == 0)
49+
{
50+
return true;
51+
}
52+
}
53+
54+
return false;
55+
}
56+
57+
bool test2_5(SSL *ssl)
58+
{
59+
int cert, result;
60+
61+
cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is not used reliably)
62+
if ((cert != 0) && (maybe()))
63+
{
64+
result = SSL_get_verify_result(ssl);
65+
if (result == 0)
66+
{
67+
return true;
68+
}
69+
}
70+
71+
return false;
72+
}
73+
74+
bool test2_6(SSL *ssl)
75+
{
76+
int cert;
77+
78+
cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
79+
if (cert == 0) return false;
80+
if (SSL_get_verify_result(ssl) != 0) return false;
81+
82+
return true;
83+
}
84+
85+
bool test2_7(SSL *ssl)
86+
{
87+
int cert;
88+
89+
cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is only called when there is not a cert)
90+
if (cert != 0) return false;
91+
if (SSL_get_verify_result(ssl) != 0) return false;
92+
93+
return true;
94+
}
95+
96+
bool test2_8(SSL *ssl)
97+
{
98+
int cert;
99+
100+
cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
101+
if (!cert) return false;
102+
if (!SSL_get_verify_result(ssl)) return false;
103+
104+
return true;
105+
}
106+
107+
bool test2_9(SSL *ssl)
108+
{
109+
int cert;
110+
111+
cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
112+
if ((!cert) || (SSL_get_verify_result(ssl) != 0)) {
113+
return false;
114+
}
115+
116+
return true;
117+
}
118+
119+
bool test2_10(SSL *ssl)
120+
{
121+
int cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
122+
123+
if (cert)
124+
{
125+
int result = SSL_get_verify_result(ssl);
126+
127+
if (result == 0)
128+
{
129+
return true;
130+
}
131+
}
132+
133+
return true;
134+
}
135+
136+
bool test2_11(SSL *ssl)
137+
{
138+
int cert;
139+
140+
cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
141+
142+
if ((cert) && (SSL_get_verify_result(ssl) == 0)) {
143+
return true;
144+
}
145+
146+
return false;
147+
}

0 commit comments

Comments
 (0)