diff --git a/java/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql b/java/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql index 7ca19969d881..c249f595f6e1 100644 --- a/java/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql +++ b/java/ql/src/Security/CWE/CWE-190/ComparisonWithWiderType.ql @@ -16,43 +16,20 @@ import java import semmle.code.java.arithmetic.Overflow -int leftWidth(ComparisonExpr e) { result = e.getLeftOperand().getType().(NumType).getWidthRank() } +int widthRank(Expr e) { result = e.getType().(NumType).getWidthRank() } -int rightWidth(ComparisonExpr e) { result = e.getRightOperand().getType().(NumType).getWidthRank() } - -abstract class WideningComparison extends BinaryExpr instanceof ComparisonExpr { - abstract Expr getNarrower(); - - abstract Expr getWider(); -} - -class LTWideningComparison extends WideningComparison { - LTWideningComparison() { - (this instanceof LEExpr or this instanceof LTExpr) and - leftWidth(this) < rightWidth(this) - } - - override Expr getNarrower() { result = this.getLeftOperand() } - - override Expr getWider() { result = this.getRightOperand() } -} - -class GTWideningComparison extends WideningComparison { - GTWideningComparison() { - (this instanceof GEExpr or this instanceof GTExpr) and - leftWidth(this) > rightWidth(this) - } - - override Expr getNarrower() { result = this.getRightOperand() } - - override Expr getWider() { result = this.getLeftOperand() } +predicate wideningComparison(ComparisonExpr c, Expr lesserOperand, Expr greaterOperand) { + lesserOperand = c.getLesserOperand() and + greaterOperand = c.getGreaterOperand() and + widthRank(lesserOperand) < widthRank(greaterOperand) } -from WideningComparison c, LoopStmt l +from ComparisonExpr c, LoopStmt l, Expr lesserOperand, Expr greaterOperand where + wideningComparison(c, lesserOperand, greaterOperand) and not c.getAnOperand().isCompileTimeConstant() and l.getCondition().getAChildExpr*() = c select c, - "Comparison between $@ of type " + c.getNarrower().getType().getName() + " and $@ of wider type " + - c.getWider().getType().getName() + ".", c.getNarrower(), "expression", c.getWider(), + "Comparison between $@ of type " + lesserOperand.getType().getName() + " and $@ of wider type " + + greaterOperand.getType().getName() + ".", lesserOperand, "expression", greaterOperand, "expression" diff --git a/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.expected b/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.expected new file mode 100644 index 000000000000..5e92fffb0efb --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.expected @@ -0,0 +1,2 @@ +| ComparisonWithWiderType.java:4:25:4:29 | ... < ... | Comparison between $@ of type int and $@ of wider type long. | ComparisonWithWiderType.java:4:25:4:25 | i | expression | ComparisonWithWiderType.java:4:29:4:29 | l | expression | +| ComparisonWithWiderType.java:16:26:16:30 | ... > ... | Comparison between $@ of type byte and $@ of wider type short. | ComparisonWithWiderType.java:16:30:16:30 | b | expression | ComparisonWithWiderType.java:16:26:16:26 | c | expression | diff --git a/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.java b/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.java new file mode 100644 index 000000000000..88c520307a4e --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.java @@ -0,0 +1,27 @@ +public class ComparisonWithWiderType { + public void testLt(long l) { + // BAD: loop variable is an int, but the upper bound is a long + for (int i = 0; i < l; i++) { + System.out.println(i); + } + + // GOOD: loop variable is a long + for (long i = 0; i < l; i++) { + System.out.println(i); + } + } + + public void testGt(short c) { + // BAD: loop variable is a byte, but the upper bound is a short + for (byte b = 0; c > b; b++) { + System.out.println(b); + } + } + + public void testLe(int i) { + // GOOD: loop variable is a long, and the upper bound is an int + for (long l = 0; l <= i; l++) { + System.out.println(l); + } + } +} \ No newline at end of file diff --git a/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.qlref b/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.qlref new file mode 100644 index 000000000000..4605189317fa --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-190/semmle/tests/ComparisonWithWiderType.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-190/ComparisonWithWiderType.ql \ No newline at end of file