Skip to content

CPP: Add query for CWE-670: Always-Incorrect Control Flow Implementation when use SSL_shutdown #9087

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jun 23, 2022
Merged

CPP: Add query for CWE-670: Always-Incorrect Control Flow Implementation when use SSL_shutdown #9087

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09cd168
create new branchihsinme-patch-88 in fork
May 9, 2022
f6ab338
Update DangerousUseSSL_shutdown.qhelp
ihsinme May 15, 2022
1a375ec
Update cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_s…
ihsinme May 16, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
...
27 changes: 27 additions & 0 deletions cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.qhelp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
<!DOCTYPE qhelp PUBLIC
"-//Semmle//qhelp//EN"
"qhelp.dtd">
<qhelp>
<overview>
<p>Incorrect closing of the connection leads to the creation of different states for the server and client, which can be exploited by an attacker.</p>

</overview>

<example>
<p>The following example shows the incorrect and correct usage of function SSL_shutdown.</p>
<sample src="DangerousUseSSL_shutdown.cpp" />

</example>
<references>

<li>
CERT Coding Standard:
<a href="https://wiki.sei.cmu.edu/confluence/display/c/EXP12-C.+Do+not+ignore+values+returned+by+functions">EXP12-C. Do not ignore values returned by functions - SEI CERT C Coding Standard - Confluence</a>.
</li>
<li>
Openssl.org:
<a href="https://www.openssl.org/docs/man3.0/man3/SSL_shutdown.html">SSL_shutdown - shut down a TLS/SSL connection</a>.
</li>

</references>
</qhelp>
33 changes: 33 additions & 0 deletions cpp/ql/src/experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.ql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that we're looking for two calls to SSL_Shutdown on the same SSL object (without a call to SSL_Free in between), where the return value of the second call is not checked. Why is it OK for the return value of the first call to be unchecked but not the second?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are absolutely right, the first check also needs to be checked.

  1. I want to avoid situations with only one call.
  2. It seems to me that when the first call check is skipped, and the second one is checked, this is a very rare case.

but if you find grain in it, I will add this detector.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to avoid situations with only one call.

Are they problematic? Is it sometimes OK for the developer to ignore the return value of SSL_shutdown in these simpler cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure that you should always check the return value of this function. however, we are talking about the problem of ambiguous understanding of states on the part of the server and the client, in this case, the presence of one call indicates that the developer does not consider this a problem.

the addition of this detection, in my opinion, will lead to an increase in hits and will correlate with the situation in these PRs 6950 6947.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, checking for an error on the first call to SSL_shutdown suggests that the program needs to handle the error to behave correctly, so an error on the second SSL_shutdown should be handled as well. But its also possible the program doesn't need to handle any errors (perhaps its about to exit anyway), so you're reluctant to catch the case of a single call to SSL_shutdown with no error handling.

(
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"
2 changes: 2 additions & 0 deletions ...erimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 |
1 change: 1 addition & 0 deletions ...experimental/query-tests/Security/CWE/CWE-670/semmle/tests/DangerousUseSSL_shutdown.qlref
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
experimental/Security/CWE/CWE-670/DangerousUseSSL_shutdown.ql
75 changes: 75 additions & 0 deletions cpp/ql/test/experimental/query-tests/Security/CWE/CWE-670/semmle/tests/test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}