diff --git a/cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql b/cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql index 4763504ca0d..cbfc2fc7f90 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql @@ -1,10 +1,13 @@ /** - * @name Integer addition may overflow inside if statement - * @description Detects "if (a+b>c) a=c-b", which is incorrect if a+b overflows. - * Should be replaced by "if (a>c-b) a=c-b", which correctly - * implements a = min(a,c-b)". This integer overflow is the root - * cause of the buffer overflow in the SHA-3 reference implementation - * (CVE-2022-37454). + * @name Integer addition may overflow inside condition + * @description Detects "c-b" when the condition "a+b>c" has been imposed, + * which is not the same as the condition "a>b-c" if "a+b" + * overflows. Rewriting improves readability and optimizability + * (CSE elimination). Also detects "b+a>c" (swapped terms in + * addition), "c=", "<", + * "<=" instead of ">" (all operators). This integer overflow + * is the root cause of the buffer overflow in the SHA-3 + * reference implementation (CVE-2022-37454). * @kind problem * @problem.severity warning * @id cpp/if-statement-addition-overflow @@ -15,14 +18,19 @@ */ import cpp +import semmle.code.cpp.controlflow.Guards +import semmle.code.cpp.valuenumbering.GlobalValueNumbering +import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis +import semmle.code.cpp.commons.Exclusions -from IfStmt ifstmt, GTExpr gtexpr, ExprStmt exprstmt, AssignExpr assignexpr, AddExpr addexpr, SubExpr subexpr -where ifstmt.getCondition() = gtexpr and - gtexpr.getLeftOperand() = addexpr and - ifstmt.getThen() = exprstmt and - exprstmt.getExpr() = assignexpr and - assignexpr.getRValue() = subexpr and - addexpr.getLeftOperand().toString() = assignexpr.getLValue().toString() and - addexpr.getRightOperand().toString() = subexpr.getRightOperand().toString() and - gtexpr.getRightOperand().toString() = subexpr.getLeftOperand().toString() -select ifstmt, "Integer addition may overflow inside if statement." +from GuardCondition guard, BasicBlock block, RelationalOperation relop, AddExpr addexpr, SubExpr subexpr +where guard.controls(block, _) and + guard.getAChild*() = relop and + pragma[only_bind_into](block) = subexpr.getBasicBlock() and + relop.getAnOperand() = addexpr and + addexpr.getUnspecifiedType() instanceof IntegralType and + not isFromMacroDefinition(relop) and + exprMightOverflowPositively(addexpr) and + globalValueNumber(addexpr.getAnOperand()) = globalValueNumber(subexpr.getRightOperand()) and + globalValueNumber(relop.getAnOperand()) = globalValueNumber(subexpr.getLeftOperand()) +select guard, "Integer addition may overflow inside condition."