From edb16a13ee33e62b046730a47843cb5dc92054e6 Mon Sep 17 00:00:00 2001 From: Phillip Webb Date: Mon, 14 Dec 2015 14:25:01 +0000 Subject: [PATCH] Protect against SpEL injections Prevent potential SpEL injection attacks by ensuring that whitelabel error view SpEL placeholders are not recursively resolved. Fixes gh-4763 --- .../web/ErrorMvcAutoConfiguration.java | 77 +++++++++++++------ ...NonRecursivePropertyPlaceholderHelper.java | 61 +++++++++++++++ .../web/DefaultErrorViewIntegrationTests.java | 22 +++++- ...cursivePropertyPlaceholderHelperTests.java | 53 +++++++++++++ 4 files changed, 187 insertions(+), 26 deletions(-) create mode 100644 spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelper.java create mode 100644 spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelperTests.java diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ErrorMvcAutoConfiguration.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ErrorMvcAutoConfiguration.java index 062a55a8259..f9cea1ac5f8 100644 --- a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ErrorMvcAutoConfiguration.java +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/ErrorMvcAutoConfiguration.java @@ -16,6 +16,7 @@ package org.springframework.boot.autoconfigure.web; +import java.util.Collections; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -51,10 +52,10 @@ import org.springframework.context.expression.MapAccessor; import org.springframework.core.Ordered; import org.springframework.core.io.support.SpringFactoriesLoader; import org.springframework.core.type.AnnotatedTypeMetadata; +import org.springframework.expression.EvaluationContext; import org.springframework.expression.Expression; import org.springframework.expression.spel.standard.SpelExpressionParser; import org.springframework.expression.spel.support.StandardEvaluationContext; -import org.springframework.util.PropertyPlaceholderHelper; import org.springframework.util.PropertyPlaceholderHelper.PlaceholderResolver; import org.springframework.web.servlet.DispatcherServlet; import org.springframework.web.servlet.View; @@ -172,19 +173,18 @@ public class ErrorMvcAutoConfiguration */ private static class SpelView implements View { + private final NonRecursivePropertyPlaceholderHelper helper; + private final String template; - private final StandardEvaluationContext context = new StandardEvaluationContext(); - - private PropertyPlaceholderHelper helper; - - private PlaceholderResolver resolver; + private final Map expressions; public SpelView(String template) { + this.helper = new NonRecursivePropertyPlaceholderHelper("${", "}"); this.template = template; - this.context.addPropertyAccessor(new MapAccessor()); - this.helper = new PropertyPlaceholderHelper("${", "}"); - this.resolver = new SpelPlaceholderResolver(this.context); + ExpressionCollector expressionCollector = new ExpressionCollector(); + this.helper.replacePlaceholders(this.template, expressionCollector); + this.expressions = expressionCollector.getExpressions(); } @Override @@ -200,36 +200,63 @@ public class ErrorMvcAutoConfiguration } Map map = new HashMap(model); map.put("path", request.getContextPath()); - this.context.setRootObject(map); - String result = this.helper.replacePlaceholders(this.template, this.resolver); + PlaceholderResolver resolver = new ExpressionResolver(this.expressions, map); + String result = this.helper.replacePlaceholders(this.template, resolver); response.getWriter().append(result); } } + /** + * {@link PlaceholderResolver} to collect placeholder expressions. + */ + private static class ExpressionCollector implements PlaceholderResolver { + + private final SpelExpressionParser parser = new SpelExpressionParser(); + + private final Map expressions = new HashMap(); + + @Override + public String resolvePlaceholder(String name) { + this.expressions.put(name, this.parser.parseExpression(name)); + return null; + } + + public Map getExpressions() { + return Collections.unmodifiableMap(this.expressions); + } + + } + /** * SpEL based {@link PlaceholderResolver}. */ - private static class SpelPlaceholderResolver implements PlaceholderResolver { + private static class ExpressionResolver implements PlaceholderResolver { - private final SpelExpressionParser parser = new SpelExpressionParser(); + private final Map expressions; - private final StandardEvaluationContext context; + private final EvaluationContext context; - public SpelPlaceholderResolver(StandardEvaluationContext context) { - this.context = context; + ExpressionResolver(Map expressions, Map map) { + this.expressions = expressions; + this.context = getContext(map); + } + + private EvaluationContext getContext(Map map) { + StandardEvaluationContext context = new StandardEvaluationContext(); + context.addPropertyAccessor(new MapAccessor()); + context.setRootObject(map); + return context; } @Override - public String resolvePlaceholder(String name) { - Expression expression = this.parser.parseExpression(name); - try { - Object value = expression.getValue(this.context); - return HtmlUtils.htmlEscape(value == null ? null : value.toString()); - } - catch (Exception ex) { - return null; - } + public String resolvePlaceholder(String placeholderName) { + Expression expression = this.expressions.get(placeholderName); + return escape(expression == null ? null : expression.getValue(this.context)); + } + + private String escape(Object value) { + return HtmlUtils.htmlEscape(value == null ? null : value.toString()); } } diff --git a/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelper.java b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelper.java new file mode 100644 index 00000000000..2919e22e7b9 --- /dev/null +++ b/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelper.java @@ -0,0 +1,61 @@ +/* + * Copyright 2012-2015 the original author or authors. + * + * Licensed 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.springframework.boot.autoconfigure.web; + +import java.util.Set; + +import org.springframework.util.PropertyPlaceholderHelper; + +/** + * {@link PropertyPlaceholderHelper} that doesn't allow recursive resolution. + * + * @author Phillip Webb + */ +class NonRecursivePropertyPlaceholderHelper extends PropertyPlaceholderHelper { + + NonRecursivePropertyPlaceholderHelper(String placeholderPrefix, + String placeholderSuffix) { + super(placeholderPrefix, placeholderSuffix); + } + + @Override + protected String parseStringValue(String strVal, + PlaceholderResolver placeholderResolver, Set visitedPlaceholders) { + return super.parseStringValue(strVal, + new NonRecursivePlaceholderResolver(placeholderResolver), + visitedPlaceholders); + } + + private static class NonRecursivePlaceholderResolver implements PlaceholderResolver { + + private final PlaceholderResolver resolver; + + public NonRecursivePlaceholderResolver(PlaceholderResolver resolver) { + this.resolver = resolver; + } + + @Override + public String resolvePlaceholder(String placeholderName) { + if (this.resolver instanceof NonRecursivePlaceholderResolver) { + return null; + } + return this.resolver.resolvePlaceholder(placeholderName); + } + + } + +} diff --git a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/DefaultErrorViewIntegrationTests.java b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/DefaultErrorViewIntegrationTests.java index 980978f882a..cdf3df6ff03 100644 --- a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/DefaultErrorViewIntegrationTests.java +++ b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/DefaultErrorViewIntegrationTests.java @@ -42,6 +42,7 @@ import org.springframework.test.web.servlet.MvcResult; import org.springframework.test.web.servlet.setup.MockMvcBuilders; import org.springframework.web.context.WebApplicationContext; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; @@ -76,7 +77,7 @@ public class DefaultErrorViewIntegrationTests { } @Test - public void testErrorWithEscape() throws Exception { + public void testErrorWithHtmlEscape() throws Exception { MvcResult response = this.mockMvc .perform(get("/error") .requestAttr("javax.servlet.error.exception", @@ -90,6 +91,21 @@ public class DefaultErrorViewIntegrationTests { assertTrue("Wrong content: " + content, content.contains("999")); } + @Test + public void testErrorWithSpelEscape() throws Exception { + String spel = "${T(" + getClass().getName() + ").injectCall()}"; + MvcResult response = this.mockMvc + .perform( + get("/error") + .requestAttr("javax.servlet.error.exception", + new RuntimeException(spel)) + .accept(MediaType.TEXT_HTML)) + .andExpect(status().is5xxServerError()).andReturn(); + String content = response.getResponse().getContentAsString(); + System.out.println(content); + assertFalse("Wrong content: " + content, content.contains("injection")); + } + @Target(ElementType.TYPE) @Retention(RetentionPolicy.RUNTIME) @Documented @@ -112,4 +128,8 @@ public class DefaultErrorViewIntegrationTests { } + public static String injectCall() { + return "injection"; + } + } diff --git a/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelperTests.java b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelperTests.java new file mode 100644 index 00000000000..dfcf27393e7 --- /dev/null +++ b/spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/NonRecursivePropertyPlaceholderHelperTests.java @@ -0,0 +1,53 @@ +/* + * Copyright 2012-2015 the original author or authors. + * + * Licensed 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.springframework.boot.autoconfigure.web; + +import java.util.Properties; + +import org.junit.Test; + +import static org.hamcrest.Matchers.equalTo; +import static org.junit.Assert.assertThat; + +/** + * Tests for {@link NonRecursivePropertyPlaceholderHelper}. + * + * @author Phillip Webb + */ +public class NonRecursivePropertyPlaceholderHelperTests { + + private final NonRecursivePropertyPlaceholderHelper helper = new NonRecursivePropertyPlaceholderHelper( + "${", "}"); + + @Test + public void canResolve() { + Properties properties = new Properties(); + properties.put("a", "b"); + String result = this.helper.replacePlaceholders("${a}", properties); + assertThat(result, equalTo("b")); + } + + @Test + public void cannotResolveRecursive() { + Properties properties = new Properties(); + properties.put("a", "${b}"); + properties.put("b", "c"); + String result = this.helper.replacePlaceholders("${a}", properties); + assertThat(result, equalTo("${b}")); + } + +}