From 3b23cd5be3f7068962322942d080f5f2fc29e143 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 27 Mar 2020 10:54:28 +0100 Subject: [PATCH 01/34] Insecure Bean Validation query --- .../CWE/CWE-094/InsecureBeanValidation.java | 48 ++++++++++++ .../CWE/CWE-094/InsecureBeanValidation.qhelp | 39 ++++++++++ .../CWE/CWE-094/InsecureBeanValidation.ql | 76 +++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java new file mode 100644 index 00000000000..59c63e8026e --- /dev/null +++ b/java/ql/src/experimental/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/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp new file mode 100644 index 00000000000..f65c51d5a56 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -0,0 +1,39 @@ + + + + +

When building custom constraint violation error messages, it is important to understand that they support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). Therefore if an attacker can inject arbitrary data in the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument, he will be able to run arbitrary Java code. 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. E.g:
    • +``` java +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](https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/messageinterpolation/util/InterpolationHelper.java#L17). +- Disable the EL interpolation and only use `ParameterMessageInterpolator`:
    • +``` java +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:

    + +     + + +
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql new file mode 100644 index 00000000000..16245e5b8b6 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -0,0 +1,76 @@ +/** + * @name Insecure Bean validation + * @description User-controlled data may be evaluated as a Java EL expressions, leading to arbitrary code execution. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/unsafe-eval + * @tags security + * external/cwe/cwe-094 + */ + +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.FlowSources +import DataFlow::PathGraph + +class BeanValidationSource extends RemoteFlowSource { + BeanValidationSource() { + exists(Method m, Parameter v | + this.asParameter() = v and + m.getParameter(0) = v and + m + .getDeclaringType() + .getASupertype+() + .getSourceDeclaration() + .hasQualifiedName("javax.validation", "ConstraintValidator") and + m.hasName("isValid") and + m.fromSource() + ) + } + + override string getSourceType() { result = "BeanValidation source" } +} + +class ExceptionTaintStep extends TaintTracking::AdditionalTaintStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(Call call, TryStmt t, CatchClause c, MethodAccess gm | + call.getEnclosingStmt().getEnclosingStmt*() = t.getBlock() and + t.getACatchClause() = c and + ( + call.getCallee().getAThrownExceptionType().getASubtype*() = c.getACaughtType() or + c.getACaughtType().getASupertype*() instanceof TypeRuntimeException + ) and + c.getVariable().getAnAccess() = gm.getQualifier() and + gm.getMethod().getName().regexpMatch("get(Localized)?Message|toString") and + n1.asExpr() = call.getAnArgument() and + n2.asExpr() = gm + ) + } +} + +class BuildConstraintViolationWithTemplateMethod extends Method { + BuildConstraintViolationWithTemplateMethod() { + getDeclaringType() + .getASupertype*() + .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and + 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" From 0bf3895327c80606763acaf762e8e3b63e247d3b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 14:14:48 +0200 Subject: [PATCH 02/34] address review comments --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 5 ++++- .../CWE/CWE-094/InsecureBeanValidation.ql | 17 ----------------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index f65c51d5a56..c60dab2cb2b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,7 +4,9 @@ -

    When building custom constraint violation error messages, it is important to understand that they support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). Therefore if an attacker can inject arbitrary data in the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument, he will be able to run arbitrary Java code. Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). +Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. +Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

     @@ -35,5 +37,6 @@ Validator validator = Validation.byDefaultProvider()
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • +
  • https://securitylab.github.com/research/bean-validation-RCE
    •      diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql index 16245e5b8b6..52d9a0d72f2 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -32,23 +32,6 @@ class BeanValidationSource extends RemoteFlowSource { override string getSourceType() { result = "BeanValidation source" } } -class ExceptionTaintStep extends TaintTracking::AdditionalTaintStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(Call call, TryStmt t, CatchClause c, MethodAccess gm | - call.getEnclosingStmt().getEnclosingStmt*() = t.getBlock() and - t.getACatchClause() = c and - ( - call.getCallee().getAThrownExceptionType().getASubtype*() = c.getACaughtType() or - c.getACaughtType().getASupertype*() instanceof TypeRuntimeException - ) and - c.getVariable().getAnAccess() = gm.getQualifier() and - gm.getMethod().getName().regexpMatch("get(Localized)?Message|toString") and - n1.asExpr() = call.getAnArgument() and - n2.asExpr() = gm - ) - } -} - class BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { getDeclaringType() From c1decf4d0d82d0b7f671768868fe5b38572b9935 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 15:12:01 +0200 Subject: [PATCH 03/34] move md links to --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index c60dab2cb2b..9e1ed63ec90 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,9 +4,11 @@ -

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). -Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. -Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, +including Java EL expressions. +Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` +argument will lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally +untrusted) bean properties flow into the custom error message.

    @@ -18,7 +20,8 @@ HibernateConstraintValidatorContext context = constraintValidatorContext.unwrap( 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](https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/messageinterpolation/util/InterpolationHelper.java#L17). +
  • 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`:
    • ``` java Validator validator = Validation.byDefaultProvider() @@ -27,7 +30,8 @@ Validator validator = Validation.byDefaultProvider() .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.
    • +
  • 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.
    •  From df4164f2c05cfb169c2b1fcaee759b21bb770d01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 14:15:35 +0200 Subject: [PATCH 04/34] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- .../Security/CWE/CWE-094/InsecureBeanValidation.ql | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql index 52d9a0d72f2..7465e692d32 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -21,8 +21,7 @@ class BeanValidationSource extends RemoteFlowSource { m.getParameter(0) = v and m .getDeclaringType() - .getASupertype+() - .getSourceDeclaration() + .getASourceSupertype+() .hasQualifiedName("javax.validation", "ConstraintValidator") and m.hasName("isValid") and m.fromSource() @@ -34,10 +33,10 @@ class BeanValidationSource extends RemoteFlowSource { class BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { - getDeclaringType() + this.getDeclaringType() .getASupertype*() .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and - hasName("buildConstraintViolationWithTemplate") + this.hasName("buildConstraintViolationWithTemplate") } } From 2bab9d22e9a0d165055e354fc13495b3a4cd287e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 14:27:26 +0100 Subject: [PATCH 05/34] move query out of experimental --- .../Security/CWE/CWE-094/InsecureBeanValidation.java | 0 .../Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 0 .../Security/CWE/CWE-094/InsecureBeanValidation.ql | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename java/ql/src/{experimental => }/Security/CWE/CWE-094/InsecureBeanValidation.java (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-094/InsecureBeanValidation.qhelp (100%) rename java/ql/src/{experimental => }/Security/CWE/CWE-094/InsecureBeanValidation.ql (100%) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.java similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java rename to java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.java diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp rename to java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql similarity index 100% rename from java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql rename to java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql From a274a1516a0dcf7d941175dc42bef9909a7c229e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 14:33:50 +0100 Subject: [PATCH 06/34] move source to FlowSources.qll --- .../CWE/CWE-094/InsecureBeanValidation.ql | 17 ----------------- 1 file changed, 17 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql index 7465e692d32..e04f3827a27 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -14,23 +14,6 @@ import semmle.code.java.dataflow.TaintTracking import semmle.code.java.dataflow.FlowSources import DataFlow::PathGraph -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 BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { this.getDeclaringType() From 671ea2f6c634e01ee1cd184733179604cf19fe93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 15:35:35 +0100 Subject: [PATCH 07/34] add test and stubs --- .../CWE-094/InsecureBeanValidation.java | 18 ++ .../CWE-094/InsecureBeanValidation.qlref | 1 + .../test/query-tests/security/CWE-094/options | 1 + .../javax/validation/ClockProvider.java | 8 + .../validation/ConstraintValidator.class | Bin 0 -> 517 bytes .../javax/validation/ConstraintValidator.java | 8 + ...lementNodeBuilderCustomizableContext.class | Bin 0 -> 1841 bytes ...inerElementNodeBuilderDefinedContext.class | Bin 0 -> 1722 bytes ...r$ContainerElementNodeContextBuilder.class | Bin 0 -> 2187 bytes ...r$LeafNodeBuilderCustomizableContext.class | Bin 0 -> 1119 bytes ...uilder$LeafNodeBuilderDefinedContext.class | Bin 0 -> 515 bytes ...lationBuilder$LeafNodeContextBuilder.class | Bin 0 -> 932 bytes ...ilder$NodeBuilderCustomizableContext.class | Bin 0 -> 2132 bytes ...ionBuilder$NodeBuilderDefinedContext.class | Bin 0 -> 1708 bytes ...tViolationBuilder$NodeContextBuilder.class | Bin 0 -> 2109 bytes ...orContext$ConstraintViolationBuilder.class | Bin 0 -> 2688 bytes .../ConstraintValidatorContext.class | Bin 0 -> 740 bytes .../ConstraintValidatorContext.java | 212 ++++++++++++++++++ 18 files changed, 248 insertions(+) create mode 100644 java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.java create mode 100644 java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.qlref create mode 100644 java/ql/test/query-tests/security/CWE-094/options create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ClockProvider.java create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.java create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderCustomizableContext.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderDefinedContext.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeContextBuilder.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderCustomizableContext.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderDefinedContext.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeContextBuilder.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderCustomizableContext.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeContextBuilder.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.class create mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.java 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.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.class new file mode 100644 index 0000000000000000000000000000000000000000..09fe904d5ab75d5c0f234e10d78309b4f7c03444 GIT binary patch literal 517 zcmb7ByH3L}6g`HJKzX!murs6!8DMGgiXR)&5+KMHZ} zRFDCwV!4mwb6>~Xug?vDdt657M(8nglvY-WQoT!tn>b&Hl}JjV=ZVnTSYegXN%j+_ z{Ru-bG&5p%bEV~|ek)|fMNtxRGFEdfY+X?t#ys=vNG&JSf5!0CZvRp&#j7 zum0Hh*6B!sI=AmShTgC0&4`eJ;edV$WEl<-kUygA+9Lq_pnyas8%jVqIZ!v literal 0 HcmV?d00001 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$ConstraintViolationBuilder$ContainerElementNodeBuilderCustomizableContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderCustomizableContext.class new file mode 100644 index 0000000000000000000000000000000000000000..d0768eee8df9b5569f46fa20ea426e839cdfd5e9 GIT binary patch literal 1841 zcmd5-O-my|5PhX4W8(L&AFHmqx_fY4a4y~=9tftk?tWC|aRp%jlSSPGhBNIET zv~j<56e_19bC~EbU`v>+8N;?3syOD@!v}+mv_^?CnRs!{{0@=Y|7i}=+pB{zHlD42uJy- zA?SQAdc{;Da86uvoUxPt=5u!cgh1W#7uf)j$1f-{1%f+YB{Sb73-RfsSE literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderDefinedContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderDefinedContext.class new file mode 100644 index 0000000000000000000000000000000000000000..195a34e45ea7d6e664c6cc16f6c233b2a271b397 GIT binary patch literal 1722 zcmd5-%St0b6g^jC+Nd+Wb;gB0~2 zqr{tzU=u>eKrd#~r?|JP&bhCu*SF^v06SRm@WaCbVO#~lnT@X5y1SDoU=P0u3$@6^ z&MIx(KOKe2>B#ISItPGpYG7Z1(tTv76MrykCZ&av*l)=xiwvCbfIASg1>JYS+PhKUAN{IWcOgp?q!s;=r z%lvJG=6YcYd((-^c~RMlR+7&4Z^%tHuB~{9u-(m*m+{9g8PC)ptzI=_IFd>=*cMln z{sc?>mAY;i3S~x&Z8{2i6NBm Z3q$1!JXsA3jtY(mjtfo*lHkvBRNTH(17^1;zL&pi$#S6->1#(l9HX2$)s%cU4M?? zqYwT7f0THWjS@oG-9r3$nKR5inS18WokPC=`1%dNQ?y*HySPJ`Q}z{~x@Z#CI+2d8 zQGvGaf+$ori1bbpggzU>VpnT6olwOw$1d&>wuEkl%;sgt1J?F+NS!DzRJ!lI*+1d~+uq8ShTe$^13ydV6$FeHBa?5jSqPQ1rJ?C`v^1J6t>4gdDmW>qJaNy}veb@7m}TaQe_aXlMNZ^i_!Ddi;Bsp~2y zTMi+q=}c7}G0vMFpCPY9Ak4^b0N~6L&Y=Ma&SN3NMJ#2wj4K&l#j5bH3ws7Pgtvy9 p+5A>E-i9OiM8;X%#hlZCE3@;07X&W~UJ|@4cujCckOViJ#!uiZ1(5&% literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderCustomizableContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderCustomizableContext.class new file mode 100644 index 0000000000000000000000000000000000000000..352ee696622ea89404d618187c187016c7496149 GIT binary patch literal 1119 zcmc&!O-sW-5Pf57Vn5XSRkasEdZ-HK;;rJrLZMLfQ1Iqq+pJ|v+(5D^#eX6AYdrV^ z{88d0trBW`CE?nsQq#2E`uxe1Pw z!k#+)V3t=;UCouPFD}s}l$4IsnQKh$^e1k{p%sP3e^xKCv{@CxYa?OhctQ`IL`XB` z=&**N44Ro!Urvc-6nbi?g^2=5c>BZe`uyhqDOQE+F6a=!S*^{iw%GHdU?AJdW4R|O z(7B1YjHid7G9IGkogh@=TGP?E?~mlb*f=7zvtdAZ%m!%5dmvn_fNv29Yy1@exjZ3< r0>HM2GW$2#yM`@3Z=;gzcan7%7TX#}=W&2_s{osK8*GVf$trvU-^hNn literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderDefinedContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderDefinedContext.class new file mode 100644 index 0000000000000000000000000000000000000000..de8e9311f70af0effbaddd28e722b2ad5b3b29d2 GIT binary patch literal 515 zcmb_ZJxfDD5S)$ioiD~1EGz`ENudZ{V`pbVAP}?=?AP2zE*y7o?-KIcEc^lfDDhr2 zkS0hV&3+8Cv$O2i_s1uIdvq0!6xxJyHl`nZl_Fc8UTtukZ14nqeKVf%f|q*1&YBgf zH<-U*h#5++COPI%r46PHw)pqJ#EvwKWm>m}MHU!r$ALrH?iM_G#^eGtm>jyHm zUh1=IXM~IGqlEUzdx?F=RV5YS@*k~kZCN|^@A~;|CX=LaN*M0v5W?*aLIr`)$nONG cNjN|=R}CDlVu4o9XSr86L0OcUt8iSDKQK+FVE_OC literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeContextBuilder.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeContextBuilder.class new file mode 100644 index 0000000000000000000000000000000000000000..23ef1484446b49d130ad3d02ab9c8e736dc61507 GIT binary patch literal 932 zcmcgrJxc>Y5Pg%6TN9(kuM+}dlOiH`ja@n;fnd-=u(MdRld$2~gS$XbKz_8U8Fdxx#6)ktXvbz3rl$I45xA~gHW1DaQ{5zk?o$R zp+zv4p;jqDkR$yGMse1nol6z;v{!O>2}0U|QM@3$u z3-_e@Lo|y=+}9x!&T>r)wp}h=Cp;}-gHC5d`CAD8cLc>mvp97DpU~fo9D>?RqtzQk zxLAY6BM?gNUjgL`p^O^fu#W8%8`yD~5_Z#b51zx8tE%7-Rj&r&Zd(q?;l5Y<21_|Q AP5=M^ literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderCustomizableContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderCustomizableContext.class new file mode 100644 index 0000000000000000000000000000000000000000..0049be51a14db4b860a4a1ddbe3eb48a17e71724 GIT binary patch literal 2132 zcmc(g-%iv(6vn@Uu;cEcAR_-%P+S!;p*P;G@q!H`n-DeG#Ka3P20Ctrbf;!IC8*Cp z;+vRw=>-qKLm5xob<->hYm=fk{W_;J=lrJg_4Mr5&r<*|@Kj(;-~pkgP1`9e_X8!c zPFQJ$CURD4+7YBibDuic6x7xwg*WIO%rgp-Yw z#tWqBdMSJ}7icqkSJ`H%$QR=-N)UFQr6Ix@E zKS_t$-d55g397_Sd6|)BlR8-se6xS34xGRf!frX$3Gd5EG@{`XbV<3#f4+pcq*}!s zNO)eLZiPUY;Qu%vCJEP21z2mC=J*YcPT(ewXE2+_Z}oK!chd0!mIw1zc-~#yOV`{_ iudArAZej|Pc!;S=6@u@svl45a^)~A~>mut?rTQBtoa+|= literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext.class new file mode 100644 index 0000000000000000000000000000000000000000..1985f0af068b767a88b7cdbf8e652338891eaa7a GIT binary patch literal 1708 zcmd5-O;6iE5PbtACVWE+lxdgQzOzE_St{_KED9?j9m{a9@Yq+3WBduz#cXTtBuIS&MIx( zcO8Yw>ByWUItE+_kz_;|3ZXztuW4m+X?SwVVTY^GO+y;a+{6oFy1B{zL)8z!LN5T$f=U)SXuP1v7*@Te_(&KfBb(9u+$N-*GU9_xRWG%G0}_?d(CG$l+D=6 zxuM4>hpEp9KT;^xTc>;@=P)~vnWtBBcInN{y}A9{)h%B;4?BdjVn`9Vm_h@-90XlZ zpTTY?Bn9;v&SivyA^MgHgc12~0Hra)C@O&9L>DJ9lReL3LDqecRU`N)`9&;c_siM! g6Uu`7C}9liD3vSlI!+y=o@u4Mx56HfCKTEeTt_Fr)b zf|R3&2ZW6r+h!8GsH;Dz!;sHQ^*PV8?W{JPze00FaH-dV3W9ew`pDLurA!HPV%cS7 z1{pjgZ0)l;9+yS8URry{hZEkb!ZIzP3@h7LA$Qrh0pmx6ol!f(NgYuJ!j};}$PI8d z^^KgN^l_Suu;gED>yA+_vC^@xf586v;}`!|0xN?C7WIbUPc}(Xi`jOR*dyN4p%i0N zO8MJqK~r{8Ng}*Yp(Ghf`zC^Su98dX3>J-xMJe=9C+w9&gYcmoM;FT~fy)mu@*e7v z;*6INLQ+z!$*wNp$pmpL1j3a3umNwHa0yjFa29hJ&SN3N#UWn9_3ZryZpyheIW>iK u`TiDeXX`uJa|0E@r}CV}7G^3{c(S@Ict!B4;F937;ELd?APL^BRDS`V(cy^z literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder.class new file mode 100644 index 0000000000000000000000000000000000000000..674855fb584fdf2dd70c8dbbb6c9730910c4b46e GIT binary patch literal 2688 zcmd5;O>fgc5PefhOwvL_fs_{7LJ5$x5cAPn5QhdTMXCT135f#|+j<+ha_nlei3T0sJV$*pAh(I2IKy;*w|0j`z*Jd9%*O-w6mvi(@eV#n?JLJt(%>&qWh|788?|8;usSxS#sD|M`M226dCiObZBCS-Crue;j(cax z_C9|eQ>J%0Q6zV{DOFzXPWI1|%gOmHBQ8_2jT6um2!tvAZwC~n2@ALiFfL*#!fUu5 z;WAb@Z5%3PqgdrvjtJc#g5iSYn)EoMk-E Yc!6<_vCMdpah~xK<7LJxjFm$1Cpu}h(EtDd literal 0 HcmV?d00001 diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.class new file mode 100644 index 0000000000000000000000000000000000000000..19ec63b6ce1eb921cac76951e75af821a62a5480 GIT binary patch literal 740 zcmb7CUrPc(9GuO6X;xNN&`VSXO3)GXl;T5%5EKQ9z4Wr`cCBrDvbSe`wjTNbeW>W? zDR`ztJ?zfG&2MIQc0a$~KLDI#H-&r(+bQfY?A1K&Tf?QcE$Jz4rKc|gsk*{2WUsXr zrlz>>3!mYjD}CNp;z~U6K-mS846$OV#c<>bJIA^ezRz9J6t8{q5e)fa>88gA+)-S+ zPQx0h-7-UthHJ{}yxbdaAj2Ss6Xwtva3#B8N@s>cbA|RO_kvUp(mqdJAB*#*;~cZ< z(_Y4wo*`)~hPJT4Vp18UNCx`N@IJ$F)FdbcQL)6aTtdBKLmcMD#^gG vMDGzu#Hlu<0K#RYL(E_$#4J`rT*Er?bHrZ42CdETYYPd&4E>4GiYL?GIc(;l literal 0 HcmV?d00001 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(); + } + } +} + From 3dcd8acf975b5db31142a8db5482c3436403e99b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 15:44:38 +0100 Subject: [PATCH 08/34] add expected results --- .../security/CWE-094/InsecureBeanValidation.expected | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 java/ql/test/query-tests/security/CWE-094/InsecureBeanValidation.expected 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 | From a36970f30674621fe9a85a4ff545e2d053a31256 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 15:45:17 +0100 Subject: [PATCH 09/34] Add beanValidation remote source --- .../semmle/code/java/dataflow/FlowSources.qll | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) 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 { } From 73fc9fda77e0f1ab2cfdc9cc32a7f84047f9eeb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 27 Mar 2020 10:54:28 +0100 Subject: [PATCH 10/34] Insecure Bean Validation query --- .../CWE/CWE-094/InsecureBeanValidation.java | 48 ++++++++++++ .../CWE/CWE-094/InsecureBeanValidation.qhelp | 39 ++++++++++ .../CWE/CWE-094/InsecureBeanValidation.ql | 76 +++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java new file mode 100644 index 00000000000..59c63e8026e --- /dev/null +++ b/java/ql/src/experimental/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/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp new file mode 100644 index 00000000000..f65c51d5a56 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -0,0 +1,39 @@ + + + + +

    When building custom constraint violation error messages, it is important to understand that they support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). Therefore if an attacker can inject arbitrary data in the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument, he will be able to run arbitrary Java code. 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. E.g:
    • +``` java +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](https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/messageinterpolation/util/InterpolationHelper.java#L17). +- Disable the EL interpolation and only use `ParameterMessageInterpolator`:
    • +``` java +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:

    + +     + + +
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql new file mode 100644 index 00000000000..16245e5b8b6 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -0,0 +1,76 @@ +/** + * @name Insecure Bean validation + * @description User-controlled data may be evaluated as a Java EL expressions, leading to arbitrary code execution. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/unsafe-eval + * @tags security + * external/cwe/cwe-094 + */ + +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.FlowSources +import DataFlow::PathGraph + +class BeanValidationSource extends RemoteFlowSource { + BeanValidationSource() { + exists(Method m, Parameter v | + this.asParameter() = v and + m.getParameter(0) = v and + m + .getDeclaringType() + .getASupertype+() + .getSourceDeclaration() + .hasQualifiedName("javax.validation", "ConstraintValidator") and + m.hasName("isValid") and + m.fromSource() + ) + } + + override string getSourceType() { result = "BeanValidation source" } +} + +class ExceptionTaintStep extends TaintTracking::AdditionalTaintStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(Call call, TryStmt t, CatchClause c, MethodAccess gm | + call.getEnclosingStmt().getEnclosingStmt*() = t.getBlock() and + t.getACatchClause() = c and + ( + call.getCallee().getAThrownExceptionType().getASubtype*() = c.getACaughtType() or + c.getACaughtType().getASupertype*() instanceof TypeRuntimeException + ) and + c.getVariable().getAnAccess() = gm.getQualifier() and + gm.getMethod().getName().regexpMatch("get(Localized)?Message|toString") and + n1.asExpr() = call.getAnArgument() and + n2.asExpr() = gm + ) + } +} + +class BuildConstraintViolationWithTemplateMethod extends Method { + BuildConstraintViolationWithTemplateMethod() { + getDeclaringType() + .getASupertype*() + .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and + 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" From 27bd9044e76107b8ea8a278018982e2cc14ed015 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 14:14:48 +0200 Subject: [PATCH 11/34] address review comments --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 5 ++++- .../CWE/CWE-094/InsecureBeanValidation.ql | 17 ----------------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index f65c51d5a56..c60dab2cb2b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,7 +4,9 @@ -

    When building custom constraint violation error messages, it is important to understand that they support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). Therefore if an attacker can inject arbitrary data in the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument, he will be able to run arbitrary Java code. Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). +Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. +Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

     @@ -35,5 +37,6 @@ Validator validator = Validation.byDefaultProvider()
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • +
  • https://securitylab.github.com/research/bean-validation-RCE
    •      diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql index 16245e5b8b6..52d9a0d72f2 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -32,23 +32,6 @@ class BeanValidationSource extends RemoteFlowSource { override string getSourceType() { result = "BeanValidation source" } } -class ExceptionTaintStep extends TaintTracking::AdditionalTaintStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(Call call, TryStmt t, CatchClause c, MethodAccess gm | - call.getEnclosingStmt().getEnclosingStmt*() = t.getBlock() and - t.getACatchClause() = c and - ( - call.getCallee().getAThrownExceptionType().getASubtype*() = c.getACaughtType() or - c.getACaughtType().getASupertype*() instanceof TypeRuntimeException - ) and - c.getVariable().getAnAccess() = gm.getQualifier() and - gm.getMethod().getName().regexpMatch("get(Localized)?Message|toString") and - n1.asExpr() = call.getAnArgument() and - n2.asExpr() = gm - ) - } -} - class BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { getDeclaringType() From 6ca28a8bc690bcdd4d3ba015f7998447bfa902b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 15:12:01 +0200 Subject: [PATCH 12/34] move md links to     --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index c60dab2cb2b..9e1ed63ec90 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,9 +4,11 @@ -

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). -Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. -Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, +including Java EL expressions. +Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` +argument will lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally +untrusted) bean properties flow into the custom error message.

    @@ -18,7 +20,8 @@ HibernateConstraintValidatorContext context = constraintValidatorContext.unwrap( 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](https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/messageinterpolation/util/InterpolationHelper.java#L17). +
  • 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`:
    • ``` java Validator validator = Validation.byDefaultProvider() @@ -27,7 +30,8 @@ Validator validator = Validation.byDefaultProvider() .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.
    • +
  • 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.
    •  From f85778e9c70e83b7b535b3e67decb46c3454fd00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 14:15:35 +0200 Subject: [PATCH 13/34] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- .../Security/CWE/CWE-094/InsecureBeanValidation.ql | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql index 52d9a0d72f2..7465e692d32 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -21,8 +21,7 @@ class BeanValidationSource extends RemoteFlowSource { m.getParameter(0) = v and m .getDeclaringType() - .getASupertype+() - .getSourceDeclaration() + .getASourceSupertype+() .hasQualifiedName("javax.validation", "ConstraintValidator") and m.hasName("isValid") and m.fromSource() @@ -34,10 +33,10 @@ class BeanValidationSource extends RemoteFlowSource { class BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { - getDeclaringType() + this.getDeclaringType() .getASupertype*() .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and - hasName("buildConstraintViolationWithTemplate") + this.hasName("buildConstraintViolationWithTemplate") } } From 65d01f5c9ea77ce63505b2e12faa9af2c5fb12db Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 14:14:48 +0200 Subject: [PATCH 14/34] address review comments --- .../Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index 9e1ed63ec90..e2714b8432f 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,11 +4,9 @@ -

    Bean validation custom constraint error messages support different types of interpolation, -including Java EL expressions. -Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` -argument will lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally -untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). +Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. +Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

     From d990f7a47013430d31e7e8ff3c0368b46449af96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 15:12:01 +0200 Subject: [PATCH 15/34] move md links to --- .../Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index e2714b8432f..9e1ed63ec90 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,9 +4,11 @@ -

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). -Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. -Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, +including Java EL expressions. +Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` +argument will lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally +untrusted) bean properties flow into the custom error message.

    From 7d7933a054b14f45184df7a64c22218c90c8b793 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 14:27:26 +0100 Subject: [PATCH 16/34] move query out of experimental --- .../CWE/CWE-094/InsecureBeanValidation.java | 48 --------------- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 46 --------------- .../CWE/CWE-094/InsecureBeanValidation.ql | 58 ------------------- 3 files changed, 152 deletions(-) delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java deleted file mode 100644 index 59c63e8026e..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java +++ /dev/null @@ -1,48 +0,0 @@ -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/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp deleted file mode 100644 index 9e1ed63ec90..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ /dev/null @@ -1,46 +0,0 @@ - - - - -

    Bean validation custom constraint error messages support different types of interpolation, -including Java EL expressions. -Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` -argument will 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. E.g:
    • -``` java -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`:
    • -``` java -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:

    - -     - - -
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • -
  • https://securitylab.github.com/research/bean-validation-RCE
    • -     -     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql deleted file mode 100644 index 7465e692d32..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ /dev/null @@ -1,58 +0,0 @@ -/** - * @name Insecure Bean validation - * @description User-controlled data may be evaluated as a Java EL expressions, leading to arbitrary code execution. - * @kind path-problem - * @problem.severity error - * @precision high - * @id java/unsafe-eval - * @tags security - * external/cwe/cwe-094 - */ - -import java -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.dataflow.FlowSources -import DataFlow::PathGraph - -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 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" From debfc686d172f10e85bd139a1fb38ab95bc1d635 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Fri, 27 Mar 2020 10:54:28 +0100 Subject: [PATCH 17/34] Insecure Bean Validation query --- .../CWE/CWE-094/InsecureBeanValidation.java | 48 ++++++++++++ .../CWE/CWE-094/InsecureBeanValidation.qhelp | 39 ++++++++++ .../CWE/CWE-094/InsecureBeanValidation.ql | 76 +++++++++++++++++++ 3 files changed, 163 insertions(+) create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp create mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java new file mode 100644 index 00000000000..59c63e8026e --- /dev/null +++ b/java/ql/src/experimental/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/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp new file mode 100644 index 00000000000..f65c51d5a56 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -0,0 +1,39 @@ + + + + +

    When building custom constraint violation error messages, it is important to understand that they support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). Therefore if an attacker can inject arbitrary data in the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument, he will be able to run arbitrary Java code. 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. E.g:
    • +``` java +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](https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/messageinterpolation/util/InterpolationHelper.java#L17). +- Disable the EL interpolation and only use `ParameterMessageInterpolator`:
    • +``` java +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:

    + +     + + +
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • +     +     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql new file mode 100644 index 00000000000..16245e5b8b6 --- /dev/null +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -0,0 +1,76 @@ +/** + * @name Insecure Bean validation + * @description User-controlled data may be evaluated as a Java EL expressions, leading to arbitrary code execution. + * @kind path-problem + * @problem.severity error + * @precision high + * @id java/unsafe-eval + * @tags security + * external/cwe/cwe-094 + */ + +import java +import semmle.code.java.dataflow.TaintTracking +import semmle.code.java.dataflow.FlowSources +import DataFlow::PathGraph + +class BeanValidationSource extends RemoteFlowSource { + BeanValidationSource() { + exists(Method m, Parameter v | + this.asParameter() = v and + m.getParameter(0) = v and + m + .getDeclaringType() + .getASupertype+() + .getSourceDeclaration() + .hasQualifiedName("javax.validation", "ConstraintValidator") and + m.hasName("isValid") and + m.fromSource() + ) + } + + override string getSourceType() { result = "BeanValidation source" } +} + +class ExceptionTaintStep extends TaintTracking::AdditionalTaintStep { + override predicate step(DataFlow::Node n1, DataFlow::Node n2) { + exists(Call call, TryStmt t, CatchClause c, MethodAccess gm | + call.getEnclosingStmt().getEnclosingStmt*() = t.getBlock() and + t.getACatchClause() = c and + ( + call.getCallee().getAThrownExceptionType().getASubtype*() = c.getACaughtType() or + c.getACaughtType().getASupertype*() instanceof TypeRuntimeException + ) and + c.getVariable().getAnAccess() = gm.getQualifier() and + gm.getMethod().getName().regexpMatch("get(Localized)?Message|toString") and + n1.asExpr() = call.getAnArgument() and + n2.asExpr() = gm + ) + } +} + +class BuildConstraintViolationWithTemplateMethod extends Method { + BuildConstraintViolationWithTemplateMethod() { + getDeclaringType() + .getASupertype*() + .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and + 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" From 8904411fe6cdfb003e4b0d9c1f47486a0119bceb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 14:14:48 +0200 Subject: [PATCH 18/34] address review comments --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 5 ++++- .../CWE/CWE-094/InsecureBeanValidation.ql | 17 ----------------- 2 files changed, 4 insertions(+), 18 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index f65c51d5a56..c60dab2cb2b 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,7 +4,9 @@ -

    When building custom constraint violation error messages, it is important to understand that they support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). Therefore if an attacker can inject arbitrary data in the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument, he will be able to run arbitrary Java code. Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). +Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. +Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

     @@ -35,5 +37,6 @@ Validator validator = Validation.byDefaultProvider()
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • +
  • https://securitylab.github.com/research/bean-validation-RCE
    •      diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql index 16245e5b8b6..52d9a0d72f2 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -32,23 +32,6 @@ class BeanValidationSource extends RemoteFlowSource { override string getSourceType() { result = "BeanValidation source" } } -class ExceptionTaintStep extends TaintTracking::AdditionalTaintStep { - override predicate step(DataFlow::Node n1, DataFlow::Node n2) { - exists(Call call, TryStmt t, CatchClause c, MethodAccess gm | - call.getEnclosingStmt().getEnclosingStmt*() = t.getBlock() and - t.getACatchClause() = c and - ( - call.getCallee().getAThrownExceptionType().getASubtype*() = c.getACaughtType() or - c.getACaughtType().getASupertype*() instanceof TypeRuntimeException - ) and - c.getVariable().getAnAccess() = gm.getQualifier() and - gm.getMethod().getName().regexpMatch("get(Localized)?Message|toString") and - n1.asExpr() = call.getAnArgument() and - n2.asExpr() = gm - ) - } -} - class BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { getDeclaringType() From 8b5aed2fe16ab2c567ce2ae17fb37695683a098b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 15:12:01 +0200 Subject: [PATCH 19/34] move md links to --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index c60dab2cb2b..9e1ed63ec90 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -4,9 +4,11 @@ -

    Bean validation custom constraint error messages support different types of interpolation, including [Java EL expressions](https://docs.jboss.org/hibernate/validator/5.1/reference/en-US/html/chapter-message-interpolation.html#section-interpolation-with-message-expressions). -Controlling part of the error message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` argument will lead to arbitrary Java code execution. -Unfortunately, it is common that validated (and therefore, normally untrusted) bean properties flow into the custom error message.

    +

    Bean validation custom constraint error messages support different types of interpolation, +including Java EL expressions. +Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` +argument will lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally +untrusted) bean properties flow into the custom error message.

    @@ -18,7 +20,8 @@ HibernateConstraintValidatorContext context = constraintValidatorContext.unwrap( 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](https://github.com/hibernate/hibernate-validator/blob/master/engine/src/main/java/org/hibernate/validator/internal/engine/messageinterpolation/util/InterpolationHelper.java#L17). +
  • 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`:
    • ``` java Validator validator = Validation.byDefaultProvider() @@ -27,7 +30,8 @@ Validator validator = Validation.byDefaultProvider() .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.
    • +
  • 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.
    •  From 40a200749701b9c2f236ce4edcbe50091f0bdb82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Thu, 22 Oct 2020 14:15:35 +0200 Subject: [PATCH 20/34] Apply suggestions from code review Co-authored-by: Anders Schack-Mulligen --- .../Security/CWE/CWE-094/InsecureBeanValidation.ql | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql index 52d9a0d72f2..7465e692d32 100644 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -21,8 +21,7 @@ class BeanValidationSource extends RemoteFlowSource { m.getParameter(0) = v and m .getDeclaringType() - .getASupertype+() - .getSourceDeclaration() + .getASourceSupertype+() .hasQualifiedName("javax.validation", "ConstraintValidator") and m.hasName("isValid") and m.fromSource() @@ -34,10 +33,10 @@ class BeanValidationSource extends RemoteFlowSource { class BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { - getDeclaringType() + this.getDeclaringType() .getASupertype*() .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and - hasName("buildConstraintViolationWithTemplate") + this.hasName("buildConstraintViolationWithTemplate") } } From 99044fc6ab9d80f18181a91d272f2d2ab16540e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 15:55:19 +0100 Subject: [PATCH 21/34] remove experimental query forr bean validation --- .../CWE/CWE-094/InsecureBeanValidation.java | 48 --------------- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 46 --------------- .../CWE/CWE-094/InsecureBeanValidation.ql | 58 ------------------- 3 files changed, 152 deletions(-) delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp delete mode 100644 java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java deleted file mode 100644 index 59c63e8026e..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.java +++ /dev/null @@ -1,48 +0,0 @@ -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/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp deleted file mode 100644 index 9e1ed63ec90..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ /dev/null @@ -1,46 +0,0 @@ - - - - -

    Bean validation custom constraint error messages support different types of interpolation, -including Java EL expressions. -Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` -argument will 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. E.g:
    • -``` java -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`:
    • -``` java -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:

    - -     - - -
  • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
    • -
  • https://securitylab.github.com/research/bean-validation-RCE
    • -     -     diff --git a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql deleted file mode 100644 index 7465e692d32..00000000000 --- a/java/ql/src/experimental/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ /dev/null @@ -1,58 +0,0 @@ -/** - * @name Insecure Bean validation - * @description User-controlled data may be evaluated as a Java EL expressions, leading to arbitrary code execution. - * @kind path-problem - * @problem.severity error - * @precision high - * @id java/unsafe-eval - * @tags security - * external/cwe/cwe-094 - */ - -import java -import semmle.code.java.dataflow.TaintTracking -import semmle.code.java.dataflow.FlowSources -import DataFlow::PathGraph - -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 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" From 3378dd526eef7be9129e7fee3b2321de2499150b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 15:56:26 +0100 Subject: [PATCH 22/34] remove compiled classes from stubs --- .../javax/validation/ConstraintValidator.class | Bin 517 -> 0 bytes ...erElementNodeBuilderCustomizableContext.class | Bin 1841 -> 0 bytes ...ntainerElementNodeBuilderDefinedContext.class | Bin 1722 -> 0 bytes ...lder$ContainerElementNodeContextBuilder.class | Bin 2187 -> 0 bytes ...lder$LeafNodeBuilderCustomizableContext.class | Bin 1119 -> 0 bytes ...onBuilder$LeafNodeBuilderDefinedContext.class | Bin 515 -> 0 bytes ...ViolationBuilder$LeafNodeContextBuilder.class | Bin 932 -> 0 bytes ...nBuilder$NodeBuilderCustomizableContext.class | Bin 2132 -> 0 bytes ...lationBuilder$NodeBuilderDefinedContext.class | Bin 1708 -> 0 bytes ...aintViolationBuilder$NodeContextBuilder.class | Bin 2109 -> 0 bytes ...datorContext$ConstraintViolationBuilder.class | Bin 2688 -> 0 bytes .../validation/ConstraintValidatorContext.class | Bin 740 -> 0 bytes 12 files changed, 0 insertions(+), 0 deletions(-) delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderCustomizableContext.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderDefinedContext.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeContextBuilder.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderCustomizableContext.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderDefinedContext.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeContextBuilder.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderCustomizableContext.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeContextBuilder.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder.class delete mode 100644 java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.class diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidator.class deleted file mode 100644 index 09fe904d5ab75d5c0f234e10d78309b4f7c03444..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 517 zcmb7ByH3L}6g`HJKzX!murs6!8DMGgiXR)&5+KMHZ} zRFDCwV!4mwb6>~Xug?vDdt657M(8nglvY-WQoT!tn>b&Hl}JjV=ZVnTSYegXN%j+_ z{Ru-bG&5p%bEV~|ek)|fMNtxRGFEdfY+X?t#ys=vNG&JSf5!0CZvRp&#j7 zum0Hh*6B!sI=AmShTgC0&4`eJ;edV$WEl<-kUygA+9Lq_pnyas8%jVqIZ!v diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderCustomizableContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderCustomizableContext.class deleted file mode 100644 index d0768eee8df9b5569f46fa20ea426e839cdfd5e9..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 1841 zcmd5-O-my|5PhX4W8(L&AFHmqx_fY4a4y~=9tftk?tWC|aRp%jlSSPGhBNIET zv~j<56e_19bC~EbU`v>+8N;?3syOD@!v}+mv_^?CnRs!{{0@=Y|7i}=+pB{zHlD42uJy- zA?SQAdc{;Da86uvoUxPt=5u!cgh1W#7uf)j$1f-{1%f+YB{Sb73-RfsSE diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderDefinedContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$ContainerElementNodeBuilderDefinedContext.class deleted file mode 100644 index 195a34e45ea7d6e664c6cc16f6c233b2a271b397..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 1722 zcmd5-%St0b6g^jC+Nd+Wb;gB0~2 zqr{tzU=u>eKrd#~r?|JP&bhCu*SF^v06SRm@WaCbVO#~lnT@X5y1SDoU=P0u3$@6^ z&MIx(KOKe2>B#ISItPGpYG7Z1(tTv76MrykCZ&av*l)=xiwvCbfIASg1>JYS+PhKUAN{IWcOgp?q!s;=r z%lvJG=6YcYd((-^c~RMlR+7&4Z^%tHuB~{9u-(m*m+{9g8PC)ptzI=_IFd>=*cMln z{sc?>mAY;i3S~x&Z8{2i6NBm Z3q$1!JXsA3jtY(mjtfo*lHkvBRNTH(17^1;zL&pi$#S6->1#(l9HX2$)s%cU4M?? zqYwT7f0THWjS@oG-9r3$nKR5inS18WokPC=`1%dNQ?y*HySPJ`Q}z{~x@Z#CI+2d8 zQGvGaf+$ori1bbpggzU>VpnT6olwOw$1d&>wuEkl%;sgt1J?F+NS!DzRJ!lI*+1d~+uq8ShTe$^13ydV6$FeHBa?5jSqPQ1rJ?C`v^1J6t>4gdDmW>qJaNy}veb@7m}TaQe_aXlMNZ^i_!Ddi;Bsp~2y zTMi+q=}c7}G0vMFpCPY9Ak4^b0N~6L&Y=Ma&SN3NMJ#2wj4K&l#j5bH3ws7Pgtvy9 p+5A>E-i9OiM8;X%#hlZCE3@;07X&W~UJ|@4cujCckOViJ#!uiZ1(5&% diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderCustomizableContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderCustomizableContext.class deleted file mode 100644 index 352ee696622ea89404d618187c187016c7496149..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 1119 zcmc&!O-sW-5Pf57Vn5XSRkasEdZ-HK;;rJrLZMLfQ1Iqq+pJ|v+(5D^#eX6AYdrV^ z{88d0trBW`CE?nsQq#2E`uxe1Pw z!k#+)V3t=;UCouPFD}s}l$4IsnQKh$^e1k{p%sP3e^xKCv{@CxYa?OhctQ`IL`XB` z=&**N44Ro!Urvc-6nbi?g^2=5c>BZe`uyhqDOQE+F6a=!S*^{iw%GHdU?AJdW4R|O z(7B1YjHid7G9IGkogh@=TGP?E?~mlb*f=7zvtdAZ%m!%5dmvn_fNv29Yy1@exjZ3< r0>HM2GW$2#yM`@3Z=;gzcan7%7TX#}=W&2_s{osK8*GVf$trvU-^hNn diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderDefinedContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeBuilderDefinedContext.class deleted file mode 100644 index de8e9311f70af0effbaddd28e722b2ad5b3b29d2..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 515 zcmb_ZJxfDD5S)$ioiD~1EGz`ENudZ{V`pbVAP}?=?AP2zE*y7o?-KIcEc^lfDDhr2 zkS0hV&3+8Cv$O2i_s1uIdvq0!6xxJyHl`nZl_Fc8UTtukZ14nqeKVf%f|q*1&YBgf zH<-U*h#5++COPI%r46PHw)pqJ#EvwKWm>m}MHU!r$ALrH?iM_G#^eGtm>jyHm zUh1=IXM~IGqlEUzdx?F=RV5YS@*k~kZCN|^@A~;|CX=LaN*M0v5W?*aLIr`)$nONG cNjN|=R}CDlVu4o9XSr86L0OcUt8iSDKQK+FVE_OC diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeContextBuilder.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$LeafNodeContextBuilder.class deleted file mode 100644 index 23ef1484446b49d130ad3d02ab9c8e736dc61507..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 932 zcmcgrJxc>Y5Pg%6TN9(kuM+}dlOiH`ja@n;fnd-=u(MdRld$2~gS$XbKz_8U8Fdxx#6)ktXvbz3rl$I45xA~gHW1DaQ{5zk?o$R zp+zv4p;jqDkR$yGMse1nol6z;v{!O>2}0U|QM@3$u z3-_e@Lo|y=+}9x!&T>r)wp}h=Cp;}-gHC5d`CAD8cLc>mvp97DpU~fo9D>?RqtzQk zxLAY6BM?gNUjgL`p^O^fu#W8%8`yD~5_Z#b51zx8tE%7-Rj&r&Zd(q?;l5Y<21_|Q AP5=M^ diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderCustomizableContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderCustomizableContext.class deleted file mode 100644 index 0049be51a14db4b860a4a1ddbe3eb48a17e71724..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 2132 zcmc(g-%iv(6vn@Uu;cEcAR_-%P+S!;p*P;G@q!H`n-DeG#Ka3P20Ctrbf;!IC8*Cp z;+vRw=>-qKLm5xob<->hYm=fk{W_;J=lrJg_4Mr5&r<*|@Kj(;-~pkgP1`9e_X8!c zPFQJ$CURD4+7YBibDuic6x7xwg*WIO%rgp-Yw z#tWqBdMSJ}7icqkSJ`H%$QR=-N)UFQr6Ix@E zKS_t$-d55g397_Sd6|)BlR8-se6xS34xGRf!frX$3Gd5EG@{`XbV<3#f4+pcq*}!s zNO)eLZiPUY;Qu%vCJEP21z2mC=J*YcPT(ewXE2+_Z}oK!chd0!mIw1zc-~#yOV`{_ iudArAZej|Pc!;S=6@u@svl45a^)~A~>mut?rTQBtoa+|= diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder$NodeBuilderDefinedContext.class deleted file mode 100644 index 1985f0af068b767a88b7cdbf8e652338891eaa7a..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 1708 zcmd5-O;6iE5PbtACVWE+lxdgQzOzE_St{_KED9?j9m{a9@Yq+3WBduz#cXTtBuIS&MIx( zcO8Yw>ByWUItE+_kz_;|3ZXztuW4m+X?SwVVTY^GO+y;a+{6oFy1B{zL)8z!LN5T$f=U)SXuP1v7*@Te_(&KfBb(9u+$N-*GU9_xRWG%G0}_?d(CG$l+D=6 zxuM4>hpEp9KT;^xTc>;@=P)~vnWtBBcInN{y}A9{)h%B;4?BdjVn`9Vm_h@-90XlZ zpTTY?Bn9;v&SivyA^MgHgc12~0Hra)C@O&9L>DJ9lReL3LDqecRU`N)`9&;c_siM! g6Uu`7C}9liD3vSlI!+y=o@u4Mx56HfCKTEeTt_Fr)b zf|R3&2ZW6r+h!8GsH;Dz!;sHQ^*PV8?W{JPze00FaH-dV3W9ew`pDLurA!HPV%cS7 z1{pjgZ0)l;9+yS8URry{hZEkb!ZIzP3@h7LA$Qrh0pmx6ol!f(NgYuJ!j};}$PI8d z^^KgN^l_Suu;gED>yA+_vC^@xf586v;}`!|0xN?C7WIbUPc}(Xi`jOR*dyN4p%i0N zO8MJqK~r{8Ng}*Yp(Ghf`zC^Su98dX3>J-xMJe=9C+w9&gYcmoM;FT~fy)mu@*e7v z;*6INLQ+z!$*wNp$pmpL1j3a3umNwHa0yjFa29hJ&SN3N#UWn9_3ZryZpyheIW>iK u`TiDeXX`uJa|0E@r}CV}7G^3{c(S@Ict!B4;F937;ELd?APL^BRDS`V(cy^z diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext$ConstraintViolationBuilder.class deleted file mode 100644 index 674855fb584fdf2dd70c8dbbb6c9730910c4b46e..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 2688 zcmd5;O>fgc5PefhOwvL_fs_{7LJ5$x5cAPn5QhdTMXCT135f#|+j<+ha_nlei3T0sJV$*pAh(I2IKy;*w|0j`z*Jd9%*O-w6mvi(@eV#n?JLJt(%>&qWh|788?|8;usSxS#sD|M`M226dCiObZBCS-Crue;j(cax z_C9|eQ>J%0Q6zV{DOFzXPWI1|%gOmHBQ8_2jT6um2!tvAZwC~n2@ALiFfL*#!fUu5 z;WAb@Z5%3PqgdrvjtJc#g5iSYn)EoMk-E Yc!6<_vCMdpah~xK<7LJxjFm$1Cpu}h(EtDd diff --git a/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.class b/java/ql/test/stubs/validation-api-2.0.1.Final/javax/validation/ConstraintValidatorContext.class deleted file mode 100644 index 19ec63b6ce1eb921cac76951e75af821a62a5480..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 740 zcmb7CUrPc(9GuO6X;xNN&`VSXO3)GXl;T5%5EKQ9z4Wr`cCBrDvbSe`wjTNbeW>W? zDR`ztJ?zfG&2MIQc0a$~KLDI#H-&r(+bQfY?A1K&Tf?QcE$Jz4rKc|gsk*{2WUsXr zrlz>>3!mYjD}CNp;z~U6K-mS846$OV#c<>bJIA^ezRz9J6t8{q5e)fa>88gA+)-S+ zPQx0h-7-UthHJ{}yxbdaAj2Ss6Xwtva3#B8N@s>cbA|RO_kvUp(mqdJAB*#*;~cZ< z(_Y4wo*`)~hPJT4Vp18UNCx`N@IJ$F)FdbcQL)6aTtdBKLmcMD#^gG vMDGzu#Hlu<0K#RYL(E_$#4J`rT*Er?bHrZ42CdETYYPd&4E>4GiYL?GIc(;l From 11e57bd2f854fa6d5c0be119b715a6c892c222f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 16:11:51 +0100 Subject: [PATCH 23/34] add change note for new Insecure Bean Validation query --- java/change-notes/2020-10-27-insecure-bean-validation.md | 6 ++++++ java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql | 4 ++-- 2 files changed, 8 insertions(+), 2 deletions(-) create mode 100644 java/change-notes/2020-10-27-insecure-bean-validation.md 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..a52990b7a18 --- /dev/null +++ b/java/change-notes/2020-10-27-insecure-bean-validation.md @@ -0,0 +1,6 @@ +lgtm,codescanning +* New query "Insecure Bean Validation" (java/insecure-bean-validation) added. This query + finds Server-Side Template Injections caused by untrusted data flowing from a Bean + property being validated into a custom constraint violation error message. This + vulnerability leads to arbitrary code execution. + diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql index e04f3827a27..fdc4d985dd9 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -1,10 +1,10 @@ /** - * @name Insecure Bean validation + * @name Insecure Bean Validation * @description User-controlled data may be evaluated as a Java EL expressions, leading to arbitrary code execution. * @kind path-problem * @problem.severity error * @precision high - * @id java/unsafe-eval + * @id java/insecure-bean-validation * @tags security * external/cwe/cwe-094 */ From 8974f252acd595df5fa46009b57c2a729504cbc3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 16:19:39 +0100 Subject: [PATCH 24/34] fix format and qlhelp errors blocking the merge --- .../ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 5 +++-- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql | 3 ++- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index 9e1ed63ec90..8ef851f08ad 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -2,17 +2,17 @@ "-//Semmle//qhelp//EN" "qhelp.dtd"> -

    Bean validation custom constraint error messages support different types of interpolation, including Java EL expressions. -Controlling part of the message template being passed to `ConstraintValidatorContext.buildConstraintViolationWithTemplate()` +Controlling part of the message template being passed to ConstraintValidatorContext.buildConstraintViolationWithTemplate() argument will 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. E.g:
      • ``` java @@ -32,6 +32,7 @@ Validator validator = Validation.byDefaultProvider() ```
    • 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.
      • +
        diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql index fdc4d985dd9..a9330ea7267 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -16,7 +16,8 @@ import DataFlow::PathGraph class BuildConstraintViolationWithTemplateMethod extends Method { BuildConstraintViolationWithTemplateMethod() { - this.getDeclaringType() + this + .getDeclaringType() .getASupertype*() .hasQualifiedName("javax.validation", "ConstraintValidatorContext") and this.hasName("buildConstraintViolationWithTemplate") From aa981caea5a873c4138331847f143c2dbcc16a63 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 16:32:13 +0100 Subject: [PATCH 25/34] more fixes to make qlhelp linter happy --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index 8ef851f08ad..cae6e77d980 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -12,17 +12,16 @@ 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. E.g:
          • +- Do not include validated bean properties in the custom error message. +- Use parameterized messages instead of string concatenation. E.g: ``` java 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`:
          • +- 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`: ``` java Validator validator = Validation.byDefaultProvider() .configure() @@ -30,9 +29,8 @@ Validator validator = Validation.byDefaultProvider() .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.
          • -
            +- 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. From 1fdf0556d2f913eb202023a7304cedf722e10f9f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mun=CC=83oz?= Date: Tue, 27 Oct 2020 17:05:00 +0100 Subject: [PATCH 26/34] more fixes to make qlhelp linter happy --- .../CWE/CWE-094/InsecureBeanValidation.qhelp | 24 ++++++++++--------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index cae6e77d980..ec9f7193281 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -12,25 +12,27 @@ 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. E.g: -``` java +
              +
            • Do not include validated bean properties in the custom error message.
              • +
            • Use parameterized messages instead of string concatenation. E.g: +
               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`:
-``` java
+
              • +
            • 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.
+
              • +
            • 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.
              • +
             From a9ea63b9767027896f098743aacdef06e36c3913 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 27 Oct 2020 21:10:46 +0100 Subject: [PATCH 27/34] Update java/change-notes/2020-10-27-insecure-bean-validation.md Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- java/change-notes/2020-10-27-insecure-bean-validation.md | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/java/change-notes/2020-10-27-insecure-bean-validation.md b/java/change-notes/2020-10-27-insecure-bean-validation.md index a52990b7a18..f95061da1b1 100644 --- a/java/change-notes/2020-10-27-insecure-bean-validation.md +++ b/java/change-notes/2020-10-27-insecure-bean-validation.md @@ -1,6 +1,5 @@ lgtm,codescanning -* New query "Insecure Bean Validation" (java/insecure-bean-validation) added. This query - finds Server-Side Template Injections caused by untrusted data flowing from a Bean - property being validated into a custom constraint violation error message. This - vulnerability leads to arbitrary code execution. - +* 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. From d221930c818aa8d6492a48f1f2f716b943159da9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 27 Oct 2020 21:10:56 +0100 Subject: [PATCH 28/34] Update java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index ec9f7193281..c9e64da593a 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -3,7 +3,7 @@ "qhelp.dtd"> -

            Bean validation custom constraint error messages support different types of interpolation, +

            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 will lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally From 9785013c29d071f160250bafd078e5725bc6188c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 27 Oct 2020 21:11:15 +0100 Subject: [PATCH 29/34] Update java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index c9e64da593a..6915a6dd7a5 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -6,7 +6,7 @@

            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 will lead to arbitrary Java code execution. Unfortunately, it is common that validated (and therefore, normally +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.

             From d5b470ea0c5ad226fafd3a6f3a3148ebe13417e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 27 Oct 2020 21:11:27 +0100 Subject: [PATCH 30/34] Update java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index 6915a6dd7a5..aa63af22e4f 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -14,7 +14,7 @@ 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. E.g: +
            • Use parameterized messages instead of string concatenation. For example: 
               HibernateConstraintValidatorContext context = constraintValidatorContext.unwrap( HibernateConstraintValidatorContext.class );
 context.addMessageParameter( "foo", "bar" );

From ac116da0dca54ab4d9e3fd7adf7e1e101928c069 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= 
Date: Tue, 27 Oct 2020 21:11:48 +0100
Subject: [PATCH 31/34] Update
 java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp

Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com>
---
 java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp
index aa63af22e4f..451a4f7de3f 100644
--- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp
+++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp
@@ -30,7 +30,7 @@ Validator validator = Validation.byDefaultProvider()
    .buildValidatorFactory()
    .getValidator();
              • -
            • Replace Hibernate-Validator with Apache BVal which in its latest version does not interpolate EL expressions by default. +
            • 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.
            From b9c75ea4624171f0b3b741267898918cb49ecb36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 27 Oct 2020 21:12:00 +0100 Subject: [PATCH 32/34] Update java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index 451a4f7de3f..09918c19bb5 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -36,7 +36,7 @@ Note that this replacement may not be a simple drop-in replacement. -

            The following Validator could result in arbitrary Java code execution:

            +

            The following validator could result in arbitrary Java code execution:

             From 77b551b69346d5577c5be524ff4b21d7612881e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alvaro=20Mu=C3=B1oz?= Date: Tue, 27 Oct 2020 21:12:17 +0100 Subject: [PATCH 33/34] Update java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index 09918c19bb5..566da6e7406 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -41,7 +41,7 @@ Note that this replacement may not be a simple drop-in replacement.             -
          • https://docs.jboss.org/hibernate/stable/validator/reference/en-US/html_single/#_the_code_constraintvalidatorcontext_code
            • -
          • https://securitylab.github.com/research/bean-validation-RCE
            • +
          • Hibernate Reference Guide:ConstraintValidatorContext.
            • +
          • GitHub Security Lab research: Bean validation.
            •  From 34ae6e0576e6e5222fc8286d8f4a69619786ca85 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Wed, 28 Oct 2020 09:15:08 +0100 Subject: [PATCH 34/34] Apply suggestions from code review Co-authored-by: Shati Patel <42641846+shati-patel@users.noreply.github.com> --- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp | 4 ++-- java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp index 566da6e7406..62f8452ef5f 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.qhelp @@ -30,7 +30,7 @@ Validator validator = Validation.byDefaultProvider() .buildValidatorFactory() .getValidator(); -
          • Replace Hibernate Validator with Apache BVal which in its latest version does not interpolate EL expressions by default. +
          • 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.
          @@ -41,7 +41,7 @@ Note that this replacement may not be a simple drop-in replacement. -
        • Hibernate Reference Guide:ConstraintValidatorContext.
          • +
        • 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 index a9330ea7267..17b80698e4a 100644 --- a/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql +++ b/java/ql/src/Security/CWE/CWE-094/InsecureBeanValidation.ql @@ -1,6 +1,6 @@ /** * @name Insecure Bean Validation - * @description User-controlled data may be evaluated as a Java EL expressions, leading to arbitrary code execution. + * @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