Merge pull request #10555 from MathiasVP/testcase-for-php-cve

C++: Fix missing bounds in range analysis

cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticExprSpecific.qll

@@ -7,6 +7,7 @@ private import semmle.code.cpp.ir.IR as IR
 private import Semantic
 private import experimental.semmle.code.cpp.rangeanalysis.Bound as IRBound
 private import semmle.code.cpp.controlflow.IRGuards as IRGuards
+private import semmle.code.cpp.ir.ValueNumbering
 
 module SemanticExprConfig {
   class Location = Cpp::Location;
@@ -120,7 +121,15 @@ module SemanticExprConfig {
 
   newtype TSsaVariable =
     TSsaInstruction(IR::Instruction instr) { instr.hasMemoryResult() } or
-    TSsaOperand(IR::Operand op) { op.isDefinitionInexact() }
+    TSsaOperand(IR::Operand op) { op.isDefinitionInexact() } or
+    TSsaPointerArithmeticGuard(IR::PointerArithmeticInstruction instr) {
+      exists(Guard g, IR::Operand use | use = instr.getAUse() |
+        g.comparesLt(use, _, _, _, _) or
+        g.comparesLt(_, use, _, _, _) or
+        g.comparesEq(use, _, _, _, _) or
+        g.comparesEq(_, use, _, _, _)
+      )
+    }
 
   class SsaVariable extends TSsaVariable {
     string toString() { none() }
@@ -129,6 +138,8 @@ module SemanticExprConfig {
 
     IR::Instruction asInstruction() { none() }
 
+    IR::PointerArithmeticInstruction asPointerArithGuard() { none() }
+
     IR::Operand asOperand() { none() }
   }
 
@@ -144,6 +155,18 @@ module SemanticExprConfig {
     final override IR::Instruction asInstruction() { result = instr }
   }
 
+  class SsaPointerArithmeticGuard extends SsaVariable, TSsaPointerArithmeticGuard {
+    IR::PointerArithmeticInstruction instr;
+
+    SsaPointerArithmeticGuard() { this = TSsaPointerArithmeticGuard(instr) }
+
+    final override string toString() { result = instr.toString() }
+
+    final override Location getLocation() { result = instr.getLocation() }
+
+    final override IR::PointerArithmeticInstruction asPointerArithGuard() { result = instr }
+  }
+
   class SsaOperand extends SsaVariable, TSsaOperand {
     IR::Operand op;
 
@@ -168,7 +191,11 @@ module SemanticExprConfig {
     )
   }
 
-  Expr getAUse(SsaVariable v) { result.(IR::LoadInstruction).getSourceValue() = v.asInstruction() }
+  Expr getAUse(SsaVariable v) {
+    result.(IR::LoadInstruction).getSourceValue() = v.asInstruction()
+    or
+    result = valueNumber(v.asPointerArithGuard()).getAnInstruction()
+  }
 
   SemType getSsaVariableType(SsaVariable v) {
     result = getSemanticType(v.asInstruction().getResultIRType())
@@ -208,7 +235,9 @@ module SemanticExprConfig {
 
     final override predicate hasRead(SsaVariable v) {
       exists(IR::Operand operand |
-        operand.getDef() = v.asInstruction() and
+        operand.getDef() = v.asInstruction() or
+        operand.getDef() = valueNumber(v.asPointerArithGuard()).getAnInstruction()
+      |
         not operand instanceof IR::PhiInputOperand and
         operand.getUse().getBlock() = block
       )
@@ -227,7 +256,9 @@ module SemanticExprConfig {
 
     final override predicate hasRead(SsaVariable v) {
       exists(IR::PhiInputOperand operand |
-        operand.getDef() = v.asInstruction() and
+        operand.getDef() = v.asInstruction() or
+        operand.getDef() = valueNumber(v.asPointerArithGuard()).getAnInstruction()
+      |
         operand.getPredecessorBlock() = pred and
         operand.getUse().getBlock() = succ
       )

cpp/ql/lib/experimental/semmle/code/cpp/semantic/SemanticSSA.qll

@@ -10,7 +10,7 @@ class SemSsaVariable instanceof Specific::SsaVariable {
 
   final Specific::Location getLocation() { result = super.getLocation() }
 
-  final SemLoadExpr getAUse() { result = Specific::getAUse(this) }
+  final SemExpr getAUse() { result = Specific::getAUse(this) }
 
   final SemType getType() { result = Specific::getSsaVariableType(this) }
 

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/InvalidPointerDeref.expected

@@ -609,6 +609,74 @@ edges
 | test.cpp:180:19:180:28 | call to mk_array_p indirection [begin] | test.cpp:165:29:165:31 | arr indirection [begin] |
 | test.cpp:180:19:180:28 | call to mk_array_p indirection [end] | test.cpp:165:29:165:31 | arr indirection [end] |
 | test.cpp:188:15:188:20 | call to malloc | test.cpp:189:15:189:15 | Load |
+| test.cpp:194:23:194:28 | call to malloc | test.cpp:195:17:195:17 | Load |
+| test.cpp:194:23:194:28 | call to malloc | test.cpp:197:8:197:8 | Load |
+| test.cpp:194:23:194:28 | call to malloc | test.cpp:201:5:201:5 | Load |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | ... + ... |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
+| test.cpp:195:17:195:17 | Load | test.cpp:195:17:195:23 | Store |
+| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
+| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
+| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
+| test.cpp:195:17:195:17 | Load | test.cpp:197:20:197:22 | Load |
+| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
+| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
+| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
+| test.cpp:195:17:195:17 | Load | test.cpp:201:5:201:12 | access to array |
+| test.cpp:195:17:195:23 | ... + ... | test.cpp:195:17:195:23 | Store |
+| test.cpp:195:17:195:23 | ... + ... | test.cpp:195:17:195:23 | Store |
+| test.cpp:195:17:195:23 | ... + ... | test.cpp:197:20:197:22 | Load |
+| test.cpp:195:17:195:23 | ... + ... | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:195:17:195:23 | ... + ... | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:195:17:195:23 | Store | test.cpp:197:20:197:22 | Load |
+| test.cpp:195:17:195:23 | Store | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:195:17:195:23 | Store | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:197:20:197:22 | Load | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:197:20:197:22 | Load | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:201:5:201:12 | access to array | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:201:5:201:12 | access to array | test.cpp:201:5:201:19 | Store: ... = ... |
+| test.cpp:205:23:205:28 | call to malloc | test.cpp:206:17:206:17 | Load |
+| test.cpp:205:23:205:28 | call to malloc | test.cpp:208:15:208:15 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | ... + ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
+| test.cpp:206:17:206:17 | Load | test.cpp:206:17:206:23 | Store |
+| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:209:12:209:14 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:5:213:6 | * ... |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
+| test.cpp:206:17:206:17 | Load | test.cpp:213:6:213:6 | Load |
+| test.cpp:206:17:206:23 | ... + ... | test.cpp:206:17:206:23 | Store |
+| test.cpp:206:17:206:23 | ... + ... | test.cpp:206:17:206:23 | Store |
+| test.cpp:206:17:206:23 | ... + ... | test.cpp:209:12:209:14 | Load |
+| test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:206:17:206:23 | ... + ... | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:206:17:206:23 | Store | test.cpp:209:12:209:14 | Load |
+| test.cpp:206:17:206:23 | Store | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:206:17:206:23 | Store | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:209:12:209:14 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:209:12:209:14 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:213:5:213:6 | * ... | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:213:5:213:6 | * ... | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:213:6:213:6 | Load | test.cpp:213:5:213:6 | * ... |
+| test.cpp:213:6:213:6 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
+| test.cpp:213:6:213:6 | Load | test.cpp:213:5:213:13 | Store: ... = ... |
 #select
 | test.cpp:6:14:6:15 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:6:14:6:15 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
 | test.cpp:8:14:8:21 | Load: * ... | test.cpp:4:15:4:20 | call to malloc | test.cpp:8:14:8:21 | Load: * ... | This read might be out of bounds, as the pointer might be equal to $@ + $@ + 1. | test.cpp:4:15:4:20 | call to malloc | call to malloc | test.cpp:5:19:5:22 | size | size |
@@ -625,3 +693,5 @@ edges
 | test.cpp:110:9:110:14 | Store: ... = ... | test.cpp:82:17:82:22 | call to malloc | test.cpp:110:9:110:14 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:82:17:82:22 | call to malloc | call to malloc | test.cpp:83:27:83:30 | size | size |
 | test.cpp:157:9:157:14 | Store: ... = ... | test.cpp:143:18:143:23 | call to malloc | test.cpp:157:9:157:14 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:143:18:143:23 | call to malloc | call to malloc | test.cpp:144:29:144:32 | size | size |
 | test.cpp:171:9:171:14 | Store: ... = ... | test.cpp:143:18:143:23 | call to malloc | test.cpp:171:9:171:14 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:143:18:143:23 | call to malloc | call to malloc | test.cpp:144:29:144:32 | size | size |
+| test.cpp:201:5:201:19 | Store: ... = ... | test.cpp:194:23:194:28 | call to malloc | test.cpp:201:5:201:19 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:194:23:194:28 | call to malloc | call to malloc | test.cpp:195:21:195:23 | len | len |
+| test.cpp:213:5:213:13 | Store: ... = ... | test.cpp:205:23:205:28 | call to malloc | test.cpp:213:5:213:13 | Store: ... = ... | This write might be out of bounds, as the pointer might be equal to $@ + $@. | test.cpp:205:23:205:28 | call to malloc | call to malloc | test.cpp:206:21:206:23 | len | len |

cpp/ql/test/experimental/query-tests/Security/CWE/CWE-193/pointer-deref/test.cpp

@@ -188,4 +188,27 @@ void test11(unsigned size) {
     char *p = malloc(size);
     char *q = p + size - 1;
     deref_plus_one(q);
-}
+}
+
+void test12(unsigned len, unsigned index) {
+    char* p = (char *)malloc(len);
+    char* end = p + len;
+    
+    if(p + index > end) {
+        return;
+    }
+    
+    p[index] = '\0'; // BAD
+}
+
+void test13(unsigned len, unsigned index) {
+    char* p = (char *)malloc(len);
+    char* end = p + len;
+    
+    char* q = p + index;
+    if(q > end) {
+        return;
+    }
+    
+    *q = '\0'; // BAD
+}