From 01c13c470350c2784d427c5a47d68a198a503fd1 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Thu, 4 Mar 2021 16:14:11 +0300 Subject: [PATCH 1/5] Add files via upload --- ...ratorPrecedenceLogicErrorWhenUseBoolType.c | 11 +++++ ...rPrecedenceLogicErrorWhenUseBoolType.qhelp | 28 +++++++++++ ...atorPrecedenceLogicErrorWhenUseBoolType.ql | 48 +++++++++++++++++++ 3 files changed, 87 insertions(+) create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.c create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.qhelp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.c b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.c new file mode 100644 index 00000000000..8458d82f7ad --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.c @@ -0,0 +1,11 @@ +if(len=funcReadData()==0) return 1; // BAD: variable `len` will not equal the value returned by function `funcReadData()` +... +if((len=funcReadData())==0) return 1; // GOOD: variable `len` equal the value returned by function `funcReadData()` +... +bool a=true; +a++;// BAD: variable `a` does not change its meaning +bool b; +b=-a;// BAD: variable `b` equal `true` +... +a=false;// GOOD: variable `a` equal `false` +b=!a;// GOOD: variable `b` equal `false` diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.qhelp new file mode 100644 index 00000000000..8114da831fe --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.qhelp @@ -0,0 +1,28 @@ + + + +

Finding places of confusing use of boolean type. For example, a unary minus does not work before a boolean type and an increment always gives true.

+ + + + + +

we recommend making the code simpler.

+ + + +

The following example demonstrates erroneous and fixed methods for using a boolean data type.

+ + + + + +
  • + CERT C Coding Standard: + EXP00-C. Use parentheses for precedence of operation. +
    • + +     +     diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql new file mode 100644 index 00000000000..1a116a83dbf --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql @@ -0,0 +1,48 @@ +/** + * @name Operator Precedence Logic Error When Use Bool Type + * @description --Finding places of confusing use of boolean type. + * --For example, a unary minus does not work before a boolean type and an increment always gives true. + * @kind problem + * @id cpp/operator-precedence-logic-error-when-use-bool-type + * @problem.severity warning + * @precision medium + * @tags correctness + * security + * external/cwe/cwe-783 + * external/cwe/cwe-480 + */ + +import cpp +import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis + +/** Holds, if it is an expression, a boolean increment. */ +predicate incrementBoolType(Expr exp) { + exp.(IncrementOperation).getOperand().getType() instanceof BoolType +} + +/** Holds, if this is an expression, applies a minus to a boolean type. */ +predicate revertSignBoolType(Expr exp) { + exp.(AssignExpr).getRValue().(UnaryMinusExpr).getAnOperand().getType() instanceof BoolType and + exp.(AssignExpr).getLValue().getType() instanceof BoolType +} + +/** Holds, if this is an expression, uses comparison and assignment outside of execution precedence. */ +predicate assignBoolType(Expr exp) { + exists(ComparisonOperation co | + exp.(AssignExpr).getRValue() = co and + exp.isCondition() and + not co.isParenthesised() and + not exp.(AssignExpr).getLValue().getType() instanceof BoolType and + co.getLeftOperand() instanceof FunctionCall and + not co.getRightOperand().getType() instanceof BoolType and + not co.getRightOperand().getValue() = "0" and + not co.getRightOperand().getValue() = "1" + ) +} + +from Expr exp +where + incrementBoolType(exp) or + revertSignBoolType(exp) or + assignBoolType(exp) +select exp, "this expression needs attention" From 10cc57428907c41e08786242051028c996935ef3 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Thu, 4 Mar 2021 16:15:26 +0300 Subject: [PATCH 2/5] Add files via upload --- ...ecedenceLogicErrorWhenUseBoolType.expected | 5 ++++ ...rPrecedenceLogicErrorWhenUseBoolType.qlref | 1 + .../CWE/CWE-788/semmle/tests/test.cpp | 26 +++++++++++++++++++ 3 files changed, 32 insertions(+) create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.qlref create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.cpp diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected new file mode 100644 index 00000000000..76062fc360a --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected @@ -0,0 +1,5 @@ +| test.cpp:10:3:10:10 | ... = ... | this expression needs attention | +| test.cpp:12:3:12:6 | ... ++ | this expression needs attention | +| test.cpp:13:3:13:6 | ++ ... | this expression needs attention | +| test.cpp:14:6:14:21 | ... = ... | this expression needs attention | +| test.cpp:16:6:16:21 | ... = ... | this expression needs attention | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.qlref new file mode 100644 index 00000000000..5189abcce5d --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.cpp new file mode 100644 index 00000000000..f08d2a45757 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/test.cpp @@ -0,0 +1,26 @@ +int tmpFunc() +{ + return 12; +} +void testFunction() +{ + int i1,i2,i3; + bool b1,b2,b3; + char c1,c2,c3; + b1 = -b2; //BAD + b1 = !b2; //GOOD + b1++; //BAD + ++b1; //BAD + if(i1=tmpFunc()!=i2) //BAD + return; + if(i1=tmpFunc()!=11) //BAD + return; + if((i1=tmpFunc())!=i2) //GOOD + return; + if((i1=tmpFunc())!=11) //GOOD + return; + if(i1=tmpFunc()!=1) //GOOD + return; + if(i1=tmpFunc()==b1) //GOOD + return; +} From 26bac9f425c2ecab54759797da19bcc1586ed95d Mon Sep 17 00:00:00 2001 From: ihsinme Date: Sun, 21 Mar 2021 15:25:29 +0300 Subject: [PATCH 3/5] Apply suggestions from code review Co-authored-by: Robert Marsh --- .../CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql index 1a116a83dbf..034df703bc3 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql @@ -15,12 +15,12 @@ import cpp import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis -/** Holds, if it is an expression, a boolean increment. */ +/** Holds if `exp` increments a boolean value. */ predicate incrementBoolType(Expr exp) { exp.(IncrementOperation).getOperand().getType() instanceof BoolType } -/** Holds, if this is an expression, applies a minus to a boolean type. */ +/** Holds if `exp` applies the unary minus operator to a boolean type. */ predicate revertSignBoolType(Expr exp) { exp.(AssignExpr).getRValue().(UnaryMinusExpr).getAnOperand().getType() instanceof BoolType and exp.(AssignExpr).getLValue().getType() instanceof BoolType From 093c63ea3b15cd33b08b8bd1d1058ffc3c6dd075 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Sun, 28 Mar 2021 23:42:36 +0300 Subject: [PATCH 4/5] Update OperatorPrecedenceLogicErrorWhenUseBoolType.expected --- .../tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected index 76062fc360a..1209c7e7830 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-788/semmle/tests/OperatorPrecedenceLogicErrorWhenUseBoolType.expected @@ -1,4 +1,4 @@ -| test.cpp:10:3:10:10 | ... = ... | this expression needs attention | +| test.cpp:10:8:10:10 | - ... | this expression needs attention | | test.cpp:12:3:12:6 | ... ++ | this expression needs attention | | test.cpp:13:3:13:6 | ++ ... | this expression needs attention | | test.cpp:14:6:14:21 | ... = ... | this expression needs attention | From 3f215d0954ea56c34f90837e259553be82675f76 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Sun, 28 Mar 2021 23:43:22 +0300 Subject: [PATCH 5/5] Update OperatorPrecedenceLogicErrorWhenUseBoolType.ql --- ...ratorPrecedenceLogicErrorWhenUseBoolType.ql | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql index 034df703bc3..4f30f112eb0 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-783/OperatorPrecedenceLogicErrorWhenUseBoolType.ql @@ -13,17 +13,17 @@ */ import cpp -import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis +import semmle.code.cpp.valuenumbering.HashCons /** Holds if `exp` increments a boolean value. */ -predicate incrementBoolType(Expr exp) { - exp.(IncrementOperation).getOperand().getType() instanceof BoolType +predicate incrementBoolType(IncrementOperation exp) { + exp.getOperand().getType() instanceof BoolType } /** Holds if `exp` applies the unary minus operator to a boolean type. */ -predicate revertSignBoolType(Expr exp) { - exp.(AssignExpr).getRValue().(UnaryMinusExpr).getAnOperand().getType() instanceof BoolType and - exp.(AssignExpr).getLValue().getType() instanceof BoolType +predicate revertSignBoolType(UnaryMinusExpr exp) { + exp.getAnOperand().getType() instanceof BoolType and + exp.getFullyConverted().getType() instanceof BoolType } /** Holds, if this is an expression, uses comparison and assignment outside of execution precedence. */ @@ -33,6 +33,12 @@ predicate assignBoolType(Expr exp) { exp.isCondition() and not co.isParenthesised() and not exp.(AssignExpr).getLValue().getType() instanceof BoolType and + not exists(Expr exbl | + hashCons(exbl.(AssignExpr).getLValue()) = hashCons(exp.(AssignExpr).getLValue()) and + not exbl.isCondition() and + exbl.(AssignExpr).getRValue().getType() instanceof BoolType and + exbl.(AssignExpr).getLValue().getType() = exp.(AssignExpr).getLValue().getType() + ) and co.getLeftOperand() instanceof FunctionCall and not co.getRightOperand().getType() instanceof BoolType and not co.getRightOperand().getValue() = "0" and