Merge pull request #7243 from geoffw0/sslquery2

C++: New query for SSL certificates not checked

cpp/change-notes/2021-11-25-certificate-not-checked.md

@@ -0,0 +1,2 @@
+lgtm,codescanning
+* 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

@@ -0,0 +1,28 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>After fetching an SSL certificate, always check the result of certificate verification.</p>
+
+</overview>
+<recommendation>
+
+<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>
+
+</recommendation>
+<example>
+
+<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>
+
+<sample src="SSLResultNotCheckedBad.cpp" />
+
+<p>In the corrected example, we use <code>SSL_get_verify_result</code> to check that certificate verification was successful.</p>
+
+<sample src="SSLResultNotCheckedGood.cpp" />
+
+</example>
+<references>
+
+</references>
+</qhelp>

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

@@ -0,0 +1,120 @@
+/**
+ * @name Certificate not checked
+ * @description Always check the result of certificate verification after fetching an SSL certificate.
+ * @kind problem
+ * @problem.severity error
+ * @security-severity 7.5
+ * @precision medium
+ * @id cpp/certificate-not-checked
+ * @tags security
+ *       external/cwe/cwe-295
+ */
+
+import cpp
+import semmle.code.cpp.valuenumbering.GlobalValueNumbering
+import semmle.code.cpp.controlflow.IRGuards
+
+/**
+ * A call to `SSL_get_peer_certificate`.
+ */
+class SSLGetPeerCertificateCall extends FunctionCall {
+  SSLGetPeerCertificateCall() {
+    getTarget().getName() = "SSL_get_peer_certificate" // SSL_get_peer_certificate(ssl)
+  }
+
+  Expr getSSLArgument() { result = getArgument(0) }
+}
+
+/**
+ * A call to `SSL_get_verify_result`.
+ */
+class SSLGetVerifyResultCall extends FunctionCall {
+  SSLGetVerifyResultCall() {
+    getTarget().getName() = "SSL_get_verify_result" // SSL_get_peer_certificate(ssl)
+  }
+
+  Expr getSSLArgument() { result = getArgument(0) }
+}
+
+/**
+ * Holds if the SSL object passed into `SSL_get_peer_certificate` is checked with
+ * `SSL_get_verify_result` entering `node`.
+ */
+predicate resultIsChecked(SSLGetPeerCertificateCall getCertCall, ControlFlowNode node) {
+  exists(Expr ssl, SSLGetVerifyResultCall check |
+    ssl = globalValueNumber(getCertCall.getSSLArgument()).getAnExpr() and
+    ssl = check.getSSLArgument() and
+    node = check
+  )
+}
+
+/**
+ * Holds if the certificate returned by `SSL_get_peer_certificate` is found to be
+ * `0` on the edge `node1` to `node2`.
+ */
+predicate certIsZero(
+  SSLGetPeerCertificateCall getCertCall, ControlFlowNode node1, ControlFlowNode node2
+) {
+  exists(Expr cert | cert = globalValueNumber(getCertCall).getAnExpr() |
+    exists(GuardCondition guard, Expr zero |
+      zero.getValue().toInt() = 0 and
+      node1 = guard and
+      (
+        // if (cert == zero) {
+        guard.comparesEq(cert, zero, 0, true, true) and
+        node2 = guard.getATrueSuccessor()
+        or
+        // if (cert != zero) { }
+        guard.comparesEq(cert, zero, 0, false, true) and
+        node2 = guard.getAFalseSuccessor()
+      )
+    )
+    or
+    (
+      // if (cert) { }
+      node1 = cert
+      or
+      // if (!cert) {
+      node1.(NotExpr).getAChild() = cert
+    ) and
+    node2 = node1.getASuccessor() and
+    not cert.(GuardCondition).controls(node2, true) // cert may be false
+  )
+}
+
+/**
+ * Holds if the SSL object passed into `SSL_get_peer_certificate` has not been checked with
+ * `SSL_get_verify_result` at `node`. Note that this is only computed at the call to
+ * `SSL_get_peer_certificate` and at the start and end of `BasicBlock`s.
+ */
+predicate certNotChecked(SSLGetPeerCertificateCall getCertCall, ControlFlowNode node) {
+  // cert is not checked at the call to `SSL_get_peer_certificate`
+  node = getCertCall
+  or
+  exists(BasicBlock bb, int pos |
+    // flow to end of a `BasicBlock`
+    certNotChecked(getCertCall, bb.getNode(pos)) and
+    node = bb.getEnd() and
+    // check for barrier node
+    not exists(int pos2 |
+      pos2 > pos and
+      resultIsChecked(getCertCall, bb.getNode(pos2))
+    )
+  )
+  or
+  exists(BasicBlock pred, BasicBlock bb |
+    // flow from the end of one `BasicBlock` to the beginning of a successor
+    certNotChecked(getCertCall, pred.getEnd()) and
+    bb = pred.getASuccessor() and
+    node = bb.getStart() and
+    // check for barrier bb
+    not certIsZero(getCertCall, pred.getEnd(), bb.getStart())
+  )
+}
+
+from SSLGetPeerCertificateCall getCertCall, ControlFlowNode node
+where
+  certNotChecked(getCertCall, node) and
+  node instanceof Function // (function exit)
+select getCertCall,
+  "This " + getCertCall.toString() + " is not followed by a call to SSL_get_verify_result."

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

@@ -0,0 +1,5 @@
+// ...
+
+X509 *cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is never called)
+
+// ...

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

@@ -0,0 +1,9 @@
+// ...
+
+X509 *cert = SSL_get_peer_certificate(ssl); // GOOD
+if (cert)
+{
+	result = SSL_get_verify_result(ssl);
+	if (result == X509_V_OK)
+	{
+		// ...

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

@@ -0,0 +1,4 @@
+| 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. |
+| 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. |
+| 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. |
+| 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

@@ -0,0 +1 @@
+Security/CWE/CWE-295/SSLResultNotChecked.ql

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

@@ -0,0 +1,147 @@
+
+struct SSL {
+	// ...
+};
+
+int SSL_get_peer_certificate(const SSL *ssl);
+int SSL_get_verify_result(const SSL *ssl);
+
+bool maybe();
+
+bool test2_1(SSL *ssl)
+{
+	int cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is never called)
+
+	return true;
+}
+
+bool test2_2(SSL *ssl)
+{
+	int cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is always called)
+	int result = SSL_get_verify_result(ssl);
+
+	return (result == 0);
+}
+
+bool test2_3(SSL *ssl)
+{
+	int cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result may not be called)
+
+	if (maybe())
+	{
+		int result = SSL_get_verify_result(ssl);
+
+		return (result == 0);
+	}
+
+	return true;
+}
+
+bool test2_4(SSL *ssl)
+{
+	int cert, result;
+
+	cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
+	if (cert != 0)
+	{
+		result = SSL_get_verify_result(ssl);
+		if (result == 0)
+		{
+			return true;
+		}
+	}
+
+	return false;
+}
+
+bool test2_5(SSL *ssl)
+{
+	int cert, result;
+
+	cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is not used reliably)
+	if ((cert != 0) && (maybe()))
+	{
+		result = SSL_get_verify_result(ssl);
+		if (result == 0)
+		{
+			return true;
+		}
+	}
+
+	return false;
+}
+
+bool test2_6(SSL *ssl)
+{
+	int cert;
+
+	cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
+	if (cert == 0) return false;
+	if (SSL_get_verify_result(ssl) != 0) return false;
+
+	return true;
+}
+
+bool test2_7(SSL *ssl)
+{
+	int cert;
+
+	cert = SSL_get_peer_certificate(ssl); // BAD (SSL_get_verify_result is only called when there is not a cert)
+	if (cert != 0) return false;
+	if (SSL_get_verify_result(ssl) != 0) return false;
+
+	return true;
+}
+
+bool test2_8(SSL *ssl)
+{
+	int cert;
+
+	cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
+	if (!cert) return false;
+	if (!SSL_get_verify_result(ssl)) return false;
+
+	return true;
+}
+
+bool test2_9(SSL *ssl)
+{
+	int cert;
+
+	cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
+	if ((!cert) || (SSL_get_verify_result(ssl) != 0)) {
+		return false;
+	}
+
+	return true;
+}
+
+bool test2_10(SSL *ssl)
+{
+	int cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
+
+	if (cert)
+	{
+		int result = SSL_get_verify_result(ssl);
+
+		if (result == 0)
+		{
+			return true;
+		}
+	}
+
+	return true;
+}
+
+bool test2_11(SSL *ssl)
+{
+	int cert;
+
+	cert = SSL_get_peer_certificate(ssl); // GOOD (SSL_get_verify_result is called when there is a cert)
+
+	if ((cert) && (SSL_get_verify_result(ssl) == 0)) {
+		return true;
+	}
+
+	return false;
+}