#1110: escape html string to prevent potential XSS issue (#1128)

* [github]#1110: XSS issues in azure-auth-helper use guava's htmlescaper. * [github #1110][devops #1764036]: update version of `azure-auth-helper` in depender
microsoft · Aug 25, 2020 · 09ac842 · 09ac842
1 parent b1afd5d
commit 09ac842
Show file tree

Hide file tree

Showing 8 changed files with 51 additions and 9 deletions.
diff --git a/azure-auth-helper/src/main/java/com/microsoft/azure/auth/LocalAuthServer.java b/azure-auth-helper/src/main/java/com/microsoft/azure/auth/LocalAuthServer.java
@@ -6,6 +6,8 @@
 
 package com.microsoft.azure.auth;
 
+import com.google.common.escape.Escaper;
+import com.google.common.html.HtmlEscapers;
 import com.microsoft.azure.auth.exception.AzureLoginFailureException;
 import com.microsoft.azure.auth.exception.AzureLoginTimeoutException;
 import com.nimbusds.jose.util.IOUtils;
@@ -46,7 +48,7 @@ public LocalAuthServer() {
         jettyServer = new Server();
         final ServerConnector connector = new ServerConnector(jettyServer);
         connector.setHost("localhost");
-        jettyServer.setConnectors(new Connector[]{ connector });
+        jettyServer.setConnectors(new Connector[]{connector});
         jettyServer.setHandler(new AbstractHandler() {
             @Override
             public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response)
@@ -64,7 +66,16 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques
                     response.setStatus(HttpServletResponse.SC_OK);
                     response.setContentType(Constants.CONTENT_TYPE_TEXT_HTML);
                     try (final PrintWriter writer = response.getWriter()) {
-                        writer.write(isSuccess ? loginSuccessHTMLTemplate : String.format(loginErrorHTMLTemplate, error, errorDescription));
+                        if (isSuccess) {
+                            writer.write(loginSuccessHTMLTemplate);
+                        } else {
+                            final Escaper escaper = HtmlEscapers.htmlEscaper();
+                            // only text is acceptable, escape html/xml markups to prevent potential XSS issues,
+                            // although `errorDescription` and `error` are passed from Azure's own OAuth server, which should be trustable.
+                            errorDescription = StringUtils.isNotEmpty(errorDescription) ? escaper.escape(errorDescription) : errorDescription;
+                            error = StringUtils.isNotEmpty(error) ? escaper.escape(error) : error;
+                            writer.write(String.format(loginErrorHTMLTemplate, error, errorDescription));
+                        }
                         writer.flush();
                     }
                     response.flushBuffer();

diff --git a/azure-auth-helper/src/test/java/com/microsoft/azure/auth/LocalAuthServerTest.java b/azure-auth-helper/src/test/java/com/microsoft/azure/auth/LocalAuthServerTest.java
@@ -72,6 +72,31 @@ public void testErrorResult() throws Exception {
         }
     }
 
+    @Test
+    public void testErrorResultWithHtml() throws Exception {
+        final String url = localAuthServer.getURI().toString();
+        final String queryString = "access_denied&error_description=<p>the+user+canceled+the+authentication</p><script>alert(1)</script>&error=<h1>error1</h1>";
+        final URLConnection conn = new URL(url + "?" + queryString).openConnection();
+        final Runnable runnable = () -> {
+            try {
+                localAuthServer.waitForCode();
+                fail();
+            } catch (AzureLoginFailureException ex) {
+                // expected
+            } catch (InterruptedException e) {
+                fail("Unexpected InterruptedException:" + e.getMessage());
+            }
+        };
+        final Thread t = new Thread(runnable);
+        t.start();
+        try (BufferedReader reader = new BufferedReader(new InputStreamReader(conn.getInputStream()))) {
+            final String html = reader.lines().collect(Collectors.joining("\n"));
+            System.out.println(html);
+            assertTrue(html.contains("&lt;h1&gt;error1&lt;/h1&gt;"));
+            assertTrue(html.contains("&lt;p&gt;the user canceled the authentication&lt;/p&gt;&lt;script&gt;alert(1)&lt;/script&gt;"));
+        }
+    }
+
     @Test
     public void testStart() throws IOException {
 

diff --git a/azure-maven-plugin-lib/pom.xml b/azure-maven-plugin-lib/pom.xml
@@ -34,7 +34,7 @@
         <powermock.version>2.0.2</powermock.version>
         <jaxb.version>2.3.2</jaxb.version>
         <zeroturnaround.zip.version>1.13</zeroturnaround.zip.version>
-        <azure.auth.helper.version>0.5.1</azure.auth.helper.version>
+        <azure.auth.helper.version>0.6.0-SNAPSHOT</azure.auth.helper.version>
         <azure-tools-common.version>0.6.0</azure-tools-common.version>
         <commons.collections4.version>4.4</commons.collections4.version>
     </properties>

diff --git a/azure-maven-plugin/pom.xml b/azure-maven-plugin/pom.xml
@@ -34,6 +34,8 @@
         <surefire.version>2.22.0</surefire.version>
         <mockito.version>2.28.2</mockito.version>
         <jacoco.version>0.8.4</jacoco.version>
+        <azure.auth.helper.version>0.6.0-SNAPSHOT</azure.auth.helper.version>
+        <azure.tools.common.version>0.3.0</azure.tools.common.version>
     </properties>
 
     <licenses>
@@ -97,9 +99,13 @@
         <dependency>
             <groupId>com.microsoft.azure</groupId>
             <artifactId>azure-auth-helper</artifactId>
-            <version>0.1.0</version>
+            <version>${azure.auth.helper.version}</version>
+        </dependency>
+        <dependency>
+            <groupId>com.microsoft.azure</groupId>
+            <artifactId>azure-tools-common</artifactId>
+            <version>${azure.tools.common.version}</version>
         </dependency>
-
         <dependency>
             <groupId>org.apache.maven</groupId>
             <artifactId>maven-core</artifactId>

diff --git a/azure-maven-plugin/src/main/java/com/microsoft/azure/plugin/login/LoginMojo.java b/azure-maven-plugin/src/main/java/com/microsoft/azure/plugin/login/LoginMojo.java
@@ -11,10 +11,10 @@
 import com.microsoft.azure.auth.AzureCredential;
 import com.microsoft.azure.auth.exception.AzureLoginFailureException;
 import com.microsoft.azure.auth.exception.DesktopNotSupportedException;
+import com.microsoft.azure.common.utils.TextUtils;
 import com.microsoft.azure.management.Azure;
 import com.microsoft.azure.management.Azure.Authenticated;
 import com.microsoft.azure.management.resources.Subscription;
-import com.microsoft.azure.maven.common.utils.TextUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.maven.plugin.MojoFailureException;
 import org.apache.maven.plugins.annotations.Mojo;

diff --git a/...e-maven-plugin/src/main/java/com/microsoft/azure/plugin/login/SelectSubscriptionMojo.java b/...e-maven-plugin/src/main/java/com/microsoft/azure/plugin/login/SelectSubscriptionMojo.java
@@ -9,11 +9,11 @@
 import com.microsoft.azure.PagedList;
 import com.microsoft.azure.auth.AzureAuthHelper;
 import com.microsoft.azure.auth.AzureCredential;
+import com.microsoft.azure.common.utils.TextUtils;
 import com.microsoft.azure.credentials.AzureTokenCredentials;
 import com.microsoft.azure.management.Azure;
 import com.microsoft.azure.management.Azure.Authenticated;
 import com.microsoft.azure.management.resources.Subscription;
-import com.microsoft.azure.maven.common.utils.TextUtils;
 import org.apache.commons.lang3.StringUtils;
 import org.apache.maven.plugin.MojoExecutionException;
 import org.apache.maven.plugin.MojoFailureException;

diff --git a/azure-maven-plugin/src/test/java/com/microsoft/azure/plugin/login/TestHelper.java b/azure-maven-plugin/src/test/java/com/microsoft/azure/plugin/login/TestHelper.java
@@ -10,13 +10,13 @@
 import com.microsoft.applicationinsights.TelemetryClient;
 import com.microsoft.azure.Page;
 import com.microsoft.azure.PagedList;
+import com.microsoft.azure.common.utils.JsonUtils;
 import com.microsoft.azure.credentials.AzureTokenCredentials;
 import com.microsoft.azure.management.Azure;
 import com.microsoft.azure.management.Azure.Authenticated;
 import com.microsoft.azure.management.resources.Subscription;
 import com.microsoft.azure.management.resources.Subscriptions;
 import com.microsoft.azure.maven.common.telemetry.AppInsightHelper;
-import com.microsoft.azure.maven.common.utils.JsonUtils;
 import com.microsoft.rest.RestException;
 import org.apache.commons.lang3.SystemUtils;
 import org.mockito.ArgumentMatchers;

diff --git a/azure-spring-cloud-maven-plugin/pom.xml b/azure-spring-cloud-maven-plugin/pom.xml
@@ -35,7 +35,7 @@
         <appplatform.sdk.version>1.0.0-beta-2</appplatform.sdk.version>
         <mockito.version>2.28.2</mockito.version>
         <jacoco.version>0.8.4</jacoco.version>
-        <azure.auth.helper.version>0.5.1</azure.auth.helper.version>
+        <azure.auth.helper.version>0.6.0-SNAPSHOT</azure.auth.helper.version>
         <azure.storage.blob.version>11.0.1</azure.storage.blob.version>
         <dom4j.version>2.1.3</dom4j.version>
         <jtwig.core.version>5.87.0.RELEASE</jtwig.core.version>