Merge remote-tracking branch 'upstream/main' into python-port-path-injection

cpp/ql/src/semmle/code/cpp/exprs/BuiltInOperations.qll

@@ -27,7 +27,7 @@ class VarArgsExpr extends BuiltInOperation, @var_args_expr { }
  * __builtin_va_start(ap, last_named_param);
  * ```
  */
-class BuiltInVarArgsStart extends BuiltInOperation, @vastartexpr {
+class BuiltInVarArgsStart extends VarArgsExpr, @vastartexpr {
   override string toString() { result = "__builtin_va_start" }
 
   override string getAPrimaryQlClass() { result = "BuiltInVarArgsStart" }
@@ -52,7 +52,7 @@ class BuiltInVarArgsStart extends BuiltInOperation, @vastartexpr {
  * __builtin_va_end(ap);
  * ```
  */
-class BuiltInVarArgsEnd extends BuiltInOperation, @vaendexpr {
+class BuiltInVarArgsEnd extends VarArgsExpr, @vaendexpr {
   override string toString() { result = "__builtin_va_end" }
 
   override string getAPrimaryQlClass() { result = "BuiltInVarArgsEnd" }
@@ -70,7 +70,7 @@ class BuiltInVarArgsEnd extends BuiltInOperation, @vaendexpr {
  * ap = __builtin_va_arg(ap, long);
  * ```
  */
-class BuiltInVarArg extends BuiltInOperation, @vaargexpr {
+class BuiltInVarArg extends VarArgsExpr, @vaargexpr {
   override string toString() { result = "__builtin_va_arg" }
 
   override string getAPrimaryQlClass() { result = "BuiltInVarArg" }
@@ -90,7 +90,7 @@ class BuiltInVarArg extends BuiltInOperation, @vaargexpr {
  * va_copy(aq, ap);
  * ```
  */
-class BuiltInVarArgCopy extends BuiltInOperation, @vacopyexpr {
+class BuiltInVarArgCopy extends VarArgsExpr, @vacopyexpr {
   override string toString() { result = "__builtin_va_copy" }
 
   override string getAPrimaryQlClass() { result = "BuiltInVarArgCopy" }

csharp/change-notes/2020-10-28-cil-to-string.md

@@ -0,0 +1,3 @@
+lgtm,codescanning
+* The `toString()` predicate is now less verbose for CIL entities. The old `toString()` behavior
+  can be recovered by using `toStringExtra()` and `getQualifiedName()`, respectively.

csharp/ql/src/semmle/code/cil/Attribute.qll

@@ -14,7 +14,7 @@ class Attribute extends Element, @cil_attribute {
   /** Gets the type of this attribute. */
   Type getType() { result = getConstructor().getDeclaringType() }
 
-  override string toString() { result = "[" + getType().toString() + "(...)]" }
+  override string toString() { result = "[" + getType().getName() + "(...)]" }
 
   /** Gets the value of the `i`th argument of this attribute. */
   string getArgument(int i) { cil_attribute_positional_argument(this, i, result) }

csharp/ql/src/semmle/code/cil/ConsistencyChecks.qll

@@ -477,7 +477,9 @@ class InvalidOverride extends MethodViolation {
   }
 
   override string getMessage() {
-    result = "Overridden method from " + base.getDeclaringType() + " is not in a base type"
+    result =
+      "Overridden method from " + base.getDeclaringType().getQualifiedName() +
+        " is not in a base type"
   }
 }
 

csharp/ql/src/semmle/code/cil/Instruction.qll

@@ -4,7 +4,10 @@ private import CIL
 
 /** An instruction. */
 class Instruction extends Element, ControlFlowNode, DataFlowNode, @cil_instruction {
-  override string toString() { result = getIndex() + ": " + getOpcodeName() + getExtraStr() }
+  override string toString() { result = getOpcodeName() }
+
+  /** Gets a more verbose textual representation of this instruction. */
+  string toStringExtra() { result = getIndex() + ": " + getOpcodeName() + getExtraStr() }
 
   /** Gets the method containing this instruction. */
   override MethodImplementation getImplementation() { cil_instruction(this, _, _, result) }

csharp/ql/src/semmle/code/cil/Method.qll

@@ -54,7 +54,11 @@ class MethodImplementation extends EntryPoint, @cil_method_implementation {
   /** Gets a string representing the disassembly of this implementation. */
   string getDisassembly() {
     result =
-      concat(Instruction i | i = this.getAnInstruction() | i.toString(), ", " order by i.getIndex())
+      concat(Instruction i |
+        i = this.getAnInstruction()
+      |
+        i.toStringExtra(), ", " order by i.getIndex()
+      )
   }
 }
 
@@ -76,7 +80,7 @@ class Method extends DotNet::Callable, Element, Member, TypeContainer, DataFlowN
 
   override string getName() { cil_method(this, result, _, _) }
 
-  override string toString() { result = this.getQualifiedName() }
+  override string toString() { result = this.getName() }
 
   override Type getDeclaringType() { cil_method(this, _, result, _) }
 

csharp/ql/src/semmle/code/cil/Type.qll

@@ -38,7 +38,7 @@ class Type extends DotNet::Type, Declaration, TypeContainer, @cil_type {
 
   override string getName() { cil_type(this, result, _, _, _) }
 
-  override string toString() { result = getQualifiedName() }
+  override string toString() { result = this.getName() }
 
   /** Gets the containing type of this type, if any. */
   override Type getDeclaringType() { result = getParent() }

csharp/ql/src/semmle/code/cil/Variable.qll

@@ -39,7 +39,8 @@ class StackVariable extends Variable, @cil_stack_variable {
  */
 class LocalVariable extends StackVariable, @cil_local_variable {
   override string toString() {
-    result = "Local variable " + getIndex() + " of method " + getImplementation().getMethod()
+    result =
+      "Local variable " + getIndex() + " of method " + getImplementation().getMethod().getName()
   }
 
   /** Gets the method implementation defining this local variable. */
@@ -65,7 +66,7 @@ class Parameter extends DotNet::Parameter, StackVariable, @cil_parameter {
   /** Gets the index of this parameter. */
   int getIndex() { cil_parameter(this, _, result, _) }
 
-  override string toString() { result = "Parameter " + getIndex() + " of " + getMethod() }
+  override string toString() { result = "Parameter " + getIndex() + " of " + getMethod().getName() }
 
   override Type getType() { cil_parameter(this, _, _, result) }
 

csharp/ql/src/semmle/code/csharp/Attribute.qll

@@ -89,7 +89,7 @@ class Attribute extends TopLevelExprParent, @attribute {
   override Location getALocation() { attribute_location(this, result) }
 
   override string toString() {
-    exists(string type, string name | type = getType().toString() |
+    exists(string type, string name | type = getType().getName() |
       (if type.matches("%Attribute") then name = type.prefix(type.length() - 9) else name = type) and
       result = "[" + name + "(...)]"
     )

csharp/ql/src/semmle/code/csharp/Generics.qll

@@ -227,7 +227,7 @@ class TypeParameterConstraints extends Element, @type_parameter_constraints {
   predicate hasNullableRefTypeConstraint() { general_type_parameter_constraints(this, 5) }
 
   /** Gets a textual representation of these constraints. */
-  override string toString() { result = "where " + this.getTypeParameter().toString() + ": ..." }
+  override string toString() { result = "where " + this.getTypeParameter().getName() + ": ..." }
 }
 
 /**

csharp/ql/src/semmle/code/csharp/dataflow/LibraryTypeDataFlow.qll

@@ -1904,6 +1904,53 @@ class SystemThreadingTasksTaskTFlow extends LibraryTypeDataFlow, SystemThreading
     sinkAp =
       AccessPath::field(any(SystemRuntimeCompilerServicesTaskAwaiterStruct s)
             .getUnderlyingTaskField())
+    or
+    // var awaitable = task.ConfigureAwait(false);  // <-- new ConfiguredTaskAwaitable<>(task, false)
+    //                                              //       m_configuredTaskAwaiter = new ConfiguredTaskAwaiter(task, false)
+    //                                              //         m_task = task
+    // var awaiter = awaitable.GetAwaiter();
+    // var result = awaiter.GetResult();
+    m = this.getConfigureAwaitMethod() and
+    source = TCallableFlowSourceQualifier() and
+    sourceAp = AccessPath::empty() and
+    sink = TCallableFlowSinkReturn() and
+    sinkAp =
+      AccessPath::cons(any(FieldContent fc |
+          fc.getField() =
+            any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct t)
+                .getUnderlyingAwaiterField()
+        ),
+        AccessPath::field(any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterStruct s
+          ).getUnderlyingTaskField()))
+  }
+
+  override predicate requiresAccessPath(Content head, AccessPath tail) {
+    head.(FieldContent).getField() =
+      any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct t).getUnderlyingAwaiterField() and
+    tail =
+      AccessPath::field(any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterStruct s
+        ).getUnderlyingTaskField())
+  }
+}
+
+/** Data flow for `System.Runtime.CompilerServices.ConfiguredTaskAwaitable<>`. */
+private class SystemRuntimeCompilerServicesConfiguredTaskAwaitableTFlow extends LibraryTypeDataFlow,
+  SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct {
+  override predicate callableFlow(
+    CallableFlowSource source, AccessPath sourceAp, CallableFlowSink sink, AccessPath sinkAp,
+    SourceDeclarationCallable c, boolean preservesValue
+  ) {
+    // var awaitable = task.ConfigureAwait(false);
+    // var awaiter = awaitable.GetAwaiter();  // <-- awaitable.m_configuredTaskAwaiter
+    // var result = awaiter.GetResult();
+    c = this.getGetAwaiterMethod() and
+    source = TCallableFlowSourceQualifier() and
+    sourceAp =
+      AccessPath::field(any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct s)
+            .getUnderlyingAwaiterField()) and
+    sink = TCallableFlowSinkReturn() and
+    sinkAp = AccessPath::empty() and
+    preservesValue = true
   }
 }
 
@@ -2011,6 +2058,32 @@ class SystemRuntimeCompilerServicesTaskAwaiterFlow extends LibraryTypeDataFlow,
   }
 }
 
+/** Data flow for `System.Runtime.CompilerServices.ConfiguredTaskAwaitable<>.ConfiguredTaskAwaiter`. */
+class SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterFlow extends LibraryTypeDataFlow,
+  SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterStruct {
+  override predicate callableFlow(
+    CallableFlowSource source, AccessPath sourceAp, CallableFlowSink sink, AccessPath sinkAp,
+    SourceDeclarationCallable c, boolean preservesValue
+  ) {
+    // var awaitable = task.ConfigureAwait(false);
+    // var awaiter = awaitable.GetAwaiter();
+    // var result = awaiter.GetResult();  // <-- task.Result
+    preservesValue = true and
+    c = this.getGetResultMethod() and
+    source = TCallableFlowSourceQualifier() and
+    sourceAp =
+      AccessPath::cons(any(FieldContent fc | fc.getField() = this.getUnderlyingTaskField()),
+        AccessPath::property(any(SystemThreadingTasksTaskTClass t).getResultProperty())) and
+    sink = TCallableFlowSinkReturn() and
+    sinkAp = AccessPath::empty()
+  }
+
+  override predicate requiresAccessPath(Content head, AccessPath tail) {
+    head.(FieldContent).getField() = this.getUnderlyingTaskField() and
+    tail = AccessPath::property(any(SystemThreadingTasksTaskTClass t).getResultProperty())
+  }
+}
+
 /** Data flow for `System.Text.Encoding`. */
 library class SystemTextEncodingFlow extends LibraryTypeDataFlow, SystemTextEncodingClass {
   override predicate callableFlow(

csharp/ql/src/semmle/code/csharp/dataflow/internal/DataFlowPrivate.qll

@@ -62,8 +62,10 @@ private class ExprNodeImpl extends ExprNode, NodeImpl {
   override string toStringImpl() {
     result = this.getControlFlowNode().toString()
     or
-    this = TCilExprNode(_) and
-    result = "CIL expression"
+    exists(CIL::Expr e |
+      this = TCilExprNode(e) and
+      result = e.toString()
+    )
   }
 }
 

csharp/ql/src/semmle/code/csharp/frameworks/system/runtime/CompilerServices.qll

@@ -28,3 +28,32 @@ class SystemRuntimeCompilerServicesTaskAwaiterStruct extends SystemRuntimeCompil
   /** Gets the field that stores the underlying task. */
   Field getUnderlyingTaskField() { result = this.getAField() and result.hasName("m_task") }
 }
+
+/** The `System.Runtime.CompilerServices.ConfiguredTaskAwaitable<>` struct. */
+class SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct extends SystemRuntimeCompilerServicesNamespaceUnboundGenericStruct {
+  SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct() {
+    this.hasName("ConfiguredTaskAwaitable<>")
+  }
+
+  /** Gets the `GetAwaiter` method. */
+  Method getGetAwaiterMethod() { result = this.getAMethod("GetAwaiter") }
+
+  /** Gets the field that stores the underlying awaiter. */
+  Field getUnderlyingAwaiterField() {
+    result = this.getAField() and result.hasName("m_configuredTaskAwaiter")
+  }
+}
+
+/** The `System.Runtime.CompilerServices.ConfiguredTaskAwaitable<>.ConfiguredTaskAwaiter` struct. */
+class SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterStruct extends Struct {
+  SystemRuntimeCompilerServicesConfiguredTaskAwaitableTConfiguredTaskAwaiterStruct() {
+    this = any(SystemRuntimeCompilerServicesConfiguredTaskAwaitableTStruct n).getANestedType() and
+    this.hasName("ConfiguredTaskAwaiter")
+  }
+
+  /** Gets the `GetResult` method. */
+  Method getGetResultMethod() { result = this.getAMethod("GetResult") }
+
+  /** Gets the field that stores the underlying task. */
+  Field getUnderlyingTaskField() { result = this.getAField() and result.hasName("m_task") }
+}

csharp/ql/src/semmle/code/csharp/frameworks/system/threading/Tasks.qll

@@ -41,4 +41,7 @@ class SystemThreadingTasksTaskTClass extends SystemThreadingTasksUnboundGenericC
 
   /** Gets the `GetAwaiter` method. */
   Method getGetAwaiterMethod() { result = this.getAMethod("GetAwaiter") }
+
+  /** Gets the `ConfigureAwait` method. */
+  Method getConfigureAwaitMethod() { result = this.getAMethod("ConfigureAwait") }
 }

csharp/ql/test/library-tests/cil/consistency/consistency.expected

@@ -1 +1 @@
-| System.Runtime.CompilerServices.AsyncTaskMethodBuilder.DebugFinalizableAsyncStateMachineBox.Finalize | Overridden method from System.Object is not in a base type |
+| Finalize | Overridden method from System.Object is not in a base type |

csharp/ql/test/library-tests/cil/pdbs/InstructionLocations.ql

@@ -11,4 +11,4 @@ from CIL::Instruction instruction, CIL::Location location
 where
   location = instruction.getLocation() and
   filterMethod(instruction.getImplementation().getMethod())
-select location.toString(), instruction.toString()
+select location.toString(), instruction.toStringExtra()

csharp/ql/test/library-tests/cil/regressions/ConstructedMethods.expected

@@ -1,2 +1,2 @@
-| Methods.dll:0:0:0:0 | Methods.Class1.F | Methods.dll:0:0:0:0 | Methods.Class1.F | Methods.dll:0:0:0:0 | Methods.Class1.G.!0 |
-| Methods.dll:0:0:0:0 | Methods.Class1.F | Methods.dll:0:0:0:0 | Methods.Class1.F | Methods.dll:0:0:0:0 | Methods.Class1.H.!0 |
+| Methods.dll:0:0:0:0 | F | Methods.dll:0:0:0:0 | F | Methods.dll:0:0:0:0 | !0 |
+| Methods.dll:0:0:0:0 | F | Methods.dll:0:0:0:0 | F | Methods.dll:0:0:0:0 | !0 |

csharp/ql/test/library-tests/dataflow/global/DataFlow.expected

@@ -51,6 +51,7 @@
 | GlobalDataFlow.cs:326:15:326:25 | access to parameter sinkParam11 |
 | GlobalDataFlow.cs:401:15:401:20 | access to local variable sink11 |
 | GlobalDataFlow.cs:424:41:424:46 | access to local variable sink20 |
+| GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 |
 | Splitting.cs:9:15:9:15 | [b (line 3): false] access to local variable x |
 | Splitting.cs:9:15:9:15 | [b (line 3): true] access to local variable x |
 | Splitting.cs:11:19:11:19 | access to local variable x |

csharp/ql/test/library-tests/dataflow/global/DataFlowPath.expected

@@ -221,6 +221,14 @@ edges
 | GlobalDataFlow.cs:402:16:402:21 | access to local variable sink11 : String | GlobalDataFlow.cs:164:22:164:43 | call to method TaintedParam : String |
 | GlobalDataFlow.cs:424:9:424:11 | value : String | GlobalDataFlow.cs:424:41:424:46 | access to local variable sink20 |
 | GlobalDataFlow.cs:435:22:435:35 | "taint source" : String | GlobalDataFlow.cs:198:22:198:32 | access to property OutProperty : String |
+| GlobalDataFlow.cs:471:20:471:49 | call to method Run [Result] : String | GlobalDataFlow.cs:472:25:472:28 | access to local variable task [Result] : String |
+| GlobalDataFlow.cs:471:35:471:48 | "taint source" : String | GlobalDataFlow.cs:471:20:471:49 | call to method Run [Result] : String |
+| GlobalDataFlow.cs:472:25:472:28 | access to local variable task [Result] : String | GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String | GlobalDataFlow.cs:473:23:473:31 | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:31 | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String | GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter [m_task, Result] : String | GlobalDataFlow.cs:474:22:474:28 | access to local variable awaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:474:22:474:28 | access to local variable awaiter [m_task, Result] : String | GlobalDataFlow.cs:474:22:474:40 | call to method GetResult : String |
+| GlobalDataFlow.cs:474:22:474:40 | call to method GetResult : String | GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 |
 | Splitting.cs:3:28:3:34 | tainted : String | Splitting.cs:8:24:8:30 | [b (line 3): false] access to parameter tainted : String |
 | Splitting.cs:3:28:3:34 | tainted : String | Splitting.cs:8:24:8:30 | [b (line 3): true] access to parameter tainted : String |
 | Splitting.cs:8:17:8:31 | [b (line 3): false] call to method Return : String | Splitting.cs:9:15:9:15 | [b (line 3): false] access to local variable x |
@@ -428,6 +436,15 @@ nodes
 | GlobalDataFlow.cs:424:9:424:11 | value : String | semmle.label | value : String |
 | GlobalDataFlow.cs:424:41:424:46 | access to local variable sink20 | semmle.label | access to local variable sink20 |
 | GlobalDataFlow.cs:435:22:435:35 | "taint source" : String | semmle.label | "taint source" : String |
+| GlobalDataFlow.cs:471:20:471:49 | call to method Run [Result] : String | semmle.label | call to method Run [Result] : String |
+| GlobalDataFlow.cs:471:35:471:48 | "taint source" : String | semmle.label | "taint source" : String |
+| GlobalDataFlow.cs:472:25:472:28 | access to local variable task [Result] : String | semmle.label | access to local variable task [Result] : String |
+| GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String | semmle.label | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:31 | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String | semmle.label | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter [m_task, Result] : String | semmle.label | call to method GetAwaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:474:22:474:28 | access to local variable awaiter [m_task, Result] : String | semmle.label | access to local variable awaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:474:22:474:40 | call to method GetResult : String | semmle.label | call to method GetResult : String |
+| GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 | semmle.label | access to local variable sink45 |
 | Splitting.cs:3:28:3:34 | tainted : String | semmle.label | tainted : String |
 | Splitting.cs:8:17:8:31 | [b (line 3): false] call to method Return : String | semmle.label | [b (line 3): false] call to method Return : String |
 | Splitting.cs:8:17:8:31 | [b (line 3): true] call to method Return : String | semmle.label | [b (line 3): true] call to method Return : String |
@@ -498,6 +515,7 @@ nodes
 | Capture.cs:122:15:122:20 | access to local variable sink40 | Capture.cs:115:26:115:39 | "taint source" : String | Capture.cs:122:15:122:20 | access to local variable sink40 | access to local variable sink40 |
 | GlobalDataFlow.cs:240:15:240:20 | access to local variable sink41 | GlobalDataFlow.cs:238:35:238:48 | "taint source" : String | GlobalDataFlow.cs:240:15:240:20 | access to local variable sink41 | access to local variable sink41 |
 | GlobalDataFlow.cs:242:15:242:20 | access to local variable sink42 | GlobalDataFlow.cs:238:35:238:48 | "taint source" : String | GlobalDataFlow.cs:242:15:242:20 | access to local variable sink42 | access to local variable sink42 |
+| GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 | GlobalDataFlow.cs:471:35:471:48 | "taint source" : String | GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 | access to local variable sink45 |
 | GlobalDataFlow.cs:145:15:145:19 | access to local variable sink5 | GlobalDataFlow.cs:18:27:18:40 | "taint source" : String | GlobalDataFlow.cs:145:15:145:19 | access to local variable sink5 | access to local variable sink5 |
 | GlobalDataFlow.cs:155:15:155:19 | access to local variable sink6 | GlobalDataFlow.cs:338:16:338:29 | "taint source" : String | GlobalDataFlow.cs:155:15:155:19 | access to local variable sink6 | access to local variable sink6 |
 | GlobalDataFlow.cs:158:15:158:19 | access to local variable sink7 | GlobalDataFlow.cs:343:13:343:26 | "taint source" : String | GlobalDataFlow.cs:158:15:158:19 | access to local variable sink7 | access to local variable sink7 |

csharp/ql/test/library-tests/dataflow/global/GetAnOutNode.expected

@@ -191,7 +191,12 @@
 | GlobalDataFlow.cs:456:23:456:35 | call to method ToString | return | GlobalDataFlow.cs:456:23:456:35 | call to method ToString |
 | GlobalDataFlow.cs:462:22:462:65 | call to method Join | return | GlobalDataFlow.cs:462:22:462:65 | call to method Join |
 | GlobalDataFlow.cs:465:23:465:65 | call to method Join | return | GlobalDataFlow.cs:465:23:465:65 | call to method Join |
-| GlobalDataFlow.cs:477:44:477:47 | delegate call | return | GlobalDataFlow.cs:477:44:477:47 | delegate call |
+| GlobalDataFlow.cs:471:20:471:49 | call to method Run | return | GlobalDataFlow.cs:471:20:471:49 | call to method Run |
+| GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait | qualifier | GlobalDataFlow.cs:472:25:472:28 | [post] access to local variable task |
+| GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait | return | GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait |
+| GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter | return | GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter |
+| GlobalDataFlow.cs:474:22:474:40 | call to method GetResult | return | GlobalDataFlow.cs:474:22:474:40 | call to method GetResult |
+| GlobalDataFlow.cs:486:44:486:47 | delegate call | return | GlobalDataFlow.cs:486:44:486:47 | delegate call |
 | Splitting.cs:8:17:8:31 | [b (line 3): false] call to method Return | return | Splitting.cs:8:17:8:31 | [b (line 3): false] call to method Return |
 | Splitting.cs:8:17:8:31 | [b (line 3): true] call to method Return | return | Splitting.cs:8:17:8:31 | [b (line 3): true] call to method Return |
 | Splitting.cs:20:22:20:30 | call to method Return | return | Splitting.cs:20:22:20:30 | call to method Return |

csharp/ql/test/library-tests/dataflow/global/GlobalDataFlow.cs

@@ -465,6 +465,15 @@ public class DataFlow
         var nonSink = string.Join(",", "whatever", "not tainted");
         Check(nonSink);
     }
+
+    public void M4()
+    {
+        var task = Task.Run(() => "taint source");
+        var awaitable = task.ConfigureAwait(false);
+        var awaiter = awaitable.GetAwaiter();
+        var sink45 = awaiter.GetResult();
+        Check(sink45);
+    }
 }
 
 static class IEnumerableExtensions

csharp/ql/test/library-tests/dataflow/global/TaintTracking.expected

@@ -57,6 +57,7 @@
 | GlobalDataFlow.cs:424:41:424:46 | access to local variable sink20 |
 | GlobalDataFlow.cs:453:15:453:20 | access to local variable sink43 |
 | GlobalDataFlow.cs:463:15:463:20 | access to local variable sink44 |
+| GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 |
 | Splitting.cs:9:15:9:15 | [b (line 3): false] access to local variable x |
 | Splitting.cs:9:15:9:15 | [b (line 3): true] access to local variable x |
 | Splitting.cs:11:19:11:19 | access to local variable x |

csharp/ql/test/library-tests/dataflow/global/TaintTrackingPath.expected

@@ -241,6 +241,14 @@ edges
 | GlobalDataFlow.cs:452:22:452:34 | call to method ToString : String | GlobalDataFlow.cs:453:15:453:20 | access to local variable sink43 |
 | GlobalDataFlow.cs:462:22:462:65 | call to method Join : String | GlobalDataFlow.cs:463:15:463:20 | access to local variable sink44 |
 | GlobalDataFlow.cs:462:51:462:64 | "taint source" : String | GlobalDataFlow.cs:462:22:462:65 | call to method Join : String |
+| GlobalDataFlow.cs:471:20:471:49 | call to method Run [Result] : String | GlobalDataFlow.cs:472:25:472:28 | access to local variable task [Result] : String |
+| GlobalDataFlow.cs:471:35:471:48 | "taint source" : String | GlobalDataFlow.cs:471:20:471:49 | call to method Run [Result] : String |
+| GlobalDataFlow.cs:472:25:472:28 | access to local variable task [Result] : String | GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String | GlobalDataFlow.cs:473:23:473:31 | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:31 | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String | GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter [m_task, Result] : String | GlobalDataFlow.cs:474:22:474:28 | access to local variable awaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:474:22:474:28 | access to local variable awaiter [m_task, Result] : String | GlobalDataFlow.cs:474:22:474:40 | call to method GetResult : String |
+| GlobalDataFlow.cs:474:22:474:40 | call to method GetResult : String | GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 |
 | Splitting.cs:3:28:3:34 | tainted : String | Splitting.cs:8:24:8:30 | [b (line 3): false] access to parameter tainted : String |
 | Splitting.cs:3:28:3:34 | tainted : String | Splitting.cs:8:24:8:30 | [b (line 3): true] access to parameter tainted : String |
 | Splitting.cs:8:17:8:31 | [b (line 3): false] call to method Return : String | Splitting.cs:9:15:9:15 | [b (line 3): false] access to local variable x |
@@ -470,6 +478,15 @@ nodes
 | GlobalDataFlow.cs:462:22:462:65 | call to method Join : String | semmle.label | call to method Join : String |
 | GlobalDataFlow.cs:462:51:462:64 | "taint source" : String | semmle.label | "taint source" : String |
 | GlobalDataFlow.cs:463:15:463:20 | access to local variable sink44 | semmle.label | access to local variable sink44 |
+| GlobalDataFlow.cs:471:20:471:49 | call to method Run [Result] : String | semmle.label | call to method Run [Result] : String |
+| GlobalDataFlow.cs:471:35:471:48 | "taint source" : String | semmle.label | "taint source" : String |
+| GlobalDataFlow.cs:472:25:472:28 | access to local variable task [Result] : String | semmle.label | access to local variable task [Result] : String |
+| GlobalDataFlow.cs:472:25:472:50 | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String | semmle.label | call to method ConfigureAwait [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:31 | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String | semmle.label | access to local variable awaitable [m_configuredTaskAwaiter, m_task, Result] : String |
+| GlobalDataFlow.cs:473:23:473:44 | call to method GetAwaiter [m_task, Result] : String | semmle.label | call to method GetAwaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:474:22:474:28 | access to local variable awaiter [m_task, Result] : String | semmle.label | access to local variable awaiter [m_task, Result] : String |
+| GlobalDataFlow.cs:474:22:474:40 | call to method GetResult : String | semmle.label | call to method GetResult : String |
+| GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 | semmle.label | access to local variable sink45 |
 | Splitting.cs:3:28:3:34 | tainted : String | semmle.label | tainted : String |
 | Splitting.cs:8:17:8:31 | [b (line 3): false] call to method Return : String | semmle.label | [b (line 3): false] call to method Return : String |
 | Splitting.cs:8:17:8:31 | [b (line 3): true] call to method Return : String | semmle.label | [b (line 3): true] call to method Return : String |
@@ -557,6 +574,7 @@ nodes
 | GlobalDataFlow.cs:424:41:424:46 | access to local variable sink20 | GlobalDataFlow.cs:18:27:18:40 | "taint source" : String | GlobalDataFlow.cs:424:41:424:46 | access to local variable sink20 | access to local variable sink20 |
 | GlobalDataFlow.cs:453:15:453:20 | access to local variable sink43 | GlobalDataFlow.cs:451:35:451:48 | "taint source" : String | GlobalDataFlow.cs:453:15:453:20 | access to local variable sink43 | access to local variable sink43 |
 | GlobalDataFlow.cs:463:15:463:20 | access to local variable sink44 | GlobalDataFlow.cs:462:51:462:64 | "taint source" : String | GlobalDataFlow.cs:463:15:463:20 | access to local variable sink44 | access to local variable sink44 |
+| GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 | GlobalDataFlow.cs:471:35:471:48 | "taint source" : String | GlobalDataFlow.cs:475:15:475:20 | access to local variable sink45 | access to local variable sink45 |
 | Splitting.cs:9:15:9:15 | [b (line 3): false] access to local variable x | Splitting.cs:3:28:3:34 | tainted : String | Splitting.cs:9:15:9:15 | [b (line 3): false] access to local variable x | [b (line 3): false] access to local variable x |
 | Splitting.cs:9:15:9:15 | [b (line 3): true] access to local variable x | Splitting.cs:3:28:3:34 | tainted : String | Splitting.cs:9:15:9:15 | [b (line 3): true] access to local variable x | [b (line 3): true] access to local variable x |
 | Splitting.cs:11:19:11:19 | access to local variable x | Splitting.cs:3:28:3:34 | tainted : String | Splitting.cs:11:19:11:19 | access to local variable x | access to local variable x |

csharp/ql/test/library-tests/dataflow/library/LibraryTypeDataFlow.expected

@@ -1921,6 +1921,8 @@ callableFlowAccessPath
 | System.Resources.ResourceReader.GetEnumerator() | qualifier [[]] -> return [Current] | true |
 | System.Resources.ResourceSet.GetEnumerator() | qualifier [[]] -> return [Current] | true |
 | System.Runtime.CompilerServices.ConditionalWeakTable<,>.GetEnumerator() | qualifier [[]] -> return [Current] | true |
+| System.Runtime.CompilerServices.ConfiguredTaskAwaitable<>.ConfiguredTaskAwaiter.GetResult() | qualifier [m_task, Result] -> return [<empty>] | true |
+| System.Runtime.CompilerServices.ConfiguredTaskAwaitable<>.GetAwaiter() | qualifier [m_configuredTaskAwaiter] -> return [<empty>] | true |
 | System.Runtime.CompilerServices.ReadOnlyCollectionBuilder<>.Add(T) | argument 0 [<empty>] -> qualifier [[]] | true |
 | System.Runtime.CompilerServices.ReadOnlyCollectionBuilder<>.Add(object) | argument 0 [<empty>] -> qualifier [[]] | true |
 | System.Runtime.CompilerServices.ReadOnlyCollectionBuilder<>.CopyTo(Array, int) | qualifier [[]] -> argument 0 [[]] | true |
@@ -2080,6 +2082,7 @@ callableFlowAccessPath
 | System.Threading.Tasks.Task.WhenAll<TResult>(params Task<TResult>[]) | argument 0 [[], Result] -> return [Result, []] | true |
 | System.Threading.Tasks.Task.WhenAny<TResult>(IEnumerable<Task<TResult>>) | argument 0 [[], Result] -> return [Result, []] | true |
 | System.Threading.Tasks.Task.WhenAny<TResult>(params Task<TResult>[]) | argument 0 [[], Result] -> return [Result, []] | true |
+| System.Threading.Tasks.Task<>.ConfigureAwait(bool) | qualifier [<empty>] -> return [m_configuredTaskAwaiter, m_task] | true |
 | System.Threading.Tasks.Task<>.ContinueWith<TNewResult>(Func<Task<>, Object, TNewResult>, object) | output from argument 0 [<empty>] -> return [Result] | true |
 | System.Threading.Tasks.Task<>.ContinueWith<TNewResult>(Func<Task<>, Object, TNewResult>, object, CancellationToken) | output from argument 0 [<empty>] -> return [Result] | true |
 | System.Threading.Tasks.Task<>.ContinueWith<TNewResult>(Func<Task<>, Object, TNewResult>, object, CancellationToken, TaskContinuationOptions, TaskScheduler) | output from argument 0 [<empty>] -> return [Result] | true |

java/change-notes/2020-10-27-insecure-bean-validation.md

@@ -0,0 +1,5 @@
+lgtm,codescanning
+* A new query "Insecure Bean Validation" (`java/insecure-bean-validation`) has been added. This query 
+  finds server-side template injections caused by untrusted data flowing from a bean 
+  property into a custom error message for a constraint validator. This 
+  vulnerability can lead to arbitrary code execution.

java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.java

@@ -0,0 +1,48 @@
+import javax.validation.ConstraintValidator;
+import javax.validation.ConstraintValidatorContext;
+import org.hibernate.validator.constraintvalidation.HibernateConstraintValidatorContext;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+public class TestValidator implements ConstraintValidator<Object, String> {
+
+    public static class InterpolationHelper {
+
+        public static final char BEGIN_TERM = '{';
+        public static final char END_TERM = '}';
+        public static final char EL_DESIGNATOR = '$';
+        public static final char ESCAPE_CHARACTER = '\\';
+
+        private static final Pattern ESCAPE_MESSAGE_PARAMETER_PATTERN = Pattern.compile( "([\\" + ESCAPE_CHARACTER + BEGIN_TERM + END_TERM + EL_DESIGNATOR + "])" );
+
+        private InterpolationHelper() {
+        }
+
+        public static String escapeMessageParameter(String messageParameter) {
+            if ( messageParameter == null ) {
+                return null;
+            }
+            return ESCAPE_MESSAGE_PARAMETER_PATTERN.matcher( messageParameter ).replaceAll( Matcher.quoteReplacement( String.valueOf( ESCAPE_CHARACTER ) ) + "$1" );
+        }
+
+    }
+
+    @Override
+    public boolean isValid(String object, ConstraintValidatorContext constraintContext) {
+        String value = object + " is invalid";
+
+        // Bad: Bean properties (normally user-controlled) are passed directly to `buildConstraintViolationWithTemplate`
+        constraintContext.buildConstraintViolationWithTemplate(value).addConstraintViolation().disableDefaultConstraintViolation();
+
+        // Good: Bean properties (normally user-controlled) are escaped 
+        String escaped = InterpolationHelper.escapeMessageParameter(value);
+        constraintContext.buildConstraintViolationWithTemplate(escaped).addConstraintViolation().disableDefaultConstraintViolation();
+
+        // Good: Bean properties (normally user-controlled) are parameterized
+        HibernateConstraintValidatorContext context = constraintContext.unwrap( HibernateConstraintValidatorContext.class );
+        context.addMessageParameter( "prop", object );
+        context.buildConstraintViolationWithTemplate( "{prop} is invalid").addConstraintViolation();
+        return false;
+    }
+
+}

java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp

@@ -0,0 +1,47 @@
+<!DOCTYPE qhelp PUBLIC
+  "-//Semmle//qhelp//EN"
+  "qhelp.dtd">
+<qhelp>
+<overview>
+<p>Custom error messages for constraint validators support different types of interpolation, 
+including <a href="https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions">Java EL expressions</a>.
+Controlling part of the message template being passed to <code>ConstraintValidatorContext.buildConstraintViolationWithTemplate()</code>
+argument can lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally 
+untrusted) bean properties flow into the custom error message.</p>
+</overview>
+
+<recommendation>
+<p>There are different approaches to remediate the issue:</p>
+<ul>
+<li>Do not include validated bean properties in the custom error message.</li>
+<li>Use parameterized messages instead of string concatenation. For example:
+<pre>
+HibernateConstraintValidatorContext context = constraintValidatorContext.unwrap( HibernateConstraintValidatorContext.class );
+context.addMessageParameter( "foo", "bar" );
+context.buildConstraintViolationWithTemplate( "My violation message contains a parameter {foo}").addConstraintViolation();
+</pre></li>
+<li>Sanitize the validated bean properties to make sure that there are no EL expressions. 
+An example of valid sanitization logic can be found <a href="https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/messageinterpolation/util/InterpolationHelper.java#L17">here</a>.</li>
+<li>Disable the EL interpolation and only use <code>ParameterMessageInterpolator</code>:
+<pre>
+Validator validator = Validation.byDefaultProvider()
+   .configure()
+   .messageInterpolator( new ParameterMessageInterpolator() )
+   .buildValidatorFactory()
+   .getValidator();
+</pre></li>
+<li>Replace Hibernate Validator with Apache BVal, which in its latest version does not interpolate EL expressions by default.
+Note that this replacement may not be a simple drop-in replacement.</li>
+</ul>
+</recommendation>
+
+<example>
+<p>The following validator could result in arbitrary Java code execution:</p>
+<sample src="InsecureBeanValidation.java" />
+</example>
+
+<references>
+<li>Hibernate Reference Guide: <a href="https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code">ConstraintValidatorContext</a>.</li>
+<li>GitHub Security Lab research: <a href="https://securitylab.github.com/research/bean-validation-RCE">Bean validation</a>.</li>
+</references>
+</qhelp>

java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql

@@ -0,0 +1,42 @@
+/**
+ * @name Insecure Bean Validation
+ * @description User-controlled data may be evaluated as a Java EL expression, leading to arbitrary code execution.
+ * @kind path-problem
+ * @problem.severity error
+ * @precision high
+ * @id java/insecure-bean-validation
+ * @tags security
+ *       external/cwe/cwe-094
+ */
+
+import java
+import semmle.code.java.dataflow.TaintTracking
+import semmle.code.java.dataflow.FlowSources
+import DataFlow::PathGraph
+
+class BuildConstraintViolationWithTemplateMethod extends Method {
+  BuildConstraintViolationWithTemplateMethod() {
+    this
+        .getDeclaringType()
+        .getASupertype*()
+        .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and
+    this.hasName("buildConstraintViolationWithTemplate")
+  }
+}
+
+class BeanValidationConfig extends TaintTracking::Configuration {
+  BeanValidationConfig() { this = "BeanValidationConfig" }
+
+  override predicate isSource(DataFlow::Node source) { source instanceof RemoteFlowSource }
+
+  override predicate isSink(DataFlow::Node sink) {
+    exists(MethodAccess ma |
+      ma.getMethod() instanceof BuildConstraintViolationWithTemplateMethod and
+      sink.asExpr() = ma.getArgument(0)
+    )
+  }
+}
+
+from BeanValidationConfig cfg, DataFlow::PathNode source, DataFlow::PathNode sink
+where cfg.hasFlowPath(source, sink)
+select sink, source, sink, "Custom constraint error message contains unsanitized user data"

java/ql/src/semmle/code/java/dataflow/FlowSources.qll

@@ -183,6 +183,23 @@ private class WebSocketMessageParameterSource extends RemoteFlowSource {
   override string getSourceType() { result = "Websocket onText parameter" }
 }
 
+private class BeanValidationSource extends RemoteFlowSource {
+  BeanValidationSource() {
+    exists(Method m, Parameter v |
+      this.asParameter() = v and
+      m.getParameter(0) = v and
+      m
+          .getDeclaringType()
+          .getASourceSupertype+()
+          .hasQualifiedName("javax.validation", "ConstraintValidator") and
+      m.hasName("isValid") and
+      m.fromSource()
+    )
+  }
+
+  override string getSourceType() { result = "BeanValidation source" }
+}
+
 /** Class for `tainted` user input. */
 abstract class UserInput extends DataFlow::Node { }
 

java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.expected

@@ -0,0 +1,7 @@
+edges
+| InsecureBeanValidation.java:7:28:7:40 | object : String | InsecureBeanValidation.java:11:64:11:68 | value |
+nodes
+| InsecureBeanValidation.java:7:28:7:40 | object : String | semmle.label | object : String |
+| InsecureBeanValidation.java:11:64:11:68 | value | semmle.label | value |
+#select
+| InsecureBeanValidation.java:11:64:11:68 | value | InsecureBeanValidation.java:7:28:7:40 | object : String | InsecureBeanValidation.java:11:64:11:68 | value | Custom constraint error message contains unsanitized user data |

java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.java

@@ -0,0 +1,18 @@
+import javax.validation.ConstraintValidator;
+import javax.validation.ConstraintValidatorContext;
+
+public class InsecureBeanValidation implements ConstraintValidator<Override, String> {
+
+    @Override
+    public boolean isValid(String object, ConstraintValidatorContext constraintContext) {
+        String value = object + " is invalid";
+
+        // Bad: Bean properties (normally user-controlled) are passed directly to `buildConstraintViolationWithTemplate`
+        constraintContext.buildConstraintViolationWithTemplate(value).addConstraintViolation().disableDefaultConstraintViolation();
+
+        // Good: Using message parameters
+        constraintContext.buildConstraintViolationWithTemplate("literal {message_parameter}").addConstraintViolation().disableDefaultConstraintViolation();
+        
+        return true;
+    }
+}

java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.qlref

@@ -0,0 +1 @@
+Security/CWE/CWE-094/InsecureBeanValidation.ql

java/ql/test/query-tests/security/CWE-094/options

@@ -0,0 +1 @@
+//semmle-extractor-options: --javac-args -cp ${testdir}/../../../stubs/validation-api-2.0.1.Final

java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ClockProvider.java

@@ -0,0 +1,8 @@
+package javax.validation;
+
+import java.time.Clock;
+
+public interface ClockProvider {
+
+	Clock getClock();
+}

java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.java

@@ -0,0 +1,8 @@
+package javax.validation;
+
+import java.lang.annotation.Annotation;
+
+public interface ConstraintValidator<A extends Annotation, T> {
+	default void initialize(A constraintAnnotation) {}
+	boolean isValid(T value, ConstraintValidatorContext context);
+}

java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.java

@@ -0,0 +1,212 @@
+package javax.validation;
+
+import java.time.Clock;
+
+public interface ConstraintValidatorContext {
+
+	
+	void disableDefaultConstraintViolation();
+
+	
+	String getDefaultConstraintMessageTemplate();
+
+	
+	ClockProvider getClockProvider();
+
+	
+	ConstraintViolationBuilder buildConstraintViolationWithTemplate(String messageTemplate);
+
+	
+	<T> T unwrap(Class<T> type);
+
+	
+	interface ConstraintViolationBuilder {
+
+		
+		NodeBuilderDefinedContext addNode(String name);
+
+		
+		NodeBuilderCustomizableContext addPropertyNode(String name);
+
+		
+		LeafNodeBuilderCustomizableContext addBeanNode();
+
+		
+		ContainerElementNodeBuilderCustomizableContext addContainerElementNode(String name,
+				Class<?> containerType, Integer typeArgumentIndex);
+
+		
+		NodeBuilderDefinedContext addParameterNode(int index);
+
+		
+		ConstraintValidatorContext addConstraintViolation();
+
+		
+		interface LeafNodeBuilderDefinedContext {
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface LeafNodeBuilderCustomizableContext {
+
+			
+			LeafNodeContextBuilder inIterable();
+
+			
+			LeafNodeBuilderCustomizableContext inContainer(Class<?> containerClass,
+														   Integer typeArgumentIndex);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface LeafNodeContextBuilder {
+
+			
+			LeafNodeBuilderDefinedContext atKey(Object key);
+
+			
+			LeafNodeBuilderDefinedContext atIndex(Integer index);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface NodeBuilderDefinedContext {
+
+			
+			NodeBuilderCustomizableContext addNode(String name);
+
+			
+			NodeBuilderCustomizableContext addPropertyNode(String name);
+
+			
+			LeafNodeBuilderCustomizableContext addBeanNode();
+
+			
+			ContainerElementNodeBuilderCustomizableContext addContainerElementNode(
+					String name, Class<?> containerType, Integer typeArgumentIndex);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface NodeBuilderCustomizableContext {
+
+			
+			NodeContextBuilder inIterable();
+
+			
+			NodeBuilderCustomizableContext inContainer(Class<?> containerClass,
+													   Integer typeArgumentIndex);
+
+			
+			NodeBuilderCustomizableContext addNode(String name);
+
+			
+			NodeBuilderCustomizableContext addPropertyNode(String name);
+
+			
+			LeafNodeBuilderCustomizableContext addBeanNode();
+
+			
+			ContainerElementNodeBuilderCustomizableContext addContainerElementNode(
+					String name, Class<?> containerType, Integer typeArgumentIndex);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface NodeContextBuilder {
+
+			
+			NodeBuilderDefinedContext atKey(Object key);
+
+			
+			NodeBuilderDefinedContext atIndex(Integer index);
+
+			
+			NodeBuilderCustomizableContext addNode(String name);
+
+			
+			NodeBuilderCustomizableContext addPropertyNode(String name);
+
+			
+			LeafNodeBuilderCustomizableContext addBeanNode();
+
+			
+			ContainerElementNodeBuilderCustomizableContext addContainerElementNode(
+					String name, Class<?> containerType, Integer typeArgumentIndex);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface ContainerElementNodeBuilderDefinedContext {
+
+			
+			NodeBuilderCustomizableContext addPropertyNode(String name);
+
+			
+			LeafNodeBuilderCustomizableContext addBeanNode();
+
+			
+			ContainerElementNodeBuilderCustomizableContext addContainerElementNode(
+					String name, Class<?> containerType, Integer typeArgumentIndex);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface ContainerElementNodeBuilderCustomizableContext {
+
+			
+			ContainerElementNodeContextBuilder inIterable();
+
+			
+			NodeBuilderCustomizableContext addPropertyNode(String name);
+
+			
+			LeafNodeBuilderCustomizableContext addBeanNode();
+
+			
+			ContainerElementNodeBuilderCustomizableContext addContainerElementNode(
+					String name, Class<?> containerType, Integer typeArgumentIndex);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+
+		
+		interface ContainerElementNodeContextBuilder {
+
+			
+			ContainerElementNodeBuilderDefinedContext atKey(Object key);
+
+			
+			ContainerElementNodeBuilderDefinedContext atIndex(Integer index);
+
+			
+			NodeBuilderCustomizableContext addPropertyNode(String name);
+
+			
+			LeafNodeBuilderCustomizableContext addBeanNode();
+
+			
+			ContainerElementNodeBuilderCustomizableContext addContainerElementNode(
+					String name, Class<?> containerType, Integer typeArgumentIndex);
+
+			
+			ConstraintValidatorContext addConstraintViolation();
+		}
+	}
+}
+

javascript/ql/src/semmle/javascript/frameworks/NoSQL.qll

@@ -737,3 +737,108 @@ private module MarsDB {
     override DataFlow::Node getACodeOperator() { result = qc.getACodeOperator() }
   }
 }
+
+/**
+ * Provides classes modeling the `Node Redis` library.
+ *
+ * Redis is an in-memory key-value store and not a database,
+ * but `Node Redis` can be exploited similarly to a NoSQL database by giving a method an array as argument instead of a string.
+ * As an example the below two invocations of `client.set` are equivalent:
+ *
+ * ```
+ * const redis = require("redis");
+ * const client = redis.createClient();
+ * client.set("key", "value");
+ * client.set(["key", "value"]);
+ * ```
+ *
+ * ioredis is a very similar library. However, ioredis does not support array arguments in the same way, and is therefore not vulnerable to the same kind of type confusion.
+ */
+private module Redis {
+  /**
+   * Gets a `Node Redis` client.
+   */
+  private API::Node client() {
+    result = API::moduleImport("redis").getMember("createClient").getReturn()
+    or
+    result = API::moduleImport("redis").getMember("RedisClient").getInstance()
+    or
+    result = client().getMember("duplicate").getReturn()
+    or
+    result = client().getMember("duplicate").getLastParameter().getParameter(1)
+  }
+
+  /**
+   * Gets a (possibly chained) reference to a batch operation object.
+   * These have the same API as a redis client, except the calls are chained, and the sequence is terminated with a `.exec` call.
+   */
+  private API::Node multi() {
+    result = client().getMember(["multi", "batch"]).getReturn()
+    or
+    result = multi().getAMember().getReturn()
+  }
+
+  /**
+   * Gets a `Node Redis` client instance. Either a client created using `createClient()`, or a batch operation object.
+   */
+  private API::Node redis() { result = [client(), multi()] }
+
+  /**
+   * Provides signatures for the query methods from Node Redis.
+   */
+  module QuerySignatures {
+    /**
+     * Holds if `method` interprets parameter `argIndex` as a key, and a later parameter determines a value/field.
+     * Thereby the method is vulnerable if parameter `argIndex` is unexpectedly an array instead of a string, as an attacker can control arguments to Redis that the attacker was not supposed to control.
+     *
+     * Only setters and similar methods are included.
+     * For getter-like methods it is not generally possible to gain access "outside" of where you are supposed to have access,
+     * it is at most possible to get a Redis call to return more results than expected (e.g. by adding more members to [`geohash`](https://redis.io/commands/geohash)).
+     */
+    bindingset[argIndex]
+    predicate argumentIsAmbiguousKey(string method, int argIndex) {
+      method =
+        ["set", "publish", "append", "bitfield", "decrby", "getset", "hincrby", "hincrbyfloat",
+            "hset", "hsetnx", "incrby", "incrbyfloat", "linsert", "lpush", "lpushx", "lset",
+            "ltrim", "rename", "renamenx", "rpushx", "setbit", "setex", "smove", "zincrby",
+            "zinterstore", "hdel", "lpush", "pfadd", "rpush", "sadd", "sdiffstore", "srem"] and
+      argIndex = 0
+      or
+      method = ["bitop", "hmset", "mset", "msetnx", "geoadd"] and argIndex >= 0
+    }
+  }
+
+  /**
+   * An expression that is interpreted as a key in a Node Redis call.
+   */
+  class RedisKeyArgument extends NoSQL::Query {
+    RedisKeyArgument() {
+      exists(string method, int argIndex |
+        QuerySignatures::argumentIsAmbiguousKey(method, argIndex)
+      |
+        this =
+          [promisify(redis().getMember(method)), redis().getMember(method)]
+              .getACall()
+              .getArgument(argIndex)
+              .asExpr()
+      )
+    }
+  }
+
+  /**
+   * Gets a promisified version of `method`.
+   */
+  private API::Node promisify(API::Node method) {
+    exists(API::Node promisify |
+      promisify = API::moduleImport(["util", "bluebird"]).getMember("promisify").getReturn() and
+      method
+          .getAnImmediateUse()
+          .flowsTo(promisify.getAnImmediateUse().(DataFlow::CallNode).getArgument(0))
+    |
+      result = promisify
+      or
+      result = promisify.getMember("bind").getReturn() and
+      result.getAnImmediateUse().(DataFlow::CallNode).getNumArgument() = 1
+    )
+  }
+}

javascript/ql/test/query-tests/Security/CWE-089/untyped/SqlInjection.expected

@@ -159,6 +159,34 @@ nodes
 | mongooseModelClient.js:12:22:12:29 | req.body |
 | mongooseModelClient.js:12:22:12:29 | req.body |
 | mongooseModelClient.js:12:22:12:32 | req.body.id |
+| redis.js:10:16:10:23 | req.body |
+| redis.js:10:16:10:23 | req.body |
+| redis.js:10:16:10:27 | req.body.key |
+| redis.js:10:16:10:27 | req.body.key |
+| redis.js:12:9:12:26 | key |
+| redis.js:12:15:12:22 | req.body |
+| redis.js:12:15:12:22 | req.body |
+| redis.js:12:15:12:26 | req.body.key |
+| redis.js:18:16:18:18 | key |
+| redis.js:18:16:18:18 | key |
+| redis.js:19:43:19:45 | key |
+| redis.js:19:43:19:45 | key |
+| redis.js:25:14:25:16 | key |
+| redis.js:25:14:25:16 | key |
+| redis.js:30:23:30:25 | key |
+| redis.js:30:23:30:25 | key |
+| redis.js:32:28:32:30 | key |
+| redis.js:32:28:32:30 | key |
+| redis.js:38:11:38:28 | key |
+| redis.js:38:17:38:24 | req.body |
+| redis.js:38:17:38:24 | req.body |
+| redis.js:38:17:38:28 | req.body.key |
+| redis.js:39:16:39:18 | key |
+| redis.js:39:16:39:18 | key |
+| redis.js:43:27:43:29 | key |
+| redis.js:43:27:43:29 | key |
+| redis.js:46:34:46:36 | key |
+| redis.js:46:34:46:36 | key |
 | socketio.js:10:25:10:30 | handle |
 | socketio.js:10:25:10:30 | handle |
 | socketio.js:11:12:11:53 | `INSERT ... andle}` |
@@ -432,6 +460,32 @@ edges
 | mongooseModelClient.js:12:22:12:29 | req.body | mongooseModelClient.js:12:22:12:32 | req.body.id |
 | mongooseModelClient.js:12:22:12:32 | req.body.id | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } |
 | mongooseModelClient.js:12:22:12:32 | req.body.id | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } |
+| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
+| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
+| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
+| redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key |
+| redis.js:12:9:12:26 | key | redis.js:18:16:18:18 | key |
+| redis.js:12:9:12:26 | key | redis.js:18:16:18:18 | key |
+| redis.js:12:9:12:26 | key | redis.js:19:43:19:45 | key |
+| redis.js:12:9:12:26 | key | redis.js:19:43:19:45 | key |
+| redis.js:12:9:12:26 | key | redis.js:25:14:25:16 | key |
+| redis.js:12:9:12:26 | key | redis.js:25:14:25:16 | key |
+| redis.js:12:9:12:26 | key | redis.js:30:23:30:25 | key |
+| redis.js:12:9:12:26 | key | redis.js:30:23:30:25 | key |
+| redis.js:12:9:12:26 | key | redis.js:32:28:32:30 | key |
+| redis.js:12:9:12:26 | key | redis.js:32:28:32:30 | key |
+| redis.js:12:15:12:22 | req.body | redis.js:12:15:12:26 | req.body.key |
+| redis.js:12:15:12:22 | req.body | redis.js:12:15:12:26 | req.body.key |
+| redis.js:12:15:12:26 | req.body.key | redis.js:12:9:12:26 | key |
+| redis.js:38:11:38:28 | key | redis.js:39:16:39:18 | key |
+| redis.js:38:11:38:28 | key | redis.js:39:16:39:18 | key |
+| redis.js:38:11:38:28 | key | redis.js:43:27:43:29 | key |
+| redis.js:38:11:38:28 | key | redis.js:43:27:43:29 | key |
+| redis.js:38:11:38:28 | key | redis.js:46:34:46:36 | key |
+| redis.js:38:11:38:28 | key | redis.js:46:34:46:36 | key |
+| redis.js:38:17:38:24 | req.body | redis.js:38:17:38:28 | req.body.key |
+| redis.js:38:17:38:24 | req.body | redis.js:38:17:38:28 | req.body.key |
+| redis.js:38:17:38:28 | req.body.key | redis.js:38:11:38:28 | key |
 | socketio.js:10:25:10:30 | handle | socketio.js:11:46:11:51 | handle |
 | socketio.js:10:25:10:30 | handle | socketio.js:11:46:11:51 | handle |
 | socketio.js:11:46:11:51 | handle | socketio.js:11:12:11:53 | `INSERT ... andle}` |
@@ -500,6 +554,15 @@ edges
 | mongooseJsonParse.js:23:19:23:23 | query | mongooseJsonParse.js:20:30:20:43 | req.query.data | mongooseJsonParse.js:23:19:23:23 | query | This query depends on $@. | mongooseJsonParse.js:20:30:20:43 | req.query.data | a user-provided value |
 | mongooseModelClient.js:11:16:11:24 | { id: v } | mongooseModelClient.js:10:22:10:29 | req.body | mongooseModelClient.js:11:16:11:24 | { id: v } | This query depends on $@. | mongooseModelClient.js:10:22:10:29 | req.body | a user-provided value |
 | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | mongooseModelClient.js:12:22:12:29 | req.body | mongooseModelClient.js:12:16:12:34 | { id: req.body.id } | This query depends on $@. | mongooseModelClient.js:12:22:12:29 | req.body | a user-provided value |
+| redis.js:10:16:10:27 | req.body.key | redis.js:10:16:10:23 | req.body | redis.js:10:16:10:27 | req.body.key | This query depends on $@. | redis.js:10:16:10:23 | req.body | a user-provided value |
+| redis.js:18:16:18:18 | key | redis.js:12:15:12:22 | req.body | redis.js:18:16:18:18 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
+| redis.js:19:43:19:45 | key | redis.js:12:15:12:22 | req.body | redis.js:19:43:19:45 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
+| redis.js:25:14:25:16 | key | redis.js:12:15:12:22 | req.body | redis.js:25:14:25:16 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
+| redis.js:30:23:30:25 | key | redis.js:12:15:12:22 | req.body | redis.js:30:23:30:25 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
+| redis.js:32:28:32:30 | key | redis.js:12:15:12:22 | req.body | redis.js:32:28:32:30 | key | This query depends on $@. | redis.js:12:15:12:22 | req.body | a user-provided value |
+| redis.js:39:16:39:18 | key | redis.js:38:17:38:24 | req.body | redis.js:39:16:39:18 | key | This query depends on $@. | redis.js:38:17:38:24 | req.body | a user-provided value |
+| redis.js:43:27:43:29 | key | redis.js:38:17:38:24 | req.body | redis.js:43:27:43:29 | key | This query depends on $@. | redis.js:38:17:38:24 | req.body | a user-provided value |
+| redis.js:46:34:46:36 | key | redis.js:38:17:38:24 | req.body | redis.js:46:34:46:36 | key | This query depends on $@. | redis.js:38:17:38:24 | req.body | a user-provided value |
 | socketio.js:11:12:11:53 | `INSERT ... andle}` | socketio.js:10:25:10:30 | handle | socketio.js:11:12:11:53 | `INSERT ... andle}` | This query depends on $@. | socketio.js:10:25:10:30 | handle | a user-provided value |
 | tst2.js:9:27:9:84 | "select ... d + "'" | tst2.js:9:66:9:78 | req.params.id | tst2.js:9:27:9:84 | "select ... d + "'" | This query depends on $@. | tst2.js:9:66:9:78 | req.params.id | a user-provided value |
 | tst3.js:9:14:9:19 | query1 | tst3.js:8:16:8:34 | req.params.category | tst3.js:9:14:9:19 | query1 | This query depends on $@. | tst3.js:8:16:8:34 | req.params.category | a user-provided value |

javascript/ql/test/query-tests/Security/CWE-089/untyped/redis.js

@@ -0,0 +1,53 @@
+
+const redis = require("redis");
+const client = redis.createClient();
+
+const Express = require('express');
+const app = Express();
+app.use(require('body-parser').json());
+
+app.post('/documents/find', (req, res) => {
+    client.set(req.body.key, "value"); // NOT OK
+
+    var key = req.body.key;
+    if (typeof key === "string") {
+        client.set(key, "value"); // OK
+        client.set(["key", "value"]);
+    }  
+
+    client.set(key, "value"); // NOT OK
+    client.hmset("key", "field", "value", key, "value2"); // NOT OK
+
+    // chain commands
+    client
+        .multi()
+        .set("constant", "value")
+        .set(key, "value") // NOT OK
+        .get(key) // OK
+        .exec(function (err, replies) { });
+
+    client.duplicate((err, newClient) => {
+        newClient.set(key, "value"); // NOT OK
+    });
+    client.duplicate().set(key, "value"); // NOT OK
+});
+
+
+import { promisify } from 'util';
+app.post('/documents/find', (req, res) => {
+    const key = req.body.key;
+    client.set(key, "value"); // NOT OK
+
+    const setAsync = promisify(client.set).bind(client);
+
+    const foo1 = setAsync(key, "value"); // NOT OK
+
+    client.setAsync = promisify(client.set);
+    const foo2 = client.setAsync(key, "value"); // NOT OK
+
+    client.unrelated = promisify(() => {});
+    const foo3 = client.unrelated(key, "value"); // OK
+
+    const unrelated = promisify(client.foobar).bind(client);
+    const foo4 = unrelated(key, "value"); // OK
+});

python/ql/src/semmle/python/regex.qll

@@ -27,7 +27,7 @@ predicate used_as_regex(Expr s, string mode) {
   (s instanceof Bytes or s instanceof Unicode) and
   /* Call to re.xxx(regex, ... [mode]) */
   exists(CallNode call, string name |
-    call.getArg(0).refersTo(_, _, s.getAFlowNode()) and
+    call.getArg(0).pointsTo(_, _, s.getAFlowNode()) and
     call.getFunction().pointsTo(Module::named("re").attr(name)) and
     not name = "escape"
   |