Merge pull request #3141 from pwntester/InsecureBeanValidation

Insecure Bean Validation query

Author: aschackmull

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();
+}