diff --git a/change_notes/2024-03-22-fix-fp-ctr55-cpp.md b/change_notes/2024-03-22-fix-fp-ctr55-cpp.md new file mode 100644 index 0000000000..98e3eb6339 --- /dev/null +++ b/change_notes/2024-03-22-fix-fp-ctr55-cpp.md @@ -0,0 +1,2 @@ +- `CTR55-CPP` - `DoNotUseAnAdditiveOperatorOnAnIterator.ql`: + - Address reported FP in #374. Improve logic on valid end checks and size checks on iterators. \ No newline at end of file diff --git a/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql b/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql index 720880dbe4..dc53b7a6d0 100644 --- a/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql +++ b/cpp/cert/src/rules/CTR52-CPP/GuaranteeGenericCppLibraryFunctionsDoNotOverflow.ql @@ -80,16 +80,7 @@ where iteratorCreationCall = outputContainer.getAnIteratorFunctionCall() and iteratorCreationCall = c.getOutputIteratorSource() | - // Guarded by a bounds check that ensures our destination is larger than "some" value - exists( - GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall, - boolean branch - | - globalValueNumber(sizeCall.getQualifier()) = - globalValueNumber(iteratorCreationCall.getQualifier()) and - guard.controls(c.getBasicBlock(), branch) and - relOpWithSwapAndNegate(guard, sizeCall, _, Greater(), _, branch) - ) + sizeCompareBoundsChecked(iteratorCreationCall, c) or // Container created with sufficient size for the input exists(ContainerAccessWithoutRangeCheck::ContainerConstructorCall outputIteratorConstructor | diff --git a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql index 8f2aec6e7d..ce1fb52667 100644 --- a/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql +++ b/cpp/cert/src/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.ql @@ -14,24 +14,81 @@ import cpp import codingstandards.cpp.cert import codingstandards.cpp.Iterators +import semmle.code.cpp.controlflow.Dominance -from ContainerIteratorAccess it +/** + * Models a call to an iterator's `operator+` + */ +class AdditionOperatorFunctionCall extends AdditiveOperatorFunctionCall { + AdditionOperatorFunctionCall() { this.getTarget().hasName("operator+") } +} + +/** + * There exists a calculation for the reference one passed the end of some container + * An example derivation is: + * `end = begin() + size()` + */ +Expr getDerivedReferenceToOnePassedTheEndElement(Expr containerReference) { + exists( + ContainerAccessWithoutRangeCheck::ContainerSizeCall size, + ContainerAccessWithoutRangeCheck::ContainerBeginCall begin, AdditionOperatorFunctionCall calc + | + result = calc + | + DataFlow::localFlow(DataFlow::exprNode(size), DataFlow::exprNode(calc.getAChild+())) and + DataFlow::localFlow(DataFlow::exprNode(begin), DataFlow::exprNode(calc.getAChild+())) and + //make sure its the same container providing its size as giving the begin + globalValueNumber(begin.getQualifier()) = globalValueNumber(size.getQualifier()) and + containerReference = begin.getQualifier() + ) +} + +/** + * a wrapper predicate for a couple of types of permitted end bounds checks + */ +Expr getReferenceToOnePassedTheEndElement(Expr containerReference) { + //a container end access - v.end() + result instanceof ContainerAccessWithoutRangeCheck::ContainerEndCall and + containerReference = result.(FunctionCall).getQualifier() + or + result = getDerivedReferenceToOnePassedTheEndElement(containerReference) +} + +/** + * some guard exists like: `iterator != end` + * where a relevant`.end()` call flowed into end + */ +predicate isUpperBoundEndCheckedIteratorAccess(IteratorSource source, ContainerIteratorAccess it) { + exists( + Expr referenceToOnePassedTheEndElement, BasicBlock basicBlockOfIteratorAccess, + GuardCondition upperBoundCheck, ContainerIteratorAccess checkedIteratorAccess, + Expr containerReferenceFromEndGuard + | + //sufficient end guard + referenceToOnePassedTheEndElement = + getReferenceToOnePassedTheEndElement(containerReferenceFromEndGuard) and + //guard controls the access + upperBoundCheck.controls(basicBlockOfIteratorAccess, _) and + basicBlockOfIteratorAccess.contains(it) and + //guard is comprised of end check and an iterator access + DataFlow::localFlow(DataFlow::exprNode(referenceToOnePassedTheEndElement), + DataFlow::exprNode(upperBoundCheck.getChild(_))) and + upperBoundCheck.getChild(_) = checkedIteratorAccess and + //make sure its the same iterator being checked in the guard as accessed + checkedIteratorAccess.getOwningContainer() = it.getOwningContainer() and + //if its the end call itself (or its parts), make sure its the same container providing its end as giving the iterator + globalValueNumber(containerReferenceFromEndGuard) = globalValueNumber(source.getQualifier()) and + // and the guard call we match must be after the assignment call (to avoid valid guards protecting new iterator accesses further down) + source.getASuccessor*() = upperBoundCheck + ) +} + +from ContainerIteratorAccess it, IteratorSource source where not isExcluded(it, IteratorsPackage::doNotUseAnAdditiveOperatorOnAnIteratorQuery()) and it.isAdditiveOperation() and not exists(RangeBasedForStmt fs | fs.getUpdate().getAChild*() = it) and - // we get the neraby assignment - not exists(STLContainer c, FunctionCall nearbyAssigningIteratorCall, FunctionCall guardCall | - nearbyAssigningIteratorCall = it.getANearbyAssigningIteratorCall() and - // we look for calls to size or end - (guardCall = c.getACallToSize() or guardCall = c.getAnIteratorEndFunctionCall()) and - // such that the call to size is before this - // access - guardCall = it.getAPredecessor*() and - // and it uses the same qualifier as the one we were just assigned - nearbyAssigningIteratorCall.getQualifier().(VariableAccess).getTarget() = - guardCall.getQualifier().(VariableAccess).getTarget() and - // and the size call we match must be after the assignment call - nearbyAssigningIteratorCall.getASuccessor*() = guardCall - ) + source = it.getANearbyAssigningIteratorCall() and + not isUpperBoundEndCheckedIteratorAccess(source, it) and + not sizeCompareBoundsChecked(source, it) select it, "Increment of iterator may overflow since its bounds are not checked." diff --git a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected index 06560517dd..0a06677b54 100644 --- a/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected +++ b/cpp/cert/test/rules/CTR55-CPP/DoNotUseAnAdditiveOperatorOnAnIterator.expected @@ -1,4 +1,8 @@ | test.cpp:8:7:8:7 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:9:9:9:9 | i | Increment of iterator may overflow since its bounds are not checked. | | test.cpp:10:9:10:9 | i | Increment of iterator may overflow since its bounds are not checked. | -| test.cpp:27:31:27:31 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:22:18:22:18 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:28:31:28:31 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:41:5:41:8 | end2 | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:53:42:53:42 | i | Increment of iterator may overflow since its bounds are not checked. | +| test.cpp:64:15:64:15 | i | Increment of iterator may overflow since its bounds are not checked. | diff --git a/cpp/cert/test/rules/CTR55-CPP/test.cpp b/cpp/cert/test/rules/CTR55-CPP/test.cpp index d80e8cfab9..e4d14ec25e 100644 --- a/cpp/cert/test/rules/CTR55-CPP/test.cpp +++ b/cpp/cert/test/rules/CTR55-CPP/test.cpp @@ -20,10 +20,47 @@ void f1(std::vector &v) { } for (auto i = v.begin(), l = (i + std::min(static_cast::size_type>(10), - v.size())); - i != l; ++i) { // COMPLIANT + v.size())); // NON_COMPLIANT - technically in the + // calculation + i != l; ++i) { // COMPLIANT } for (auto i = v.begin();; ++i) { // NON_COMPLIANT } +} + +void test_fp_reported_in_374(std::vector &v) { + { + auto end = v.end(); + for (auto i = v.begin(); i != end; ++i) { // COMPLIANT + } + } + + { + auto end2 = v.end(); + end2++; // NON_COMPLIANT + for (auto i = v.begin(); i != end2; + ++i) { // NON_COMPLIANT[FALSE_NEGATIVE] - case of invalidations to + // check before use expected to be less frequent, can model in + // future if need be + } + } +} + +void test(std::vector &v, std::vector &v2) { + { + auto end = v2.end(); + for (auto i = v.begin(); i != end; ++i) { // NON_COMPLIANT - wrong check + } + } +} + +void test2(std::vector &v) { + auto i = v.begin(); + while (1) { + auto i2 = ((i != v.end()) != 0); + if (!i2) + break; + (void)((++i)); // COMPLIANT[FALSE_POSITIVE] + } } \ No newline at end of file diff --git a/cpp/common/src/codingstandards/cpp/Iterators.qll b/cpp/common/src/codingstandards/cpp/Iterators.qll index 593da544ea..1b5199a806 100644 --- a/cpp/common/src/codingstandards/cpp/Iterators.qll +++ b/cpp/common/src/codingstandards/cpp/Iterators.qll @@ -6,6 +6,10 @@ import cpp import codingstandards.cpp.dataflow.DataFlow import codingstandards.cpp.dataflow.TaintTracking import codingstandards.cpp.StdNamespace +import codingstandards.cpp.rules.containeraccesswithoutrangecheck.ContainerAccessWithoutRangeCheck as ContainerAccessWithoutRangeCheck +import semmle.code.cpp.controlflow.Guards +import semmle.code.cpp.valuenumbering.GlobalValueNumbering +import semmle.code.cpp.rangeanalysis.RangeAnalysisUtils abstract class ContainerAccess extends VariableAccess { abstract Variable getOwningContainer(); @@ -63,9 +67,11 @@ class ContainerIteratorAccess extends ContainerAccess { ) } - // get a function call to cbegin/begin that - // assigns its value to the iterator represented by this - // access + /** + * gets a function call to cbegin/begin that + * assigns its value to the iterator represented by this + * access + */ FunctionCall getANearbyAssigningIteratorCall() { // the underlying container for this variable is one wherein // there is an assigned value of cbegin/cend @@ -462,3 +468,18 @@ ControlFlowNode getANonInvalidatedSuccessor(ContainerInvalidationOperation op) { not result instanceof ContainerInvalidationOperation ) } + +/** + * Guarded by a bounds check that ensures our destination is larger than "some" value + */ +predicate sizeCompareBoundsChecked(IteratorSource iteratorCreationCall, Expr guarded) { + exists( + GuardCondition guard, ContainerAccessWithoutRangeCheck::ContainerSizeCall sizeCall, + boolean branch + | + globalValueNumber(sizeCall.getQualifier()) = + globalValueNumber(iteratorCreationCall.getQualifier()) and + guard.controls(guarded.getBasicBlock(), branch) and + relOpWithSwapAndNegate(guard, sizeCall, _, Greater(), _, branch) + ) +} diff --git a/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll b/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll index a3dabedd5a..71e18a5c05 100644 --- a/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll +++ b/cpp/common/src/codingstandards/cpp/rules/containeraccesswithoutrangecheck/ContainerAccessWithoutRangeCheck.qll @@ -85,6 +85,26 @@ class ContainerEmptyCall extends FunctionCall { } } +/** + * A call to either `begin()` on a container. + */ +class ContainerBeginCall extends FunctionCall { + ContainerBeginCall() { + getTarget().getDeclaringType() instanceof ContainerType and + getTarget().getName() = "begin" + } +} + +/** + * A call to either `end()` on a container. + */ +class ContainerEndCall extends FunctionCall { + ContainerEndCall() { + getTarget().getDeclaringType() instanceof ContainerType and + getTarget().getName() = "end" + } +} + /** * A call to either `size()` or `length()` on a container. */