Protect against SpEL injections

Prevent potential SpEL injection attacks by ensuring that whitelabel
error view SpEL placeholders are not recursively resolved.

Fixes gh-4763

Author: philwebb

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.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 ConditionOutcome getMatchOutcome(ConditionContext context,
 	 */
 	private static class SpelView implements View {
 
-		private final String template;
-
-		private final StandardEvaluationContext context = new StandardEvaluationContext();
+		private final NonRecursivePropertyPlaceholderHelper helper;
 
-		private PropertyPlaceholderHelper helper;
+		private final String template;
 
-		private PlaceholderResolver resolver;
+		private final Map<String, Expression> 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 void render(Map<String, ?> model, HttpServletRequest request,
 			}
 			Map<String, Object> map = new HashMap<String, Object>(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);
 		}
 
 	}
 
 	/**
-	 * SpEL based {@link PlaceholderResolver}.
+	 * {@link PlaceholderResolver} to collect placeholder expressions.
 	 */
-	private static class SpelPlaceholderResolver implements PlaceholderResolver {
+	private static class ExpressionCollector implements PlaceholderResolver {
 
 		private final SpelExpressionParser parser = new SpelExpressionParser();
 
-		private final StandardEvaluationContext context;
+		private final Map<String, Expression> expressions = new HashMap<String, Expression>();
 
-		public SpelPlaceholderResolver(StandardEvaluationContext context) {
-			this.context = context;
+		@Override
+		public String resolvePlaceholder(String name) {
+			this.expressions.put(name, this.parser.parseExpression(name));
+			return null;
+		}
+
+		public Map<String, Expression> getExpressions() {
+			return Collections.unmodifiableMap(this.expressions);
+		}
+
+	}
+
+	/**
+	 * SpEL based {@link PlaceholderResolver}.
+	 */
+	private static class ExpressionResolver implements PlaceholderResolver {
+
+		private final Map<String, Expression> expressions;
+
+		private final EvaluationContext context;
+
+		ExpressionResolver(Map<String, Expression> expressions, Map<String, ?> map) {
+			this.expressions = expressions;
+			this.context = getContext(map);
+		}
+
+		private EvaluationContext getContext(Map<String, ?> 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());
 		}
 
 	}

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

spring-boot-autoconfigure/src/test/java/org/springframework/boot/autoconfigure/web/DefaultErrorViewIntegrationTests.java

@@ -42,6 +42,7 @@
 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 void testErrorForBrowserClient() throws Exception {
 	}
 
 	@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 void testErrorWithEscape() throws Exception {
 		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 static void main(String[] args) {
 
 	}
 
+	public static String injectCall() {
+		return "injection";
+	}
+
 }

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