C++: Emit InitializeDynamicAllocation instructions for NewExpr and NewArrayExpr

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedCall.qll

@@ -206,7 +206,44 @@ abstract class TranslatedCall extends TranslatedExpr {
 
   predicate hasPreciseSideEffect() { exists(getSideEffects()) }
 
-  TranslatedSideEffects getSideEffects() { result.getCall() = expr }
+  final TranslatedSideEffects getSideEffects() { result.getExpr() = expr }
+}
+
+abstract class TranslatedSideEffects extends TranslatedElement {
+  abstract Expr getExpr();
+
+  final override Locatable getAST() { result = getExpr() }
+
+  final override Function getFunction() { result = getExpr().getEnclosingFunction() }
+
+  override TranslatedElement getChild(int i) {
+    result =
+      rank[i + 1](TranslatedSideEffect tse, int isWrite, int index |
+        (
+          tse.getCall() = getExpr() and
+          tse.getArgumentIndex() = index and
+          if tse.isWrite() then isWrite = 1 else isWrite = 0
+        )
+      |
+        tse order by isWrite, index
+      )
+  }
+
+  final override Instruction getChildSuccessor(TranslatedElement te) {
+    exists(int i |
+      getChild(i) = te and
+      if exists(getChild(i + 1))
+      then result = getChild(i + 1).getFirstInstruction()
+      else result = getParent().getChildSuccessor(this)
+    )
+  }
+
+  /**
+   * Gets the `TranslatedFunction` containing this expression.
+   */
+  final TranslatedFunction getEnclosingFunction() {
+    result = getTranslatedFunction(getExpr().getEnclosingFunction())
+  }
 }
 
 /**
@@ -308,66 +345,27 @@ class TranslatedStructorCall extends TranslatedFunctionCall {
   override predicate hasQualifier() { any() }
 }
 
-class TranslatedSideEffects extends TranslatedElement, TTranslatedSideEffects {
-  Call expr;
-
-  TranslatedSideEffects() { this = TTranslatedSideEffects(expr) }
+class TranslatedAllocationSideEffects extends TranslatedSideEffects,
+  TTranslatedAllocationSideEffects {
+  AllocationExpr expr;
 
-  override string toString() { result = "(side effects  for " + expr.toString() + ")" }
+  TranslatedAllocationSideEffects() { this = TTranslatedAllocationSideEffects(expr) }
 
-  override Locatable getAST() { result = expr }
+  final override AllocationExpr getExpr() { result = expr }
 
-  Call getCall() { result = expr }
+  override string toString() { result = "(allocation side effects for " + expr.toString() + ")" }
 
-  override TranslatedElement getChild(int i) {
-    result =
-      rank[i + 1](TranslatedSideEffect tse, int isWrite, int index |
-        (
-          tse.getCall() = getCall() and
-          tse.getArgumentIndex() = index and
-          if tse.isWrite() then isWrite = 1 else isWrite = 0
-        )
-      |
-        tse order by isWrite, index
-      )
-  }
-
-  override Instruction getChildSuccessor(TranslatedElement te) {
-    exists(int i |
-      getChild(i) = te and
-      if exists(getChild(i + 1))
-      then result = getChild(i + 1).getFirstInstruction()
-      else result = getParent().getChildSuccessor(this)
-    )
-  }
+  override Instruction getFirstInstruction() { result = getInstruction(OnlyInstructionTag()) }
 
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) {
-    expr.getTarget() instanceof AllocationFunction and
-    not exists(NewOrNewArrayExpr newExpr |
-      // we synthesize allocator calls for `new` and `new[]`, so don't add instructions to
-      // the existing allocator call when it exists.
-      newExpr.getAllocatorCall() = expr
-    ) and
     opcode instanceof Opcode::InitializeDynamicAllocation and
     tag = OnlyInstructionTag() and
     type = getUnknownType()
   }
 
-  override Instruction getFirstInstruction() {
-    if expr.getTarget() instanceof AllocationFunction
-    then result = getInstruction(OnlyInstructionTag())
-    else result = getChild(0).getFirstInstruction()
-  }
-
   override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) {
     tag = OnlyInstructionTag() and
     kind = gotoEdge() and
-    expr.getTarget() instanceof AllocationFunction and
-    not exists(NewOrNewArrayExpr newExpr |
-      // we synthesize allocator calls for `new` and `new[]`, so don't add instructions to
-      // the existing allocator call when it exists.
-      newExpr.getAllocatorCall() = expr
-    ) and
     if exists(getChild(0))
     then result = getChild(0).getFirstInstruction()
     else result = getParent().getChildSuccessor(this)
@@ -381,23 +379,34 @@ class TranslatedSideEffects extends TranslatedElement, TTranslatedSideEffects {
 
   override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
     tag = OnlyInstructionTag() and
-    result = getTranslatedExpr(expr).getInstruction(CallTag())
+    if expr instanceof NewOrNewArrayExpr
+    then result = getTranslatedAllocatorCall(expr).getInstruction(CallTag())
+    else result = getTranslatedExpr(expr).getInstruction(CallTag())
   }
+}
 
-  /**
-   * Gets the `TranslatedFunction` containing this expression.
-   */
-  final TranslatedFunction getEnclosingFunction() {
-    result = getTranslatedFunction(expr.getEnclosingFunction())
-  }
+class TranslatedCallSideEffects extends TranslatedSideEffects, TTranslatedCallSideEffects {
+  Call expr;
 
-  /**
-   * Gets the `Function` containing this expression.
-   */
-  override Function getFunction() { result = expr.getEnclosingFunction() }
+  TranslatedCallSideEffects() { this = TTranslatedCallSideEffects(expr) }
+
+  override string toString() { result = "(side effects  for " + expr.toString() + ")" }
+
+  override Call getExpr() { result = expr }
+
+  override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType type) { none() }
+
+  override Instruction getFirstInstruction() { result = getChild(0).getFirstInstruction() }
+
+  override Instruction getInstructionSuccessor(InstructionTag tag, EdgeKind kind) { none() }
+
+  override Instruction getPrimaryInstructionForSideEffect(InstructionTag tag) {
+    tag = OnlyInstructionTag() and
+    result = getTranslatedExpr(expr).getInstruction(CallTag())
+  }
 }
 
-class TranslatedStructorCallSideEffects extends TranslatedSideEffects {
+class TranslatedStructorCallSideEffects extends TranslatedCallSideEffects {
   TranslatedStructorCallSideEffects() { getParent().(TranslatedStructorCall).hasQualifier() }
 
   override predicate hasInstruction(Opcode opcode, InstructionTag tag, CppType t) {

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedElement.qll

@@ -442,11 +442,22 @@ newtype TTranslatedElement =
   // The declaration/initialization part of a `ConditionDeclExpr`
   TTranslatedConditionDecl(ConditionDeclExpr expr) { not ignoreExpr(expr) } or
   // The side effects of a `Call`
-  TTranslatedSideEffects(Call expr) {
-    exists(TTranslatedArgumentSideEffect(expr, _, _, _)) or
-    expr instanceof ConstructorCall or
-    expr.getTarget() instanceof AllocationFunction
-  } or // A precise side effect of an argument to a `Call`
+  TTranslatedCallSideEffects(Call expr) {
+    // Exclude allocations such as `malloc` (which happen to also be function calls).
+    // Both `TranslatedCallSideEffects` and `TranslatedAllocationSideEffects` generate
+    // the same side effects for its children as they both extend the `TranslatedSideEffects`
+    // class.
+    // Note: We can separate allocation side effects and call side effects into two
+    // translated elements as no call can be both a `ConstructorCall` and an `AllocationExpr`.
+    not expr instanceof AllocationExpr and
+    (
+      exists(TTranslatedArgumentSideEffect(expr, _, _, _)) or
+      expr instanceof ConstructorCall
+    )
+  } or
+  // The side effects of an allocation, i.e. `new`, `new[]` or `malloc`
+  TTranslatedAllocationSideEffects(AllocationExpr expr) or
+  // A precise side effect of an argument to a `Call`
   TTranslatedArgumentSideEffect(Call call, Expr expr, int n, boolean isWrite) {
     (
       expr = call.getArgument(n).getFullyConverted()

cpp/ql/src/semmle/code/cpp/ir/implementation/raw/internal/TranslatedExpr.qll

@@ -1649,6 +1649,11 @@ class TranslatedAllocatorCall extends TTranslatedAllocatorCall, TranslatedDirect
 
   final override int getNumberOfArguments() {
     result = expr.getAllocatorCall().getNumberOfArguments()
+    or
+    // Make sure there's a result even when there is no allocator, as otherwise
+    // TranslatedCall::getChild() will not return the side effects for this call.
+    not exists(expr.getAllocatorCall()) and
+    result = 0
   }
 
   final override TranslatedExpr getArgument(int index) {

cpp/ql/test/library-tests/dataflow/security-taint/tainted.expected

@@ -73,3 +73,18 @@
 | test.cpp:100:17:100:22 | buffer | test.cpp:93:18:93:18 | s |  |
 | test.cpp:100:17:100:22 | buffer | test.cpp:97:7:97:12 | buffer |  |
 | test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | buffer |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:8:24:8:25 | s1 |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:20:11:21 | s1 |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:36:11:37 | s2 |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:17:106:24 | userName |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:33 | call to getenv |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:46 | (const char *)... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:108:8:108:11 | copy |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:2:109:7 | call to strcpy |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:9:109:12 | copy |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:15:109:22 | userName |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:6:111:27 | ! ... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:12 | call to strcmp |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:27 | (bool)... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | (const char *)... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | copy |  |

cpp/ql/test/library-tests/dataflow/security-taint/tainted_diff.expected

@@ -14,3 +14,6 @@
 | test.cpp:100:12:100:15 | call to gets | test.cpp:100:2:100:8 | pointer | AST only |
 | test.cpp:100:17:100:22 | buffer | test.cpp:97:7:97:12 | buffer | AST only |
 | test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | array to pointer conversion | IR only |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:20:11:21 | s1 | AST only |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:108:8:108:11 | copy | AST only |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:9:109:12 | copy | AST only |

cpp/ql/test/library-tests/dataflow/security-taint/tainted_ir.expected

@@ -57,3 +57,15 @@
 | test.cpp:100:17:100:22 | buffer | test.cpp:93:18:93:18 | s |  |
 | test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | array to pointer conversion |  |
 | test.cpp:100:17:100:22 | buffer | test.cpp:100:17:100:22 | buffer |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:8:24:8:25 | s1 |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:11:36:11:37 | s2 |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:17:106:24 | userName |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:33 | call to getenv |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:106:28:106:46 | (const char *)... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:2:109:7 | call to strcpy |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:109:15:109:22 | userName |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:6:111:27 | ! ... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:12 | call to strcmp |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:7:111:27 | (bool)... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | (const char *)... |  |
+| test.cpp:106:28:106:33 | call to getenv | test.cpp:111:14:111:17 | copy |  |

cpp/ql/test/library-tests/dataflow/security-taint/test.cpp

@@ -99,3 +99,16 @@ void test_gets()
 
 	pointer = gets(buffer);
 }
+
+const char *alias_global_new;
+
+void newBuffer() {
+    const char *userName = getenv("USER_NAME");
+	char *alias = new char[4096];
+	char *copy = new char[4096];
+	strcpy(copy, userName);
+	alias_global_new = alias; // to force a Chi node on all aliased memory
+	if (!strcmp(copy, "admin")) { // copy should be tainted
+		isAdmin = true;
+	}
+}