From 74f8145970eede4655f2e3b4ca9b12c501dc0af1 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Fri, 25 Feb 2022 11:18:38 +0300 Subject: [PATCH 1/7] Add files via upload --- ...erousUseOfTransformationAfterOperation.cpp | 5 + ...ousUseOfTransformationAfterOperation.qhelp | 24 +++++ ...gerousUseOfTransformationAfterOperation.ql | 94 +++++++++++++++++++ 3 files changed, 123 insertions(+) create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.cpp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp create mode 100644 cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.cpp b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.cpp new file mode 100644 index 000000000000..6a16e9924714 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.cpp @@ -0,0 +1,5 @@ +... + vUnsignedLong = (unsigned long)(vUnsignedInt*vUnsignedInt); // BAD +... + vUnsignedLong = ((unsigned long)vUnsignedInt*vUnsignedInt); // GOOD +... diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp new file mode 100644 index 000000000000..d0c78b226c05 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp @@ -0,0 +1,24 @@ + + + +

Searching for places where data type conversion occurs late and does not affect the computational result.

+ + + + + +

The following example demonstrates erroneous and fixed methods for working with type conversion.

+ + + + + +
  • + CERT C Coding Standard: + INT30-C. Ensure that unsigned integer operations do not wrap. +
    • + +     +     diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql new file mode 100644 index 000000000000..66b5dd9c3521 --- /dev/null +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql @@ -0,0 +1,94 @@ +/** + * @name Dangerous use of transformation after operation. + * @description By using the transformation after the operation, you are doing a pointless and dangerous action. + * @kind problem + * @id cpp/dangerous-use-of-transformation-after-operation. + * @problem.severity warning + * @precision medium + * @tags correctness + * security + * external/cwe/cwe-190 + */ + +import cpp + +/** Returns the number of the expression in the function call arguments. */ +int argumentPosition(FunctionCall fc, Expr exp, int n) { + n in [0 .. fc.getNumberOfArguments() - 1] and + fc.getArgument(n) = exp and + result = n +} + +/** Holds if a nonsensical type conversion situation is found. */ +predicate conversionDoneLate(MulExpr mexp, Expr e1, Expr e2) { + mexp.getConversion().hasExplicitConversion() and + mexp.getConversion() instanceof ParenthesisExpr and + mexp.getConversion().getConversion() instanceof CStyleCast and + mexp.getConversion().getConversion().getType().getSize() > mexp.getType().getSize() and + mexp.getConversion().getConversion().getType().getSize() > e2.getType().getSize() and + mexp.getConversion().getConversion().getType().getSize() > e1.getType().getSize() and + exists(Expr e0 | + e0.(AssignExpr).getRValue() = mexp.getParent*() and + e0.(AssignExpr).getLValue().getType().getSize() = + mexp.getConversion().getConversion().getType().getSize() + or + mexp.getEnclosingElement().(ComparisonOperation).hasOperands(mexp, e0) and + e0.getType().getSize() = mexp.getConversion().getConversion().getType().getSize() + or + e0.(FunctionCall) + .getTarget() + .getParameter(argumentPosition(e0.(FunctionCall), mexp, _)) + .getType() + .getSize() = mexp.getConversion().getConversion().getType().getSize() + ) +} + +/** Holds if the situation of a possible signed overflow used in pointer arithmetic is found. */ +predicate signSmallerWithEqualSizes(MulExpr mexp, Expr e1, Expr e2) { + mexp.getConversion+().getUnderlyingType().getSize() = e1.getUnderlyingType().getSize() and + ( + e2.isConstant() or + mexp.getConversion+().getUnderlyingType().getSize() = e2.getUnderlyingType().getSize() + ) and + mexp.getConversion+().getUnderlyingType().getSize() = e1.getUnderlyingType().getSize() and + exists(AssignExpr ae | + ae.getRValue() = mexp.getParent*() and + ae.getRValue().getUnderlyingType().(IntegralType).isUnsigned() and + ae.getLValue().getUnderlyingType().(IntegralType).isSigned() and + ( + not exists(DivExpr de | mexp.getParent*() = de) + or + exists(DivExpr de, Expr ec | + e2.isConstant() and + de.hasOperands(mexp.getParent*(), ec) and + ec.isConstant() and + e2.getValue().toInt() > ec.getValue().toInt() + ) + ) and + exists(PointerAddExpr pa | + ae.getASuccessor+() = pa and + pa.getAnOperand().(VariableAccess).getTarget() = ae.getLValue().(VariableAccess).getTarget() + ) + ) +} + +from MulExpr mexp, string msg +where + exists(Expr e1, Expr e2 | + mexp.hasOperands(e1, e2) and + not e1.isConstant() and + not e1.hasConversion() and + not e1.hasConversion() and + ( + e2.isConstant() or + not e2.hasConversion() + ) and + ( + conversionDoneLate(mexp, e1, e2) and + msg = "this transformation is applied after multiplication" + or + signSmallerWithEqualSizes(mexp, e1, e2) and + msg = "possible signed overflow followed by offset of the pointer out of bounds" + ) + ) +select mexp, msg From ffdca61f9aec829bd1fb0acdaf1bc3c055c65b1d Mon Sep 17 00:00:00 2001 From: ihsinme Date: Fri, 25 Feb 2022 11:20:23 +0300 Subject: [PATCH 2/7] Add files via upload --- ...UseOfTransformationAfterOperation.expected | 4 ++++ ...ousUseOfTransformationAfterOperation.qlref | 1 + .../test.cpp | 23 +++++++++++++++++++ 3 files changed, 28 insertions(+) create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.qlref create mode 100644 cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected new file mode 100644 index 000000000000..0f378a179468 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected @@ -0,0 +1,4 @@ +| test.cpp:9:8:9:12 | ... * ... | possible signed overflow followed by offset of the pointer out of bounds | +| test.cpp:13:24:13:28 | ... * ... | this transformation is applied after multiplication | +| test.cpp:16:28:16:32 | ... * ... | this transformation is applied after multiplication | +| test.cpp:19:22:19:26 | ... * ... | this transformation is applied after multiplication | diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.qlref b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.qlref new file mode 100644 index 000000000000..84f717acda79 --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.qlref @@ -0,0 +1 @@ +experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp new file mode 100644 index 000000000000..4561c86b72fc --- /dev/null +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp @@ -0,0 +1,23 @@ +void testCall (unsigned long); +void functionWork() { + unsigned long aL; + char aA[10],*aP; + unsigned char aUC; + int aI; + unsigned int aUI; + aI = (aUI*8)/10; // GOOD + aI = aUI*8; // BAD + aP = aA+aI; + aI = (int)aUI*8; // GOOD + + aL = (unsigned long)(aI*aI); // BAD + aL = ((unsigned long)aI*aI); // GOOD + + testCall((unsigned long)(aI*aI)); // BAD + testCall(((unsigned long)aI*aI)); // GOOD + + if((unsigned long)(aI*aI) > aL) // BAD + return; + if(((unsigned long)aI*aI) > aL) // GOOD + return; +} From c6083a6f9528a56319ba0f5e5b6dc8c606dffeb6 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Tue, 1 Mar 2022 09:37:57 +0300 Subject: [PATCH 3/7] Apply suggestions from code review Co-authored-by: Mathias Vorreiter Pedersen --- .../CWE-190/DangerousUseOfTransformationAfterOperation.ql | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql index 66b5dd9c3521..799f9b4da528 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql @@ -2,7 +2,7 @@ * @name Dangerous use of transformation after operation. * @description By using the transformation after the operation, you are doing a pointless and dangerous action. * @kind problem - * @id cpp/dangerous-use-of-transformation-after-operation. + * @id cpp/dangerous-use-of-transformation-after-operation * @problem.severity warning * @precision medium * @tags correctness @@ -85,10 +85,10 @@ where ) and ( conversionDoneLate(mexp, e1, e2) and - msg = "this transformation is applied after multiplication" + msg = "This transformation is applied after multiplication." or signSmallerWithEqualSizes(mexp, e1, e2) and - msg = "possible signed overflow followed by offset of the pointer out of bounds" + msg = "Possible signed overflow followed by offset of the pointer out of bounds." ) ) select mexp, msg From bc22b9b2083cca1be6cc8a85eb385ed911f1b701 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Tue, 1 Mar 2022 09:43:15 +0300 Subject: [PATCH 4/7] Update test.cpp --- .../DangerousUseOfTransformationAfterOperation/test.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp index 4561c86b72fc..472c8ac0afac 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/test.cpp @@ -1,10 +1,10 @@ void testCall (unsigned long); -void functionWork() { +void functionWork(char aA[10],unsigned int aUI) { + unsigned long aL; - char aA[10],*aP; - unsigned char aUC; + char *aP; int aI; - unsigned int aUI; + aI = (aUI*8)/10; // GOOD aI = aUI*8; // BAD aP = aA+aI; From f5267ba8c6a09added637f984eaef689b13f18df Mon Sep 17 00:00:00 2001 From: ihsinme Date: Wed, 2 Mar 2022 10:24:40 +0300 Subject: [PATCH 5/7] Update DangerousUseOfTransformationAfterOperation.qhelp --- .../CWE-190/DangerousUseOfTransformationAfterOperation.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp index d0c78b226c05..458b5e508668 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.qhelp @@ -3,7 +3,7 @@ "qhelp.dtd"> -

    Searching for places where data type conversion occurs late and does not affect the computational result.

    +

    Search for places where the result of the multiplication is subjected to explicit conversion, not the arguments. Therefore, during the multiplication period, you can lose meaningful data.

     From 9e76260f1d1d6f2051190cb6cb3dfa67f98e5850 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Wed, 2 Mar 2022 10:38:57 +0300 Subject: [PATCH 6/7] Update DangerousUseOfTransformationAfterOperation.ql --- ...gerousUseOfTransformationAfterOperation.ql | 114 ++++++++++-------- 1 file changed, 61 insertions(+), 53 deletions(-) diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql index 799f9b4da528..0bebc69a8bfd 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation.ql @@ -20,60 +20,41 @@ int argumentPosition(FunctionCall fc, Expr exp, int n) { } /** Holds if a nonsensical type conversion situation is found. */ -predicate conversionDoneLate(MulExpr mexp, Expr e1, Expr e2) { - mexp.getConversion().hasExplicitConversion() and - mexp.getConversion() instanceof ParenthesisExpr and - mexp.getConversion().getConversion() instanceof CStyleCast and - mexp.getConversion().getConversion().getType().getSize() > mexp.getType().getSize() and - mexp.getConversion().getConversion().getType().getSize() > e2.getType().getSize() and - mexp.getConversion().getConversion().getType().getSize() > e1.getType().getSize() and - exists(Expr e0 | - e0.(AssignExpr).getRValue() = mexp.getParent*() and - e0.(AssignExpr).getLValue().getType().getSize() = - mexp.getConversion().getConversion().getType().getSize() - or - mexp.getEnclosingElement().(ComparisonOperation).hasOperands(mexp, e0) and - e0.getType().getSize() = mexp.getConversion().getConversion().getType().getSize() - or - e0.(FunctionCall) - .getTarget() - .getParameter(argumentPosition(e0.(FunctionCall), mexp, _)) - .getType() - .getSize() = mexp.getConversion().getConversion().getType().getSize() - ) -} - -/** Holds if the situation of a possible signed overflow used in pointer arithmetic is found. */ -predicate signSmallerWithEqualSizes(MulExpr mexp, Expr e1, Expr e2) { - mexp.getConversion+().getUnderlyingType().getSize() = e1.getUnderlyingType().getSize() and - ( - e2.isConstant() or - mexp.getConversion+().getUnderlyingType().getSize() = e2.getUnderlyingType().getSize() - ) and - mexp.getConversion+().getUnderlyingType().getSize() = e1.getUnderlyingType().getSize() and - exists(AssignExpr ae | - ae.getRValue() = mexp.getParent*() and - ae.getRValue().getUnderlyingType().(IntegralType).isUnsigned() and - ae.getLValue().getUnderlyingType().(IntegralType).isSigned() and +predicate conversionDoneLate(MulExpr mexp) { + exists(Expr e1, Expr e2 | + mexp.hasOperands(e1, e2) and + not e1.isConstant() and + not e1.hasConversion() and + not e1.hasConversion() and ( - not exists(DivExpr de | mexp.getParent*() = de) - or - exists(DivExpr de, Expr ec | - e2.isConstant() and - de.hasOperands(mexp.getParent*(), ec) and - ec.isConstant() and - e2.getValue().toInt() > ec.getValue().toInt() - ) + e2.isConstant() or + not e2.hasConversion() ) and - exists(PointerAddExpr pa | - ae.getASuccessor+() = pa and - pa.getAnOperand().(VariableAccess).getTarget() = ae.getLValue().(VariableAccess).getTarget() + mexp.getConversion().hasExplicitConversion() and + mexp.getConversion() instanceof ParenthesisExpr and + mexp.getConversion().getConversion() instanceof CStyleCast and + mexp.getConversion().getConversion().getType().getSize() > mexp.getType().getSize() and + mexp.getConversion().getConversion().getType().getSize() > e2.getType().getSize() and + mexp.getConversion().getConversion().getType().getSize() > e1.getType().getSize() and + exists(Expr e0 | + e0.(AssignExpr).getRValue() = mexp.getParent*() and + e0.(AssignExpr).getLValue().getType().getSize() = + mexp.getConversion().getConversion().getType().getSize() + or + mexp.getEnclosingElement().(ComparisonOperation).hasOperands(mexp, e0) and + e0.getType().getSize() = mexp.getConversion().getConversion().getType().getSize() + or + e0.(FunctionCall) + .getTarget() + .getParameter(argumentPosition(e0.(FunctionCall), mexp, _)) + .getType() + .getSize() = mexp.getConversion().getConversion().getType().getSize() ) ) } -from MulExpr mexp, string msg -where +/** Holds if the situation of a possible signed overflow used in pointer arithmetic is found. */ +predicate signSmallerWithEqualSizes(MulExpr mexp) { exists(Expr e1, Expr e2 | mexp.hasOperands(e1, e2) and not e1.isConstant() and @@ -83,12 +64,39 @@ where e2.isConstant() or not e2.hasConversion() ) and + mexp.getConversion+().getUnderlyingType().getSize() = e1.getUnderlyingType().getSize() and ( - conversionDoneLate(mexp, e1, e2) and - msg = "This transformation is applied after multiplication." - or - signSmallerWithEqualSizes(mexp, e1, e2) and - msg = "Possible signed overflow followed by offset of the pointer out of bounds." + e2.isConstant() or + mexp.getConversion+().getUnderlyingType().getSize() = e2.getUnderlyingType().getSize() + ) and + mexp.getConversion+().getUnderlyingType().getSize() = e1.getUnderlyingType().getSize() and + exists(AssignExpr ae | + ae.getRValue() = mexp.getParent*() and + ae.getRValue().getUnderlyingType().(IntegralType).isUnsigned() and + ae.getLValue().getUnderlyingType().(IntegralType).isSigned() and + ( + not exists(DivExpr de | mexp.getParent*() = de) + or + exists(DivExpr de, Expr ec | + e2.isConstant() and + de.hasOperands(mexp.getParent*(), ec) and + ec.isConstant() and + e2.getValue().toInt() > ec.getValue().toInt() + ) + ) and + exists(PointerAddExpr pa | + ae.getASuccessor+() = pa and + pa.getAnOperand().(VariableAccess).getTarget() = ae.getLValue().(VariableAccess).getTarget() + ) ) ) +} + +from MulExpr mexp, string msg +where + conversionDoneLate(mexp) and + msg = "This transformation is applied after multiplication." + or + signSmallerWithEqualSizes(mexp) and + msg = "Possible signed overflow followed by offset of the pointer out of bounds." select mexp, msg From b32be69e0a9fdc24075941a7a1c19df44f6dbcd6 Mon Sep 17 00:00:00 2001 From: ihsinme Date: Thu, 3 Mar 2022 19:55:30 +0300 Subject: [PATCH 7/7] Update DangerousUseOfTransformationAfterOperation.expected --- .../DangerousUseOfTransformationAfterOperation.expected | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected index 0f378a179468..dd37faf2d56e 100644 --- a/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected +++ b/cpp/ql/test/experimental/query-tests/Security/CWE/CWE-190/DangerousUseOfTransformationAfterOperation/DangerousUseOfTransformationAfterOperation.expected @@ -1,4 +1,4 @@ -| test.cpp:9:8:9:12 | ... * ... | possible signed overflow followed by offset of the pointer out of bounds | -| test.cpp:13:24:13:28 | ... * ... | this transformation is applied after multiplication | -| test.cpp:16:28:16:32 | ... * ... | this transformation is applied after multiplication | -| test.cpp:19:22:19:26 | ... * ... | this transformation is applied after multiplication | +| test.cpp:9:8:9:12 | ... * ... | Possible signed overflow followed by offset of the pointer out of bounds. | +| test.cpp:13:24:13:28 | ... * ... | This transformation is applied after multiplication. | +| test.cpp:16:28:16:32 | ... * ... | This transformation is applied after multiplication. | +| test.cpp:19:22:19:26 | ... * ... | This transformation is applied after multiplication. |