Merge pull request github#4629 from pwntester/improve_bean_validation_query

Java: add some improvements to the bean validation query

Author: adityasharad

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

@@ -14,6 +14,48 @@ import semmle.code.java.dataflow.TaintTracking
 import semmle.code.java.dataflow.FlowSources
 import DataFlow::PathGraph
 
+/**
+ * A message interpolator Type that perform Expression Language (EL) evaluations
+ */
+class ELMessageInterpolatorType extends RefType {
+  ELMessageInterpolatorType() {
+    this
+        .getASourceSupertype*()
+        .hasQualifiedName("org.hibernate.validator.messageinterpolation",
+          ["ResourceBundleMessageInterpolator", "ValueFormatterMessageInterpolator"])
+  }
+}
+
+/**
+ * A method call that sets the application's default message interpolator.
+ */
+class SetMessageInterpolatorCall extends MethodAccess {
+  SetMessageInterpolatorCall() {
+    exists(Method m, RefType t |
+      this.getMethod() = m and
+      m.getDeclaringType().getASourceSupertype*() = t and
+      (
+        t.hasQualifiedName("javax.validation", ["Configuration", "ValidatorContext"]) and
+        m.getName() = "messageInterpolator"
+        or
+        t
+            .hasQualifiedName("org.springframework.validation.beanvalidation",
+              ["CustomValidatorBean", "LocalValidatorFactoryBean"]) and
+        m.getName() = "setMessageInterpolator"
+      )
+    )
+  }
+
+  /**
+   * The message interpolator is likely to be safe, because it does not process Java Expression Language expressions.
+   */
+  predicate isSafe() { not this.getAnArgument().getType() instanceof ELMessageInterpolatorType }
+}
+
+/**
+ * A method named `buildConstraintViolationWithTemplate` declared on a subtype
+ * of `javax.validation.ConstraintValidatorContext`.
+ */
 class BuildConstraintViolationWithTemplateMethod extends Method {
   BuildConstraintViolationWithTemplateMethod() {
     this
@@ -24,6 +66,10 @@ class BuildConstraintViolationWithTemplateMethod extends Method {
   }
 }
 
+/**
+ * Taint tracking BeanValidationConfiguration describing the flow of data from user input
+ * to the argument of a method that builds constraint error messages.
+ */
 class BeanValidationConfig extends TaintTracking::Configuration {
   BeanValidationConfig() { this = "BeanValidationConfig" }
 
@@ -38,5 +84,12 @@ class BeanValidationConfig extends TaintTracking::Configuration {
 }
 
 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"
+where
+  (
+    not exists(SetMessageInterpolatorCall c)
+    or
+    exists(SetMessageInterpolatorCall c | not c.isSafe())
+  ) and
+  cfg.hasFlowPath(source, sink)
+select sink.getNode(), source, sink,
+  "Custom constraint error message contains unsanitized user data"