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 cbfc2fc7f90..5bfa265fca5 100644 --- a/cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql +++ b/cpp/ql/src/experimental/Security/CWE/CWE-190/IfStatementAdditionOverflow.ql @@ -1,13 +1,13 @@ /** - * @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). + * @name Integer addition may overflow inside if statement + * @description Detects "if (a+b>c) a=c-b", which incorrectly implements + * a = min(a,c-b) if a+b overflows. Should be replaced by + * "if (a>c-b) a=c-b". Also detects "if (b+a>c) a=c-b" + * (swapped terms in addition), if (a+b>c) { a=c-b }" + * (assignment inside block), "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 @@ -18,19 +18,27 @@ */ import cpp -import semmle.code.cpp.controlflow.Guards import semmle.code.cpp.valuenumbering.GlobalValueNumbering +import semmle.code.cpp.valuenumbering.HashCons import semmle.code.cpp.rangeanalysis.SimpleRangeAnalysis import semmle.code.cpp.commons.Exclusions -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 +from IfStmt ifstmt, RelationalOperation relop, ExprStmt exprstmt, BlockStmt blockstmt, AssignExpr assignexpr, AddExpr addexpr, SubExpr subexpr +where ifstmt.getCondition() = relop and relop.getAnOperand() = addexpr and addexpr.getUnspecifiedType() instanceof IntegralType and + subexpr.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." + (ifstmt.getThen() = exprstmt or + (ifstmt.getThen() = blockstmt and + blockstmt.getAStmt() = exprstmt)) and + exprstmt.getExpr() = assignexpr and + assignexpr.getRValue() = subexpr and + ((hashCons(addexpr.getLeftOperand()) = hashCons(assignexpr.getLValue()) and + globalValueNumber(addexpr.getRightOperand()) = globalValueNumber(subexpr.getRightOperand())) or + (hashCons(addexpr.getRightOperand()) = hashCons(assignexpr.getLValue()) and + globalValueNumber(addexpr.getLeftOperand()) = globalValueNumber(subexpr.getRightOperand()))) and + globalValueNumber(relop.getAnOperand()) = globalValueNumber(subexpr.getLeftOperand()) and + not globalValueNumber(addexpr.getAnOperand()) = globalValueNumber(relop.getAnOperand()) +select ifstmt, "Integer addition may overflow inside if statement."