Better HTML encoding in WhitelabelApprovalEndpoint

[artem-smotrakov]

(after discussing this on security@pivotal.io, it was decided not to fix it)

WhitelabelApprovalEndpoint class in spring-security-oauth doesn't apply HTML escaping for the context path, see the createTemplate() method:

String requestPath = ServletUriComponentsBuilder.fromContextPath(request).build().getPath();
if (requestPath == null) {
    requestPath = "";
}

builder.append(requestPath).append("/oauth/authorize\" method=\"post\">");
builder.append("<input name=\"user_oauth_approval\" value=\"true\" type=\"hidden\"/>");

Hypothetically, it may result to an XSS. However, it looks pretty unrealistic. An attacker should be quite powerful, or there must be other vulnerabilities, to allow injection an arbitrary HTML with the context path here. One of the ways to inject an arbitrary data here is to use the X-Forwarded-Prefix header which sounds pretty hard for the adversary to control. If I understand correctly, ServletUriComponentsBuilder.fromContextPath() takes this header into account.

Nevertheless, I am wondering if the WhitelabelApprovalEndpoint class should encode the context path before inserting it to the approval page. The implementation of ServletUriComponentsBuilder.fromContextPath() method may change, and as a result, open new attack vectors. Or, there may be other attack vectors I don't know about.

[artem-smotrakov]

The patch below contains a possible solution and tests. Although, in general URL encoding is not recommended to prevent XSS, I think in this context it should be okay. Otherwise, HtmlUtils.htmlEscape() method can be used.

I can open a pull request if this patch looks okay.

diff --git a/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpoint.java b/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpoint.java
index 836b66b1..999acc41 100644
--- a/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpoint.java
+++ b/spring-security-oauth2/src/main/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpoint.java
@@ -1,5 +1,6 @@
 package org.springframework.security.oauth2.provider.endpoint;

+import java.io.UnsupportedEncodingException;
 import org.springframework.security.oauth2.provider.AuthorizationRequest;
 import org.springframework.security.web.csrf.CsrfToken;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -12,6 +13,7 @@ import org.springframework.web.util.HtmlUtils;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 import java.util.Map;
+import org.springframework.web.util.UriUtils;

 /**
  * Controller for displaying the approval page for the authorization server.
@@ -57,6 +59,11 @@ public class WhitelabelApprovalEndpoint {
        if (requestPath == null) {
            requestPath = "";
        }
+       try {
+           requestPath = UriUtils.encodePath(requestPath, "UTF-8");
+       } catch (UnsupportedEncodingException e) {
+           throw new IllegalArgumentException(e);
+       }

        builder.append(requestPath).append("/oauth/authorize\" method=\"post\">");
        builder.append("<input name=\"user_oauth_approval\" value=\"true\" type=\"hidden\"/>");
diff --git a/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpointTests.java b/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpointTests.java
index f4c408dc..efd9156c 100644
--- a/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpointTests.java
+++ b/spring-security-oauth2/src/test/java/org/springframework/security/oauth2/provider/endpoint/WhitelabelApprovalEndpointTests.java
@@ -26,11 +26,12 @@ import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;

+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;

 /**
  * @author Dave Syer
- *
+ * @author Artem Smotrakov
  */
 public class WhitelabelApprovalEndpointTests {

@@ -154,4 +155,66 @@ public class WhitelabelApprovalEndpointTests {
        String content = response.getContentAsString();
        assertTrue("Wrong content: " + content, content.equals(expectedContent));
    }
+
+   @Test
+   public void testApprovalPageWithInvalidContextPaths() throws Exception {
+       String[] values = {
+               "<img src=x onerror=alert(1)>",
+               "‘; alert(1);",
+               "'; alert(1);",
+               "\"; alert(1);"
+       };
+       for (String value : values) {
+           request.setContextPath("/foo" + value);
+           parameters.put("client_id", "client");
+           HashMap<String, Object> model = new HashMap<String, Object>();
+           model.put("authorizationRequest", createFromParameters(parameters));
+           ModelAndView result = endpoint.getAccessConfirmation(model, request);
+           result.getView().render(result.getModel(), request, response);
+           String content = response.getContentAsString();
+           assertFalse(content.contains(value));
+       }
+   }
+
+   @Test
+   public void testApprovalPageWithValidContextPaths() throws Exception {
+       Map<String, String> values = new HashMap<String, String>();
+       values.put("/foo/bar", "/foo/bar");
+       values.put("/foo-bar", "/foo-bar");
+       values.put("/foo.bar", "/foo.bar");
+       values.put("/foo/bar;", "/foo/bar;");
+       values.put("/foo/bar>", "/foo/bar%3E");
+       for (Map.Entry<String, String> entry : values.entrySet()) {
+           String actualPath = entry.getKey();
+           String expectedPath = entry.getValue();
+           request.setContextPath(actualPath);
+           parameters.put("client_id", "client");
+           HashMap<String, Object> model = new HashMap<String, Object>();
+           model.put("authorizationRequest", createFromParameters(parameters));
+           ModelAndView result = endpoint.getAccessConfirmation(model, request);
+           result.getView().render(result.getModel(), request, response);
+           String content = response.getContentAsString();
+           assertTrue(content.contains(expectedPath));
+       }
+   }
+
+   @Test
+   public void testApprovalPageWithInvalidXForwardedPathHeader() throws Exception {
+       String[] values = {
+               "<img src=x onerror=alert(1)>",
+               "‘; alert(1);",
+               "'; alert(1);",
+               "\"; alert(1);"
+       };
+       for (String value : values) {
+           request.addHeader("X-Forwarded-Path", "/foo" + value);
+           parameters.put("client_id", "client");
+           HashMap<String, Object> model = new HashMap<String, Object>();
+           model.put("authorizationRequest", createFromParameters(parameters));
+           ModelAndView result = endpoint.getAccessConfirmation(model, request);
+           result.getView().render(result.getModel(), request, response);
+           String content = response.getContentAsString();
+           assertFalse(content.contains(value));
+       }
+   }
 }