disallow EL evaluation of custom message templates without explicit permission granted via configuration property
Project: http://git-wip-us.apache.org/repos/asf/bval/repo Commit: http://git-wip-us.apache.org/repos/asf/bval/commit/7d63a22f Tree: http://git-wip-us.apache.org/repos/asf/bval/tree/7d63a22f Diff: http://git-wip-us.apache.org/repos/asf/bval/diff/7d63a22f Branch: refs/heads/bv2 Commit: 7d63a22f2921e7ce3c006fb0b6f6345b4ae9b406 Parents: f76bed1 Author: Matt Benson <[email protected]> Authored: Mon Oct 15 18:24:31 2018 -0500 Committer: Matt Benson <[email protected]> Committed: Tue Oct 16 12:28:21 2018 -0500 ---------------------------------------------------------------------- .../apache/bval/jsr/ApacheFactoryContext.java | 4 + .../apache/bval/jsr/ApacheMessageContext.java | 36 ++++++ .../bval/jsr/ApacheValidatorConfiguration.java | 10 ++ .../bval/jsr/DefaultMessageInterpolator.java | 55 ++++---- .../jsr/job/ConstraintValidatorContextImpl.java | 9 +- .../jsr/DefaultMessageInterpolatorTest.java | 128 +++++++++++-------- 6 files changed, 161 insertions(+), 81 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/bval/blob/7d63a22f/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheFactoryContext.java ---------------------------------------------------------------------- diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheFactoryContext.java b/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheFactoryContext.java index 1dcc0d3..a48e379 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheFactoryContext.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheFactoryContext.java @@ -181,4 +181,8 @@ public class ApacheFactoryContext implements ValidatorContext { public ConstraintCached getConstraintsCache() { return factory.getConstraintsCache(); } + + public ApacheValidatorFactory getFactory() { + return factory; + } } http://git-wip-us.apache.org/repos/asf/bval/blob/7d63a22f/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheMessageContext.java ---------------------------------------------------------------------- diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheMessageContext.java b/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheMessageContext.java new file mode 100644 index 0000000..2733299 --- /dev/null +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheMessageContext.java @@ -0,0 +1,36 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.apache.bval.jsr; + +import javax.validation.MessageInterpolator; +import javax.validation.MessageInterpolator.Context; + +/** + * Vendor-specific {@link MessageInterpolator.Context} interface extension to + * provide access to validator configuration properties. + */ +public interface ApacheMessageContext extends Context { + + /** + * Get the configuration property value specified by {@code propertyKey}, if available. + * @param propertyKey + * @return {@link String} or {@code null} + */ + String getConfigurationProperty(String propertyKey); +} http://git-wip-us.apache.org/repos/asf/bval/blob/7d63a22f/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheValidatorConfiguration.java ---------------------------------------------------------------------- diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheValidatorConfiguration.java b/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheValidatorConfiguration.java index ce68d30..7c82c8b 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheValidatorConfiguration.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/ApacheValidatorConfiguration.java @@ -49,5 +49,15 @@ public interface ApacheValidatorConfiguration extends Configuration<ApacheValida * Size to use for caching of constraint-related information. Default is {@code 50}. */ String CONSTRAINTS_CACHE_SIZE = "apache.bval.constraints-cache-size"; + + /** + * Specifies whether EL evaluation is permitted in non-default message + * templates. By default this feature is disabled; if you enable it you + * should ensure that no constraint validator builds violations using + * message templates containing unchecked text (e.g. the validated + * value). To do otherwise is to expose your system to potential + * injection attacks. + */ + String CUSTOM_TEMPLATE_EXPRESSION_EVALUATION = "apache.bval.custom-template-expression-evaluation"; } } http://git-wip-us.apache.org/repos/asf/bval/blob/7d63a22f/bval-jsr/src/main/java/org/apache/bval/jsr/DefaultMessageInterpolator.java ---------------------------------------------------------------------- diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/DefaultMessageInterpolator.java b/bval-jsr/src/main/java/org/apache/bval/jsr/DefaultMessageInterpolator.java index b0697b3..32cd23a 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/DefaultMessageInterpolator.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/DefaultMessageInterpolator.java @@ -21,6 +21,7 @@ import java.util.Arrays; import java.util.Locale; import java.util.Map; import java.util.MissingResourceException; +import java.util.Objects; import java.util.Optional; import java.util.ResourceBundle; import java.util.concurrent.ConcurrentHashMap; @@ -101,7 +102,7 @@ public class DefaultMessageInterpolator implements MessageInterpolator { /** {@inheritDoc} */ @Override - public String interpolate(String message, Context context) { + public String interpolate(final String message, final Context context) { // probably no need for caching, but it could be done by parameters since the map // is immutable and uniquely built per Validation definition, the comparison has to be based on == and not equals though return interpolate(message, context, defaultLocale); @@ -109,28 +110,11 @@ public class DefaultMessageInterpolator implements MessageInterpolator { /** {@inheritDoc} */ @Override - public String interpolate(String message, Context context, Locale locale) { - return interpolateMessage(message, context.getConstraintDescriptor().getAttributes(), locale, - context.getValidatedValue()); - } - - /** - * Runs the message interpolation according to algorithm specified in JSR 303. - * <br/> - * Note: - * <br/> - * Lookups in user bundles are recursive whereas lookups in default bundle are not! - * - * @param message the message to interpolate - * @param annotationParameters the parameters of the annotation for which to interpolate this message - * @param locale the <code>Locale</code> to use for the resource bundle. - * @return the interpolated message. - */ - private String interpolateMessage(String message, Map<String, Object> annotationParameters, Locale locale, - Object validatedValue) { - ResourceBundle userResourceBundle = findUserResourceBundle(locale); - ResourceBundle defaultResourceBundle = findDefaultResourceBundle(locale); + public String interpolate(final String message, final Context context, final Locale locale) { + final ResourceBundle userResourceBundle = findUserResourceBundle(locale); + final ResourceBundle defaultResourceBundle = findDefaultResourceBundle(locale); + final Map<String, Object> annotationParameters = context.getConstraintDescriptor().getAttributes(); String userBundleResolvedMessage; String resolvedMessage = message; boolean evaluatedDefaultBundleOnce = false; @@ -143,10 +127,8 @@ public class DefaultMessageInterpolator implements MessageInterpolator { if (evaluatedDefaultBundleOnce && !hasReplacementTakenPlace(userBundleResolvedMessage, resolvedMessage)) { break; } - // search the default bundle non recursive (step2) resolvedMessage = replaceVariables(userBundleResolvedMessage, defaultResourceBundle, locale, false); - evaluatedDefaultBundleOnce = true; } while (true); @@ -154,13 +136,32 @@ public class DefaultMessageInterpolator implements MessageInterpolator { resolvedMessage = replaceAnnotationAttributes(resolvedMessage, annotationParameters); // EL handling - if (evaluator != null) { - resolvedMessage = evaluator.interpolate(resolvedMessage, annotationParameters, validatedValue); + if (evaluateExpressionLanguage(message, context)) { + resolvedMessage = evaluator.interpolate(resolvedMessage, annotationParameters, context.getValidatedValue()); } - return resolveEscapeSequences(resolvedMessage); } + private boolean evaluateExpressionLanguage(String template, Context context) { + if (evaluator != null) { + if (Objects.equals(template, context.getConstraintDescriptor().getMessageTemplate())) { + return true; + } + final Optional<ApacheMessageContext> apacheMessageContext = Optional.of(context).map(ctx -> { + try { + return ctx.unwrap(ApacheMessageContext.class); + } catch (Exception e) { + return null; + } + }); + return !apacheMessageContext.isPresent() || apacheMessageContext + .map(amc -> amc.getConfigurationProperty( + ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION)) + .filter(Boolean::parseBoolean).isPresent(); + } + return false; + } + private String resolveEscapeSequences(String s) { int pos = s.indexOf('\\'); if (pos < 0) { http://git-wip-us.apache.org/repos/asf/bval/blob/7d63a22f/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java ---------------------------------------------------------------------- diff --git a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java index 156fa79..94ec318 100644 --- a/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java +++ b/bval-jsr/src/main/java/org/apache/bval/jsr/job/ConstraintValidatorContextImpl.java @@ -30,6 +30,8 @@ import javax.validation.ValidationException; import javax.validation.metadata.ConstraintDescriptor; import javax.validation.metadata.CrossParameterDescriptor; +import org.apache.bval.jsr.ApacheFactoryContext; +import org.apache.bval.jsr.ApacheMessageContext; import org.apache.bval.jsr.descriptor.ComposedD; import org.apache.bval.jsr.descriptor.ConstraintD; import org.apache.bval.jsr.descriptor.CrossParameterD; @@ -43,7 +45,7 @@ import org.apache.bval.util.Exceptions; import org.apache.bval.util.Lazy; import org.apache.bval.util.Validate; -public class ConstraintValidatorContextImpl<T> implements ConstraintValidatorContext, MessageInterpolator.Context { +public class ConstraintValidatorContextImpl<T> implements ConstraintValidatorContext, ApacheMessageContext { public class ConstraintViolationBuilderImpl implements ConstraintValidatorContext.ConstraintViolationBuilder { private final String template; private final PathImpl path; @@ -201,4 +203,9 @@ public class ConstraintValidatorContextImpl<T> implements ConstraintValidatorCon private void addError(String messageTemplate, PathImpl propertyPath) { violations.get().add(((ValidationJob) frame.getJob()).createViolation(messageTemplate, this, propertyPath)); } + + @Override + public String getConfigurationProperty(String propertyKey) { + return frame.context.getValidatorContext().getFactory().getProperties().get(propertyKey); + } } http://git-wip-us.apache.org/repos/asf/bval/blob/7d63a22f/bval-jsr/src/test/java/org/apache/bval/jsr/DefaultMessageInterpolatorTest.java ---------------------------------------------------------------------- diff --git a/bval-jsr/src/test/java/org/apache/bval/jsr/DefaultMessageInterpolatorTest.java b/bval-jsr/src/test/java/org/apache/bval/jsr/DefaultMessageInterpolatorTest.java index a7d37e1..cb7b47d 100644 --- a/bval-jsr/src/test/java/org/apache/bval/jsr/DefaultMessageInterpolatorTest.java +++ b/bval-jsr/src/test/java/org/apache/bval/jsr/DefaultMessageInterpolatorTest.java @@ -23,6 +23,8 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.junit.Assume.assumeThat; import static org.junit.Assume.assumeTrue; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.when; import java.lang.annotation.Annotation; import java.net.URL; @@ -42,6 +44,7 @@ import javax.validation.constraints.Pattern; import javax.validation.metadata.ConstraintDescriptor; import org.apache.bval.constraints.NotEmpty; +import org.apache.bval.jsr.ApacheValidatorConfiguration; import org.apache.bval.jsr.example.Author; import org.apache.bval.jsr.example.PreferredGuest; import org.junit.After; @@ -51,6 +54,7 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; import org.junit.runners.Parameterized.Parameters; +import org.mockito.Mockito; /** * MessageResolverImpl Tester. @@ -75,26 +79,6 @@ public class DefaultMessageInterpolatorTest { return d -> Objects.equals(type, d.getAnnotation().annotationType()); } - private static MessageInterpolator.Context context(Object validatedValue, Supplier<ConstraintDescriptor<?>> descriptor){ - return new MessageInterpolator.Context() { - - @Override - public <T> T unwrap(Class<T> type) { - return null; - } - - @Override - public Object getValidatedValue() { - return validatedValue; - } - - @Override - public ConstraintDescriptor<?> getConstraintDescriptor() { - return descriptor.get(); - } - }; - } - private String elImpl; private String elFactory; private DefaultMessageInterpolator interpolator; @@ -203,6 +187,23 @@ public class DefaultMessageInterpolatorTest { public void testNoELAvailable() { assumeThat(elImpl, equalTo("invalid")); assertFalse(elAvailable); + + ApacheMessageContext context = context("12345678", + () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") + .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() + .orElseThrow(() -> new AssertionError("expected constraint missing"))); + + when(context + .getConfigurationProperty(ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION)) + .thenAnswer(invocation -> Boolean.toString(true)); + + assertEquals("${regexp.charAt(4)}", interpolator.interpolate("${regexp.charAt(4)}", + context)); + } + + @Test + public void testDisallowCustomTemplateExpressionEvaluationByDefault() { + assumeTrue(elAvailable); assertEquals("${regexp.charAt(4)}", interpolator.interpolate("${regexp.charAt(4)}", context("12345678", @@ -215,24 +216,26 @@ public class DefaultMessageInterpolatorTest { public void testExpressionLanguageEvaluation() { assumeTrue(elAvailable); - assertEquals("Expected value of length 8 to match pattern", - interpolator.interpolate("Expected value of length ${validatedValue.length()} to match pattern", - context("12345678", - () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") - .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() - .orElseThrow(() -> new AssertionError("expected constraint missing"))))); + final MessageInterpolator.Context context = context("12345678", + () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("anotherValue") + .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() + .orElseThrow(() -> new AssertionError("expected constraint missing"))); + + assertEquals("Another value should match ....$", + interpolator.interpolate(context.getConstraintDescriptor().getMessageTemplate(), context)); } - + @Test public void testMixedEvaluation() { assumeTrue(elAvailable); - assertEquals("Expected value of length 8 to match pattern ....$", - interpolator.interpolate("Expected value of length ${validatedValue.length()} to match pattern {regexp}", - context("12345678", - () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") - .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() - .orElseThrow(() -> new AssertionError("expected constraint missing"))))); + final MessageInterpolator.Context context = context("12345678", + () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("mixedMessageValue") + .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() + .orElseThrow(() -> new AssertionError("expected constraint missing"))); + + assertEquals("Mixed message value of length 8 should match ....$", + interpolator.interpolate(context.getConstraintDescriptor().getMessageTemplate(), context)); } @Test @@ -245,17 +248,20 @@ public class DefaultMessageInterpolatorTest { // non-escaped $, but that would only expose us to inconsistency for composite expressions containing more // than one component EL expression + ApacheMessageContext context = context("12345678", + () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") + .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() + .orElseThrow(() -> new AssertionError("expected constraint missing"))); + + when(context + .getConfigurationProperty(ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION)) + .thenAnswer(invocation -> Boolean.toString(true)); + assertEquals("${regexp.charAt(4)}", interpolator.interpolate("\\${regexp.charAt(4)}", - context("12345678", - () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") - .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() - .orElseThrow(() -> new AssertionError("expected constraint missing"))))); + context)); assertEquals("${regexp.charAt(4)}", interpolator.interpolate("\\\\${regexp.charAt(4)}", - context("12345678", - () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") - .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() - .orElseThrow(() -> new AssertionError("expected constraint missing"))))); + context)); } @Test @@ -263,26 +269,26 @@ public class DefaultMessageInterpolatorTest { assumeTrue(elAvailable); assumeThat(elImpl, equalTo("ri")); + ApacheMessageContext context = context("12345678", + () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") + .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() + .orElseThrow(() -> new AssertionError("expected constraint missing"))); + + when(context + .getConfigurationProperty(ApacheValidatorConfiguration.Properties.CUSTOM_TEMPLATE_EXPRESSION_EVALUATION)) + .thenAnswer(invocation -> Boolean.toString(true)); + assertEquals("returns literal", "${regexp.charAt(4)}", interpolator.interpolate("\\${regexp.charAt(4)}", - context("12345678", - () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") - .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() - .orElseThrow(() -> new AssertionError("expected constraint missing"))))); + context)); assertEquals("returns literal \\ followed by $, later interpreted as an escape sequence", "$", interpolator.interpolate("\\\\${regexp.charAt(4)}", - context("12345678", - () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") - .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() - .orElseThrow(() -> new AssertionError("expected constraint missing"))))); + context)); assertEquals("returns literal \\ followed by .", "\\.", interpolator.interpolate("\\\\${regexp.charAt(3)}", - context("12345678", - () -> validator.getConstraintsForClass(Person.class).getConstraintsForProperty("idNumber") - .getConstraintDescriptors().stream().filter(forConstraintType(Pattern.class)).findFirst() - .orElseThrow(() -> new AssertionError("expected constraint missing"))))); + context)); } @Test @@ -309,6 +315,16 @@ public class DefaultMessageInterpolatorTest { .orElseThrow(() -> new AssertionError("expected constraint missing"))))); } + @SuppressWarnings("unchecked") + private ApacheMessageContext context(Object validatedValue, Supplier<ConstraintDescriptor<?>> descriptor) { + final ApacheMessageContext result = Mockito.mock(ApacheMessageContext.class); + when(result.unwrap(any(Class.class))) + .thenAnswer(invocation -> invocation.getArgumentAt(0, Class.class).cast(result)); + when(result.getValidatedValue()).thenReturn(validatedValue); + when(result.getConstraintDescriptor()).thenAnswer(invocation -> descriptor.get()); + return result; + } + public static class Person { @Pattern(message = "Id number should match {regexp}", regexp = "....$") @@ -316,5 +332,11 @@ public class DefaultMessageInterpolatorTest { @Pattern(message = "Other id should match {regexp}", regexp = ".\\n") public String otherId; + + @Pattern(message = "Another value should match ${regexp.intern()}", regexp = "....$") + public String anotherValue; + + @Pattern(message = "Mixed message value of length ${validatedValue.length()} should match {regexp}", regexp = "....$") + public String mixedMessageValue; } }
