CPP: Add query for CWE-691 Insufficient Control Flow Management After Refactoring The Code #5601

ihsinme · 2021-04-06T07:37:20Z

Good day.
This PR looks for situations of losses after refactoring. Two situations are considered. The first forgotten empty while after the loop. And arephmetics in comparison.
the first case is quite rare.
and can lead to an impact on the availability of resources. (hang)
but the second is widely represented.
and is more often influenced by the logic of the program
I tried to remove the false detection as much as possible.

Information about found and accepted fix in the project:
axboe/fio#1194

MathiasVP · 2021-04-07T09:24:07Z

Hi @ihsinme,

Thanks for another experimental query! Please see my comment on #5600 :)

ihsinme · 2021-04-07T10:14:55Z

hi @MathiasVP.
sorry it's really not beautiful. (
I hope everything is all right now?

geoffw0

This looks like two queries in one file, they should be split up. The while/for query looks quite good, the arithmetic query is a promising idea but the QL still needs work.

...experimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.c

...rimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.qhelp

...xperimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-691/semmle/tests/test.c

...xperimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql

geoffw0 · 2021-04-09T09:46:21Z

...xperimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql

+    not this.getParent*() instanceof AssignBitwiseOperation and
+    not this.getParent*() instanceof AssignArithmeticOperation and
+    not this.getParent*() instanceof EqualityOperation and
+    not this.getParent*() instanceof RelationalOperation


Can you briefly explain the purpose of all these exceptions? Do you just want something along the lines of :

( this.getParent() instanceof IfStmt or this.getParent() instanceof BinaryLogicalOperation ) and not this.isConstant()

I want to remove from the consideration of the situation:

AssignBitwiseOperation if(a&=((b+c)&&(d+e)))

AssignArithmeticOperation if((a*=(b+c))&&())

EqualityOperation if((a+b)==3)

RelationalOperation if((a+b)>3)

I have been working on optimizing this part. but unfortunately I haven't found a more beautiful view yet.

your proposal looks very nice and concise. but this will reveal situations that I am not looking for.
eg:
func(a+b&&c+d) or a==b+c&&d+e

I discovered another false detection situation and unfortunately I have to complicate this construction.
if you do not mind, I would close this recommendation as done?

geoffw0 · 2021-04-09T09:46:21Z

...xperimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql

+  }
+
+  /** Holds when a comparison expression exists. */
+  predicate compareWithOutZero() {


What are we trying to establish here?

the idea was to detect situations when an expression has no comparisons with zero, but has comparisons with something else.
now I see that it is better to remove this predicate by correcting the work of the previous one.
I'll figure out how to do it.
Thank you.

unfortunately I don't see any other way to exclude null comparison and check for non-null comparison, how to use two predicates.
if I leave one corrected predicate, it will find any non-null comparison and does not guarantee that there is no null.
maybe I'm confused.
but the original version works as I intended.

if you do not mind, I also suggest leaving this site unchanged?

ihsinme · 2021-04-12T05:43:18Z

I was trying to make a union query that detects error situations after refactoring.
these two errors are very similar:

since both can have an impact like cwe-691
2.they are both born of careless refactoring.

perhaps my words will be more convincing if you take a look at the arithmetic in the condition. as to a more complex expression, the presence of a comparison not with zero elsewhere in the code, no work in a loop, no work with a pointer and a value.
in my opinion, this is no longer working with an expression in a condition, but just searching for a forgotten comparison expression, as well as finding a forgotten while after refactoring.

you have an aggregation of requests by type, by placing them in single folders, in this situation it is still too early, and by dividing requests we risk losing their connectivity in the future.

in any case it is up to you to decide therefore I am waiting for your instructions.

ihsinme · 2021-04-12T06:06:16Z

thanks for your corrections.
it is better than any study material.

...xperimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql

geoffw0 · 2021-04-12T11:12:30Z

I still think I'd prefer if this were split into two queries, so that users have more fine grained control about which parts they're interested in. But I think this is OK in experimental (and we can always change it later). There are still a couple of other comments to address, I will merge this when you're done.

ihsinme · 2021-04-12T14:23:12Z

I still think I'd prefer if this were split into two queries, so that users have more fine grained control about which parts they're interested in. But I think this is OK in experimental (and we can always change it later). There are still a couple of other comments to address, I will merge this when you're done.

thanks for the opportunity to leave this request as it is.
in the future I will try to exclude such associations.

geoffw0 · 2021-04-14T17:35:01Z

There's a merge conflict with the existing test.c in this directory. Do you know how to fix it?

ihsinme · 2021-04-14T17:38:27Z

There's a merge conflict with the existing test.c in this directory. Do you know how to fix it?

Yes of course.
I originally intended this test file to be generic.

geoffw0

OK, time we merge this.

Here are some runs of the query on LGTM:

the whole query: https://lgtm.com/query/3548450999479559284/
just the UsingArithmeticInComparison case: https://lgtm.com/query/2789590730408126002/ (the query does what it is supposed to, but some of the results look like they are intended to be that way)
just the UsingWhileAfterWhile case: https://lgtm.com/query/589998443415799114/ (no results found on these projects)

ihsinme requested a review from as a code owner Apr 6, 2021

github-actions bot added C++ documentation labels Apr 6, 2021

MathiasVP mentioned this pull request Apr 7, 2021

CPP: Add query for CWE-691 Insufficient Control Flow Management When Using Bit Operations #5600

Merged

ihsinme added 2 commits Apr 7, 2021

Add files via upload

36de496

Add files via upload

cbf158e

MathiasVP requested a review from geoffw0 Apr 7, 2021

Update test.c

3d11724

geoffw0 requested changes Apr 9, 2021

View changes

ihsinme added 4 commits Apr 12, 2021

Update InsufficientControlFlowManagementAfterRefactoringTheCode.ql

17d1c77

Update InsufficientControlFlowManagementAfterRefactoringTheCode.c

3da88f2

Update test.c

6924c6c

Update InsufficientControlFlowManagementAfterRefactoringTheCode.expected

feb3a8d

geoffw0 reviewed Apr 12, 2021

View changes

...xperimental/Security/CWE/CWE-691/InsufficientControlFlowManagementAfterRefactoringTheCode.ql Show resolved Hide resolved

ihsinme added 2 commits Apr 12, 2021

Update InsufficientControlFlowManagementAfterRefactoringTheCode.qhelp

d7c1477

Update InsufficientControlFlowManagementAfterRefactoringTheCode.ql

58d5ad4

ihsinme added 2 commits Apr 14, 2021

Update InsufficientControlFlowManagementAfterRefactoringTheCode.ql

b30ae39

Merge branch 'main' into ihsinme-patch-259

c2d97b9

geoffw0 approved these changes Apr 21, 2021

View changes

geoffw0 merged commit ba33508 into github:main Apr 21, 2021
7 checks passed

ihsinme mentioned this pull request Apr 25, 2021

ihsinme: CPP Add query for CWE-691 Insufficient Control Flow Management After Refactoring The Code github/securitylab#347

Closed

1 task

github / codeql Public

CPP: Add query for CWE-691 Insufficient Control Flow Management After Refactoring The Code #5601

CPP: Add query for CWE-691 Insufficient Control Flow Management After Refactoring The Code #5601

ihsinme commented Apr 6, 2021

MathiasVP commented Apr 7, 2021

ihsinme commented Apr 7, 2021

geoffw0 left a comment

geoffw0 Apr 9, 2021

ihsinme Apr 12, 2021

ihsinme Apr 14, 2021

geoffw0 Apr 9, 2021

ihsinme Apr 12, 2021

ihsinme Apr 12, 2021

ihsinme Apr 14, 2021

ihsinme commented Apr 12, 2021 •

edited

ihsinme commented Apr 12, 2021

geoffw0 commented Apr 12, 2021

ihsinme commented Apr 12, 2021

geoffw0 commented Apr 14, 2021

ihsinme commented Apr 14, 2021 •

edited

geoffw0 left a comment

github / codeql Public

CPP: Add query for CWE-691 Insufficient Control Flow Management After Refactoring The Code #5601

CPP: Add query for CWE-691 Insufficient Control Flow Management After Refactoring The Code #5601

Conversation

ihsinme commented Apr 6, 2021

MathiasVP commented Apr 7, 2021

ihsinme commented Apr 7, 2021

geoffw0 left a comment

geoffw0 Apr 9, 2021

Choose a reason for hiding this comment

ihsinme Apr 12, 2021

Choose a reason for hiding this comment

ihsinme Apr 14, 2021

Choose a reason for hiding this comment

geoffw0 Apr 9, 2021

Choose a reason for hiding this comment

ihsinme Apr 12, 2021

Choose a reason for hiding this comment

ihsinme Apr 12, 2021

Choose a reason for hiding this comment

ihsinme Apr 14, 2021

Choose a reason for hiding this comment

ihsinme commented Apr 12, 2021 • edited

ihsinme commented Apr 12, 2021

geoffw0 commented Apr 12, 2021

ihsinme commented Apr 12, 2021

geoffw0 commented Apr 14, 2021

ihsinme commented Apr 14, 2021 • edited

geoffw0 left a comment

ihsinme commented Apr 12, 2021 •

edited

ihsinme commented Apr 14, 2021 •

edited