security: mitigate cross-origin credential leaks, prevent CRLF header injection, and disable XML DTD processing

google-http-client-xml/src/main/java/com/google/api/client/xml/Xml.java

@@ -87,7 +87,13 @@ public static XmlSerializer createSerializer() {
 
   /** Returns a new XML pull parser. */
   public static XmlPullParser createParser() throws XmlPullParserException {
-    return getParserFactory().newPullParser();
+    XmlPullParser parser = getParserFactory().newPullParser();
+    try {
+      parser.setFeature(XmlPullParser.FEATURE_PROCESS_DOCDECL, false);
+    } catch (XmlPullParserException e) {
+      // Ignore if the feature is not supported
+    }
+    return parser;
   }
 
   /**

google-http-client-xml/src/test/java/com/google/api/client/xml/XmlTest.java

@@ -23,7 +23,9 @@
 import com.google.api.client.util.ArrayMap;
 import com.google.api.client.util.Key;
 import java.io.ByteArrayOutputStream;
+import java.io.IOException;
 import java.io.StringReader;
+import org.xmlpull.v1.XmlPullParserException;
 import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
@@ -621,4 +623,26 @@ private static class AllType {
     @Key public String[] stringArray;
     @Key public List<Integer> integerCollection;
   }
+
+  @Test
+  public void testCreateParser_disablesDocDecl() throws Exception {
+    XmlPullParser parser = Xml.createParser();
+    String xmlWithDtd = "<?xml version=\"1.0\"?>\n"
+        + "<!DOCTYPE any [\n"
+        + "  <!ENTITY xxe \"injected\">\n"
+        + "]>\n"
+        + "<any>&xxe;</any>";
+    parser.setInput(new StringReader(xmlWithDtd));
+    try {
+      SimpleTypeString xml = new SimpleTypeString();
+      XmlNamespaceDictionary namespaceDictionary = new XmlNamespaceDictionary().set("", "");
+      Xml.parseElement(parser, xml, namespaceDictionary, null);
+      if (xml.value != null) {
+        assertTrue(xml.value.isEmpty() || xml.value.equals("&xxe;"));
+      }
+    } catch (XmlPullParserException | IOException e) {
+      // Expected exception if the parser fails on DOCDECL
+    }
+  }
 }
+

google-http-client/src/main/java/com/google/api/client/http/HttpHeaders.java

@@ -838,6 +838,9 @@ private static void addHeader(
     }
     // compute value
     String stringValue = toStringValue(value);
+    if (name.contains("\r") || name.contains("\n") || stringValue.contains("\r") || stringValue.contains("\n")) {
+      throw new IllegalArgumentException("Header name or value contains CRLF characters.");
+    }
     // log header
     String loggedStringValue = stringValue;
     if (("Authorization".equalsIgnoreCase(name) || "Cookie".equalsIgnoreCase(name))

google-http-client/src/main/java/com/google/api/client/http/HttpRequest.java

@@ -29,6 +29,7 @@
 import io.opencensus.trace.Tracer;
 import java.io.IOException;
 import java.io.InputStream;
+import java.net.URL;
 import java.util.Properties;
 import java.util.concurrent.Callable;
 import java.util.concurrent.Executor;
@@ -860,6 +861,10 @@ public HttpResponse execute() throws IOException {
     Preconditions.checkNotNull(requestMethod);
     Preconditions.checkNotNull(url);
 
+    final String originalScheme = url.getScheme();
+    final String originalHost = url.getHost();
+    final int originalPort = url.getPort();
+
     Span span =
         tracer
             .spanBuilder(OpenCensusUtils.SPAN_NAME_HTTP_REQUEST_EXECUTE)
@@ -879,6 +884,11 @@ public HttpResponse execute() throws IOException {
       if (executeInterceptor != null) {
         executeInterceptor.intercept(this);
       }
+      // Prevent credential leak on cross-origin redirects
+      if (!isSameOrigin(originalScheme, originalHost, originalPort, url.getScheme(), url.getHost(), url.getPort())) {
+        headers.setAuthorization((String) null);
+        headers.setCookie((String) null);
+      }
       // build low-level HTTP request
       String urlString = url.build();
       addSpanAttribute(span, HttpTraceAttributeConstants.HTTP_METHOD, requestMethod);
@@ -1022,10 +1032,17 @@ public HttpResponse execute() throws IOException {
           response = new HttpResponse(this, lowLevelHttpResponse);
           responseConstructed = true;
         } finally {
-          if (!responseConstructed) {
-            InputStream lowLevelContent = lowLevelHttpResponse.getContent();
-            if (lowLevelContent != null) {
-              lowLevelContent.close();
+          if (!responseConstructed && lowLevelHttpResponse != null) {
+            try {
+              InputStream lowLevelContent = lowLevelHttpResponse.getContent();
+              if (lowLevelContent != null) {
+                lowLevelContent.close();
+              }
+            } catch (IOException ignored) {
+            }
+            try {
+              lowLevelHttpResponse.disconnect();
+            } catch (IOException ignored) {
             }
           }
         }
@@ -1180,7 +1197,25 @@ public boolean handleRedirect(int statusCode, HttpHeaders responseHeaders) {
         && HttpStatusCodes.isRedirect(statusCode)
         && redirectLocation != null) {
       // resolve the redirect location relative to the current location
-      setUrl(new GenericUrl(url.toURL(redirectLocation), useRawRedirectUrls));
+      URL newURL = url.toURL(redirectLocation);
+
+      // Check if redirecting to a different origin
+      String oldScheme = url.getScheme();
+      String oldHost = url.getHost();
+      int oldPort = url.getPort();
+
+      String newScheme = newURL.getProtocol();
+      String newHost = newURL.getHost();
+      int newPort = newURL.getPort();
+
+      int oldEffectivePort = getEffectivePort(oldScheme, oldPort);
+      int newEffectivePort = getEffectivePort(newScheme, newPort);
+
+      boolean sameOrigin = (oldScheme == null ? newScheme == null : oldScheme.equalsIgnoreCase(newScheme))
+          && (oldHost == null ? newHost == null : oldHost.equalsIgnoreCase(newHost))
+          && (oldEffectivePort == newEffectivePort);
+
+      setUrl(new GenericUrl(newURL, useRawRedirectUrls));
       // on 303 change method to GET
       if (statusCode == HttpStatusCodes.STATUS_CODE_SEE_OTHER) {
         setRequestMethod(HttpMethods.GET);
@@ -1194,11 +1229,39 @@ public boolean handleRedirect(int statusCode, HttpHeaders responseHeaders) {
       headers.setIfModifiedSince((String) null);
       headers.setIfUnmodifiedSince((String) null);
       headers.setIfRange((String) null);
+
+      // remove Cookie header if redirect is cross-origin
+      if (!sameOrigin) {
+        headers.setCookie((String) null);
+      }
       return true;
     }
     return false;
   }
 
+  private static int getEffectivePort(String scheme, int port) {
+    if (port != -1) {
+      return port;
+    }
+    if ("http".equalsIgnoreCase(scheme)) {
+      return 80;
+    }
+    if ("https".equalsIgnoreCase(scheme)) {
+      return 443;
+    }
+    return -1;
+  }
+
+  private static boolean isSameOrigin(
+      String scheme1, String host1, int port1,
+      String scheme2, String host2, int port2) {
+    int effectivePort1 = getEffectivePort(scheme1, port1);
+    int effectivePort2 = getEffectivePort(scheme2, port2);
+    return (scheme1 == null ? scheme2 == null : scheme1.equalsIgnoreCase(scheme2))
+        && (host1 == null ? host2 == null : host1.equalsIgnoreCase(host2))
+        && (effectivePort1 == effectivePort2);
+  }
+
   /**
    * Returns the sleeper.
    *

google-http-client/src/test/java/com/google/api/client/http/HttpHeadersTest.java

@@ -17,6 +17,7 @@
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import com.google.api.client.http.HttpRequestTest.E;
 import com.google.api.client.testing.http.MockLowLevelHttpRequest;
@@ -322,4 +323,29 @@ public void testFromHttpResponse_void() throws Exception {
     assertNull(v.v);
     assertEquals("svalue", v.s);
   }
+
+  @Test
+  public void testSerializeHeaders_crlfInjectionInName() throws Exception {
+    HttpHeaders headers = new HttpHeaders();
+    headers.set("foo\r\nbar", "value");
+    try {
+      HttpHeaders.serializeHeaders(headers, null, null, null, new MockLowLevelHttpRequest(), null);
+      fail("Expected IllegalArgumentException");
+    } catch (IllegalArgumentException e) {
+      // Expected
+    }
+  }
+
+  @Test
+  public void testSerializeHeaders_crlfInjectionInValue() throws Exception {
+    HttpHeaders headers = new HttpHeaders();
+    headers.set("foo", "value\r\nbar");
+    try {
+      HttpHeaders.serializeHeaders(headers, null, null, null, new MockLowLevelHttpRequest(), null);
+      fail("Expected IllegalArgumentException");
+    } catch (IllegalArgumentException e) {
+      // Expected
+    }
+  }
 }
+