diff --git a/java/change-notes/2020-10-27-insecure-bean-validation.md b/java/change-notes/2020-10-27-insecure-bean-validation.md new file mode 100644 index 00000000000..f95061da1b1 --- /dev/null +++ b/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. diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.java b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.java new file mode 100644 index 00000000000..59c63e8026e --- /dev/null +++ b/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 { + + 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; + } + +} diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp new file mode 100644 index 00000000000..62f8452ef5f --- /dev/null +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -0,0 +1,47 @@ + + + +

Custom error messages for constraint validators support different types of interpolation, +including Java EL expressions. +Controlling part of the message template being passed to ConstraintValidatorContext.buildConstraintViolationWithTemplate() +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.

+ + + +

There are different approaches to remediate the issue:

+
    +
  • Do not include validated bean properties in the custom error message.
    • +
  • Use parameterized messages instead of string concatenation. For example: +
    +HibernateConstraintValidatorContext context = constraintValidatorContext.unwrap( HibernateConstraintValidatorContext.class );
+context.addMessageParameter( "foo", "bar" );
+context.buildConstraintViolationWithTemplate( "My violation message contains a parameter {foo}").addConstraintViolation();
+
    • +
  • Sanitize the validated bean properties to make sure that there are no EL expressions. +An example of valid sanitization logic can be found here.
    • +
  • Disable the EL interpolation and only use ParameterMessageInterpolator: +
    +Validator validator = Validation.byDefaultProvider()
+   .configure()
+   .messageInterpolator( new ParameterMessageInterpolator() )
+   .buildValidatorFactory()
+   .getValidator();
+
    • +
  • 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.
    • +
+ + + +

The following validator could result in arbitrary Java code execution:

+ + + + +
  • Hibernate Reference Guide: ConstraintValidatorContext.
    • +
  • GitHub Security Lab research: Bean validation.
    • +     +     diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql new file mode 100644 index 00000000000..17b80698e4a --- /dev/null +++ b/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" diff --git a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll b/java/ql/src/semmle/code/java/dataflow/FlowSources.qll index fb2a0345b8b..f92aedbc038 100644 --- a/java/ql/src/semmle/code/java/dataflow/FlowSources.qll +++ b/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 { } diff --git a/java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.expected b/java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.expected new file mode 100644 index 00000000000..91b794a0682 --- /dev/null +++ b/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 | diff --git a/java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.java b/java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.java new file mode 100644 index 00000000000..71d4145adfc --- /dev/null +++ b/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 + 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; + } +} diff --git a/java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.qlref b/java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.qlref new file mode 100644 index 00000000000..b01949bbcaf --- /dev/null +++ b/java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.qlref @@ -0,0 +1 @@ +Security/CWE/CWE-094/InsecureBeanValidation.ql diff --git a/java/ql/test/query-tests/security/CWE-094/options b/java/ql/test/query-tests/security/CWE-094/options new file mode 100644 index 00000000000..468d90aeabc --- /dev/null +++ b/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 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ClockProvider.java b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ClockProvider.java new file mode 100644 index 00000000000..d92c3220286 --- /dev/null +++ b/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(); +} diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.java b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.java new file mode 100644 index 00000000000..8d14e227a20 --- /dev/null +++ b/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 { + default void initialize(A constraintAnnotation) {} + boolean isValid(T value, ConstraintValidatorContext context); +} diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.java b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.java new file mode 100644 index 00000000000..66029b72f3b --- /dev/null +++ b/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 unwrap(Class 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(); + } + } +} +