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

[ihsinme]

this query looks for improperly closed connections.

CVE-2008-1531

when the situations of calling the second function to close are not processed.

[geoffw0]

Thank you for this contribution.

The code and tests LGTM, though comments in the code would have made it easier to read and understand.

[geoffw0]

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?

[ihsinme]

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.

[geoffw0]

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?

[ihsinme]

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.

[geoffw0]

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.

[geoffw0]

A run of this query on LGTM: https://lgtm.com/query/1680253232874044114/

[ihsinme]

A run of this query on LGTM: https://lgtm.com/query/1680253232874044114/

https://lgtm.com/query/4523088424855036098/

[ihsinme]

could you show me what kind of errors are generated during checks.

[geoffw0]

Great, that's three real world results between our two LGTM runs!

Running the checks now...

[geoffw0]

I'm happy with this, when the checks pass. And I believe it would have caught the issue described in https://redmine.lighttpd.net/issues/285#comment:18 .

[ihsinme]

I'm happy with this, when the checks pass. And I believe it would have caught the issue described in https://redmine.lighttpd.net/issues/285#comment:18 .

this is a very interesting point.
but it is difficult to determine correctly.
there's a non-null error here, and it's used elsewhere.
to work with this goal, we will need to determine that the client will continue after it is possible to close the connection, and ensure that it uses the shared error store.

I really like this item in the description, but I'm afraid that it will complicate the query very much, and narrow the detection very much.

[ihsinme]

if you still think it's reasonable, I am ready to agree with you. let's just talk about what you mean

1. detection of situations when the first or only call is made without checking.
2. detection of a situation when the accumulated error is not reset and can be used in another place, which can lead to a connection break.

[geoffw0]

I'm not advising any changes at this time, I'm just trying to better understand why the query works the way it does. I think I'm happy with the QL and the real world (lgtm.com) results we have seen so far.

[geoffw0]

I think its about time I approve this. It does what its supposed to do and finds a few good results. 👍

[ihsinme]

I think its about time I approve this. It does what its supposed to do and finds a few good results. 👍

thanks

[ihsinme]

What do I need to do for this PR?

[geoffw0]

I believe the bug bounty process concluded that this PR is not eligible for a bounty award. I'm happy to accept the code into experimental, with that understanding.

[ihsinme]

despite the bounty decision, I still find the request useful.
so I will be glad if you accept it and it will be used)

[geoffw0]

Great, thanks for your contribution!