diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.cpp new file mode 100644 index 00000000000..291cbc8edca --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.cpp @@ -0,0 +1,11 @@ +... +SSL_shutdown(ssl); +SSL_shutdown(ssl); // BAD +... + switch ((ret = SSL_shutdown(ssl))) { + case 1: + break; + case 0: + ERR_clear_error(); + if (-1 != (ret = SSL_shutdown(ssl))) break; // GOOD +... diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.qhelp new file mode 100644 index 00000000000..e093e05a10f --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.qhelp @@ -0,0 +1,23 @@ + + + +

Incorrect closing of the connection leads to the creation of different states for the server and client, which can be exploited by an attacker.

+ +
+ + +

The following example shows the incorrect and correct usage of function SSL_shutdown.

+ + +
+ + +
  • + CERT Coding Standard: + EXP12-C. Do not ignore values returned by functions - SEI CERT C Coding Standard - Confluence. +
  • + +
    +
    diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.ql b/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.ql new file mode 100644 index 00000000000..548fc0d3748 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.ql @@ -0,0 +1,33 @@ +/** + * @name Dangerous use SSL_shutdown. + * @description Incorrect closing of the connection leads to the creation of different states for the server and client, which can be exploited by an attacker. + * @kind problem + * @id cpp/dangerous-use-of-ssl_shutdown + * @problem.severity warning + * @precision medium + * @tags correctness + * security + * external/cwe/cwe-670 + */ + +import cpp +import semmle.code.cpp.commons.Exclusions +import semmle.code.cpp.valuenumbering.GlobalValueNumbering + +from FunctionCall fc, FunctionCall fc1 +where + fc != fc1 and + fc.getASuccessor+() = fc1 and + fc.getTarget().hasName("SSL_shutdown") and + fc1.getTarget().hasName("SSL_shutdown") and + fc1 instanceof ExprInVoidContext and + ( + globalValueNumber(fc.getArgument(0)) = globalValueNumber(fc1.getArgument(0)) or + fc.getArgument(0).(VariableAccess).getTarget() = fc1.getArgument(0).(VariableAccess).getTarget() + ) and + not exists(FunctionCall fctmp | + fctmp.getTarget().hasName("SSL_free") and + fc.getASuccessor+() = fctmp and + fctmp.getASuccessor+() = fc1 + ) +select fc, "You need to handle the return value SSL_shutdown" diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.expected new file mode 100644 index 00000000000..bb3d9157148 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.expected @@ -0,0 +1,2 @@ +| test.cpp:45:20:45:31 | call to SSL_shutdown | You need to handle the return value SSL_shutdown | +| test.cpp:61:11:61:22 | call to SSL_shutdown | You need to handle the return value SSL_shutdown | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.qlref new file mode 100644 index 00000000000..0c2096f68ff --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/test.cpp new file mode 100644 index 00000000000..9ebe1cc10a5 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/test.cpp @@ -0,0 +1,75 @@ +// it's not exact, but it's enough for an example +typedef int SSL; + + +int SSL_shutdown(SSL *ssl); +int SSL_get_error(const SSL *ssl, int ret); +void ERR_clear_error(void); +void print_error(char *buff,int code); + +int gootTest1(SSL *ssl) +{ + int ret; + switch ((ret = SSL_shutdown(ssl))) { + case 1: + break; + case 0: + ERR_clear_error(); + if ((ret = SSL_shutdown(ssl)) == 1) break; // GOOD + default: + print_error("error shutdown", + SSL_get_error(ssl, ret)); + return -1; + } + return 0; +} +int gootTest2(SSL *ssl) +{ + int ret; + switch ((ret = SSL_shutdown(ssl))) { + case 1: + break; + case 0: + ERR_clear_error(); + if (-1 != (ret = SSL_shutdown(ssl))) break; // GOOD + default: + print_error("error shutdown", + SSL_get_error(ssl, ret)); + return -1; + } + return 0; +} +int badTest1(SSL *ssl) +{ + int ret; + switch ((ret = SSL_shutdown(ssl))) { + case 1: + break; + case 0: + SSL_shutdown(ssl); // BAD + break; + default: + print_error("error shutdown", + SSL_get_error(ssl, ret)); + return -1; + } + return 0; +} +int badTest2(SSL *ssl) +{ + int ret; + ret = SSL_shutdown(ssl); + switch (ret) { + case 1: + break; + case 0: + SSL_shutdown(ssl); // BAD + break; + default: + print_error("error shutdown", + SSL_get_error(ssl, ret)); + return -1; + } + return 0; +} +